From: Chao Li <li.evan.chao@gmail.com>
Message-Id: <5244008D-79E1-484B-9407-21F5D388EC7F@gmail.com>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_38E6E3E8-9E39-4136-815C-2223F51B7DBB"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.300.41.1.7\))
Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
Date: Tue, 27 Jan 2026 07:13:04 +0800
In-Reply-To: <CAN4CZFPX_t9q4W=jTh4jW4TOz=Xhfv0gPi9jG0MfXv9Ek7BO5w@mail.gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
To: Zsolt Parragi <zsolt.parragi@percona.com>
References: <CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com>
 <CAEoWx2k5800OjHO5KhTDv4JFYpDmyfh3MTtuB=7PP-0hLmG8yQ@mail.gmail.com>
 <07773235-2E94-478F-BEF6-38C73B0553B8@gmail.com>
 <EA6CB317-60C2-4BA9-8E02-DBCAD384F107@gmail.com>
 <CAN4CZFMUJWM0veLK93Ew8mYaL5pcU6G550STh6AZ-_LWV2zS+w@mail.gmail.com>
 <16D5D52A-1B99-4371-982E-257C195D2924@gmail.com>
 <CAN4CZFPX_t9q4W=jTh4jW4TOz=Xhfv0gPi9jG0MfXv9Ek7BO5w@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/5244008D-79E1-484B-9407-21F5D388EC7F%40gmail.com>
Precedence: bulk


--Apple-Mail=_38E6E3E8-9E39-4136-815C-2223F51B7DBB
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8


> On Jan 27, 2026, at 01:26, Zsolt Parragi <zsolt.parragi@percona.com> =
wrote:
>=20
> +ALTER TABLE nonpartitioned INHERIT partitioned; -- ok
> ERROR:  cannot inherit from partitioned table "partitioned"
> -- cannot add NO INHERIT constraint to partitioned tables
>=20
> That comment should be fail

Fixed.

>=20
> Otherwise the patches look good.
>=20

Thanks a lot for confirming.


> The rest is about the two checks that seem redundant to me - I don't
> have a problem with leaving them as is, but they do seem redundant to
> me.
>=20
>> So, I would leave the check there, maybe use a separate discussion =
for removal of the check.
>=20
> I tried to find a way to trigger it and couldn't figure out anything,
> to me it seems unreachable.
>=20
>> However, there is a call path: vacuum -> vacuum_rel -> cluster_rel -> =
rebuild_relation -> mark_index_clustered. I am not sure if the check =
plays some role there.
>=20
> VACUUM FULL always passes InvalidOid to the cluster_rel for the index
> parameter, so we can't hit the error.
>=20
> CLUSTER is more difficult to follow, but to me that also seems like to
> never hit this error, and the behavior I see is also described in the
> documentation (mark_index_clustered is only called for leaf
> partitions, where it works). Following the calls in the code also
> shows the same to me, that this method is now only called for
> partitions.

I will need more investigation on this, so let=E2=80=99s leave it for a =
seperate discussion.

>=20
>> No, the check is not redundant. It checks for child partitions, while =
ATPrepChangeInherit only blocks partitioned tables.
>=20
> And I have the same issue with this one: I modified that error in
> ATExecDropInherit to an assertion locally. The test suite had no new
> failures, I also tried to write a few tests manually, but I wasn't
> able to trigger it. Maybe I'm missing something, but I think it's
> redundant now.

I added two new test cases in 0002 that trigger the check.

BTW, this is the CF entry: =
https://commitfest.postgresql.org/patch/6415/. You may mark yourself as =
a reviewer, and once you consider the patch is ready to go, would you =
mind change the status to Ready For Committer?

PFA v6.


Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/


--Apple-Mail=_38E6E3E8-9E39-4136-815C-2223F51B7DBB
Content-Disposition: attachment;
	filename=v6-0001-tablecmds-reject-CLUSTER-ON-for-partitioned-table.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="v6-0001-tablecmds-reject-CLUSTER-ON-for-partitioned-table.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=205489e109c0a06cd437d33f72d603e6e01825e794=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20"Chao=20Li=20(Evan)"=20<lic@highgo.com>=0A=
Date:=20Wed,=2021=20Jan=202026=2011:27:03=20+0800=0ASubject:=20[PATCH=20=
v6=201/2]=20tablecmds:=20reject=20CLUSTER=20ON=20for=20partitioned=20=
tables=0A=20earlier=0A=0AALTER=20TABLE=20...=20CLUSTER=20ON=20and=20SET=20=
WITHOUT=20CLUSTER=20are=20not=20supported=20for=0Apartitioned=20tables=20=
and=20already=20fail=20today,=20but=20only=20at=20exec=20time.=0A=0A=
Reject=20these=20commands=20earlier=20via=20ATSimplePermissions(),=20=
matching=20the=0Ahandling=20of=20other=20unsupported=20ALTER=20TABLE=20=
actions=20on=20partitioned=20tables=0A(such=20as=20SET=20LOGGED=20/=20=
SET=20UNLOGGED).=20This=20centralizes=20the=20relation-kind=0Acheck=20in=20=
the=20ALTER=20TABLE=20preparation=20phase,=20improving=20consistency=20=
and=0Amaintainability.=0A=0AAs=20a=20result,=20partitioned=20tables=20=
now=20report=20the=20standard=20ATSimplePermissions()=0Aerror=20for=20=
unsupported=20ALTER=20TABLE=20actions.=0A=0AAuthor:=20Chao=20Li=20=
<lic@highgo.com>=0AReviewed-by:=20Zsolt=20Parragi=20=
<zsolt.parragi@percona.com>=0ADiscussion:=20=
https://postgr.es/m/CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@ma=
il.gmail.com=0A---=0A=20src/backend/commands/tablecmds.c=20=20=20=20=20=20=
|=202=20+-=0A=20src/test/regress/expected/cluster.out=20|=206=20++++--=0A=
=202=20files=20changed,=205=20insertions(+),=203=20deletions(-)=0A=0A=
diff=20--git=20a/src/backend/commands/tablecmds.c=20=
b/src/backend/commands/tablecmds.c=0Aindex=20f976c0e5c7e..3e9f62e9d37=20=
100644=0A---=20a/src/backend/commands/tablecmds.c=0A+++=20=
b/src/backend/commands/tablecmds.c=0A@@=20-5142,7=20+5142,7=20@@=20=
ATPrepCmd(List=20**wqueue,=20Relation=20rel,=20AlterTableCmd=20*cmd,=0A=20=
=09=09case=20AT_ClusterOn:=09=09/*=20CLUSTER=20ON=20*/=0A=20=09=09case=20=
AT_DropCluster:=09/*=20SET=20WITHOUT=20CLUSTER=20*/=0A=20=09=09=09=
ATSimplePermissions(cmd->subtype,=20rel,=0A-=09=09=09=09=09=09=09=09=
ATT_TABLE=20|=20ATT_PARTITIONED_TABLE=20|=20ATT_MATVIEW);=0A+=09=09=09=09=
=09=09=09=09ATT_TABLE=20|=20ATT_MATVIEW);=0A=20=09=09=09/*=20These=20=
commands=20never=20recurse=20*/=0A=20=09=09=09/*=20No=20command-specific=20=
prep=20needed=20*/=0A=20=09=09=09pass=20=3D=20AT_PASS_MISC;=0Adiff=20=
--git=20a/src/test/regress/expected/cluster.out=20=
b/src/test/regress/expected/cluster.out=0Aindex=20=
4d40a6809ab..07c52e647f7=20100644=0A---=20=
a/src/test/regress/expected/cluster.out=0A+++=20=
b/src/test/regress/expected/cluster.out=0A@@=20-492,9=20+492,11=20@@=20=
Number=20of=20partitions:=203=20(Use=20\d+=20to=20list=20them.)=0A=20=
CLUSTER=20clstrpart;=0A=20ERROR:=20=20there=20is=20no=20previously=20=
clustered=20index=20for=20table=20"clstrpart"=0A=20ALTER=20TABLE=20=
clstrpart=20SET=20WITHOUT=20CLUSTER;=0A-ERROR:=20=20cannot=20mark=20=
index=20clustered=20in=20partitioned=20table=0A+ERROR:=20=20ALTER=20=
action=20SET=20WITHOUT=20CLUSTER=20cannot=20be=20performed=20on=20=
relation=20"clstrpart"=0A+DETAIL:=20=20This=20operation=20is=20not=20=
supported=20for=20partitioned=20tables.=0A=20ALTER=20TABLE=20clstrpart=20=
CLUSTER=20ON=20clstrpart_idx;=0A-ERROR:=20=20cannot=20mark=20index=20=
clustered=20in=20partitioned=20table=0A+ERROR:=20=20ALTER=20action=20=
CLUSTER=20ON=20cannot=20be=20performed=20on=20relation=20"clstrpart"=0A=
+DETAIL:=20=20This=20operation=20is=20not=20supported=20for=20=
partitioned=20tables.=0A=20DROP=20TABLE=20clstrpart;=0A=20--=20Ownership=20=
of=20partitions=20is=20checked=0A=20CREATE=20TABLE=20ptnowner(i=20int=20=
unique)=20PARTITION=20BY=20LIST=20(i);=0A--=20=0A2.50.1=20(Apple=20=
Git-155)=0A=0A=

--Apple-Mail=_38E6E3E8-9E39-4136-815C-2223F51B7DBB
Content-Disposition: attachment;
	filename=v6-0002-tablecmds-reject-INHERIT-NO-INHERIT-for-partition.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="v6-0002-tablecmds-reject-INHERIT-NO-INHERIT-for-partition.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=209599ea3e3e6ee9b5168c5e373168340a39729205=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20"Chao=20Li=20(Evan)"=20<lic@highgo.com>=0A=
Date:=20Thu,=2022=20Jan=202026=2016:47:58=20+0800=0ASubject:=20[PATCH=20=
v6=202/2]=20tablecmds:=20reject=20INHERIT=20/=20NO=20INHERIT=20for=20=
partitioned=0A=20tables=20earlier=0A=0AALTER=20TABLE=20...=20INHERIT=20=
and=20NO=20INHERIT=20are=20not=20supported=20for=20partitioned=0Atables=20=
and=20already=20fail=20today,=20but=20only=20via=20ad-hoc=20checks=20in=20=
command-=0Aspecific=20preparation=20code=20or=20later=20at=20execution=20=
time.=0A=0AReject=20these=20commands=20earlier=20via=20=
ATSimplePermissions(),=20matching=20the=0Ahandling=20of=20other=20=
unsupported=20ALTER=20TABLE=20actions=20on=20partitioned=20tables.=0A=
This=20centralizes=20relation-kind=20checks=20in=20the=20common=20=
permission=20framework=0Aand=20produces=20the=20standard,=20consistent=20=
error=20message.=0A=0AWhile=20doing=20this,=20fix=20two=20related=20=
issues=20in=20the=20preparation=20logic:=0A-=20The=20header=20comment=20=
of=20ATPrepAddInherit=20was=20a=20stale=20copy-paste=20that=0A=20=20=
described=20behavior=20no=20longer=20implemented=20there.=0A-=20NO=20=
INHERIT=20previously=20didn't=20use=20ATPrepAddInherit,=20causing=20it=20=
to=20reach=0A=20=20execution-time=20checks=20unnecessarily.=20=
Consolidate=20preparation=20into=20a=0A=20=20shared=20helper=20used=20by=20=
both=20INHERIT=20and=20NO=20INHERIT,=20ensuring=20both=20are=0A=20=20=
rejected=20uniformly=20and=20as=20early=20as=20possible.=0A=0AAuthor:=20=
Chao=20Li=20<lic@highgo.com>=0AReviewed-by:=20Zsolt=20Parragi=20=
<zsolt.parragi@percona.com>=0ADiscussion:=20=
https://postgr.es/m/CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@ma=
il.gmail.com=0A---=0A=20src/backend/commands/tablecmds.c=20=20=20=20=20=20=
=20=20=20=20|=2031=20++++++++++++-----------=0A=20=
src/test/regress/expected/alter_table.out=20|=2015=20++++++++---=0A=20=
src/test/regress/sql/alter_table.sql=20=20=20=20=20=20|=20=208=20++++--=0A=
=203=20files=20changed,=2034=20insertions(+),=2020=20deletions(-)=0A=0A=
diff=20--git=20a/src/backend/commands/tablecmds.c=20=
b/src/backend/commands/tablecmds.c=0Aindex=203e9f62e9d37..379f4d4ebaf=20=
100644=0A---=20a/src/backend/commands/tablecmds.c=0A+++=20=
b/src/backend/commands/tablecmds.c=0A@@=20-686,7=20+686,7=20@@=20static=20=
void=20ATExecEnableDisableTrigger(Relation=20rel,=20const=20char=20=
*trigname,=0A=20=09=09=09=09=09=09=09=09=09=20=20=20LOCKMODE=20=
lockmode);=0A=20static=20void=20ATExecEnableDisableRule(Relation=20rel,=20=
const=20char=20*rulename,=0A=20=09=09=09=09=09=09=09=09=09char=20=
fires_when,=20LOCKMODE=20lockmode);=0A-static=20void=20=
ATPrepAddInherit(Relation=20child_rel);=0A+static=20void=20=
ATPrepChangeInherit(Relation=20child_rel);=0A=20static=20ObjectAddress=20=
ATExecAddInherit(Relation=20child_rel,=20RangeVar=20*parent,=20LOCKMODE=20=
lockmode);=0A=20static=20ObjectAddress=20ATExecDropInherit(Relation=20=
rel,=20RangeVar=20*parent,=20LOCKMODE=20lockmode);=0A=20static=20void=20=
drop_parent_dependency(Oid=20relid,=20Oid=20refclassid,=20Oid=20=
refobjid,=0A@@=20-5194,16=20+5194,16=20@@=20ATPrepCmd(List=20**wqueue,=20=
Relation=20rel,=20AlterTableCmd=20*cmd,=0A=20=09=09=09break;=0A=20=09=09=
case=20AT_AddInherit:=09=09/*=20INHERIT=20*/=0A=20=09=09=09=
ATSimplePermissions(cmd->subtype,=20rel,=0A-=09=09=09=09=09=09=09=09=
ATT_TABLE=20|=20ATT_PARTITIONED_TABLE=20|=20ATT_FOREIGN_TABLE);=0A+=09=09=
=09=09=09=09=09=09ATT_TABLE=20|=20ATT_FOREIGN_TABLE);=0A=20=09=09=09/*=20=
This=20command=20never=20recurses=20*/=0A-=09=09=09=
ATPrepAddInherit(rel);=0A+=09=09=09ATPrepChangeInherit(rel);=0A=20=09=09=09=
pass=20=3D=20AT_PASS_MISC;=0A=20=09=09=09break;=0A=20=09=09case=20=
AT_DropInherit:=09/*=20NO=20INHERIT=20*/=0A=20=09=09=09=
ATSimplePermissions(cmd->subtype,=20rel,=0A-=09=09=09=09=09=09=09=09=
ATT_TABLE=20|=20ATT_PARTITIONED_TABLE=20|=20ATT_FOREIGN_TABLE);=0A+=09=09=
=09=09=09=09=09=09ATT_TABLE=20|=20ATT_FOREIGN_TABLE);=0A=20=09=09=09/*=20=
This=20command=20never=20recurses=20*/=0A-=09=09=09/*=20No=20=
command-specific=20prep=20needed=20*/=0A+=09=09=09=
ATPrepChangeInherit(rel);=0A=20=09=09=09pass=20=3D=20AT_PASS_MISC;=0A=20=09=
=09=09break;=0A=20=09=09case=20AT_AlterConstraint:=09/*=20ALTER=20=
CONSTRAINT=20*/=0A@@=20-17259,14=20+17259,14=20@@=20=
ATExecEnableDisableRule(Relation=20rel,=20const=20char=20*rulename,=0A=20=
}=0A=20=0A=20/*=0A-=20*=20ALTER=20TABLE=20INHERIT=0A+=20*=20Prepare=20to=20=
change=20inheritance.=0A=20=20*=0A-=20*=20Add=20a=20parent=20to=20the=20=
child's=20parents.=20This=20verifies=20that=20all=20the=20columns=20and=0A=
-=20*=20check=20constraints=20of=20the=20parent=20appear=20in=20the=20=
child=20and=20that=20they=20have=20the=0A-=20*=20same=20data=20types=20=
and=20expressions.=0A+=20*=20A=20child=20table=20cannot=20be=20a=20typed=20=
table=20or=20a=20partition.=20We=20don't=20need=20to=20check=0A+=20*=20=
partitioned=20table=20here,=20because=20it=20has=20been=20blocked=20by=20=
ATSimplePermissions=20in=0A+=20*=20ATPrepCmd.=0A=20=20*/=0A=20static=20=
void=0A-ATPrepAddInherit(Relation=20child_rel)=0A=
+ATPrepChangeInherit(Relation=20child_rel)=0A=20{=0A=20=09if=20=
(child_rel->rd_rel->reloftype)=0A=20=09=09ereport(ERROR,=0A@@=20=
-17277,14=20+17277,15=20@@=20ATPrepAddInherit(Relation=20child_rel)=0A=20=
=09=09ereport(ERROR,=0A=20=09=09=09=09=
(errcode(ERRCODE_WRONG_OBJECT_TYPE),=0A=20=09=09=09=09=20errmsg("cannot=20=
change=20inheritance=20of=20a=20partition")));=0A-=0A-=09if=20=
(child_rel->rd_rel->relkind=20=3D=3D=20RELKIND_PARTITIONED_TABLE)=0A-=09=09=
ereport(ERROR,=0A-=09=09=09=09(errcode(ERRCODE_WRONG_OBJECT_TYPE),=0A-=09=
=09=09=09=20errmsg("cannot=20change=20inheritance=20of=20partitioned=20=
table")));=0A=20}=0A=20=0A=20/*=0A+=20*=20ALTER=20TABLE=20INHERIT=0A+=20=
*=0A+=20*=20Add=20a=20parent=20to=20the=20child's=20parents.=20This=20=
verifies=20that=20all=20the=20columns=20and=0A+=20*=20check=20=
constraints=20of=20the=20parent=20appear=20in=20the=20child=20and=20that=20=
they=20have=20the=0A+=20*=20same=20data=20types=20and=20expressions.=0A+=20=
*=0A=20=20*=20Return=20the=20address=20of=20the=20new=20parent=20=
relation.=0A=20=20*/=0A=20static=20ObjectAddress=0Adiff=20--git=20=
a/src/test/regress/expected/alter_table.out=20=
b/src/test/regress/expected/alter_table.out=0Aindex=20=
ac1a7345d0f..27b3518f94c=20100644=0A---=20=
a/src/test/regress/expected/alter_table.out=0A+++=20=
b/src/test/regress/expected/alter_table.out=0A@@=20-3997,10=20+3997,19=20=
@@=20CREATE=20TABLE=20nonpartitioned=20(=0A=20=09a=20int,=0A=20=09b=20=
int=0A=20);=0A-ALTER=20TABLE=20partitioned=20INHERIT=20nonpartitioned;=0A=
-ERROR:=20=20cannot=20change=20inheritance=20of=20partitioned=20table=0A=
-ALTER=20TABLE=20nonpartitioned=20INHERIT=20partitioned;=0A+ALTER=20=
TABLE=20partitioned=20INHERIT=20nonpartitioned;=20--=20fail=0A+ERROR:=20=20=
ALTER=20action=20INHERIT=20cannot=20be=20performed=20on=20relation=20=
"partitioned"=0A+DETAIL:=20=20This=20operation=20is=20not=20supported=20=
for=20partitioned=20tables.=0A+ALTER=20TABLE=20partitioned=20NO=20=
INHERIT=20nonpartitioned;=20--=20fail=0A+ERROR:=20=20ALTER=20action=20NO=20=
INHERIT=20cannot=20be=20performed=20on=20relation=20"partitioned"=0A=
+DETAIL:=20=20This=20operation=20is=20not=20supported=20for=20=
partitioned=20tables.=0A+ALTER=20TABLE=20nonpartitioned=20INHERIT=20=
partitioned;=20--=20fail=0A=20ERROR:=20=20cannot=20inherit=20from=20=
partitioned=20table=20"partitioned"=0A+CREATE=20TABLE=20partitioned_p1=20=
PARTITION=20OF=20partitioned=20FOR=20VALUES=20FROM=20(0,=200)=20TO=20=
(10,=20100);=0A+ALTER=20TABLE=20partitioned_p1=20INHERIT=20=
nonpartitioned;=20--=20fail=0A+ERROR:=20=20cannot=20change=20inheritance=20=
of=20a=20partition=0A+ALTER=20TABLE=20partitioned_p1=20NO=20INHERIT=20=
nonpartitioned;=20--=20fail=0A+ERROR:=20=20cannot=20change=20inheritance=20=
of=20a=20partition=0A=20--=20cannot=20add=20NO=20INHERIT=20constraint=20=
to=20partitioned=20tables=0A=20ALTER=20TABLE=20partitioned=20ADD=20=
CONSTRAINT=20chk_a=20CHECK=20(a=20>=200)=20NO=20INHERIT;=0A=20ERROR:=20=20=
cannot=20add=20NO=20INHERIT=20constraint=20to=20partitioned=20table=20=
"partitioned"=0Adiff=20--git=20a/src/test/regress/sql/alter_table.sql=20=
b/src/test/regress/sql/alter_table.sql=0Aindex=20=
417202430a5..24936ecdd34=20100644=0A---=20=
a/src/test/regress/sql/alter_table.sql=0A+++=20=
b/src/test/regress/sql/alter_table.sql=0A@@=20-2397,8=20+2397,12=20@@=20=
CREATE=20TABLE=20nonpartitioned=20(=0A=20=09a=20int,=0A=20=09b=20int=0A=20=
);=0A-ALTER=20TABLE=20partitioned=20INHERIT=20nonpartitioned;=0A-ALTER=20=
TABLE=20nonpartitioned=20INHERIT=20partitioned;=0A+ALTER=20TABLE=20=
partitioned=20INHERIT=20nonpartitioned;=20--=20fail=0A+ALTER=20TABLE=20=
partitioned=20NO=20INHERIT=20nonpartitioned;=20--=20fail=0A+ALTER=20=
TABLE=20nonpartitioned=20INHERIT=20partitioned;=20--=20fail=0A+CREATE=20=
TABLE=20partitioned_p1=20PARTITION=20OF=20partitioned=20FOR=20VALUES=20=
FROM=20(0,=200)=20TO=20(10,=20100);=0A+ALTER=20TABLE=20partitioned_p1=20=
INHERIT=20nonpartitioned;=20--=20fail=0A+ALTER=20TABLE=20partitioned_p1=20=
NO=20INHERIT=20nonpartitioned;=20--=20fail=0A=20=0A=20--=20cannot=20add=20=
NO=20INHERIT=20constraint=20to=20partitioned=20tables=0A=20ALTER=20TABLE=20=
partitioned=20ADD=20CONSTRAINT=20chk_a=20CHECK=20(a=20>=200)=20NO=20=
INHERIT;=0A--=20=0A2.50.1=20(Apple=20Git-155)=0A=0A=

--Apple-Mail=_38E6E3E8-9E39-4136-815C-2223F51B7DBB--