Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier

From: Zsolt Parragi <[email-masked]>
Date: Tue, 27 Jan 2026 08:55:23 +0000

> 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

---

From: Chao Li <[email-masked]>
Date: Wed, 28 Jan 2026 10:15:29 +0800

> On Jan 27, 2026, at 16:55, Zsolt Parragi <[email-masked]> wrote:
> 

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/

---

From: Chao Li <[email-masked]>
Date: Fri, 13 Mar 2026 11:38:17 +0800

> On Jan 28, 2026, at 10:15, Chao Li <[email-masked]> wrote:
> 
> 
> 
> <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,

---

From: Michael Paquier <[email-masked]>
Date: Mon, 16 Mar 2026 17:51:34 +0900

On Fri, Mar 13, 2026 at 11:38:17AM +0800, Chao Li 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

---

From: Michael Paquier <[email-masked]>
Date: Tue, 17 Mar 2026 07:57:57 +0900

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

---

From: Michael Paquier <[email-masked]>
Date: Tue, 17 Mar 2026 14:36:48 +0900

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