Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

On Sat, 20 Dec 2025 at 16:51, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> I have updated the patch and here are changes done
> 1. Splitted into 2 patches, 0001- for catalog related changes
> 0002-inserting conflict into the conflict table, Vignesh need to
> rebase the dump and upgrade related patch on this latest changes
> 2. Subscription option changed to conflict_log_destination=(log/table/all/'')
> 3. For internal processing we will use ConflictLogDest enum whereas
> for taking input or storing into catalog we will use string [1].
> 4. As suggested by Sawada San, if conflict_log_destination is 'table'
> we log the information about conflict but don't log the tuple
> details[3]

Few comments:
1) when a conflict_log_destination is specified as log:
create subscription sub1 connection 'dbname=postgres host=localhost
port=5432' publication pub1 with ( conflict_log_destination='log');
postgres=# select subname, subconflictlogrelid,sublogdestination from
pg_subscription where subname = 'sub4';
 subname | subconflictlogrelid | sublogdestination
---------+---------------------+-------------------
 sub4    |                   0 | log
(1 row)

Currently it displays as 0, instead we can show as NULL in this case

2) can we include displaying of conflict log table also  in describe
subscriptions:
+               /* Conflict log destination is supported in v19 and higher */
+               if (pset.sversion >= 190000)
+               {
+                       appendPQExpBuffer(&buf,
+                                                         ",
sublogdestination AS \"%s\"\n",
+
gettext_noop("Conflict log destination"));
+               }

3) Can we include pg_ in the conflict table to indicate it is an
internally created table:
+/*
+ * Format the standardized internal conflict log table name for a subscription
+ *
+ * Use the OID to prevent collisions during rename operations.
+ */
+void
+GetConflictLogTableName(char *dest, Oid subid)
+{
+       snprintf(dest, NAMEDATALEN, "conflict_log_table_%u", subid);
+}

4) Can the table be deleted now with the dependency associated between
the table and the subscription?
+       conflictlogrel = table_open(conflictlogrelid, RowExclusiveLock);
+
+       /* Conflict log table is dropped or not accessible. */
+       if (conflictlogrel == NULL)
+               ereport(WARNING,
+                               (errcode(ERRCODE_UNDEFINED_TABLE),
+                                errmsg("conflict log table with OID
%u does not exist",
+                                               conflictlogrelid)));
+
+       return conflictlogrel;

5) Should this code be changed to just prepare the conflict log tuple
here, validation and insertion can happen at start_apply if elevel >=
ERROR to avoid ValidateConflictLogTable here as well as at start_apply
function:
+               if (ValidateConflictLogTable(conflictlogrel))
+               {
+                       /*
+                        * Prepare the conflict log tuple. If the
error level is below
+                        * ERROR, insert it immediately. Otherwise,
defer the insertion to
+                        * a new transaction after the current one
aborts, ensuring the
+                        * insertion of the log tuple is not rolled back.
+                        */
+                       prepare_conflict_log_tuple(estate,
+
    relinfo->ri_RelationDesc,
+
    conflictlogrel,
+                                                                          type,
+
    searchslot,
+
    conflicttuples,
+
    remoteslot);
+                       if (elevel < ERROR)
+                               InsertConflictLogTuple(conflictlogrel);
+               }
+               else
+                       ereport(WARNING,
+
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                                       errmsg("conflict log table
\"%s.%s\" structure changed, skipping insertion",
+
get_namespace_name(RelationGetNamespace(conflictlogrel)),
+
RelationGetRelationName(conflictlogrel)));

to:
prepare_conflict_log_tuple(estate,
   relinfo->ri_RelationDesc,
   conflictlogrel,
   type,
   searchslot,
   conflicttuples,
   remoteslot);
if (elevel < ERROR)
{
if (ValidateConflictLogTable(conflictlogrel))
InsertConflictLogTuple(conflictlogrel);
else
ereport(WARNING,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("conflict log table \"%s.%s\" structure changed, skipping insertion",
get_namespace_name(RelationGetNamespace(conflictlogrel)),
RelationGetRelationName(conflictlogrel)));
}

Regards,
Vignesh