public inbox for [email protected]help / color / mirror / Atom feed
Re: tablecmds: reject CLUSTER ON for partitioned tables earlier 6+ messages / 3 participants [nested] [flat]
* Re: tablecmds: reject CLUSTER ON for partitioned tables earlier @ 2026-01-27 08:55 Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Zsolt Parragi @ 2026-01-27 08:55 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]> > I added two new test cases in 0002 that trigger the check. I also tested these scenarios previously. It's good that they are part of the test suite, but they don't hit that error path. Verified with this: diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 379f4d4ebaf..50f80724cb3 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -17857,9 +17857,7 @@ ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode) Relation parent_rel; if (rel->rd_rel->relispartition) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot change inheritance of a partition"))); + Assert(0); /* * AccessShareLock on the parent is probably enough, seeing that DROP ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: tablecmds: reject CLUSTER ON for partitioned tables earlier @ 2026-01-28 02:15 Chao Li <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Chao Li @ 2026-01-28 02:15 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Postgres hackers <[email protected]> > On Jan 27, 2026, at 16:55, Zsolt Parragi <[email protected]> wrote: > >> I added two new test cases in 0002 that trigger the check. > > I also tested these scenarios previously. It's good that they are part > of the test suite, but they don't hit that error path. Verified with > this: > > diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c > index 379f4d4ebaf..50f80724cb3 100644 > --- a/src/backend/commands/tablecmds.c > +++ b/src/backend/commands/tablecmds.c > @@ -17857,9 +17857,7 @@ ATExecDropInherit(Relation rel, RangeVar > *parent, LOCKMODE lockmode) > Relation parent_rel; > > if (rel->rd_rel->relispartition) > - ereport(ERROR, > - (errcode(ERRCODE_WRONG_OBJECT_TYPE), > - errmsg("cannot change inheritance of a partition"))); > + Assert(0); > > /* > * AccessShareLock on the parent is probably enough, seeing that DROP Thank you so much for pointing out this, and sorry for misunderstanding you. You are right, as the check has been added to ATPrepChangeInherit(), the same check in ATExecDropInherit becomes redundant. I thought you were talking about the check in ATPrepChangeInherit(). PFA v7: * In 0001, replaced ereport with assert in mark_index_clustered(). See my previous email for the analysis. * In 0002, removed the redundant check of relispartition from ATExecDropInherit(). Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ Attachments: [application/octet-stream] v7-0001-tablecmds-reject-CLUSTER-ON-for-partitioned-table.patch (3.7K, 2-v7-0001-tablecmds-reject-CLUSTER-ON-for-partitioned-table.patch) download | inline diff: From 42ae05132151bc881f1e280bf4fff563a1ff2d37 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Wed, 21 Jan 2026 11:27:03 +0800 Subject: [PATCH v7 1/2] tablecmds: reject CLUSTER ON for partitioned tables earlier ALTER TABLE ... CLUSTER ON and SET WITHOUT CLUSTER are not supported for partitioned tables and already fail today, but only at exec time. Reject these commands earlier via ATSimplePermissions(), matching the handling of other unsupported ALTER TABLE actions on partitioned tables (such as SET LOGGED / SET UNLOGGED). This centralizes the relation-kind check in the ALTER TABLE preparation phase, improving consistency and maintainability. As a result, partitioned tables now report the standard ATSimplePermissions() error for unsupported ALTER TABLE actions. Also, replace ereport(ERROR) with an Assert() in mark_index_clustered(), because after this change no code path should call it for a partitioned table. Author: Chao Li <[email protected]> Reviewed-by: Zsolt Parragi <[email protected]> Discussion: https://postgr.es/m/CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com --- src/backend/commands/cluster.c | 7 ++----- src/backend/commands/tablecmds.c | 2 +- src/test/regress/expected/cluster.out | 6 ++++-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 60a4617a585..f9ee5a5ab60 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -558,11 +558,8 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal) Relation pg_index; ListCell *index; - /* Disallow applying to a partitioned table */ - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot mark index clustered in partitioned table"))); + /* This function is only valid for non-partitioned tables */ + Assert(rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE); /* * If the index is already marked clustered, no need to do anything. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f976c0e5c7e..3e9f62e9d37 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5142,7 +5142,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, case AT_ClusterOn: /* CLUSTER ON */ case AT_DropCluster: /* SET WITHOUT CLUSTER */ ATSimplePermissions(cmd->subtype, rel, - ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW); + ATT_TABLE | ATT_MATVIEW); /* These commands never recurse */ /* No command-specific prep needed */ pass = AT_PASS_MISC; diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 4d40a6809ab..07c52e647f7 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -492,9 +492,11 @@ Number of partitions: 3 (Use \d+ to list them.) CLUSTER clstrpart; ERROR: there is no previously clustered index for table "clstrpart" ALTER TABLE clstrpart SET WITHOUT CLUSTER; -ERROR: cannot mark index clustered in partitioned table +ERROR: ALTER action SET WITHOUT CLUSTER cannot be performed on relation "clstrpart" +DETAIL: This operation is not supported for partitioned tables. ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; -ERROR: cannot mark index clustered in partitioned table +ERROR: ALTER action CLUSTER ON cannot be performed on relation "clstrpart" +DETAIL: This operation is not supported for partitioned tables. DROP TABLE clstrpart; -- Ownership of partitions is checked CREATE TABLE ptnowner(i int unique) PARTITION BY LIST (i); -- 2.50.1 (Apple Git-155) [application/octet-stream] v7-0002-tablecmds-reject-INHERIT-NO-INHERIT-for-partition.patch (7.3K, 3-v7-0002-tablecmds-reject-INHERIT-NO-INHERIT-for-partition.patch) download | inline diff: From 6300e04bda0ae3e7d633dae94c63f35acfdea70a Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Thu, 22 Jan 2026 16:47:58 +0800 Subject: [PATCH v7 2/2] tablecmds: reject INHERIT / NO INHERIT for partitioned tables earlier ALTER TABLE ... INHERIT and NO INHERIT are not supported for partitioned tables and already fail today, but only via ad-hoc checks in command- specific preparation code or later at execution time. Reject these commands earlier via ATSimplePermissions(), matching the handling of other unsupported ALTER TABLE actions on partitioned tables. This centralizes relation-kind checks in the common permission framework and produces the standard, consistent error message. While doing this, fix two related issues in the preparation logic: - The header comment of ATPrepAddInherit was a stale copy-paste that described behavior no longer implemented there. - NO INHERIT previously didn't use ATPrepAddInherit, causing it to reach execution-time checks unnecessarily. Consolidate preparation into a shared helper used by both INHERIT and NO INHERIT, ensuring both are rejected uniformly and as early as possible. Author: Chao Li <[email protected]> Reviewed-by: Zsolt Parragi <[email protected]> Discussion: https://postgr.es/m/CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com --- src/backend/commands/tablecmds.c | 36 ++++++++++------------- src/test/regress/expected/alter_table.out | 15 ++++++++-- src/test/regress/sql/alter_table.sql | 8 +++-- 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3e9f62e9d37..17dd94d6017 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -686,7 +686,7 @@ static void ATExecEnableDisableTrigger(Relation rel, const char *trigname, LOCKMODE lockmode); static void ATExecEnableDisableRule(Relation rel, const char *rulename, char fires_when, LOCKMODE lockmode); -static void ATPrepAddInherit(Relation child_rel); +static void ATPrepChangeInherit(Relation child_rel); static ObjectAddress ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode); static ObjectAddress ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode); static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid, @@ -5194,16 +5194,16 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, break; case AT_AddInherit: /* INHERIT */ ATSimplePermissions(cmd->subtype, rel, - ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE); + ATT_TABLE | ATT_FOREIGN_TABLE); /* This command never recurses */ - ATPrepAddInherit(rel); + ATPrepChangeInherit(rel); pass = AT_PASS_MISC; break; case AT_DropInherit: /* NO INHERIT */ ATSimplePermissions(cmd->subtype, rel, - ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE); + ATT_TABLE | ATT_FOREIGN_TABLE); /* This command never recurses */ - /* No command-specific prep needed */ + ATPrepChangeInherit(rel); pass = AT_PASS_MISC; break; case AT_AlterConstraint: /* ALTER CONSTRAINT */ @@ -17259,14 +17259,14 @@ ATExecEnableDisableRule(Relation rel, const char *rulename, } /* - * ALTER TABLE INHERIT + * Prepare to change inheritance. * - * Add a parent to the child's parents. This verifies that all the columns and - * check constraints of the parent appear in the child and that they have the - * same data types and expressions. + * A child table cannot be a typed table or a partition. We don't need to check + * partitioned table here, because it has been blocked by ATSimplePermissions in + * ATPrepCmd. */ static void -ATPrepAddInherit(Relation child_rel) +ATPrepChangeInherit(Relation child_rel) { if (child_rel->rd_rel->reloftype) ereport(ERROR, @@ -17277,14 +17277,15 @@ ATPrepAddInherit(Relation child_rel) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot change inheritance of a partition"))); - - if (child_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot change inheritance of partitioned table"))); } /* + * ALTER TABLE INHERIT + * + * Add a parent to the child's parents. This verifies that all the columns and + * check constraints of the parent appear in the child and that they have the + * same data types and expressions. + * * Return the address of the new parent relation. */ static ObjectAddress @@ -17855,11 +17856,6 @@ ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode) ObjectAddress address; Relation parent_rel; - if (rel->rd_rel->relispartition) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot change inheritance of a partition"))); - /* * AccessShareLock on the parent is probably enough, seeing that DROP * TABLE doesn't lock parent tables at all. We need some lock since we'll diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index ac1a7345d0f..27b3518f94c 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3997,10 +3997,19 @@ CREATE TABLE nonpartitioned ( a int, b int ); -ALTER TABLE partitioned INHERIT nonpartitioned; -ERROR: cannot change inheritance of partitioned table -ALTER TABLE nonpartitioned INHERIT partitioned; +ALTER TABLE partitioned INHERIT nonpartitioned; -- fail +ERROR: ALTER action INHERIT cannot be performed on relation "partitioned" +DETAIL: This operation is not supported for partitioned tables. +ALTER TABLE partitioned NO INHERIT nonpartitioned; -- fail +ERROR: ALTER action NO INHERIT cannot be performed on relation "partitioned" +DETAIL: This operation is not supported for partitioned tables. +ALTER TABLE nonpartitioned INHERIT partitioned; -- fail ERROR: cannot inherit from partitioned table "partitioned" +CREATE TABLE partitioned_p1 PARTITION OF partitioned FOR VALUES FROM (0, 0) TO (10, 100); +ALTER TABLE partitioned_p1 INHERIT nonpartitioned; -- fail +ERROR: cannot change inheritance of a partition +ALTER TABLE partitioned_p1 NO INHERIT nonpartitioned; -- fail +ERROR: cannot change inheritance of a partition -- cannot add NO INHERIT constraint to partitioned tables ALTER TABLE partitioned ADD CONSTRAINT chk_a CHECK (a > 0) NO INHERIT; ERROR: cannot add NO INHERIT constraint to partitioned table "partitioned" diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 417202430a5..24936ecdd34 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2397,8 +2397,12 @@ CREATE TABLE nonpartitioned ( a int, b int ); -ALTER TABLE partitioned INHERIT nonpartitioned; -ALTER TABLE nonpartitioned INHERIT partitioned; +ALTER TABLE partitioned INHERIT nonpartitioned; -- fail +ALTER TABLE partitioned NO INHERIT nonpartitioned; -- fail +ALTER TABLE nonpartitioned INHERIT partitioned; -- fail +CREATE TABLE partitioned_p1 PARTITION OF partitioned FOR VALUES FROM (0, 0) TO (10, 100); +ALTER TABLE partitioned_p1 INHERIT nonpartitioned; -- fail +ALTER TABLE partitioned_p1 NO INHERIT nonpartitioned; -- fail -- cannot add NO INHERIT constraint to partitioned tables ALTER TABLE partitioned ADD CONSTRAINT chk_a CHECK (a > 0) NO INHERIT; -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: tablecmds: reject CLUSTER ON for partitioned tables earlier @ 2026-03-13 03:38 Chao Li <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Chao Li @ 2026-03-13 03:38 UTC (permalink / raw) To: Postgres hackers <[email protected]> > On Jan 28, 2026, at 10:15, Chao Li <[email protected]> wrote: > > > >> On Jan 27, 2026, at 16:55, Zsolt Parragi <[email protected]> wrote: >> >>> I added two new test cases in 0002 that trigger the check. >> >> I also tested these scenarios previously. It's good that they are part >> of the test suite, but they don't hit that error path. Verified with >> this: >> >> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c >> index 379f4d4ebaf..50f80724cb3 100644 >> --- a/src/backend/commands/tablecmds.c >> +++ b/src/backend/commands/tablecmds.c >> @@ -17857,9 +17857,7 @@ ATExecDropInherit(Relation rel, RangeVar >> *parent, LOCKMODE lockmode) >> Relation parent_rel; >> >> if (rel->rd_rel->relispartition) >> - ereport(ERROR, >> - (errcode(ERRCODE_WRONG_OBJECT_TYPE), >> - errmsg("cannot change inheritance of a partition"))); >> + Assert(0); >> >> /* >> * AccessShareLock on the parent is probably enough, seeing that DROP > > Thank you so much for pointing out this, and sorry for misunderstanding you. You are right, as the check has been added to ATPrepChangeInherit(), the same check in ATExecDropInherit becomes redundant. I thought you were talking about the check in ATPrepChangeInherit(). > > PFA v7: > > * In 0001, replaced ereport with assert in mark_index_clustered(). See my previous email for the analysis. > * In 0002, removed the redundant check of relispartition from ATExecDropInherit(). > > Best regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > > > > <v7-0001-tablecmds-reject-CLUSTER-ON-for-partitioned-table.patch><v7-0002-tablecmds-reject-INHERIT-NO-INHERIT-for-partition.patch> PFA v8: rebase requested by CF https://commitfest.postgresql.org/patch/6415/ Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ Attachments: [application/octet-stream] v8-0001-tablecmds-reject-CLUSTER-ON-for-partitioned-table.patch (3.7K, 2-v8-0001-tablecmds-reject-CLUSTER-ON-for-partitioned-table.patch) download | inline diff: From ea40fc1bcfd317db87d970c67568f6a22a483337 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Wed, 21 Jan 2026 11:27:03 +0800 Subject: [PATCH v8 1/2] tablecmds: reject CLUSTER ON for partitioned tables earlier ALTER TABLE ... CLUSTER ON and SET WITHOUT CLUSTER are not supported for partitioned tables and already fail today, but only at exec time. Reject these commands earlier via ATSimplePermissions(), matching the handling of other unsupported ALTER TABLE actions on partitioned tables (such as SET LOGGED / SET UNLOGGED). This centralizes the relation-kind check in the ALTER TABLE preparation phase, improving consistency and maintainability. As a result, partitioned tables now report the standard ATSimplePermissions() error for unsupported ALTER TABLE actions. Also, replace ereport(ERROR) with an Assert() in mark_index_clustered(), because after this change no code path should call it for a partitioned table. Author: Chao Li <[email protected]> Reviewed-by: Zsolt Parragi <[email protected]> Discussion: https://postgr.es/m/CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com --- src/backend/commands/cluster.c | 7 ++----- src/backend/commands/tablecmds.c | 2 +- src/test/regress/expected/cluster.out | 6 ++++-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 3bfaa663699..2ef9c08fa03 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -572,11 +572,8 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal) Relation pg_index; ListCell *index; - /* Disallow applying to a partitioned table */ - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot mark index clustered in partitioned table"))); + /* This function is only valid for non-partitioned tables */ + Assert(rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE); /* * If the index is already marked clustered, no need to do anything. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index cd6d720386f..7d677feb1ea 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5160,7 +5160,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, case AT_ClusterOn: /* CLUSTER ON */ case AT_DropCluster: /* SET WITHOUT CLUSTER */ ATSimplePermissions(cmd->subtype, rel, - ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW); + ATT_TABLE | ATT_MATVIEW); /* These commands never recurse */ /* No command-specific prep needed */ pass = AT_PASS_MISC; diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 24b0b1a8fce..269f163efa6 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -492,9 +492,11 @@ Number of partitions: 3 (Use \d+ to list them.) CLUSTER clstrpart; ERROR: there is no previously clustered index for table "clstrpart" ALTER TABLE clstrpart SET WITHOUT CLUSTER; -ERROR: cannot mark index clustered in partitioned table +ERROR: ALTER action SET WITHOUT CLUSTER cannot be performed on relation "clstrpart" +DETAIL: This operation is not supported for partitioned tables. ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; -ERROR: cannot mark index clustered in partitioned table +ERROR: ALTER action CLUSTER ON cannot be performed on relation "clstrpart" +DETAIL: This operation is not supported for partitioned tables. -- and they cannot get an index-ordered REPACK without an explicit index name REPACK clstrpart USING INDEX; ERROR: cannot execute REPACK on partitioned table "clstrpart" USING INDEX with no index name -- 2.50.1 (Apple Git-155) [application/octet-stream] v8-0002-tablecmds-reject-INHERIT-NO-INHERIT-for-partition.patch (7.3K, 3-v8-0002-tablecmds-reject-INHERIT-NO-INHERIT-for-partition.patch) download | inline diff: From 3f77f1b0e1e189ede9bafbb18cd20aeb385b0050 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Thu, 22 Jan 2026 16:47:58 +0800 Subject: [PATCH v8 2/2] tablecmds: reject INHERIT / NO INHERIT for partitioned tables earlier ALTER TABLE ... INHERIT and NO INHERIT are not supported for partitioned tables and already fail today, but only via ad-hoc checks in command- specific preparation code or later at execution time. Reject these commands earlier via ATSimplePermissions(), matching the handling of other unsupported ALTER TABLE actions on partitioned tables. This centralizes relation-kind checks in the common permission framework and produces the standard, consistent error message. While doing this, fix two related issues in the preparation logic: - The header comment of ATPrepAddInherit was a stale copy-paste that described behavior no longer implemented there. - NO INHERIT previously didn't use ATPrepAddInherit, causing it to reach execution-time checks unnecessarily. Consolidate preparation into a shared helper used by both INHERIT and NO INHERIT, ensuring both are rejected uniformly and as early as possible. Author: Chao Li <[email protected]> Reviewed-by: Zsolt Parragi <[email protected]> Discussion: https://postgr.es/m/CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com --- src/backend/commands/tablecmds.c | 36 ++++++++++------------- src/test/regress/expected/alter_table.out | 15 ++++++++-- src/test/regress/sql/alter_table.sql | 8 +++-- 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7d677feb1ea..629cf795bf4 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -694,7 +694,7 @@ static void ATExecEnableDisableTrigger(Relation rel, const char *trigname, LOCKMODE lockmode); static void ATExecEnableDisableRule(Relation rel, const char *rulename, char fires_when, LOCKMODE lockmode); -static void ATPrepAddInherit(Relation child_rel); +static void ATPrepChangeInherit(Relation child_rel); static ObjectAddress ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode); static ObjectAddress ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode); static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid, @@ -5212,16 +5212,16 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, break; case AT_AddInherit: /* INHERIT */ ATSimplePermissions(cmd->subtype, rel, - ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE); + ATT_TABLE | ATT_FOREIGN_TABLE); /* This command never recurses */ - ATPrepAddInherit(rel); + ATPrepChangeInherit(rel); pass = AT_PASS_MISC; break; case AT_DropInherit: /* NO INHERIT */ ATSimplePermissions(cmd->subtype, rel, - ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE); + ATT_TABLE | ATT_FOREIGN_TABLE); /* This command never recurses */ - /* No command-specific prep needed */ + ATPrepChangeInherit(rel); pass = AT_PASS_MISC; break; case AT_AlterConstraint: /* ALTER CONSTRAINT */ @@ -17456,14 +17456,14 @@ ATExecEnableDisableRule(Relation rel, const char *rulename, } /* - * ALTER TABLE INHERIT + * Prepare to change inheritance. * - * Add a parent to the child's parents. This verifies that all the columns and - * check constraints of the parent appear in the child and that they have the - * same data types and expressions. + * A child table cannot be a typed table or a partition. We don't need to check + * partitioned table here, because it has been blocked by ATSimplePermissions in + * ATPrepCmd. */ static void -ATPrepAddInherit(Relation child_rel) +ATPrepChangeInherit(Relation child_rel) { if (child_rel->rd_rel->reloftype) ereport(ERROR, @@ -17474,14 +17474,15 @@ ATPrepAddInherit(Relation child_rel) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot change inheritance of a partition"))); - - if (child_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot change inheritance of partitioned table"))); } /* + * ALTER TABLE INHERIT + * + * Add a parent to the child's parents. This verifies that all the columns and + * check constraints of the parent appear in the child and that they have the + * same data types and expressions. + * * Return the address of the new parent relation. */ static ObjectAddress @@ -18052,11 +18053,6 @@ ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode) ObjectAddress address; Relation parent_rel; - if (rel->rd_rel->relispartition) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot change inheritance of a partition"))); - /* * AccessShareLock on the parent is probably enough, seeing that DROP * TABLE doesn't lock parent tables at all. We need some lock since we'll diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 5998c670aa3..ccd79dfecc0 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -4007,10 +4007,19 @@ CREATE TABLE nonpartitioned ( a int, b int ); -ALTER TABLE partitioned INHERIT nonpartitioned; -ERROR: cannot change inheritance of partitioned table -ALTER TABLE nonpartitioned INHERIT partitioned; +ALTER TABLE partitioned INHERIT nonpartitioned; -- fail +ERROR: ALTER action INHERIT cannot be performed on relation "partitioned" +DETAIL: This operation is not supported for partitioned tables. +ALTER TABLE partitioned NO INHERIT nonpartitioned; -- fail +ERROR: ALTER action NO INHERIT cannot be performed on relation "partitioned" +DETAIL: This operation is not supported for partitioned tables. +ALTER TABLE nonpartitioned INHERIT partitioned; -- fail ERROR: cannot inherit from partitioned table "partitioned" +CREATE TABLE partitioned_p1 PARTITION OF partitioned FOR VALUES FROM (0, 0) TO (10, 100); +ALTER TABLE partitioned_p1 INHERIT nonpartitioned; -- fail +ERROR: cannot change inheritance of a partition +ALTER TABLE partitioned_p1 NO INHERIT nonpartitioned; -- fail +ERROR: cannot change inheritance of a partition -- cannot add NO INHERIT constraint to partitioned tables ALTER TABLE partitioned ADD CONSTRAINT chk_a CHECK (a > 0) NO INHERIT; ERROR: cannot add NO INHERIT constraint to partitioned table "partitioned" diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index d6b6381ae5c..f5f13bbd3e7 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2405,8 +2405,12 @@ CREATE TABLE nonpartitioned ( a int, b int ); -ALTER TABLE partitioned INHERIT nonpartitioned; -ALTER TABLE nonpartitioned INHERIT partitioned; +ALTER TABLE partitioned INHERIT nonpartitioned; -- fail +ALTER TABLE partitioned NO INHERIT nonpartitioned; -- fail +ALTER TABLE nonpartitioned INHERIT partitioned; -- fail +CREATE TABLE partitioned_p1 PARTITION OF partitioned FOR VALUES FROM (0, 0) TO (10, 100); +ALTER TABLE partitioned_p1 INHERIT nonpartitioned; -- fail +ALTER TABLE partitioned_p1 NO INHERIT nonpartitioned; -- fail -- cannot add NO INHERIT constraint to partitioned tables ALTER TABLE partitioned ADD CONSTRAINT chk_a CHECK (a > 0) NO INHERIT; -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: tablecmds: reject CLUSTER ON for partitioned tables earlier @ 2026-03-16 08:51 Michael Paquier <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Michael Paquier @ 2026-03-16 08:51 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]> On Fri, Mar 13, 2026 at 11:38:17AM +0800, Chao Li wrote: > On Jan 28, 2026, at 10:15, Chao Li <[email protected]> wrote: >> * In 0001, replaced ereport with assert in >> mark_index_clustered(). See my previous email for the analysis. I have looked at this one, and I think that it is right. Even in the CLUSTER/VACUUM path, we have a relkind check before the sole caller of rebuild_relation() that discards partitioned tables, so we would never read mark_index_clustered() under this relkind. Applied. >> * In 0002, removed the redundant check of relispartition from >> * ATExecDropInherit(). I have not looked at this one. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: tablecmds: reject CLUSTER ON for partitioned tables earlier @ 2026-03-16 22:57 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Michael Paquier @ 2026-03-16 22:57 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]> On Mon, Mar 16, 2026 at 05:07:51PM +0800, Chao Li wrote: > Basically, 0002 does the same thing as 0001 just on a different > sub-command of ALTER TABLE. case AT_DropInherit: /* NO INHERIT */ [...] - /* No command-specific prep needed */ + ATPrepChangeInherit(rel); This change means that we are plugging in earlier a check based on a typed table for the NO INHERIT case. This sequence fails already on HEAD and with the patch, but generates a different error in the last query between HEAD and the patch, and is not covered by your patch: CREATE TYPE person_type AS (id int, name text); CREATE TABLE persons OF person_type; CREATE TABLE stuff (a int); ALTER TABLE persons NO INHERIT stuff; I'd suggest the addition of a test in typed_table.sql, just after the "ALTER TABLE persons INHERIT stuff;". The INHERIT case is already blocked, so NO INHERIT is a no-op anyway because we complain about the typed table not being inherited. How about doing that as a separate patch, with the second patch for tablescmds.c updating the regression test output? I thought that the NO INHERIT command was allowed, but we fail, so blocking it with a different, somewhat clearer error is OK by me. You are right that the comment on top of ATPrepAddInherit() is wrong, and that we'd better clean it up. The code does not do what the comment tells. That does not seem worth troubling stable branches. A second thing is that we'd better add an assertion in ATExecDropInherit() to make sure that this code path is never taken for a RELKIND_PARTITIONED_TABLE, ensuring that the prep phase blocked this case? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: tablecmds: reject CLUSTER ON for partitioned tables earlier @ 2026-03-17 05:36 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Michael Paquier @ 2026-03-17 05:36 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]> On Tue, Mar 17, 2026 at 09:12:13AM +0800, Chao Li wrote: > Anyway, I added the asserts in ATExecAddInherit() and > ATExecDropInherit() in v9. If you have a second thought, I can tune > it further. I have removed the assertions at the end, after looking at the surroundings for hints. > PFA v9: > * 0001 - added a NO INHERIT test case in typed_table.sql > * 0002 - added asserts in ATExecAddInherit and ATExecDropInherit. The comments rmeoved from ATPrepAddInherit() were still not at the correct location. CreateInheritance() is the routine in charge of the column and constraint checks. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-03-17 05:36 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-01-27 08:55 Re: tablecmds: reject CLUSTER ON for partitioned tables earlier Zsolt Parragi <[email protected]> 2026-01-28 02:15 ` Chao Li <[email protected]> 2026-03-13 03:38 ` Chao Li <[email protected]> 2026-03-16 08:51 ` Michael Paquier <[email protected]> 2026-03-16 22:57 ` Michael Paquier <[email protected]> 2026-03-17 05:36 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox