On Wed, Jun 10, 2026 at 3:07 PM shveta malik <[email protected]> wrote: > > On Wed, Jun 10, 2026 at 12:48 PM Amit Kapila <[email protected]> wrote: > > > > On Mon, Jun 8, 2026 at 7:01 PM Dilip Kumar <[email protected]> wrote: > > > > > > On Mon, Jun 8, 2026 at 11:43 AM shveta malik <[email protected]> > > > wrote: > > > > > > > > On Fri, Jun 5, 2026 at 4:22 PM Dilip Kumar <[email protected]> > > > > wrote: > > > > > > After rethinking my previous stance on blocking these operations, let > > > me clarify the core principle I think we should follow for CLTs. I am > > > completely open to feedback on this approach: > > > > > > 1. Block Direct Mutations: We should block any operation that directly > > > modifies the CLT or its underlying data (e.g., DROP TABLE, ALTER > > > TABLE, INSERT, UPDATE), which impact the operation on CLT or update > > > the CLT data. > > > 2. Don't block Indirect/Edge-Case Operations: We should not write > > > custom code just to block edge cases that don't directly modify CLT > > > data or impact the operations on CLT. > > > > Unlike system catalogs which are allowed to be modified with the > > allow_system_table_mods GUC, the TOAST model seems more appropriate > > for conflict log tables where the external modifications are blocked, > > due to following reasons: > > 1. The apply worker assumes a fixed schema. ConflictLogSchema[] in > > conflict.c is a hardcoded array of column definitions used to build > > tuples at runtime. If allow_system_table_mods=on lets someone add a > > column or attach a trigger that errors out, the apply worker's > > insertion path breaks with error or crash. There is no recovery path — > > it's not like a user catalog where a DBA might legitimately need to > > patch something in an emergency. > > 2. allow_system_table_mods allowed to modify catalog tables and that > > seems to be designed for bootstrap, not conflict log tables. The GUC > > exists so initdb can modify system catalogs they own. Conflict log > > tables are subscription-specific runtime objects. No legitimate > > internal tooling scenario requires adding triggers or rules to them. > > > > There could be other cases for allow_system_table_mods to allow > > modifying system catalog for emergency repair of catalog table or some > > upgrade scripts used by extensions to allow adding additional entries > > in catalog tables like pg_proc but I don't see such maintenance > > required for conflict log tables. So, it seems better to block all the > > additional operations discussed. > > > > +1. irrespective of GUC value, we can block such operations. Some > operations are allowed even with this GUC set to false (as stated in > my previous emails).
Yeah that makes sense. Here is the POC patch to block such operations for the conflict log table. Please let me know your thoughts -- Regards, Dilip Kumar Google
From 33bcdda8b0b84bab60282403f6eb3c2c930c7e53 Mon Sep 17 00:00:00 2001 From: Dilip Kumar <[email protected]> Date: Wed, 10 Jun 2026 16:52:46 +0530 Subject: [PATCH v1] Report error for ddls on conflict log tables --- src/backend/commands/lockcmds.c | 19 ++++++++ src/backend/commands/policy.c | 12 +++++ src/backend/commands/statscmds.c | 12 +++++ src/backend/commands/tablecmds.c | 73 +++++++++++++++++++++++++++++ src/backend/commands/trigger.c | 25 ++++++++++ src/backend/rewrite/rewriteDefine.c | 22 +++++++++ 6 files changed, 163 insertions(+) diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index f66b8f17b9b..de56dcc6d48 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -16,6 +16,7 @@ #include "access/table.h" #include "access/xact.h" +#include "catalog/catalog.h" #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" @@ -92,6 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, rv->relname), errdetail_relkind_not_supported(relkind))); + /* + * Conflict log tables are managed by the system for logical replication + * and should not be locked explicitly. + */ + if (IsConflictLogTableNamespace(get_rel_namespace(relid))) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + rv->relname), + errdetail("Conflict log tables are managed by the system for logical replication."))); + /* * Make note if a temporary relation has been accessed in this * transaction. @@ -198,6 +210,13 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context) relkind != RELKIND_VIEW) continue; + /* + * Conflict log tables are managed by the system for logical + * replication and should not be locked explicitly. + */ + if (IsConflictLogTableNamespace(get_rel_namespace(relid))) + continue; + /* * We might be dealing with a self-referential view. If so, we * can just stop recursing, since we already locked it. diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index 21b8eebe32d..9919a7e4124 100644 --- a/src/backend/commands/policy.c +++ b/src/backend/commands/policy.c @@ -79,6 +79,18 @@ RangeVarCallbackForPolicy(const RangeVar *rv, Oid relid, Oid oldrelid, if (!object_ownercheck(RelationRelationId, relid, GetUserId())) aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname); + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not be modified directly, as it could disrupt + * conflict logging. + */ + if (IsConflictLogTableClass(classform)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + rv->relname), + errdetail("Conflict log tables are managed by the system for logical replication."))); + /* No system table modifications unless explicitly allowed. */ if (!allowSystemTableMods && IsSystemClass(relid, classform)) ereport(ERROR, diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c index b354723be44..18143a9cb76 100644 --- a/src/backend/commands/statscmds.c +++ b/src/backend/commands/statscmds.c @@ -147,6 +147,18 @@ CreateStatistics(CreateStatsStmt *stmt, bool check_rights) aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(rel->rd_rel->relkind), RelationGetRelationName(rel)); + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not have extended statistics, as it + * could disrupt conflict logging. + */ + if (IsConflictLogTableClass(rel->rd_rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + RelationGetRelationName(rel)), + errdetail("Conflict log tables are managed by the system for logical replication."))); + /* Creating statistics on system catalogs is not allowed */ if (!allowSystemTableMods && IsSystemRelation(rel)) ereport(ERROR, diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 6ffd21c68c4..d8754aa93db 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -2750,6 +2750,18 @@ MergeAttributes(List *columns, const List *supers, char relpersistence, errmsg("cannot inherit from partition \"%s\"", RelationGetRelationName(relation)))); + /* + * Conflict log tables are managed by the system for logical + * replication and should not be used as parent tables, as + * inheritance could interfere with the logging behavior. + */ + if (IsConflictLogTableNamespace(relation->rd_rel->relnamespace)) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot inherit from conflict log table \"%s\"", + RelationGetRelationName(relation)), + errdetail("Conflict log tables are managed by the system for logical replication."))); + if (relation->rd_rel->relkind != RELKIND_RELATION && relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE && relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) @@ -3887,6 +3899,19 @@ renameatt_check(Oid myrelid, Form_pg_class classform, bool recursing) if (!object_ownercheck(RelationRelationId, myrelid, GetUserId())) aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(myrelid)), NameStr(classform->relname)); + + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not be modified directly, as it could disrupt + * conflict logging. + */ + if (IsConflictLogTableClass(classform)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + NameStr(classform->relname)), + errdetail("Conflict log tables are managed by the system for logical replication."))); + if (!allowSystemTableMods && IsSystemClass(myrelid, classform)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -6889,6 +6914,18 @@ ATSimplePermissions(AlterTableType cmdtype, Relation rel, int allowed_targets) aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(rel->rd_rel->relkind), RelationGetRelationName(rel)); + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not be altered directly, as it could disrupt conflict + * logging. + */ + if (IsConflictLogTableClass(rel->rd_rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + RelationGetRelationName(rel)), + errdetail("Conflict log tables are managed by the system for logical replication."))); + if (!allowSystemTableMods && IsSystemRelation(rel)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -10198,6 +10235,18 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, errmsg("referenced relation \"%s\" is not a table", RelationGetRelationName(pkrel)))); + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not be referenced by foreign keys, as it could + * disrupt conflict logging. + */ + if (IsConflictLogTableClass(pkrel->rd_rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + RelationGetRelationName(pkrel)), + errdetail("Conflict log tables are managed by the system for logical replication."))); + if (!allowSystemTableMods && IsSystemRelation(pkrel)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -19818,6 +19867,18 @@ RangeVarCallbackOwnsRelation(const RangeVar *relation, aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relId)), relation->relname); + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not be modified directly, as it could disrupt + * conflict logging. + */ + if (IsConflictLogTableClass((Form_pg_class) GETSTRUCT(tuple))) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + relation->relname), + errdetail("Conflict log tables are managed by the system for logical replication."))); + if (!allowSystemTableMods && IsSystemClass(relId, (Form_pg_class) GETSTRUCT(tuple))) ereport(ERROR, @@ -19853,6 +19914,18 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid, if (!object_ownercheck(RelationRelationId, relid, GetUserId())) aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname); + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not be altered directly, as it could disrupt conflict + * logging. + */ + if (IsConflictLogTableClass(classform)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + rv->relname), + errdetail("Conflict log tables are managed by the system for logical replication."))); + /* No system table modifications unless explicitly allowed. */ if (!allowSystemTableMods && IsSystemClass(relid, classform)) ereport(ERROR, diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index b87b4b40d07..5fce1cf4d48 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -314,6 +314,18 @@ CreateTriggerFiringOn(const CreateTrigStmt *stmt, const char *queryString, RelationGetRelationName(rel)), errdetail_relkind_not_supported(rel->rd_rel->relkind))); + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not have triggers, as it could disrupt conflict + * logging. + */ + if (IsConflictLogTableClass(rel->rd_rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + RelationGetRelationName(rel)), + errdetail("Conflict log tables are managed by the system for logical replication."))); + if (!allowSystemTableMods && IsSystemRelation(rel)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -1443,6 +1455,19 @@ RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid, /* you must own the table to rename one of its triggers */ if (!object_ownercheck(RelationRelationId, relid, GetUserId())) aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname); + + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not have triggers, as it could disrupt conflict + * logging. + */ + if (IsConflictLogTableClass(form)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + rv->relname), + errdetail("Conflict log tables are managed by the system for logical replication."))); + if (!allowSystemTableMods && IsSystemClass(relid, form)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c index 6a223fbeaa4..343d74cc72e 100644 --- a/src/backend/rewrite/rewriteDefine.c +++ b/src/backend/rewrite/rewriteDefine.c @@ -262,6 +262,17 @@ DefineQueryRewrite(const char *rulename, RelationGetRelationName(event_relation)), errdetail_relkind_not_supported(event_relation->rd_rel->relkind))); + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not have rules, as it could disrupt conflict logging. + */ + if (IsConflictLogTableClass(event_relation->rd_rel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + RelationGetRelationName(event_relation)), + errdetail("Conflict log tables are managed by the system for logical replication."))); + if (!allowSystemTableMods && IsSystemRelation(event_relation)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -772,6 +783,17 @@ RangeVarCallbackForRenameRule(const RangeVar *rv, Oid relid, Oid oldrelid, errmsg("relation \"%s\" cannot have rules", rv->relname), errdetail_relkind_not_supported(form->relkind))); + /* + * Conflict log tables are used internally for logical replication conflict + * logging and should not have rules, as it could disrupt conflict logging. + */ + if (IsConflictLogTableClass(form)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a conflict log table", + rv->relname), + errdetail("Conflict log tables are managed by the system for logical replication."))); + if (!allowSystemTableMods && IsSystemClass(relid, form)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), -- 2.49.0
