On Monday, February 24, 2025 5:50 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Dec 11, 2024 at 12:37 PM Masahiko Sawada > <sawada.m...@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
0001-add-comments-for-txns-without-base-snapshot.patch
Description: 0001-add-comments-for-txns-without-base-snapshot.patch
