Обсуждение: ALTER TABLE: warn when actions do not recurse to partitions
Hi Hacker,
This patch is part of a broader effort to make ALTER TABLE actions behave more consistently with respect to partitioned tables. There has been ongoing discussion around this area; see [1], which also links to earlier related threads.
In short, changing ALTER TABLE semantics requires more discussion and coordination than a single patch can realistically achieve. Before that larger work happens, I’m following up on [1] by trying to clarify the current behavior, both in documentation and in user-facing feedback.
This patch adds warning messages for sub-commands that appear to recurse but in fact do not. These currently include:
* SET STATISTICS
* SET/RESET (attribute_option = value)
* ENABLE/DISABLE [ REPLICA | ALWAYS] RULE
* ENABLE/DISABLE ROW LEVEL SECURITY
* NO FORCE / FORCE ROW LEVEL SECURITY
* OWNER TO
* REPLICA IDENTITY
* SET SCHEMA
* SET/RESET (attribute_option = value)
* ENABLE/DISABLE [ REPLICA | ALWAYS] RULE
* ENABLE/DISABLE ROW LEVEL SECURITY
* NO FORCE / FORCE ROW LEVEL SECURITY
* OWNER TO
* REPLICA IDENTITY
* SET SCHEMA
For example, if a user runs:
```
ALTER TABLE ONLY a_partitioned_table REPLICA IDENTITY FULL;
```
the semantics are clear: only the partitioned table itself is modified.
However, if the user runs the same command without ONLY:
```
ALTER TABLE a_partitioned_table REPLICA IDENTITY FULL;
```
```
there is potential confusion. From the command syntax alone, it is reasonable to assume that the change would propagate to child partitions, but in reality, it does not. Since the ALTER TABLE documentation does not explicitly spell this out, users often need to test the behavior themselves to be sure, which is a poor user experience.
With this patch, the command instead emits a warning such as:
```
evantest=# alter table sensor_data replica identity full;
WARNING: REPLICA IDENTITY is only applied to the partitioned table itself
ALTER TABLE
```
This makes the behavior explicit and removes the ambiguity.
For now, I’ve limited the change to REPLICA IDENTITY to see whether there are objections to this approach. If there are none, I plan to extend the same warning behavior to the other sub-commands listed above. After that, users can reasonably assume that an ALTER TABLE partitioned_table ... action will recurse to child partitions unless a warning explicitly tells them otherwise.
Вложения
On Monday, January 12, 2026, Chao Li <li.evan.chao@gmail.com> wrote:
For now, I’ve limited the change to REPLICA IDENTITY to see whether there are objections to this approach. If there are none, I plan to extend the same warning behavior to the other sub-commands listed above. After that, users can reasonably assume that an ALTER TABLE partitioned_table ... action will recurse to child partitions unless a warning explicitly tells them otherwise.
It should be a notice, not a warning.
How about indicating how many partitions were affected in the notice and allowing the absence of such a notice to be the indicator that cascading did not happen?
David J.
On Mon, Jan 12, 2026 at 9:40 AM David G. Johnston <david.g.johnston@gmail.com> wrote:
How about indicating how many partitions were affected in the notice and allowing the absence of such a notice to be the indicator that cascading did not happen?
I like the idea of number of partitions, but think we need to be more explicit than people surmising the lack of a notice is significant.
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
Hi David and Greg,
Thanks a lot for your reviews and feedbacks.
On Jan 12, 2026, at 22:40, David G. Johnston <david.g.johnston@gmail.com> wrote:
On Monday, January 12, 2026, Chao Li <li.evan.chao@gmail.com> wrote:
For now, I’ve limited the change to REPLICA IDENTITY to see whether there are objections to this approach. If there are none, I plan to extend the same warning behavior to the other sub-commands listed above. After that, users can reasonably assume that an ALTER TABLE partitioned_table ... action will recurse to child partitions unless a warning explicitly tells them otherwise.
It should be a notice, not a warning.
Make sense. I changed to NOTICE in v2.
How about indicating how many partitions were affected in the notice and
I added partition count in the notice message in v2.
allowing the absence of such a notice to be the indicator that cascading did not happen?
David J.
I don’t think relying on the absence of a notice works well in this case.
Currently, cascading never happens, which is exactly why I’m adding a NOTICE. If we rely on silence to indicate “no cascade”, users have no signal that their expectation was incorrect.
The ALTER TABLE documentation says:
```
If ONLY is specified before the table name, only that table is altered. If ONLY is not specified, the table and all its descendant tables (if any) are altered.
```
Given this, users reasonably expect that omitting ONLY will cause REPLICA IDENTITY to cascade to partitions. In reality, it never does, which breaks that expectation. The NOTICE is intended to make this behavior explicit in exactly that case.
On Jan 12, 2026, at 23:23, Greg Sabino Mullane <htamfids@gmail.com> wrote:
On Mon, Jan 12, 2026 at 9:40 AM David G. Johnston <david.g.johnston@gmail.com> wrote:
How about indicating how many partitions were affected in the notice and allowing the absence of such a notice to be the indicator that cascading did not happen?
I like the idea of number of partitions, but think we need to be more explicit than people surmising the lack of a notice is significant.
Cheers,
Greg
As explained above, the NOTICE is only emitted in the case where the documented ALTER TABLE semantics suggest cascading, but REPLICA IDENTITY
does not actually cascade to partitions. This makes the behavior explicit, rather than relying on users to infer meaning from the absence of a message.
PSA v2:
* Changed level log to NOTICE
* Rephrased the notice message and included partition count in the message.
Now, the message is like:
```
evantest=# alter table sensor_data replica identity full;
NOTICE: REPLICA IDENTITY does not apply to partitions (1 affected)
ALTER TABLE
```
Best regards,
Вложения
On Monday, January 12, 2026, Chao Li <li.evan.chao@gmail.com> wrote:
Now, the message is like:```evantest=# alter table sensor_data replica identity full;NOTICE: REPLICA IDENTITY does not apply to partitions (1 affected)ALTER TABLE
If it doesn't recurse there should be no count. It would either always be 1, so not helpful, or if did show a partition count, beside the point. In the later case suppress the message if there are no partitions present.
The statement “does not apply to partitions” is also factually wrong. One would just need to name the partition explicitly.
NOTICE: present partitions not affected
HINT: partitions may be modified individually using separate commands
ALTER TABLE
David J.
On Tue, Jan 13, 2026 at 11:42 AM David G. Johnston <david.g.johnston@gmail.com> wrote:
On Monday, January 12, 2026, Chao Li <li.evan.chao@gmail.com> wrote:Now, the message is like:```evantest=# alter table sensor_data replica identity full;NOTICE: REPLICA IDENTITY does not apply to partitions (1 affected)ALTER TABLEIf it doesn't recurse there should be no count. It would either always be 1, so not helpful, or if did show a partition count, beside the point. In the later case suppress the message if there are no partitions present.
The count was real. I agree that we can suppress the message when there are no partitions. Addressed that in v3.
The statement “does not apply to partitions” is also factually wrong. One would just need to name the partition explicitly.NOTICE: present partitions not affectedHINT: partitions may be modified individually using separate commandsALTER TABLE
Thanks for the suggestion, I took that in v3.
PSA v3:
* Rephrased the notice message as David's suggestion.
* Removed partition count from notice message.
* If a partitioned table doesn't have any partition, then suppress the message.
Now the command outputs look like:
```
evantest=# alter table sensor_data replica identity full;
NOTICE: present partitions not affected
HINT: partitions may be modified individually using separate commands
ALTER TABLE
```
```
evantest=# alter table sensor_data replica identity full;
NOTICE: present partitions not affected
HINT: partitions may be modified individually using separate commands
ALTER TABLE
```
Best regards,
--
Chao Li (Evan)HighGo Software Co., Ltd.
Вложения
Hi Chao On 13/01/2026 05:02, Chao Li wrote: > > PSA v3: > > * Rephrased the notice message as David's suggestion. > * Removed partition count from notice message. > * If a partitioned table doesn't have any partition, then suppress the > message. I've been playing with this patch, and it seems to work as expected - I'm surprised it didn't break any existing tests :). Do you plan to extend this patch to other subcommands mentioned in your initial post, such as SET STATISTICS? Thanks for the patch Best, Jim == tests == CREATE TABLE m (a int NOT NULL, b int) PARTITION BY RANGE (a); CREATE TABLE m_p1 PARTITION OF m FOR VALUES FROM (1) TO (10); CREATE TABLE m_p2 PARTITION OF m FOR VALUES FROM (10) TO (20); CREATE UNIQUE INDEX m_idx ON m(a); CREATE UNIQUE INDEX m_p1_idx ON m_p1(a); CREATE UNIQUE INDEX m_p2_idx ON m_p2(a); CREATE TABLE CREATE TABLE CREATE TABLE CREATE INDEX CREATE INDEX CREATE INDEX -- issue a NOTICE (m has partitions) ALTER TABLE m REPLICA IDENTITY NOTHING; ALTER TABLE m REPLICA IDENTITY FULL; ALTER TABLE m REPLICA IDENTITY DEFAULT; ALTER TABLE m REPLICA IDENTITY USING INDEX m_idx; NOTICE: present partitions not affected HINT: partitions may be modified individually using separate commands ALTER TABLE NOTICE: present partitions not affected HINT: partitions may be modified individually using separate commands ALTER TABLE NOTICE: present partitions not affected HINT: partitions may be modified individually using separate commands ALTER TABLE NOTICE: present partitions not affected HINT: partitions may be modified individually using separate commands ALTER TABLE -- does not issue a NOTICE (with ONLY: no recursion into partitions) ALTER TABLE ONLY m REPLICA IDENTITY NOTHING; ALTER TABLE ONLY m REPLICA IDENTITY FULL; ALTER TABLE ONLY m REPLICA IDENTITY DEFAULT; ALTER TABLE ONLY m REPLICA IDENTITY USING INDEX m_idx; ALTER TABLE ALTER TABLE ALTER TABLE ALTER TABLE -- does not issue a NOTICE (p1 has no partitions) ALTER TABLE m_p1 REPLICA IDENTITY NOTHING; ALTER TABLE m_p1 REPLICA IDENTITY FULL; ALTER TABLE m_p1 REPLICA IDENTITY DEFAULT; ALTER TABLE m_p1 REPLICA IDENTITY USING INDEX m_p1_idx; ALTER TABLE ALTER TABLE ALTER TABLE ALTER TABLE -- does not issue a NOTICE (m no longer has partitions) DROP TABLE m_p1, m_p2; DROP TABLE ALTER TABLE m REPLICA IDENTITY NOTHING; ALTER TABLE m REPLICA IDENTITY FULL; ALTER TABLE m REPLICA IDENTITY DEFAULT; ALTER TABLE m REPLICA IDENTITY USING INDEX m_idx; ALTER TABLE ALTER TABLE ALTER TABLE ALTER TABLE
> On Jan 13, 2026, at 19:16, Jim Jones <jim.jones@uni-muenster.de> wrote: > > Hi Chao > > On 13/01/2026 05:02, Chao Li wrote: >> >> PSA v3: >> >> * Rephrased the notice message as David's suggestion. >> * Removed partition count from notice message. >> * If a partitioned table doesn't have any partition, then suppress the >> message. > > I've been playing with this patch, and it seems to work as expected - > I'm surprised it didn't break any existing tests :). Do you plan to > extend this patch to other subcommands mentioned in your initial post, > such as SET STATISTICS? > > Thanks for the patch > > Best, Jim > Hi Jim, Thanks for your testing. Yes, I plan to add the notice to other sub-commands as needed. I only updated REPLICA IDENTITY firstto call for feedback. As you see, David has suggested the great wording for the notice message. Once the change on REPLICAIDENTITY is reviewed, it’s easy to extend to other sub-commands. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
On Jan 14, 2026, at 08:52, Chao Li <li.evan.chao@gmail.com> wrote:On Jan 13, 2026, at 19:16, Jim Jones <jim.jones@uni-muenster.de> wrote:
Hi Chao
On 13/01/2026 05:02, Chao Li wrote:
PSA v3:
* Rephrased the notice message as David's suggestion.
* Removed partition count from notice message.
* If a partitioned table doesn't have any partition, then suppress the
message.
I've been playing with this patch, and it seems to work as expected -
I'm surprised it didn't break any existing tests :). Do you plan to
extend this patch to other subcommands mentioned in your initial post,
such as SET STATISTICS?
Thanks for the patch
Best, Jim
Hi Jim,
Thanks for your testing. Yes, I plan to add the notice to other sub-commands as needed. I only updated REPLICA IDENTITY first to call for feedback. As you see, David has suggested the great wording for the notice message. Once the change on REPLICA IDENTITY is reviewed, it’s easy to extend to other sub-commands.
PFA v4.
I’ve extended the NOTICE to cover all sub-commands for which ONLY has no effect and where actions on a partitioned table do not propagate to its partitions:
- ALTER COLUMN SET/RESET attribute_option
- ALTER COLUMN SET COMPRESSION
- ENABLE/DISABLE RULE
- ENABLE/DISABLE/FORCE/NO FORCE ROW LEVEL SECURITY
- REPLICA IDENTITY
- OWNER TO
- SET TABLESPACE
- SET SCHEMA
RENAME is intentionally excluded. Using ONLY (or not) has no effect for RENAME, since relation names are independent by nature and there is no expectation of recursion.
OF / NOT OF are also excluded. Using ONLY has no effect for these commands, as they apply only to the partitioned table itself and not to its partitions.
One thing worth noting: following David’s suggestion, I removed the action name from the NOTICE message in v2. However, I later realized that we do need to include the action name, because an ALTER TABLE command may contain multiple sub-commands, and the NOTICE would otherwise be ambiguous.
In v4, I reuse alter_table_type_to_string() to construct the action name, consistent with what ATSimplePermissions() does. The NOTICE message itself also follows the same style as messages emitted by ATSimplePermissions(). For example, when an ALTER TABLE contains multiple sub-commands, the output now looks like:
```
evantest=# alter table p_test replica identity full, alter column username set (n_distinct = 0.1);
NOTICE: ALTER action REPLICA IDENTITY on relation "p_test" does not affect present partitions
HINT: partitions may be modified individually, or specify ONLY to suppress this message
NOTICE: ALTER action ALTER COLUMN ... SET on relation "p_test" does not affect present partitions
HINT: partitions may be modified individually, or specify ONLY to suppress this message
ALTER TABLE
```
Regression tests have been updated, and a few new tests have been added. v4 should now be ready for review.
Вложения
Hi Chao
On 22/01/2026 06:45, Chao Li wrote:
> evantest=# alter table p_test replica identity full, alter column
> username set (n_distinct = 0.1);
> NOTICE: ALTER action REPLICA IDENTITY on relation "p_test" does not
> affect present partitions
> HINT: partitions may be modified individually, or specify ONLY to
> suppress this message
> NOTICE: ALTER action ALTER COLUMN ... SET on relation "p_test" does not
> affect present partitions
> HINT: partitions may be modified individually, or specify ONLY to
> suppress this message
> ALTER TABLE
One could argue that encapsulating all conditions in
EmitPartitionNoRecurseNotice(), meaning it is called all the time, is
slightly inefficient, but the impact is really negligible in this case -
and it is how it is done in similar functions in tablecmds.c :) The code
LGTM.
One small thing:
errhint is supposed to be capitalised - see Error Message Style Guide[1]
"Detail and hint messages: Use complete sentences, and end each with a
period. Capitalize the first word of sentences. Put two spaces after the
period if another sentence follows (for English text; might be
inappropriate in other languages)."
ereport(NOTICE,
errmsg("ALTER action %s on relation \"%s\" does not affect present
partitions",
action_str,
RelationGetRelationName(rel)),
errhint("partitions may be modified individually, or specify ONLY to
suppress this message"));
What about this?
HINT: To update partitions, apply the command to each one individually,
or specify ONLY to suppress this message.
I'll test the newly covered subcomands tomorrow.
Best, Jim
1 - https://www.postgresql.org/docs/current/error-style-guide.html
> On Jan 23, 2026, at 03:27, Jim Jones <jim.jones@uni-muenster.de> wrote:
>
> Hi Chao
>
> On 22/01/2026 06:45, Chao Li wrote:
>> evantest=# alter table p_test replica identity full, alter column
>> username set (n_distinct = 0.1);
>> NOTICE: ALTER action REPLICA IDENTITY on relation "p_test" does not
>> affect present partitions
>> HINT: partitions may be modified individually, or specify ONLY to
>> suppress this message
>> NOTICE: ALTER action ALTER COLUMN ... SET on relation "p_test" does not
>> affect present partitions
>> HINT: partitions may be modified individually, or specify ONLY to
>> suppress this message
>> ALTER TABLE
>
>
> One could argue that encapsulating all conditions in
> EmitPartitionNoRecurseNotice(), meaning it is called all the time, is
> slightly inefficient, but the impact is really negligible in this case -
> and it is how it is done in similar functions in tablecmds.c :) The code
> LGTM.
Hi Jim, thanks a lot for the review.
>
> One small thing:
>
> errhint is supposed to be capitalised - see Error Message Style Guide[1]
Thanks for the info, I wasn’t aware of that. When I wrote the code, I searched “errhint” over the source tree, and
didn’tfind a standard to follow.
>
> "Detail and hint messages: Use complete sentences, and end each with a
> period. Capitalize the first word of sentences. Put two spaces after the
> period if another sentence follows (for English text; might be
> inappropriate in other languages)."
>
> ereport(NOTICE,
> errmsg("ALTER action %s on relation \"%s\" does not affect present
> partitions",
> action_str,
> RelationGetRelationName(rel)),
> errhint("partitions may be modified individually, or specify ONLY to
> suppress this message"));
>
> What about this?
>
> HINT: To update partitions, apply the command to each one individually,
> or specify ONLY to suppress this message.
Looks good. I will integrate your edit to the next version.
>
> I'll test the newly covered subcomands tomorrow.
Thanks again for testing. I will wait to see the test results and address all issues together in next version.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
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.
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).
This patch touches a lot of regression tests, but mostly to add the
keyword ONLY to the ALTER TABLE statements, to avoid the NOTICE message,
so that's ok.
Thanks!
Best, Jim
> On Jan 23, 2026, at 17:57, Jim Jones <jim.jones@uni-muenster.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 NOTICEand 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 tomove 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 messagefirst and emits it later. With that approach, in v5, the NOTICE is shown only when the sub-command succeeds, duplicatedNOTICE 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 itwith a period. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
Вложения
Hi Chao On 09/03/2026 07:46, Chao Li wrote: > I agree with you that the NOTICE should only be emitted when the action succeeds. Cool. v5 fixes this issue. The error is now emitted prior to any NOTICE: postgres=# ALTER TABLE m ALTER COLUMN a SET COMPRESSION pglz; ERROR: column data type integer does not support compression > Actually, there was another known issue in v4. Since an ALTER TABLE command may contain multiple sub-commands, the NOTICEand HINT could be repeated multiple times, and the HINT was completely duplicate. Nice. The messages are now collected in CollectPartitionNoRecurseNotice and emitted in EmitPartitionNoRecurseNotice, and duplicates are ignored, if applicable ... ALTER TABLE m ALTER COLUMN b SET COMPRESSION pglz, ALTER COLUMN b SET COMPRESSION pglz, DISABLE RULE r, ENABLE ROW LEVEL SECURITY, FORCE ROW LEVEL SECURITY, REPLICA IDENTITY FULL, OWNER TO u1; NOTICE: ALTER action ALTER COLUMN ... SET COMPRESSION on relation "m" does not affect present partitions NOTICE: ALTER action DISABLE RULE on relation "m" does not affect present partitions NOTICE: ALTER action ENABLE ROW SECURITY on relation "m" does not affect present partitions NOTICE: ALTER action FORCE ROW SECURITY on relation "m" does not affect present partitions NOTICE: ALTER action REPLICA IDENTITY on relation "m" does not affect present partitions NOTICE: ALTER action OWNER TO on relation "m" does not affect present partitions HINT: Partitions may be modified individually, or specify ONLY to suppress this message. ALTER TABLE --- ALTER TABLE ONLY m ALTER COLUMN b SET COMPRESSION pglz, ALTER COLUMN b SET COMPRESSION pglz, DISABLE RULE r, ENABLE ROW LEVEL SECURITY, FORCE ROW LEVEL SECURITY, REPLICA IDENTITY FULL, OWNER TO u1; ALTER TABLE ... which brings me to a few nitpicks: 1) A test containing multiple sub-commands in an ALTER TABLE (as shown above) would be nice. 2) There are some tests with misleading comments. For instance: -- set column attribute on partitioned table should get a notice ALTER TABLE list_parted4 ALTER COLUMN b SET (n_distinct = 0.2); ALTER TABLE list_parted4 ALTER COLUMN b RESET (n_distinct); ALTER TABLE ONLY list_parted4 ALTER COLUMN b SET (n_distinct = 0.2); ALTER TABLE ONLY list_parted4 ALTER COLUMN b RESET (n_distinct); The last two will not emit any NOTICE, since they're using the keyword ONLY. I'd say that either these ONLY tests need to be in a different code block with a proper comment, or the current comments need to be changed to make it clear, e.g. "set column attribute on partitioned table should get a NOTICE; ONLY suppresses the NOTICE" I also reviewed the code and it LGTM! Thanks for the patch! Best, Jim
> On Mar 9, 2026, at 21:02, Jim Jones <jim.jones@uni-muenster.de> wrote: > > Hi Chao > > On 09/03/2026 07:46, Chao Li wrote: >> I agree with you that the NOTICE should only be emitted when the action succeeds. > > Cool. v5 fixes this issue. > The error is now emitted prior to any NOTICE: > > postgres=# ALTER TABLE m ALTER COLUMN a SET COMPRESSION pglz; > ERROR: column data type integer does not support compression > >> Actually, there was another known issue in v4. Since an ALTER TABLE command may contain multiple sub-commands, the NOTICEand HINT could be repeated multiple times, and the HINT was completely duplicate. > > Nice. The messages are now collected in CollectPartitionNoRecurseNotice > and emitted in EmitPartitionNoRecurseNotice, and duplicates are ignored, > if applicable ... > > ALTER TABLE m > ALTER COLUMN b SET COMPRESSION pglz, > ALTER COLUMN b SET COMPRESSION pglz, > DISABLE RULE r, > ENABLE ROW LEVEL SECURITY, > FORCE ROW LEVEL SECURITY, > REPLICA IDENTITY FULL, > OWNER TO u1; > NOTICE: ALTER action ALTER COLUMN ... SET COMPRESSION on relation "m" > does not affect present partitions > NOTICE: ALTER action DISABLE RULE on relation "m" does not affect > present partitions > NOTICE: ALTER action ENABLE ROW SECURITY on relation "m" does not > affect present partitions > NOTICE: ALTER action FORCE ROW SECURITY on relation "m" does not affect > present partitions > NOTICE: ALTER action REPLICA IDENTITY on relation "m" does not affect > present partitions > NOTICE: ALTER action OWNER TO on relation "m" does not affect present > partitions > HINT: Partitions may be modified individually, or specify ONLY to > suppress this message. > ALTER TABLE > > --- > > ALTER TABLE ONLY m > ALTER COLUMN b SET COMPRESSION pglz, > ALTER COLUMN b SET COMPRESSION pglz, > DISABLE RULE r, > ENABLE ROW LEVEL SECURITY, > FORCE ROW LEVEL SECURITY, > REPLICA IDENTITY FULL, > OWNER TO u1; > ALTER TABLE > > ... which brings me to a few nitpicks: > > 1) A test containing multiple sub-commands in an ALTER TABLE (as shown > above) would be nice. Added a test case that contains multiple sub-commands. > 2) There are some tests with misleading comments. For instance: > > -- set column attribute on partitioned table should get a notice > ALTER TABLE list_parted4 ALTER COLUMN b SET (n_distinct = 0.2); > ALTER TABLE list_parted4 ALTER COLUMN b RESET (n_distinct); > ALTER TABLE ONLY list_parted4 ALTER COLUMN b SET (n_distinct = 0.2); > ALTER TABLE ONLY list_parted4 ALTER COLUMN b RESET (n_distinct); > > The last two will not emit any NOTICE, since they're using the keyword > ONLY. I'd say that either these ONLY tests need to be in a different > code block with a proper comment, or the current comments need to be > changed to make it clear, e.g. "set column attribute on partitioned > table should get a NOTICE; ONLY suppresses the NOTICE" > I updated the comments like: ``` -- enable/disable rules on partitioned tables without ONLY should get a notice ``` “Without ONLY” should eliminate the confusion. > I also reviewed the code and it LGTM! > Thanks a lot for your test and review. PFA v6, addressed Jim’s comments as explained above. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
Вложения
On 10/03/2026 09:04, Chao Li wrote: > PFA v6, addressed Jim’s comments as explained above. No further comments from me. If there are no objections, I'll mark the CF entry as Ready for Committer. Best, Jim
Review of v6:
typedef struct partitionNoRecurseNotice
{
List *notices;
} partitionNoRecurseNotice;
Not sure why we need a struct here, rather than just passing the list around?
Also should be PartitionNoRecurseNotice (CamelCase)
foreach(cell, postNotice->notices)
{
if (strcmp((char *) lfirst(cell), notice_msg) == 0)
{
pfree(notice_msg);
found = true;
break;
}
}
This seems a lot of extra work that could be avoided. Since we know each message is unique to the cmdtype/AlterTableType and the rel/Relation combination, use those two to drive the duplicate check. Then we can only build the notice_msg if needed! Perhaps adding two more fields to that lonely struct above?
partitionNoRecurseNotice * postNotice);
postNotice is a little misleading - maybe pending_notices or just notices?
does not affect present partitions
s/present/existing/g
CollectPartitionNoRecurseNotice(AT_SetSchema, rel, stmt->relation->inh, false, &postNotice);
This hard-coded AT_SetSchema just to return a "SET SCHEMA" later on feels hacky. Don't have a workaround off the top of my head, just registering my mild unease. :)
/* Emit a notice only if there are partitions */
if (nparts == 0)
return;
It doesn't look like this particular case is tested. Other than that, the tests look very good.
Cheers,
Greg
> On Mar 10, 2026, at 23:32, Greg Sabino Mullane <htamfids@gmail.com> wrote:
>
> Review of v6:
Thank you very much for the review.
>
> typedef struct partitionNoRecurseNotice
> {
> List *notices;
> } partitionNoRecurseNotice;
> Not sure why we need a struct here, rather than just passing the list around?
Initially I thought there might be a few fields in the structure, but ended up only one List field. Yes, this structure
isnot needed now. Removed it in v7.
>
> Also should be PartitionNoRecurseNotice (CamelCase)
>
> foreach(cell, postNotice->notices)
> {
> if (strcmp((char *) lfirst(cell), notice_msg) == 0)
> {
> pfree(notice_msg);
> found = true;
> break;
> }
> }
>
> This seems a lot of extra work that could be avoided. Since we know each message is unique to the
cmdtype/AlterTableTypeand the rel/Relation combination, use those two to drive the duplicate check. Then we can only
buildthe notice_msg if needed! Perhaps adding two more fields to that lonely struct above?
Are you concerning that rendering the full message text is the extra work? This is not a hot path, so I don’t think
thatwould be a big deal. Actually, adding two more fields sounds more expensive.
>
> partitionNoRecurseNotice * postNotice);
>
> postNotice is a little misleading - maybe pending_notices or just notices?
Yes, “pending” makes sense. Updated in v7.
>
> does not affect present partitions
>
> s/present/existing/g
> CollectPartitionNoRecurseNotice(AT_SetSchema, rel, stmt->relation->inh, false, &postNotice);
>
> This hard-coded AT_SetSchema just to return a "SET SCHEMA" later on feels hacky. Don't have a workaround off the top
ofmy head, just registering my mild unease. :)
Yes, as SET SCHEMA doesn’t go through the standard ALTER TABLE process: AlterTable() -> ATController() -> ATPrepCmd().
If you get some idea, please let me know.
>
> /* Emit a notice only if there are partitions */
> if (nparts == 0)
> return;
>
> It doesn't look like this particular case is tested. Other than that, the tests look very good.
Good catch. Added a test case to cover that.
PFA v7: addressed Greg’s review comments.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Вложения
On Wed, Mar 11, 2026 at 3:05 AM Chao Li <li.evan.chao@gmail.com> wrote:
Are you concerning that rendering the full message text is the extra work? This is not a hot path, so I don’t think that would be a big deal. Actually, adding two more fields sounds more expensive
Well, the recurring creation and freeing of the strings is the part that seems inefficient. But you don't even need to store the strings at all if you are tracking the action+rel. In such a case, the final strings can be created on the fly inside of EmitPartitionNoRecurseNotice, right? Then you just need a list to store the combos of action+relation.
Yes, as SET SCHEMA doesn’t go through the standard ALTER TABLE process: AlterTable() -> ATController() -> ATPrepCmd().
If you get some idea, please let me know.
Nothing worth the trouble, to be honest. As you rightly pointed out, this is not a hot path.
Cheers,
Greg
> On Mar 12, 2026, at 00:39, Greg Sabino Mullane <htamfids@gmail.com> wrote: > > On Wed, Mar 11, 2026 at 3:05 AM Chao Li <li.evan.chao@gmail.com> wrote: > Are you concerning that rendering the full message text is the extra work? This is not a hot path, so I don’t think thatwould be a big deal. Actually, adding two more fields sounds more expensive > > Well, the recurring creation and freeing of the strings is the part that seems inefficient. But you don't even need tostore the strings at all if you are tracking the action+rel. In such a case, the final strings can be created on the flyinside of EmitPartitionNoRecurseNotice, right? Then you just need a list to store the combos of action+relation. > Fully understood your point. My considerations are: * This is not on a hot path, and that’s a very trivial performance impact. * I would believe in most of use cases, ALTER TABLE won’t take duplicate sub-commands, thus duplicated messages should rarelyhappen. * If we take your approach, actually, we don’t have to store action+relation in the list, only action is okay. But, if wedefer building the notice message to EmitPartitionNoRecurseNotice, then we have to leave relation open till EmitPartitionNoRecurseNotice,which feels a worse burden. Looking at ATController(), rel is closed early. An alternative isto store the relation name in a temp variable, which also introduces extra code. As a trade-off, I think building the noticemessage in CollectPartitionNoRecurseNotice() is an easy implementation. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
Okay, that's all fine with me, thank you for the explanations!
One more small idea: lose the bool found = false, and simply return; on a matching strcmp
Cheers,
Greg
> On Mar 12, 2026, at 10:09, Greg Sabino Mullane <htamfids@gmail.com> wrote: > > Okay, that's all fine with me, thank you for the explanations! > > One more small idea: lose the bool found = false, and simply return; on a matching strcmp > Make sense. PFA v8. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
Вложения
Looks good to me!
Hello > Affected sub-commands include: > ... Shouldn't SET ACCESS METHOD be included in this list? It only changes the catalog entry for the parent, existing partitions stay as-is. It only sets the access method for future direct child partitions.
> On Mar 13, 2026, at 05:45, Zsolt Parragi <zsolt.parragi@percona.com> wrote: > > Hello > >> Affected sub-commands include: >> ... > > Shouldn't SET ACCESS METHOD be included in this list? > > It only changes the catalog entry for the parent, existing partitions > stay as-is. It only sets the access method for future direct child > partitions. Good catch. I missed SET ACCESS METHOD. Thanks a lot. PFA v9: * Included SET ACCESS METHOD * Added a new test case where if the sub-command fails, then the notice message should not show up Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/