Re: Conflict detection for update_deleted in logical replication

On Mon, Jan 6, 2025 at 3:22 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Friday, January 3, 2025 2:36 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> Hi,
>
> >
> > I have one comment on the 0001 patch:
>
> Thanks for the comments!
>
> >
> > + /*
> > + * The changes made by this and later transactions are still
> > non-removable
> > + * to allow for the detection of update_deleted conflicts when
> > applying
> > + * changes in this logical replication worker.
> > + *
> > + * Note that this info cannot directly protect dead tuples from being
> > + * prematurely frozen or removed. The logical replication launcher
> > + * asynchronously collects this info to determine whether to advance
> > the
> > + * xmin value of the replication slot.
> > + *
> > + * Therefore, FullTransactionId that includes both the
> > transaction ID and
> > + * its epoch is used here instead of a single Transaction ID. This is
> > + * critical because without considering the epoch, the transaction ID
> > + * alone may appear as if it is in the future due to transaction ID
> > + * wraparound.
> > + */
> > + FullTransactionId oldest_nonremovable_xid;
> >
> > The last paragraph of the comment mentions that we need to use
> > FullTransactionId to properly compare XIDs even after the XID wraparound
> > happens. But once we set the oldest-nonremovable-xid it prevents XIDs from
> > being wraparound, no? I mean that workers'
> > oldest-nonremovable-xid values and slot's non-removal-xid (i.e., its
> > xmin) are never away from more than 2^31 XIDs.
>
> I think the issue is that the launcher may create the replication slot after
> the apply worker has already set the 'oldest_nonremovable_xid' because the
> launcher are doing that asynchronously. So, Before the slot is created, there's
> a window where transaction IDs might wrap around. If initially the apply worker
> has computed a candidate_xid (755) and the xid wraparound before the launcher
> creates the slot, causing the new current xid to be (740), then the old
> candidate_xid(755) looks like a xid in the future, and the launcher could
> advance the xmin to 755 which cause the dead tuples to be removed prematurely.
> (We are trying to reproduce this to ensure that it's a real issue and will
> share after finishing)

The slot's first xmin is calculated by
GetOldestSafeDecodingTransactionId(false). The initial computed
cancidate_xid could be newer than this xid?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com