> On Mar 17, 2026, at 06:57, Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.
Yep, my focus was only on partitioned table and ignored typed table.
As your suggestion, I added a NO INHERIT test case in typed_table.sql as 0001.
>
> 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
Actually, I doubt if this assert is really needed. If we add the assert in ATExecDropInherit(), then likely we should
alsoadd the same assert in ATExecAddInherit(), since both commands now share the same preparation path via
ATSimplePermissions()and ATPrepChangeInherit(). And if we assert for RELKIND_PARTITIONED_TABLE, then likely we should
alsoassert for partitions and typed tables, since those cases are also rejected in the prep phase.
But looking at other sub-commands not touched by this patch, for example:
```
case AT_GenericOptions:
ATSimplePermissions(cmd->subtype, rel, ATT_FOREIGN_TABLE);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
```
This blocks all relation kinds except ATT_FOREIGN_TABLE in the prep phase, but ATExecGenericOptions() does not assert
onthe table type.
Anyway, I added the asserts in ATExecAddInherit() and ATExecDropInherit() in v9. If you have a second thought, I can
tuneit further.
PFA v9:
* 0001 - added a NO INHERIT test case in typed_table.sql
* 0002 - added asserts in ATExecAddInherit and ATExecDropInherit.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
