RE: Conflict detection for update_deleted in logical replication

On Mon, Jul 7, 2025 at 10:13 AM Zhijie Hou (Fujitsu) wrote:
>
> On Sun, Jul 6, 2025 at 10:51 PM Masahiko Sawada wrote:
> >
> > On Fri, Jul 4, 2025 at 8:18 PM Zhijie Hou (Fujitsu)
> > <houzj(dot)fnst(at)fujitsu(dot)com>
> > wrote:
> > >
> > > On Wed, Jul 2, 2025 at 3:28 PM Hou, Zhijie wrote:
> > > > Kindly use the latest patch set for performance testing.
> > >
> > > During testing, we observed a limitation in cascading logical
> > > replication setups, such as (A -> B -> C). When retain_conflict_info
> > > is enabled on Node C, it may not retain information necessary for
> > > conflict detection when applying changes originally replicated from
> > > Node A. This happens because Node C only waits for locally
> > > originated changes on Node B to be applied before advancing the
> > > non-removable
> > transaction ID.
> > >
> > > For example, Consider a logical replication setup as mentioned above
> > > : A -> B
> > -> C.
> > > - All three nodes have a table t1 with two tuples (1,1) (2,2).
> > > - Node B subscribed to all changes of t1 from Node A
> > > - Node-C subscribed to all changes from Node B.
> > > - Subscriptions use the default origin=ANY, as this is not a bidirectional
> > > setup.
> > >
> > > Now, consider two concurrent operations:
> > > - @9:00 Node A - UPDATE (1,1) -> (1,11)
> > >
> > > - @9:02 Node C - DELETE (1,1)
> > >
> > > Assume a slight delay at Node B before it applies the update from Node A.
> > >
> > > @9:03 Node C - advances the non-removable XID because it sees no
> > > concurrent transactions from Node B. It is unaware of Node A’s
> > > concurrent
> > update.
> > >
> > > @9:04 Node B - receives Node A's UPDATE and applies (1,1) -> (1,11)
> > > t1 has tuples : (1,11), (2,2)
> > >
> > > @9:05 Node C - receives the UPDATE (1,1) -> (1,11)
> > > - As conflict slot’s xmin is advanced, the deleted tuple may
> > > already
> > have
> > > been removed.
> > > - Conflict resolution fails to detect update_deleted and instead raises
> > > update_missing.
> > >
> > > Note that, as per decoding logic Node C sees the commit timestamp of
> > > the update as 9:00 (origin commit_ts from Node A), not 9:04 (commit
> > > time on Node B). In this case, since the UPDATE's timestamp is
> > > earlier than the DELETE, Node C should ideally detect an
> > > update_deleted conflict. However, it cannot, because it no longer retains
> the deleted tuple.
> > >
> > > Even if Node C attempts to retrieve the latest WAL position from
> > > Node A, Node C doesn't maintain any LSN which we could use to compare
> with it.
> > >
> > > This scenario is similar to another restriction in the patch where
> > > retain_conflict_info is not supported if the publisher is also a
> > > physical standby, as the required transaction information from the
> > > original primary is unavailable. Moreover, this limitation is
> > > relevant only when the subscription origin option is set to ANY, as
> > > only in that case changes from other origins can be replicated.
> > > Since retain_conflict_info is primarily useful for conflict
> > > detection in bidirectional clusters where the origin option is set
> > > to NONE, this limitation
> > appears acceptable.
> > >
> > > Given these findings, to help users avoid unintended configurations,
> > > we plan to issue a warning in scenarios where replicated changes may
> > > include origins other than the direct publisher, similar to the
> > > existing checks in the
> > > check_publications_origin() function.
> > >
> > > Here is the latest patch that implements the warning and documents
> > > this case. Only 0001 is modified for this.
> > >
> > > A big thanks to Nisha for invaluable assistance in identifying this
> > > case and preparing the analysis for it.
> >
> > I'm still reviewing the 0001 patch but let me share some comments and
> > questions I have so far:
>
> Thanks for the comments!
>
> >
> > ---
> > +/*
> > + * Determine the minimum non-removable transaction ID across all
> > +apply workers
> > + * for subscriptions that have retain_conflict_info enabled. Store
> > +the result
> > + * in *xmin.
> > + *
> > + * If the replication slot cannot be advanced during this cycle, due
> > +to either
> > + * a disabled subscription or an inactive worker, set
> > +*can_advance_xmin to
> > + * false.
> > + */
> > +static void
> > +compute_min_nonremovable_xid(LogicalRepWorker *worker,
> > + bool retain_conflict_info, TransactionId
> *xmin,
> > + bool *can_advance_xmin)
> >
> > I think this function is quite confusing for several reasons. For
> > instance, it's doing more things than described in the comments such
> > as trying to create the CONFLICT_DETECTION_SLOT if no worker is
> > passed. Also, one of the caller
> > describes:
> >
> > + /*
> > + * This is required to ensure that we don't advance the xmin
> > + * of CONFLICT_DETECTION_SLOT even if one of the
> > subscriptions
> > + * is not enabled. Otherwise, we won't be able to detect
> > + * conflicts reliably for such a subscription even though it
> > + * has set the retain_conflict_info option.
> > + */
> > + compute_min_nonremovable_xid(NULL,
> > sub->retainconflictinfo,
> > + &xmin,
> > + &can_advance_xmin);
> >
> > but it's unclear to me from the function name that it tries to create
> > the replication slot. Furthermore, in this path it doesn't actually
> > compute xmin. I guess we can try to create CONFLICT_DETECTION_SLOT in
> > the loop of "foreach(lc, sublist)" and set false to can_advance_xmin
> > if either the subscription is disabled or the worker is not running.
>
> I understand. The original code was similar to your suggestion, but we decided
> to encapsulate it within a separate function to maintain a clean and concise
> main loop. However, your suggestion also makes sense, so I will proceed with
> the change.

I have made this change in the 0002 patch for reference. What do you think ? If
there are no objections, I plan to merge it in the next version.

Other comment adjustments that do not alter the logic have already been
merged into the 0001 patch.

Best Regards,
Hou zj