Re: Proposal: Conflict log history table for Logical Replication

On Wed, Jun 3, 2026 at 10:11 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 2 Jun 2026 at 10:51, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > PFA, latest version of patch
> >
> > Changes:
> > 1. Includes Nisha's suggestion from v43_comments_nisha.patch
> > 2. Include Amit's suggestion from v43-0001_amit.1.patch
> > 3. Include Amit's suggestion: change GetLogDestination to
> > GetConflictLogDest(), drop_sub_conflict_log_table() to
> > drop_sub_conflict_log_table(), CONFLICTS_LOGGED_TO_FILE to
> > CONFLICTS_LOGGED_TO_LOG and renamed IsConflictNamespace() to
> > IsConflictLogTableNamespace()
> > 4. Include Vignesh's fix Parallel_apply_failure.patch
> > 5. Change conflict log table name back to conflict_log_table_<subid>
> > 6. Changed acl to exclude INSERT/UPDATE/USAGE as it was there in initial patch
> > 7. Merged 0002 to 0001 and 0004 to 0003 after review
> >
> > Open
> > 1. Vignesh please rebase upgrade and \dRs+ page
>
> Here is the rebased version of the upgrade and \dRs patch which is
> present in v45-0003 and v45-0004. There is no change in v45-0001 and
> v45-0002, they are the same patch as in [1].

Thanks for the updated patches, please find my comments on the v44
patches below. These appear to apply to v45-001 and v45-002 as well,
as both are unchanged.

1) The patches require a catversion bump. It may be worth adding it
now so it does not get missed later during commit.
~~~

2) As ProcessPendingConflictLogTuple() may perform a heap_insert, it
could fail for various reasons, such as table access errors, write
failures, WAL write failures, or ownership-related issues.
I looked into the error handling when CLT insert fails:

2a) When disable_on_error = false

/* Try to allocate a worker for the streaming transaction. */
@@ -5666,6 +5674,7 @@ start_apply(XLogRecPtr origin_startpos)
*/
AbortOutOfAnyTransaction();
pgstat_report_subscription_error(MySubscription->oid);
+ ProcessPendingConflictLogTuple();

PG_RE_THROW();
}

If ProcessPendingConflictLogTuple() fails, PG_RE_THROW() is never
reached, so the original conflict error is lost and only the secondary
CLT-related error is reported, e.g.:

ERROR: could not open relation with OID 16421
LOG: background worker "logical replication apply worker" exited
with exit code 1

If the failure persists, the apply worker will keep retrying and users
may never see the actual conflict error. The same concern applies to
the parallel worker case too.

Is there a clean way to ensure the original apply/conflict error is
still logged even if ProcessPendingConflictLogTuple() itself fails?
~~

2b) When disable_on_error = true i.e. DisableSubscriptionAndExit() path

@@ -6040,6 +6049,8 @@ DisableSubscriptionAndExit(void)
*/
pgstat_report_subscription_error(MyLogicalRepWorker->subid);

+ ProcessPendingConflictLogTuple();
+
/* Disable the subscription */
StartTransactionCommand();

This introduces a potential failure point before the subscription is
disabled. If ProcessPendingConflictLogTuple() fails, the subscription
may never be disabled and the worker could repeatedly hit the same
error.

Would it be safer to move ProcessPendingConflictLogTuple() until after
the subscription has been disabled, so that disabling the subscription
is not dependent on successful CLT processing?
~~

2c) When "conflict_log_destination = table" the log reports:

ERROR: conflict detected on relation "public.t1": conflict=insert_exists
DETAIL: Conflict details are logged to the conflict log table:
pg_conflict_log_16403

However, the subsequent CLT write can still fail:
...
ERROR: could not open relation with OID 16426
LOG: background worker "logical replication apply worker" exited
with exit code 1

Since the CLT write has not yet occurred when this message is emitted,
would it make sense to change it to:
"Conflict details will be logged to the conflict log table: ..."

to avoid implying that the logging has already succeeded?
~~~

3) The v45-002 commit message still refers to the old conflict log
table name, pg_conflict.pg_conflict_log_for_subid_16396. It should be
updated to pg_conflict_log_16396.
~~~

4) 035_conflicts.pl: I see that v45-002 has corrected the CLT name in
below test -
+# Verify the contents of the Conflict Log Table (CLT)
+# This section ensures that the CLT contains the expected
+# type and specific key data.
+my $subid = $node_subscriber->safe_psql('postgres',
+ "SELECT oid FROM pg_subscription WHERE subname = 'sub_tab';");
+my $clt = "pg_conflict.pg_conflict_log_$subid";
+
+# Wait for the conflict to be logged in the CLT
+my $log_check = $node_subscriber->poll_query_until(
+ 'postgres',
+ "SELECT count(*) > 0 FROM $clt;"
+);

I wonder why this wasn't caught in the v44 tests. It seems the check
below is silently broken because poll_query_until() keeps retrying
until timeout when the table does not exist.
Should we replace it with an ok() test instead? something like -

# Wait for the conflict to be logged in the CLT
ok($node_subscriber->poll_query_until(
'postgres',
"SELECT count(*) > 0 FROM $clt;"),
'conflict appeared in CLT after insert');
~~~

--
Thanks,
Nisha