public inbox for [email protected]help / color / mirror / Atom feed
ALTER TABLE: warn when actions do not recurse to partitions 5+ messages / 2 participants [nested] [flat]
* ALTER TABLE: warn when actions do not recurse to partitions @ 2026-01-12 09:02 Chao Li <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Chao Li @ 2026-01-12 09:02 UTC (permalink / raw) To: Postgres hackers <[email protected]> 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 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. [1] https://postgr.es/m/[email protected] Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ Attachments: [application/octet-stream] v1-0001-Add-warning-when-ALTER-TABLE-REPLICA-IDENTITY-doe.patch (3.5K, 3-v1-0001-Add-warning-when-ALTER-TABLE-REPLICA-IDENTITY-doe.patch) download | inline diff: From 19abb2a52dd778a5c15254f41cb1bae0222747bc Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Mon, 12 Jan 2026 16:56:58 +0800 Subject: [PATCH v1] Add warning when ALTER TABLE REPLICA IDENTITY does not recurse ALTER TABLE ... REPLICA IDENTITY accepts a recursive form on partitioned tables, but the change is applied only to the partitioned table itself and does not propagate to child partitions. Previously this case was silently accepted, which could mislead users into assuming that the setting would recurse. Add a warning when recursion is requested on a partitioned table to make the behavior explicit and avoid confusion. This change does not alter semantics; it only provides user-visible feedback. Similar warnings may be added for other ALTER TABLE sub-commands with non-recursive behavior in follow-up commits. Author: Chao Li <[email protected]> --- src/backend/commands/tablecmds.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f976c0e5c7e..40a4d308334 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -693,7 +693,7 @@ static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid, DependencyType deptype); static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode); static void ATExecDropOf(Relation rel, LOCKMODE lockmode); -static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode); +static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, bool recurse, LOCKMODE lockmode); static void ATExecGenericOptions(Relation rel, List *options); static void ATExecSetRowSecurity(Relation rel, bool rls); static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls); @@ -5227,8 +5227,15 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW); pass = AT_PASS_MISC; - /* This command never recurses */ - /* No command-specific prep needed */ + + /* + * This command now doesn't recurse, but we want to notify user if + * recurse is set + * + * No command-specific prep needed + */ + if (recurse) + cmd->recurse = true; break; case AT_EnableTrig: /* ENABLE TRIGGER variants */ case AT_EnableAlwaysTrig: @@ -5643,7 +5650,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, ATExecDropOf(rel, lockmode); break; case AT_ReplicaIdentity: - ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def, lockmode); + ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def, + cmd->recurse, lockmode); break; case AT_EnableRowSecurity: ATExecSetRowSecurity(rel, true); @@ -18515,12 +18523,16 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, * ALTER TABLE <name> REPLICA IDENTITY ... */ static void -ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode) +ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, bool recurse, LOCKMODE lockmode) { Oid indexOid; Relation indexRel; int key; + if (recurse && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + ereport(WARNING, + (errmsg("REPLICA IDENTITY is only applied to the partitioned table itself"))); + if (stmt->identity_type == REPLICA_IDENTITY_DEFAULT) { relation_mark_replica_identity(rel, stmt->identity_type, InvalidOid, true); -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: ALTER TABLE: warn when actions do not recurse to partitions @ 2026-01-12 14:40 David G. Johnston <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: David G. Johnston @ 2026-01-12 14:40 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]> On Monday, January 12, 2026, Chao Li <[email protected]> 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. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: ALTER TABLE: warn when actions do not recurse to partitions @ 2026-01-13 03:18 Chao Li <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Chao Li @ 2026-01-13 03:18 UTC (permalink / raw) To: David G. Johnston <[email protected]>; Greg Sabino Mullane <[email protected]>; +Cc: Postgres hackers <[email protected]> Hi David and Greg, Thanks a lot for your reviews and feedbacks. On Jan 12, 2026, at 22:40, David G. Johnston <[email protected]> wrote: On Monday, January 12, 2026, Chao Li <[email protected]> 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 <[email protected]> wrote: On Mon, Jan 12, 2026 at 9:40 AM David G. Johnston < [email protected]> 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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ Attachments: [application/octet-stream] v2-0001-Add-notice-when-ALTER-TABLE-REPLICA-IDENTITY-does.patch (3.6K, 3-v2-0001-Add-notice-when-ALTER-TABLE-REPLICA-IDENTITY-does.patch) download | inline diff: From 2fb2578efc74dbf9027ca0f65e4171ad4d6d964c Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Mon, 12 Jan 2026 16:56:58 +0800 Subject: [PATCH v2] Add notice when ALTER TABLE REPLICA IDENTITY does not recurse ALTER TABLE ... REPLICA IDENTITY accepts a recursive form on partitioned tables, but the change is applied only to the partitioned table itself and does not propagate to child partitions. Previously this case was silently accepted, which could mislead users into assuming that the setting would recurse. Add a notice when recursion is requested on a partitioned table to make the behavior explicit and avoid confusion. This change does not alter semantics; it only provides user-visible feedback. Similar notices may be added for other ALTER TABLE sub-commands with non-recursive behavior in follow-up commits. Author: Chao Li <[email protected]> --- src/backend/commands/tablecmds.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f976c0e5c7e..2e8d769065c 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -693,7 +693,7 @@ static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid, DependencyType deptype); static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode); static void ATExecDropOf(Relation rel, LOCKMODE lockmode); -static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode); +static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, bool recurse, LOCKMODE lockmode); static void ATExecGenericOptions(Relation rel, List *options); static void ATExecSetRowSecurity(Relation rel, bool rls); static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls); @@ -5227,8 +5227,15 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW); pass = AT_PASS_MISC; - /* This command never recurses */ - /* No command-specific prep needed */ + + /* + * This command now doesn't recurse, but we want to notify user if + * recurse is set + * + * No command-specific prep needed + */ + if (recurse) + cmd->recurse = true; break; case AT_EnableTrig: /* ENABLE TRIGGER variants */ case AT_EnableAlwaysTrig: @@ -5643,7 +5650,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, ATExecDropOf(rel, lockmode); break; case AT_ReplicaIdentity: - ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def, lockmode); + ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def, + cmd->recurse, lockmode); break; case AT_EnableRowSecurity: ATExecSetRowSecurity(rel, true); @@ -18515,12 +18523,22 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, * ALTER TABLE <name> REPLICA IDENTITY ... */ static void -ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode) +ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, bool recurse, LOCKMODE lockmode) { Oid indexOid; Relation indexRel; int key; + if (recurse && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + PartitionDesc pd = RelationGetPartitionDesc(rel, true); + int nparts = pd->nparts; + + ereport(NOTICE, + (errmsg("REPLICA IDENTITY does not apply to partitions (%d affected)", + nparts))); + } + if (stmt->identity_type == REPLICA_IDENTITY_DEFAULT) { relation_mark_replica_identity(rel, stmt->identity_type, InvalidOid, true); -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: ALTER TABLE: warn when actions do not recurse to partitions @ 2026-01-13 03:42 David G. Johnston <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: David G. Johnston @ 2026-01-13 03:42 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; Postgres hackers <[email protected]> On Monday, January 12, 2026, Chao Li <[email protected]> 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. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: ALTER TABLE: warn when actions do not recurse to partitions @ 2026-01-13 04:02 Chao Li <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Chao Li @ 2026-01-13 04:02 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; Postgres hackers <[email protected]> On Tue, Jan 13, 2026 at 11:42 AM David G. Johnston < [email protected]> wrote: > On Monday, January 12, 2026, Chao Li <[email protected]> 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 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 affected > HINT: partitions may be modified individually using separate commands > ALTER 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 ``` Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ Attachments: [application/octet-stream] v3-0001-Add-notice-when-ALTER-TABLE-REPLICA-IDENTITY-does.patch (4.0K, 3-v3-0001-Add-notice-when-ALTER-TABLE-REPLICA-IDENTITY-does.patch) download | inline diff: From 6dd5cb5ff078e03944f180bac57e9432e880700e Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Mon, 12 Jan 2026 16:56:58 +0800 Subject: [PATCH v3] Add notice when ALTER TABLE REPLICA IDENTITY does not recurse ALTER TABLE ... REPLICA IDENTITY accepts a recursive form on partitioned tables, but the change is applied only to the partitioned table itself and does not propagate to child partitions. Previously this case was silently accepted, which could mislead users into assuming that the setting would recurse. Add a notice when recursion is requested on a partitioned table to make the behavior explicit and avoid confusion. This change does not alter semantics; it only provides user-visible feedback. Similar notices may be added for other ALTER TABLE sub-commands with non-recursive behavior in follow-up commits. Author: Chao Li <[email protected]> Reviewed-by: David G. Johnston <[email protected]> Reviewed-by: Greg Sabino Mullane <[email protected]> Discussion: https://postgr.es/m/CAEoWx2=SLga-xH09Cq_PAvsHhQHrBK+V0vF821JKgzS=Bm0haA@mail.gmail.com --- src/backend/commands/tablecmds.c | 33 +++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f976c0e5c7e..31ae0b9e827 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -693,7 +693,7 @@ static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid, DependencyType deptype); static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode); static void ATExecDropOf(Relation rel, LOCKMODE lockmode); -static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode); +static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, bool recurse, LOCKMODE lockmode); static void ATExecGenericOptions(Relation rel, List *options); static void ATExecSetRowSecurity(Relation rel, bool rls); static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls); @@ -5227,8 +5227,15 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_MATVIEW); pass = AT_PASS_MISC; - /* This command never recurses */ - /* No command-specific prep needed */ + + /* + * This command now doesn't recurse, but we want to notify user if + * recurse is set + * + * No command-specific prep needed + */ + if (recurse) + cmd->recurse = true; break; case AT_EnableTrig: /* ENABLE TRIGGER variants */ case AT_EnableAlwaysTrig: @@ -5643,7 +5650,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, ATExecDropOf(rel, lockmode); break; case AT_ReplicaIdentity: - ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def, lockmode); + ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def, + cmd->recurse, lockmode); break; case AT_EnableRowSecurity: ATExecSetRowSecurity(rel, true); @@ -18515,12 +18523,27 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, * ALTER TABLE <name> REPLICA IDENTITY ... */ static void -ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode) +ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, bool recurse, LOCKMODE lockmode) { Oid indexOid; Relation indexRel; int key; + if (recurse && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + PartitionDesc pd = RelationGetPartitionDesc(rel, true); + int nparts = pd->nparts; + + /* + * The recurse flag is set, but this command does not actually + * recurse. Emit a NOTICE to make this behavior explicit to the user. + */ + if (nparts > 0) + ereport(NOTICE, + (errmsg("present partitions not affected")), + errhint("partitions may be modified individually using separate commands")); + } + if (stmt->identity_type == REPLICA_IDENTITY_DEFAULT) { relation_mark_replica_identity(rel, stmt->identity_type, InvalidOid, true); -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-01-13 04:02 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-01-12 09:02 ALTER TABLE: warn when actions do not recurse to partitions Chao Li <[email protected]> 2026-01-12 14:40 ` David G. Johnston <[email protected]> 2026-01-13 03:18 ` Chao Li <[email protected]> 2026-01-13 03:42 ` David G. Johnston <[email protected]> 2026-01-13 04:02 ` Chao Li <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox