Error for WITH options on partitioned tables
Someone on general list recently complained that the error message
from trying to use options on a partitioned table was misleading,
which it definitely is:
CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a)
WITH (fillfactor=100);
ERROR: unrecognized parameter "fillfactor"
Which is verified by patch 001.
Patch 002 replaces this with a more meaningful error message, which
matches our fine manual.
https://www.postgresql.org/docs/current/sql-createtable.html
ERROR: cannot specify storage options for a partitioned table
HINT: specify storage options on leaf partitions instead
--
Simon Riggs http://www.EnterpriseDB.com/
Attachments:
001_test_options_with_partitioned_table.v1.patchapplication/octet-stream; name=001_test_options_with_partitioned_table.v1.patchDownload
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 02d0999580..6deeadbfab 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -987,6 +987,9 @@ Partition key: LIST (a)
Number of partitions: 0
DROP TABLE parted_col_comment;
+-- specify options with partitioning
+CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a) WITH (fillfactor=100);
+ERROR: unrecognized parameter "fillfactor"
-- list partitioning on array type column
CREATE TABLE arrlp (a int[]) PARTITION BY LIST (a);
CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 37dac6b5fb..87118800ed 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -652,6 +652,9 @@ SELECT obj_description('parted_col_comment'::regclass);
\d+ parted_col_comment
DROP TABLE parted_col_comment;
+-- specify options with partitioning
+CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a) WITH (fillfactor=100);
+
-- list partitioning on array type column
CREATE TABLE arrlp (a int[]) PARTITION BY LIST (a);
CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
002_better_error_for_options_on_partitioned.v1.patchapplication/octet-stream; name=002_better_error_for_options_on_partitioned.v1.patchDownload
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index cd946c7692..be970d20ef 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -254,6 +254,11 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("cannot create partitioned table as inheritance child")));
+ if (stmt->options)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot specify storage options for a partitioned table"),
+ errhint("specify storage options on leaf partitions instead")));
}
/*
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 6deeadbfab..b4eefff63f 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -989,7 +989,8 @@ Number of partitions: 0
DROP TABLE parted_col_comment;
-- specify options with partitioning
CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a) WITH (fillfactor=100);
-ERROR: unrecognized parameter "fillfactor"
+ERROR: cannot specify storage options for a partitioned table
+HINT: specify storage options on leaf partitions instead
-- list partitioning on array type column
CREATE TABLE arrlp (a int[]) PARTITION BY LIST (a);
CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
On Fri, 16 Sep 2022 at 20:13, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
Someone on general list recently complained that the error message
from trying to use options on a partitioned table was misleading,
which it definitely is:CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a)
WITH (fillfactor=100);
ERROR: unrecognized parameter "fillfactor"Which is verified by patch 001.
Patch 002 replaces this with a more meaningful error message, which
matches our fine manual.
https://www.postgresql.org/docs/current/sql-createtable.htmlERROR: cannot specify storage options for a partitioned table
HINT: specify storage options on leaf partitions instead
Looks good. Does this means we don't need the partitioned_table_reloptions()
function and remove the reloptions validation in DefineRelation() for
partitioned table. Or we can ereport() in partitioned_table_reloptions().
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
I wrote:
On Fri, 16 Sep 2022 at 20:13, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
Patch 002 replaces this with a more meaningful error message, which
matches our fine manual.
https://www.postgresql.org/docs/current/sql-createtable.htmlERROR: cannot specify storage options for a partitioned table
HINT: specify storage options on leaf partitions insteadLooks good. Does this means we don't need the partitioned_table_reloptions()
function and remove the reloptions validation in DefineRelation() for
partitioned table. Or we can ereport() in partitioned_table_reloptions().
I want to know why we should do validation for partitioned tables even if it
doesn't support storage parameters?
/*
* There are no options for partitioned tables yet, but this is able to do
* some validation.
*/
return (bytea *) build_reloptions(reloptions, validate,
RELOPT_KIND_PARTITIONED,
0, NULL, 0);
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Hi, Simon!
The new error message looks better. But I believe it is better to use
"parameters" instead of "options" as it is called "storage parameters"
in the documentation. I also believe it is better to report error in
partitioned_table_reloptions() (thanks to Japin Li for mentioning this
function) as it also fixes the error message in the following situation:
test=# CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST
(a);
CREATE TABLE
test=# ALTER TABLE parted_col_comment SET (fillfactor=100);
ERROR: unrecognized parameter "fillfactor"
I attached the new versions of patches.
I'm not sure about the errcode. May be it is better to report error with
ERRCODE_INVALID_OBJECT_DEFINITION for CREATE TABLE and with
ERRCODE_WRONG_OBJECT_TYPE for ALTER TABLE (as when you try "ALTER TABLE
partitioned INHERIT nonpartitioned;" an ERROR with ERRCODE_WRONG_OBJECT_TYPE
is reported). Then either the desired code should be passed to
partitioned_table_reloptions() or similar checks and ereport calls should be
placed in two different places. I'm not sure whether it is a good idea to
change the code in one of these ways just to change the error code.
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
Attachments:
v2-0002-better-error-message-for-setting-parameters-for-p.patchtext/x-patch; charset=US-ASCII; name=v2-0002-better-error-message-for-setting-parameters-for-p.patchDownload
From 6407538f0bfd3eb56f5a4574d8893f9494f2a810 Mon Sep 17 00:00:00 2001
From: Karina Litskevich <l_karinka00@mail.ru>
Date: Fri, 14 Oct 2022 17:48:27 +0300
Subject: [PATCH v2 2/2] better error message for setting parameters for
partitioned table
---
src/backend/access/common/reloptions.c | 13 ++++++-------
src/test/regress/expected/alter_table.out | 3 ++-
src/test/regress/expected/create_table.out | 3 ++-
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 6458a9c276..75d6ff5040 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1984,13 +1984,12 @@ build_local_reloptions(local_relopts *relopts, Datum options, bool validate)
bytea *
partitioned_table_reloptions(Datum reloptions, bool validate)
{
- /*
- * There are no options for partitioned tables yet, but this is able to do
- * some validation.
- */
- return (bytea *) build_reloptions(reloptions, validate,
- RELOPT_KIND_PARTITIONED,
- 0, NULL, 0);
+ if (validate && reloptions)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot specify storage parameters for a partitioned table"),
+ errhint("specify storage parameters on leaf partitions instead"));
+ return NULL;
}
/*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index cec7bfa1f1..a719ef22c7 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3803,7 +3803,8 @@ ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
ERROR: cannot alter column "b" because it is part of the partition key of relation "partitioned"
-- specifying storage parameters for partitioned tables is not supported
ALTER TABLE partitioned SET (fillfactor=100);
-ERROR: unrecognized parameter "fillfactor"
+ERROR: cannot specify storage parameters for a partitioned table
+HINT: specify storage parameters on leaf partitions instead
-- partitioned table cannot participate in regular inheritance
CREATE TABLE nonpartitioned (
a int,
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 45c1a0a23e..f16be87729 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -989,7 +989,8 @@ Number of partitions: 0
DROP TABLE parted_col_comment;
-- specifying storage parameters for partitioned tables is not supported
CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a) WITH (fillfactor=100);
-ERROR: unrecognized parameter "fillfactor"
+ERROR: cannot specify storage parameters for a partitioned table
+HINT: specify storage parameters on leaf partitions instead
-- list partitioning on array type column
CREATE TABLE arrlp (a int[]) PARTITION BY LIST (a);
CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
--
2.25.1
v2-0001-test-parameters-for-partitioned-table.patchtext/x-patch; charset=US-ASCII; name=v2-0001-test-parameters-for-partitioned-table.patchDownload
From 580566815bc5abd6193f86ddbd55fac1ac941873 Mon Sep 17 00:00:00 2001
From: Karina Litskevich <l_karinka00@mail.ru>
Date: Fri, 14 Oct 2022 16:22:57 +0300
Subject: [PATCH v2 1/2] test parameters for partitioned table
---
src/test/regress/expected/alter_table.out | 3 +++
src/test/regress/expected/create_table.out | 3 +++
src/test/regress/sql/alter_table.sql | 3 +++
src/test/regress/sql/create_table.sql | 3 +++
4 files changed, 12 insertions(+)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 346f594ad0..cec7bfa1f1 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3801,6 +3801,9 @@ ALTER TABLE partitioned DROP COLUMN b;
ERROR: cannot drop column "b" because it is part of the partition key of relation "partitioned"
ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
ERROR: cannot alter column "b" because it is part of the partition key of relation "partitioned"
+-- specifying storage parameters for partitioned tables is not supported
+ALTER TABLE partitioned SET (fillfactor=100);
+ERROR: unrecognized parameter "fillfactor"
-- partitioned table cannot participate in regular inheritance
CREATE TABLE nonpartitioned (
a int,
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 4407a017a9..45c1a0a23e 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -987,6 +987,9 @@ Partition key: LIST (a)
Number of partitions: 0
DROP TABLE parted_col_comment;
+-- specifying storage parameters for partitioned tables is not supported
+CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a) WITH (fillfactor=100);
+ERROR: unrecognized parameter "fillfactor"
-- list partitioning on array type column
CREATE TABLE arrlp (a int[]) PARTITION BY LIST (a);
CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 9f773aeeb9..5a71961f02 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2325,6 +2325,9 @@ ALTER TABLE partitioned ALTER COLUMN a TYPE char(5);
ALTER TABLE partitioned DROP COLUMN b;
ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
+-- specifying storage parameters for partitioned tables is not supported
+ALTER TABLE partitioned SET (fillfactor=100);
+
-- partitioned table cannot participate in regular inheritance
CREATE TABLE nonpartitioned (
a int,
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 5175f404f7..93ccf77d4a 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -652,6 +652,9 @@ SELECT obj_description('parted_col_comment'::regclass);
\d+ parted_col_comment
DROP TABLE parted_col_comment;
+-- specifying storage parameters for partitioned tables is not supported
+CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a) WITH (fillfactor=100);
+
-- list partitioning on array type column
CREATE TABLE arrlp (a int[]) PARTITION BY LIST (a);
CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
--
2.25.1
Hi Karina,
I am not very clear about why `build_reloptions` is removed in patch
`v2-0002-better-error-message-for-setting-parameters-for-p.patch`, if
you can help explain would be great.
From my observation, it seems the WITH option has different behavior
when creating partitioned table and index. For example,
pgbench -i --partitions=2 --partition-method=range -d postgres
postgres=# create index idx_bid on pgbench_accounts using btree(bid)
with (fillfactor = 90);
CREATE INDEX
postgres=# select relname, relkind, reloptions from pg_class where
relnamespace=2200 order by oid;
relname | relkind | reloptions
----------------------------+---------+------------------
idx_bid | I | {fillfactor=90}
pgbench_accounts_1_bid_idx | i | {fillfactor=90}
pgbench_accounts_2_bid_idx | i | {fillfactor=90}
I can see the `fillfactor` parameter has been added to the indexes,
however, if I try to alter `fillfactor`, I got an error like below.
postgres=# alter index idx_bid set (fillfactor=40);
ERROR: ALTER action SET cannot be performed on relation "idx_bid"
DETAIL: This operation is not supported for partitioned indexes.
This generic error message is reported by
`errdetail_relkind_not_supported`, and there is a similar error message
for partitioned tables. Anyone knows if this can be an option for
reporting this `fillfactor` parameter not supported error.
Best regards,
David
Show quoted text
On 2022-10-14 8:16 a.m., Karina Litskevich wrote:
Hi, Simon!
The new error message looks better. But I believe it is better to use
"parameters" instead of "options" as it is called "storage parameters"
in the documentation. I also believe it is better to report error in
partitioned_table_reloptions() (thanks to Japin Li for mentioning this
function) as it also fixes the error message in the following situation:test=# CREATE TABLE parted_col_comment (a int, b text) PARTITION BY
LIST (a);
CREATE TABLE
test=# ALTER TABLE parted_col_comment SET (fillfactor=100);
ERROR: unrecognized parameter "fillfactor"I attached the new versions of patches.
I'm not sure about the errcode. May be it is better to report error with
ERRCODE_INVALID_OBJECT_DEFINITION for CREATE TABLE and with
ERRCODE_WRONG_OBJECT_TYPE for ALTER TABLE (as when you try "ALTER TABLE
partitioned INHERIT nonpartitioned;" an ERROR with
ERRCODE_WRONG_OBJECT_TYPE
is reported). Then either the desired code should be passed to
partitioned_table_reloptions() or similar checks and ereport calls
should be
placed in two different places. I'm not sure whether it is a good idea to
change the code in one of these ways just to change the error code.Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
Apologies, I only just noticed these messages. I have set WoA until I
have read, understood and can respond to your detailed input, thanks.
On Fri, 28 Oct 2022 at 22:21, David Zhang <david.zhang@highgo.ca> wrote:
Hi Karina,
I am not very clear about why `build_reloptions` is removed in patch
`v2-0002-better-error-message-for-setting-parameters-for-p.patch`, if
you can help explain would be great.From my observation, it seems the WITH option has different behavior
when creating partitioned table and index. For example,pgbench -i --partitions=2 --partition-method=range -d postgres
postgres=# create index idx_bid on pgbench_accounts using btree(bid)
with (fillfactor = 90);
CREATE INDEXpostgres=# select relname, relkind, reloptions from pg_class where
relnamespace=2200 order by oid;
relname | relkind | reloptions
----------------------------+---------+------------------
idx_bid | I | {fillfactor=90}
pgbench_accounts_1_bid_idx | i | {fillfactor=90}
pgbench_accounts_2_bid_idx | i | {fillfactor=90}I can see the `fillfactor` parameter has been added to the indexes,
however, if I try to alter `fillfactor`, I got an error like below.
postgres=# alter index idx_bid set (fillfactor=40);
ERROR: ALTER action SET cannot be performed on relation "idx_bid"
DETAIL: This operation is not supported for partitioned indexes.This generic error message is reported by
`errdetail_relkind_not_supported`, and there is a similar error message
for partitioned tables. Anyone knows if this can be an option for
reporting this `fillfactor` parameter not supported error.Best regards,
David
On 2022-10-14 8:16 a.m., Karina Litskevich wrote:
Hi, Simon!
The new error message looks better. But I believe it is better to use
"parameters" instead of "options" as it is called "storage parameters"
in the documentation. I also believe it is better to report error in
partitioned_table_reloptions() (thanks to Japin Li for mentioning this
function) as it also fixes the error message in the following situation:test=# CREATE TABLE parted_col_comment (a int, b text) PARTITION BY
LIST (a);
CREATE TABLE
test=# ALTER TABLE parted_col_comment SET (fillfactor=100);
ERROR: unrecognized parameter "fillfactor"I attached the new versions of patches.
I'm not sure about the errcode. May be it is better to report error with
ERRCODE_INVALID_OBJECT_DEFINITION for CREATE TABLE and with
ERRCODE_WRONG_OBJECT_TYPE for ALTER TABLE (as when you try "ALTER TABLE
partitioned INHERIT nonpartitioned;" an ERROR with
ERRCODE_WRONG_OBJECT_TYPE
is reported). Then either the desired code should be passed to
partitioned_table_reloptions() or similar checks and ereport calls
should be
placed in two different places. I'm not sure whether it is a good idea to
change the code in one of these ways just to change the error code.Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
--
Simon Riggs http://www.EnterpriseDB.com/
Hi David,
I am not very clear about why `build_reloptions` is removed in patch
`v2-0002-better-error-message-for-setting-parameters-for-p.patch`, if
you can help explain would be great.
"build_reloptions" parses "reloptions" and takes for it a list of allowed
options defined by the 5th argument "relopt_elems" and the 6th argument
"num_relopt_elems", which are NULL and 0 in the removed call. If "validate"
is false, it ignores options, which are not in the list, while parsing. If
"validate" is true, it "elog"s ERROR when it meets option, which is not in
the
allowed list.
As in the deleted call "build_reloptions" is called with an empty list of
allowed options, it does nothing (returns NULL) when "validate" is false,
and
"elog"s ERROR when "validate" is true and "reloptions" is not empty. That is
what the deleted comment above the deleted call about. This call is there
only
for validation. So as I wanted to make a specific error message for the
case of
partitioned tables, I added the validation in "partitioned_table_reloptions"
and saw no reason to call "build_reloptions" any more because it would just
return NULL in other cases.
This generic error message is reported by
`errdetail_relkind_not_supported`, and there is a similar error message
for partitioned tables. Anyone knows if this can be an option for
reporting this `fillfactor` parameter not supported error.
This error is reported by "ATSimplePermissions" and, as we can see in the
beginning of this function, it makes no difference between regular relations
and partitioned tables now. To make it report errors for partitioned tables
we
should add new "alter table target-type flag" and add it to a mask of each
"AlterTableType" if partitioned table is an allowed target for it (see that
huge switch-case in function "ATPrepCmd"). Then
"partitioned_table_reloptions"
may become useless and we also should check weather some other functions
become
useless. Maybe that is the right way, but it looks much harder than the
existing solutions, so I believe, before anyone began going this way, it's
better to know whether there are any pitfalls there.
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
On Mon, 7 Nov 2022 at 08:55, Karina Litskevich
<litskevichkarina@gmail.com> wrote:
Hi David,
I am not very clear about why `build_reloptions` is removed in patch
`v2-0002-better-error-message-for-setting-parameters-for-p.patch`, if
you can help explain would be great."build_reloptions" parses "reloptions" and takes for it a list of allowed
options defined by the 5th argument "relopt_elems" and the 6th argument
"num_relopt_elems", which are NULL and 0 in the removed call. If "validate"
is false, it ignores options, which are not in the list, while parsing. If
"validate" is true, it "elog"s ERROR when it meets option, which is not in the
allowed list.
Karina's changes make sense to me, so +1.
This is a minor patch, so I will set this as Ready For Committer.
--
Simon Riggs http://www.EnterpriseDB.com/
Simon Riggs <simon.riggs@enterprisedb.com> writes:
Karina's changes make sense to me, so +1.
This is a minor patch, so I will set this as Ready For Committer.
Pushed with minor fiddling:
* I concur with Karina's thought that ERRCODE_WRONG_OBJECT_TYPE
is the most on-point errcode for this. The complaint is specifically
about the table relkind and has nothing to do with the storage
parameter per se. I also agree that it's not worth trying to use
a different errcode for CREATE vs. ALTER.
* The HINT message wasn't per project style (it should be formatted as
a complete sentence), and I thought using "parameters for" in the
main message but "parameters on" in the hint was oddly inconsistent.
regards, tom lane