On 2017/03/27 23:27, Robert Haas wrote:
> On Thu, Mar 23, 2017 at 8:54 PM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> On 2017/03/23 23:47, Amit Langote wrote:
>>> On Thu, Mar 23, 2017 at 11:27 PM, Maksim Milyutin
>>> <m.milyu...@postgrespro.ru> wrote:
>>>> Hi!
>>>>
>>>> I have noticed that there is scheduled unlinking of nonexistent physical
>>>> storage under partitioned table when we execute DROP TABLE statement on
>>>> this
>>>> partitioned table. Though this action doesn't generate any error under
>>>> typical behavior of postgres because the error of storage's lack is caught
>>>> through if-statement [1] I think it is not safe.
>>>>
>>>> My patch fixes this issue.
>>>>
>>>> 1. src/backend/storage/smgr/md.c:1385
>>>
>>> Good catch, will incorporate that in the main patch.
>>
>> And here is the updated patch.
>
> I think you should go back to the earlier strategy of disallowing heap
> reloptions to be set on the partitioned table. The thing is, we don't
> really know which way we're going to want to go in the future. Maybe
> we'll come up with a system where you can set options on the
> partitioned table, and those options will cascade to the children. Or
> maybe we'll come up with a system where partitioned tables have a
> completely different set of options to control behaviors specific to
> partitioned tables. If we do the latter, then we don't want to also
> have to support useless heap reloptions for forward compatibility, nor
> do we want to break backward-compatibility to remove support. If we
> do the former, then it's better if we allow it in the same release
> where it starts working.
You're right, modified the patch accordingly.
By the way, the previous version of the patch didn't really "disallow"
specifying heap reloptions though. What I'd imagine that should entail is
CREATE TABLE or ALTER TABLE raising error if one of those reloptions is
specified, which didn't really occur with the patch. The options were
silently accepted and stored into pg_class, but their values were never
used. I modified the patch so that an error occurs instead of silently
accepting the user input.
create table p (a int) partition by list (a) with (fillfactor = 10);
ERROR: unrecognized parameter "fillfactor" for a partitioned table
Thanks,
Amit
>From da33f808c022f763aa7395f965d48e07f17339de Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 24 Jan 2017 14:22:34 +0900
Subject: [PATCH] Do not allocate storage for partitioned tables.
Currently, it is not possible to insert any data into a partitioned
table. So they're empty at all times, which means it is wasteful to
allocate relfilenode and related storage objects.
Also, because there is no storage, it doesn't make sense to allow
specifying storage parameters such as fillfactor, etc.
Patch by: Amit Langote & Maksim Milyutin
---
doc/src/sgml/ref/create_table.sgml | 6 ++++
src/backend/access/common/reloptions.c | 3 +-
src/backend/catalog/heap.c | 20 +++++++------
src/backend/commands/tablecmds.c | 45 ++++++++++++++++++++++++++++++
src/test/regress/expected/alter_table.out | 6 ++++
src/test/regress/expected/create_table.out | 3 ++
src/test/regress/sql/alter_table.sql | 5 ++++
src/test/regress/sql/create_table.sql | 3 ++
8 files changed, 81 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 283d53e203..5f13f240f5 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1040,6 +1040,12 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
will use the table's parameter value.
</para>
+ <para>
+ Note that specifying the following parameters for partitioned tables is
+ not supported. You must specify them for individual leaf partitions if
+ necessary.
+ </para>
+
<variablelist>
<varlistentry>
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 72e12532ab..d9ea89db12 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -967,7 +967,6 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
options = heap_reloptions(classForm->relkind, datum, false);
break;
case RELKIND_VIEW:
@@ -976,6 +975,7 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_INDEX:
options = index_reloptions(amoptions, datum, false);
break;
+ case RELKIND_PARTITIONED_TABLE:
case RELKIND_FOREIGN_TABLE:
options = NULL;
break;
@@ -1418,7 +1418,6 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
return (bytea *) rdopts;
case RELKIND_RELATION:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
default:
/* other relkinds are not supported */
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index eee5e2f6ca..ff2f2e8efb 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -293,6 +293,7 @@ heap_create(const char *relname,
case RELKIND_VIEW:
case RELKIND_COMPOSITE_TYPE:
case RELKIND_FOREIGN_TABLE:
+ case RELKIND_PARTITIONED_TABLE:
create_storage = false;
/*
@@ -1347,14 +1348,13 @@ heap_create_with_catalog(const char *relname,
if (oncommit != ONCOMMIT_NOOP)
register_on_commit_action(relid, oncommit);
- if (relpersistence == RELPERSISTENCE_UNLOGGED)
- {
- Assert(relkind == RELKIND_RELATION || relkind == RELKIND_MATVIEW ||
- relkind == RELKIND_TOASTVALUE ||
- relkind == RELKIND_PARTITIONED_TABLE);
-
+ /*
+ * We do not want to create any storage objects for a partitioned
+ * table, including the init fork.
+ */
+ if (relpersistence == RELPERSISTENCE_UNLOGGED &&
+ relkind != RELKIND_PARTITIONED_TABLE)
heap_create_init_fork(new_rel_desc);
- }
/*
* ok, the relation has been cataloged, so close our relations and return
@@ -1378,6 +1378,9 @@ heap_create_with_catalog(const char *relname,
void
heap_create_init_fork(Relation rel)
{
+ Assert(rel->rd_rel->relkind == RELKIND_RELATION ||
+ rel->rd_rel->relkind == RELKIND_MATVIEW ||
+ rel->rd_rel->relkind == RELKIND_TOASTVALUE);
RelationOpenSmgr(rel);
smgrcreate(rel->rd_smgr, INIT_FORKNUM, false);
log_smgrcreate(&rel->rd_smgr->smgr_rnode.node, INIT_FORKNUM);
@@ -1824,7 +1827,8 @@ heap_drop_with_catalog(Oid relid)
*/
if (rel->rd_rel->relkind != RELKIND_VIEW &&
rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE &&
- rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
+ rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
+ rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
{
RelationDropStorage(rel);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4cf2efb2ad..6f1f16e2e7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -583,6 +583,29 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
ownerId = GetUserId();
/*
+ * We haven't invented special storage options for partitioned tables,
+ * nor do "heap" storage options (or any other for that matter) make sense
+ * in their case. So disallow anything but "oids" in the WITH option
+ * list.
+ */
+ if (relkind == RELKIND_PARTITIONED_TABLE && stmt->options != NIL)
+ {
+ ListCell *cell;
+
+ foreach(cell, stmt->options)
+ {
+ DefElem *def = (DefElem *) lfirst(cell);
+
+ if (def->defnamespace == NULL &&
+ pg_strcasecmp(def->defname, "oids") != 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("unrecognized parameter \"%s\" for a partitioned table",
+ def->defname)));
+ }
+ }
+
+ /*
* Parse and validate reloptions, if any.
*/
reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
@@ -9953,6 +9976,28 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
&isnull);
}
+ /*
+ * We haven't invented special storage options for partitioned tables,
+ * nor do "heap" storage options (or any other for that matter) make sense
+ * in their case. So disallow anything but "oids" to set/reset.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && defList != NIL)
+ {
+ ListCell *cell;
+
+ foreach(cell, defList)
+ {
+ DefElem *def = (DefElem *) lfirst(cell);
+
+ if (def->defnamespace == NULL &&
+ pg_strcasecmp(def->defname, "oids") != 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("unrecognized parameter \"%s\" for a partitioned table",
+ def->defname)));
+ }
+ }
+
/* Generate new proposed reloptions (text array) */
newOptions = transformRelOptions(isnull ? (Datum) 0 : datum,
defList, NULL, validnsps, false,
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 2227f2d977..2aedd620ca 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3372,3 +3372,9 @@ ERROR: partition constraint is violated by some row
-- cleanup
drop table p;
drop table p1;
+-- partitioned tables accept no options other than "oids" in the WITH list
+create table parted_with_test (a int) partition by list (a) with (oids = true);
+alter table parted_with_test set (fillfactor = 90);
+ERROR: unrecognized parameter "fillfactor" for a partitioned table
+alter table parted_with_test reset (fillfactor);
+ERROR: unrecognized parameter "fillfactor" for a partitioned table
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 6f8645ddbd..0cf600383c 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -669,3 +669,6 @@ Number of partitions: 3 (Use \d+ to list them.)
-- cleanup
DROP TABLE parted, list_parted, range_parted, list_parted2, range_parted2, range_parted3;
+-- partitioned tables accept no options other than "oids" in the WITH list
+create table parted_with_test (a int) partition by list (a) with (fillfactor = 10);
+ERROR: unrecognized parameter "fillfactor" for a partitioned table
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 8cd6786a90..9825165d4c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2228,3 +2228,8 @@ alter table p attach partition p1 for values from (1, 2) to (1, 10);
-- cleanup
drop table p;
drop table p1;
+
+-- partitioned tables accept no options other than "oids" in the WITH list
+create table parted_with_test (a int) partition by list (a) with (oids = true);
+alter table parted_with_test set (fillfactor = 90);
+alter table parted_with_test reset (fillfactor);
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 1f0fa8e16d..537d747d55 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -597,3 +597,6 @@ CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
-- cleanup
DROP TABLE parted, list_parted, range_parted, list_parted2, range_parted2, range_parted3;
+
+-- partitioned tables accept no options other than "oids" in the WITH list
+create table parted_with_test (a int) partition by list (a) with (fillfactor = 10);
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
