MIME-Version: 1.0
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>
In-Reply-To: <16D5D52A-1B99-4371-982E-257C195D2924@gmail.com>
From: Zsolt Parragi <zsolt.parragi@percona.com>
Date: Mon, 26 Jan 2026 17:26:26 +0000
Message-ID: <CAN4CZFPX_t9q4W=jTh4jW4TOz=Xhfv0gPi9jG0MfXv9Ek7BO5w@mail.gmail.com>
Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
To: Chao Li <li.evan.chao@gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAN4CZFPX_t9q4W%3DjTh4jW4TOz%3DXhfv0gPi9jG0MfXv9Ek7BO5w%40mail.gmail.com>
Precedence: bulk

+ALTER TABLE nonpartitioned INHERIT partitioned; -- ok
 ERROR:  cannot inherit from partitioned table "partitioned"
 -- cannot add NO INHERIT constraint to partitioned tables

That comment should be fail

Otherwise the patches look good.

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.

> So, I would leave the check there, maybe use a separate discussion for removal of the check.

I tried to find a way to trigger it and couldn't figure out anything,
to me it seems unreachable.

> 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.

VACUUM FULL always passes InvalidOid to the cluster_rel for the index
parameter, so we can't hit the error.

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.

> No, the check is not redundant. It checks for child partitions, while ATPrepChangeInherit only blocks partitioned tables.

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.