public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Zsolt Parragi <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
Date: Wed, 28 Jan 2026 10:15:29 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAN4CZFMjyPYqNY6qD4ArRZKUyC4oEPAfu-OesfChGMwMq1CG_w@mail.gmail.com>
References: <CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com>
<CAEoWx2k5800OjHO5KhTDv4JFYpDmyfh3MTtuB=7PP-0hLmG8yQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAN4CZFMUJWM0veLK93Ew8mYaL5pcU6G550STh6AZ-_LWV2zS+w@mail.gmail.com>
<[email protected]>
<CAN4CZFPX_t9q4W=jTh4jW4TOz=Xhfv0gPi9jG0MfXv9Ek7BO5w@mail.gmail.com>
<[email protected]>
<CAN4CZFMjyPYqNY6qD4ArRZKUyC4oEPAfu-OesfChGMwMq1CG_w@mail.gmail.com>
> 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)
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox