Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 17 Mar 2026 07:57:57 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Chao Li <li.evan.chao@gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
Message-ID: <abiK9Qyhzz24Yx6I@paquier.xyz>
References: <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>
 <5244008D-79E1-484B-9407-21F5D388EC7F@gmail.com>
 <CAN4CZFMjyPYqNY6qD4ArRZKUyC4oEPAfu-OesfChGMwMq1CG_w@mail.gmail.com>
 <075DECA0-5214-4794-A265-5A979E1A49B7@gmail.com>
 <8A3FAABD-F297-41D1-9B07-20A37E311C29@gmail.com>
 <abfElj6aqmazkgb1@paquier.xyz>
 <602D405C-63BC-4383-AD43-28841CAF91C1@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Dh+E22PSgqVMbgTu"
Content-Disposition: inline
In-Reply-To: <602D405C-63BC-4383-AD43-28841CAF91C1@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/abiK9Qyhzz24Yx6I%40paquier.xyz>
Precedence: bulk


--Dh+E22PSgqVMbgTu
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--Dh+E22PSgqVMbgTu
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmm4ivUACgkQnvQgOdby
QH2ZjQ//VPDox3D0M8dqiNmYUV+u75m02YlB7f06tlkZQjRCUn099KtvL0LHctlx
dr3lHb/XHNjfPDVW+FAJHiRniLVX42pItHnfUH7FStOMS8dz1XNvMfsiLJ9AyIIR
W3NPzTe1QC9OG6kY9WhyxZPvSjn1tIJrBPUtRWRJc4BiBWfwGi302u+Xcmo+x/Jz
hbyFWM/SZ4/1A3Rqgm75R6mEQj2I/9S1qI24MzoYmTWmFYZFDryX4LxtF/j4qXnv
GfUEeSLpCI6KlCUBbneDb/lC2ZxQAg6DYTLAQFk0h3wBfzyEuDXso7DVrupkSc4e
FsCx+hRLv9iFqd0yCBBaMEEwUfPTICI8H5OJaI4avrjRC6uT/+pSNoUWpJwBWnso
SlLpkQFqI0mZJguEM8Ef6sEiQKH2Yi7mBDIYREP8Vy+Od9kfj/K9V3fUEXynJ82E
M0VoL21H/MktPyBxdjTtaR8q15HfD1tTcRX5kEScAJbJDqBz2fH/QfpAAeIpJhzx
fIQ+IHGbrhMUnsp2XBNwDcoj7IB9SnL9XH5OBwX4o6jX0Pvfv/PpJV5zqYz6VsSA
pjCKdo+0b1usurYwDtXv6S0pBuSJN9sCQweScFGYrnxyzuhMb7r8ZoxS6E2XGi00
hTkI9F4+hxQTyW29VwfuoROIMIynbHWCgPU8Teo6rSJdKc1W0Dk=
=bXDw
-----END PGP SIGNATURE-----

--Dh+E22PSgqVMbgTu--