Re: Conflict detection for update_deleted in logical replication

On Sat, Aug 30, 2025 at 10:17 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Fri, Aug 29, 2025 at 11:49 AM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > Here is the new version patch set which also addressed Shveta's comments[1].
> >
>
> Thanks for the patches here, I tested the v68-001 patch alone, please
> find review comments -
>

Further review comments for v68-0001 patch -

3) v68 seems to have introduced a bug:

@@ -1254,9 +1257,12 @@ ApplyLauncherMain(Datum main_arg)
/*
* Compute the minimum xmin required to protect dead tuples
* required for conflict detection among all running apply
- * workers that enables retain_dead_tuples.
+ * workers.
*/
- if (sub->retaindeadtuples && can_advance_xmin)
+ if (TransactionIdIsValid(MyReplicationSlot->data.xmin) &&
+ sub->retaindeadtuples &&
+ sub->retentionactive &&
+ can_update_xmin)
compute_min_nonremovable_xid(w, &xmin);

The new check "TransactionIdIsValid(MyReplicationSlot->data.xmin)" can
cause a segmentation fault in the launcher when a default subscription
is created (i.e., retain_dead_tuples=off) and the conflict slot does
not exist.
Perhaps it should first check "sub->retaindeadtuples" before accessing the slot.

--
Thanks,
Nisha