Re: ALTER TABLE: warn when actions do not recurse to partitions

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>
Cc: "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Greg Sabino Mullane <htamfids(at)gmail(dot)com>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: ALTER TABLE: warn when actions do not recurse to partitions
Date: 2026-03-09 06:46:57
Message-ID: 9174F0CF-2F70-4B4F-AED4-CAF113B7F093@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Jan 23, 2026, at 17:57, Jim Jones <jim(dot)jones(at)uni-muenster(dot)de> wrote:
>
>
>
> On 23/01/2026 01:11, Chao Li wrote:
>> I will wait to see the test results and address all issues together in next version.
>
> While testing some edge cases I found out that the NOTICE is being
> emitted too early in the code path, e.g.

Hi Jim, thank you very much for your review and test.

>
> postgres=# ALTER TABLE m ALTER COLUMN b SET COMPRESSION pglz;
> NOTICE: ALTER action ALTER COLUMN ... SET COMPRESSION on relation "m"
> does not affect present partitions
> HINT: partitions may be modified individually, or specify ONLY to
> suppress this message
> ERROR: column data type integer does not support compression
>
> I'd argue that emitting only the ERROR message in this case would be the
> right approach. What about moving the EmitPartitionNoRecurseNotice()
> call to ATExecCmd, right **after** the changes were successfully
> executed? For instance, in the case I mentioned above, you could explore:
>
> @@ -5446,6 +5475,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
> case AT_SetCompression: /* ALTER COLUMN SET COMPRESSION */
> address = ATExecSetCompression(rel, cmd->name,
> cmd->def,
>
> lockmode);
> + /* Emit notice after validation passes */
> + EmitPartitionNoRecurseNotice(cmd->subtype, rel,
> cmd->recurse, false);
> break;
>
> Not sure if cmd->recurse is propagated in this code path. If not, you
> might need to do it manually, e.g.
>
> @@ -4936,6 +4937,14 @@ ATPrepCmd(List **wqueue, Relation rel,
> AlterTableCmd *cmd,
> */
> cmd = copyObject(cmd);
>
> + if (recurse)
> + cmd->recurse = true;
> +
>
> I'm not saying it should be exactly this way, but it sounds more
> reasonable to me to emit the NOTICE only if we know that the command is
> going to be successfully executed (or was successfully executed).
>

I agree with you that the NOTICE should only be emitted when the action succeeds.

Actually, there was another known issue in v4. Since an ALTER TABLE command may contain multiple sub-commands, the NOTICE and HINT could be repeated multiple times, and the HINT was completely duplicate.

I had tried to keep the patch simple because I was worried that a larger refactoring might make it harder for the patch to move forward. But now it looks like I have to some refactoring, though I still to limit the refactoring as minimum as possible.

I hesitated to move EmitPartitionNoRecurseNotice to ATExecCmd. Because ATExecCmd lacks info about recursing, and do cmd->recurse = true; only for notice seems not the right way.

After some investigation, I decided to borrow the idea from 1d92e0c2cc4789255c630d8776bbe85ca9ebc27f, which caches the message first and emits it later. With that approach, in v5, the NOTICE is shown only when the sub-command succeeds, duplicated NOTICE are filtered, and the HINT is shown only once at the end.

In v5, I also updated the HINT message to better comply with the error style guide: capitalize the first letter and end it with a period.

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

Attachment Content-Type Size
v5-0001-ALTER-TABLE-emit-NOTICE-when-ONLY-is-omitted-but-.patch application/octet-stream 38.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Chao Li 2026-03-09 06:57:03 Re: Add pg_stat_recovery system view
Previous Message Pavel Stehule 2026-03-09 06:44:03 Re: POC: PLpgSQL FOREACH IN JSON ARRAY