RE: long-standing data loss bug in initial sync of logical replication

On Monday, February 24, 2025 5:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Wed, Dec 11, 2024 at 12:37 PM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> >
> > I confirmed that the proposed patch fixes these issues. I have one
> > question about the patch:
> >
> > In the main loop in SnapBuildDistributeSnapshotAndInval(), we have the
> > following code:
> >
> >        /*
> >         * If we don't have a base snapshot yet, there are no changes in this
> >         * transaction which in turn implies we don't yet need a snapshot at
> >         * all. We'll add a snapshot when the first change gets queued.
> >         *
> >         * NB: This works correctly even for subtransactions because
> >         * ReorderBufferAssignChild() takes care to transfer the base
> snapshot
> >         * to the top-level transaction, and while iterating the changequeue
> >         * we'll get the change from the subtxn.
> >         */
> >        if (!ReorderBufferXidHasBaseSnapshot(builder->reorder, txn->xid))
> >            continue;
> >
> > Is there any case where we need to distribute inval messages to
> > transactions that don't have the base snapshot yet but eventually need
> > the inval messages?
> >
> 
> Good point. It is mentioned that for snapshots: "We'll add a snapshot
> when the first change gets queued.". I think we achieve this via
> builder->committed.xip array such that when we set a base snapshot for
> a transaction, we use that array to form a snapshot. However, I don't
> see any such consideration for invalidations. Now, we could either
> always add invalidations to xacts that don't have base_snapshot yet or
> have a mechanism similar committed.xid array. But it is better to
> first reproduce the problem.

I think distributing invalidations to a transaction that has not yet built a
base snapshot is un-necessary. This is because, during the process of building
its base snapshot, such a transaction will have already recorded the XID of the
transaction that altered the publication information into its array of
committed XIDs. Consequently, it will reflect the latest changes in the catalog
from the beginning. In the context of logical decoding, this scenario is
analogous to decoding a new transaction initiated after the catalog-change
transaction has been committed.

The original issue arises because the catalog cache was constructed using an
outdated snapshot that could not reflect the latest catalog changes. However,
this is not a problem in cases without a base snapshot. Since the existing
catalog cache should have been invalidated upon decoding the committed
catalog-change transaction, the subsequent transactions will construct a new
cache with the latest snapshot.

I also considered the scenario where only a sub-transaction has a base snapshot
that has not yet been transferred to its top-level transaction. However, I
think this is not problematic because a sub-transaction transfers its snapshot
immediately upon building it (see ReorderBufferSetBaseSnapshot). The only
exception is if the sub-transaction is independent (i.e., not yet associated
with its top-level transaction). In such a case, the sub-transaction is treated
as a top-level transaction, and invalidations will be distributed to this
sub-transaction after applying the patch which is sufficient to resolve the
issue.

Considering the complexity of this topic, I think it would be better to add some
comments like the attachment

Best Regards,
Hou zj