On Sat, May 30, 2026 at 1:12 AM Dilip Kumar <[email protected]> wrote:
>
Few comments on 0001 and 0002
===========================
1.
+ Oid subconflictlogrelid; /* Relid of the conflict log table. */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
+ /*
+ * Strategy for logging replication conflicts:
+ * 'log' - server log only,
+ * 'table' - conflict log table only,
+ * 'all' - both log and table.
+ */
+ text subconflictlogdest BKI_FORCE_NOT_NULL;
'log' sounds redundant in the above two field names. I feel naming
them as subconflictrelid and subconflictdest should be sufficient.
2. If you agree with the above, then let's make similar changes at
other places in the patch. We can change
alter_sub_conflictlogdestination to alter_sub_conflict_destination.
Also, similar to AlterSubscription_refresh and
AlterSubscription_refresh_seq, we can name this new function as
AlterSubscription_conflict_dest.
3. Now, let's consider whether we should change the option name to
conflict_data_destination instead of conflict_log_destination? The
reason I am asking to consider this change is that one of the option
values is 'log', so it sounded a bit odd to name the option as
conflict_log_destination. If we change this then we can consider
changing the name of Enum ConflictLogDest as well.
Apart from above, I have made some changes in the attached. Kindly
review and see which all can be incorporated in the next version.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 46d27ed02a9..b7fcaaf6796 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -234,8 +234,7 @@ IsToastClass(Form_pg_class reltuple)
/*
* IsConflictLogTableClass
- * True iff Form_pg_class tuple represents a subscription-specific
- * Conflict Log Table.
+ * True iff pg_class tuple represents a Conflict Log Table.
*
* Does not perform any catalog accesses.
*/
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 3b6a163c184..96def3019b3 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -327,7 +327,7 @@ heap_create(const char *relname,
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to create
\"%s.%s\"",
- get_namespace_name(relnamespace),
relname),
+
get_namespace_name(relnamespace), relname),
errdetail("Conflict schema
modifications are currently disallowed.")));
}
diff --git a/src/backend/catalog/pg_publication.c
b/src/backend/catalog/pg_publication.c
index 93791210e35..2a27f6f4266 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -156,7 +156,8 @@ check_publication_add_schema(Oid schemaid)
* is really inadequate for that, since the information_schema could be
* dropped and reloaded and then it'll be considered publishable. The best
* long-term solution may be to add a "relispublishable" bool to pg_class,
- * and depend on that instead of OID checks.
+ * and depend on that instead of OID checks. IsConflictLogTableClass()
+ * excludes tables in conflict schema.
*/
static bool
is_publishable_class(Oid relid, Form_pg_class reltuple)
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 88f22bbb286..af2470f60c4 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -845,9 +845,8 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
CStringGetTextDatum(ConflictLogDestNames[opts.conflictlogdest]);
/*
- * If logging to a table is required, physically create it now. We
create
- * the conflict log table here so its relation OID can be stored when
- * inserting the pg_subscription tuple below.
+ * We create the conflict log table here, if required, so that its
relation
+ * OID can be stored when inserting the pg_subscription tuple below.
*/
if (CONFLICTS_LOGGED_TO_TABLE(opts.conflictlogdest))
logrelid = create_conflict_log_table(subid, stmt->subname,
owner);
@@ -877,8 +876,8 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
}
/*
- * If conflicts are logs to table establish an internal dependency
- * between the conflict log table and the subscription.
+ * Establish an internal dependency between conflict log table and
+ * subscription.
*
* We use DEPENDENCY_INTERNAL to signify that the table's lifecycle is
* strictly tied to the subscription, similar to how a TOAST table
relates
@@ -887,7 +886,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
* This ensures the conflict log table is automatically reaped during a
* DROP SUBSCRIPTION via performDeletion().
*/
- if (CONFLICTS_LOGGED_TO_TABLE(opts.conflictlogdest))
+ if (OidIsValid(logrelid))
{
ObjectAddress clt;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2dcf64fa53b..0a0ca2b850e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2457,8 +2457,8 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
* pg_largeobject and pg_largeobject_metadata to be truncated as part of
* pg_upgrade, because we need to change its relfilenode to match the
old
* cluster, and allowing a TRUNCATE command to be executed is the
easiest
- * way of doing that. We also allow TRUNCATE on the conflict log
tables,
- * to permit users to manually prune these logs to manage disk space.
+ * way of doing that. We also allow TRUNCATE on the conflict log tables,
+ * to permit users to manually prune conflict data to manage disk space.
*/
if (!allowSystemTableMods && IsSystemClass(relid, reltuple) &&
!IsConflictLogTableClass(reltuple)
