On Sat, May 30, 2026 at 1:42 PM Dilip Kumar <[email protected]> wrote:
>
> In latest patch set I have fixed Nisha's comments by creating a toast
> table, a separate patch
> (v43-0005-Create-conflict-log-table-after-inserting-subscr.patch)
> attached for creating conflict log table after inserting subscription
> row.
>
Thanks for the patches. Please find a couple of comments on v43:
1) A non-superuser cannot read the new columns 'subconflictlogrelid'
and 'subconflictlogdest' from pg_subscription.
A comment in system_views.sql say:
"-- All columns of pg_subscription except subconninfo are publicly readable."
I think we should grant public access to these new columns as well.
2) patch-002: conflict.c
-const ConflictLogColumnDef ConflictLogSchema[] = {
+StaticAssertDecl(lengthof(ConflictLogDestNames) == 3,
+ "ConflictLogDestNames length mismatch");
+
Should we use "CONFLICT_LOG_DEST_ALL + 1" instead of the hard-coded value "3"?
The attached diff fixes the above items, along with a few indentation
and whitespace. Please consider it if you agree with the changes.
--
Thanks,
Nisha
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 19c568f4705..16ab446a4a7 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3357,14 +3357,14 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode
mask,
classForm->relkind != RELKIND_VIEW)
{
/*
- * Deny anyone permission to update a system
catalog unless
- * pg_authid.rolsuper is set.
- *
- * As of 7.4 we have some updatable system
views; those
- * shouldn't be protected in this way. Assume
the view rules
- * can take care of themselves. ACL_USAGE is if
we ever have
- * system sequences.
- */
+ * Deny anyone permission to update a system
catalog unless
+ * pg_authid.rolsuper is set.
+ *
+ * As of 7.4 we have some updatable system
views; those
+ * shouldn't be protected in this way. Assume
the view rules
+ * can take care of themselves. ACL_USAGE is
if we ever have
+ * system sequences.
+ */
mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE
| ACL_TRUNCATE |
ACL_USAGE);
}
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 46d27ed02a9..9aabef3ccef 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -293,7 +293,6 @@ IsConflictNamespace(Oid namespaceId)
return namespaceId == PG_CONFLICT_NAMESPACE;
}
-
/*
* IsReservedName
* True iff name starts with the pg_ prefix.
diff --git a/src/backend/catalog/system_views.sql
b/src/backend/catalog/system_views.sql
index 73a1c1c4670..166329551da 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1527,7 +1527,8 @@ GRANT SELECT (oid, subdbid, subskiplsn, subname,
subowner, subenabled,
subbinary, substream, subtwophasestate, subdisableonerr,
subpasswordrequired, subrunasowner, subfailover,
subretaindeadtuples, submaxretention, subretentionactive,
- subserver, subslotname, subsynccommit, subpublications,
suborigin)
+ subserver, subconflictlogrelid, subconflictlogdest,
+ subslotname, subsynccommit, subpublications,
suborigin)
ON pg_subscription TO public;
CREATE VIEW pg_stat_subscription_stats AS
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 67311a65e29..83615277941 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -844,7 +844,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
CStringGetTextDatum(ConflictLogDestNames[opts.conflictlogdest]);
values[Anum_pg_subscription_subconflictlogrelid - 1] =
-
ObjectIdGetDatum(InvalidOid);
+ ObjectIdGetDatum(InvalidOid);
tup = heap_form_tuple(RelationGetDescr(rel), values, nulls);
@@ -869,9 +869,9 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
memset(replaces, false, sizeof(replaces));
values[Anum_pg_subscription_subconflictlogrelid - 1] =
-
ObjectIdGetDatum(logrelid);
+ ObjectIdGetDatum(logrelid);
replaces[Anum_pg_subscription_subconflictlogrelid - 1] =
- true;
+ true;
/* Make subscription tuple visible before updating it. */
CommandCounterIncrement();
@@ -1897,9 +1897,9 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
if (update_relid)
{
values[Anum_pg_subscription_subconflictlogrelid - 1] =
-
ObjectIdGetDatum(relid);
+
ObjectIdGetDatum(relid);
replaces[Anum_pg_subscription_subconflictlogrelid - 1] =
-
true;
+ true;
}
}
}
diff --git a/src/backend/replication/logical/conflict.c
b/src/backend/replication/logical/conflict.c
index 27830ccb0cc..22442cbbe4f 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -44,7 +44,7 @@ const char *const ConflictLogDestNames[] = {
[CONFLICT_LOG_DEST_ALL] = "all"
};
-StaticAssertDecl(lengthof(ConflictLogDestNames) == 3,
+StaticAssertDecl(lengthof(ConflictLogDestNames) == CONFLICT_LOG_DEST_ALL + 1,
"ConflictLogDestNames length mismatch");
/* Structure to hold metadata for one column of the conflict log table */
diff --git a/src/include/catalog/pg_subscription.h
b/src/include/catalog/pg_subscription.h
index cc31b4d00bc..89d2300abe1 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -95,7 +95,7 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW
Oid subserver BKI_LOOKUP_OPT(pg_foreign_server);
/* If connection uses
* server */
- Oid subconflictlogrelid; /* Relid of the conflict log table. */
+ Oid subconflictlogrelid; /* Relid of the conflict
log table. */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
/*
* Strategy for logging replication conflicts:
diff --git a/src/include/replication/worker_internal.h
b/src/include/replication/worker_internal.h
index 6a447da6510..6b6525dc2e2 100644
--- a/src/include/replication/worker_internal.h
+++ b/src/include/replication/worker_internal.h
@@ -260,7 +260,7 @@ extern PGDLLIMPORT List *table_states_not_ready;
extern XLogRecPtr remote_final_lsn;
extern TimestampTz remote_commit_ts;
-extern TransactionId remote_xid;
+extern TransactionId remote_xid;
extern void logicalrep_worker_attach(int slot);
extern LogicalRepWorker *logicalrep_worker_find(LogicalRepWorkerType wtype,
