On 2017/03/29 23:58, Robert Haas wrote:
> On Wed, Mar 29, 2017 at 4:21 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> Looks correct, so incorporated in the attached updated patch. Thanks.
>
> This seems like a hacky way to limit the reloptions to just OIDs.
> Shouldn't we instead have a new RELOPT_KIND_PARTITIONED or something
> like that?
OK, I tried that in the updated patch.
DefineRelation() and ATExecSetRelOptions() still calls heap_reloptions(),
but passes the new relopt_kind. None of the options listed in
reloptions.c are of this kind as of now, so parseRelOptions() simply
outputs the "unrecognized parameter %s" in the case of partitioned tables
(except in some cases described below, but not because of the limitations
of this patch). If and when partitioned tables start supporting the
existing parameters, we'll update the relopt_gen.kinds bitmask of those
parameters to allow them to be specified for partitioned tables.
With the patch:
create table parted (a int) partition by list (a) with (fillfactor = 10);
ERROR: unrecognized parameter "fillfactor"
create table parted (a int) partition by list (a) with (toast.fillfactor =
10);
ERROR: unrecognized parameter "fillfactor"
and:
create table parted (a int) partition by list (a) with (oids = true);
alter table parted set (fillfactor = 90);
ERROR: unrecognized parameter "fillfactor"
but:
-- appears to succeed, whereas an error should have been reported (I think)
alter table parted set (toast.fillfactor = 10);
ALTER TABLE
-- even with views
create table foo (a int);
create view foov with (toast.fillfactor = 10) as select * from foo;
CREATE VIEW
alter view foov set (toast.fillfactor = 20);
ALTER VIEW
Nothing is stored in pg_class.reloptions really, but the validation that
should have occurred in parseRelOptions() doesn't. This happens because
of the way transformRelOptions() works. It will ignore the DefElem's that
don't apply to the specified relation based on the value of the namspace
parameter ("toast" or NULL). That means it won't be included in the array
of options later received by parseRelOptions(), which is where the
validation occurs.
Also, alter table reset (xxx) never validates anything:
alter table foo reset (foo);
ALTER TABLE
alter table foo reset (foo.bar);
ALTER TABLE
Again, no pg_class.reloptions update occurs in this case. The reason this
happens is because transformRelOptions() never includes the options to be
reset in the array of options received by parseRelOptions(), so no
validation occurs.
But since both are existing behaviors, perhaps we can worry about it some
other time.
Thanks,
Amit
>From f150dec562985642b1af83236b65a42f7350033e 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
Reviwed by: Kyotaro Horiguchi
---
doc/src/sgml/ref/create_table.sgml | 6 ++++++
src/backend/access/common/reloptions.c | 33 ++++++++++++++++++++++--------
src/backend/catalog/heap.c | 20 ++++++++++--------
src/include/access/reloptions.h | 3 ++-
src/test/regress/expected/alter_table.out | 5 +++++
src/test/regress/expected/create_table.out | 5 +++++
src/test/regress/sql/alter_table.sql | 5 +++++
src/test/regress/sql/create_table.sql | 4 ++++
8 files changed, 63 insertions(+), 18 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..f3f5ade38f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1024,21 +1024,28 @@ parseRelOptions(Datum options, bool validate, relopt_kind kind,
if (relOpts[i]->kinds & kind)
numoptions++;
- if (numoptions == 0)
+ /*
+ * Even if we found no options of kind, it's better to raise the
+ * "unrecognized parameter %s" error here, instead of at the caller.
+ */
+ if (numoptions == 0 && !validate)
{
*numrelopts = 0;
return NULL;
}
- reloptions = palloc(numoptions * sizeof(relopt_value));
-
- for (i = 0, j = 0; relOpts[i]; i++)
+ if (numoptions > 0)
{
- if (relOpts[i]->kinds & kind)
+ reloptions = palloc(numoptions * sizeof(relopt_value));
+
+ for (i = 0, j = 0; relOpts[i]; i++)
{
- reloptions[j].gen = relOpts[i];
- reloptions[j].isset = false;
- j++;
+ if (relOpts[i]->kinds & kind)
+ {
+ reloptions[j].gen = relOpts[i];
+ reloptions[j].isset = false;
+ j++;
+ }
}
}
@@ -1418,8 +1425,16 @@ 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);
+
+ /*
+ * There aren't any partitioned table options yet really, so the
+ * following simply serves to reject whatever's specified as being
+ * "unrecognized storage parameter" if validate is true.
+ */
+ case RELKIND_PARTITIONED_TABLE:
+ return default_reloptions(reloptions, validate,
+ RELOPT_KIND_PARTITIONED);
default:
/* other relkinds are not supported */
return NULL;
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/include/access/reloptions.h b/src/include/access/reloptions.h
index 861977a608..91b2cd7bb2 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -48,8 +48,9 @@ typedef enum relopt_kind
RELOPT_KIND_SPGIST = (1 << 8),
RELOPT_KIND_VIEW = (1 << 9),
RELOPT_KIND_BRIN = (1 << 10),
+ RELOPT_KIND_PARTITIONED = (1 << 11),
/* if you add a new kind, make sure you update "last_default" too */
- RELOPT_KIND_LAST_DEFAULT = RELOPT_KIND_BRIN,
+ RELOPT_KIND_LAST_DEFAULT = RELOPT_KIND_PARTITIONED,
/* some compilers treat enums as signed ints, so we can't use 1 << 31 */
RELOPT_KIND_MAX = (1 << 30)
} relopt_kind;
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 2227f2d977..1230ad776d 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3372,3 +3372,8 @@ 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_withopt_test (a int) partition by list (a) with (oids = true);
+alter table parted_withopt_test set (fillfactor = 90);
+ERROR: unrecognized parameter "fillfactor"
+drop table parted_withopt_test;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 6f8645ddbd..6294a160fa 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -669,3 +669,8 @@ 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_withopt_test (a int) partition by list (a) with (fillfactor = 10);
+ERROR: unrecognized parameter "fillfactor"
+create table parted_withopt_test (a int) partition by list (a) with (toast.autvacuum_enabled = true);
+ERROR: unrecognized parameter "autvacuum_enabled"
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 8cd6786a90..e50f408b59 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_withopt_test (a int) partition by list (a) with (oids = true);
+alter table parted_withopt_test set (fillfactor = 90);
+drop table parted_withopt_test;
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 1f0fa8e16d..0c8cae001f 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -597,3 +597,7 @@ 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_withopt_test (a int) partition by list (a) with (fillfactor = 10);
+create table parted_withopt_test (a int) partition by list (a) with (toast.autvacuum_enabled = true);
--
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
