Re: Proposal: Conflict log history table for Logical Replication

On Sat, 20 Dec 2025 at 16:51, Dilip Kumar <dilipbalaut(at)gmail(dot)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