RE: Avoid distributing new catalog snapshot again for the transaction being decoded.

On Sat, Nov 26, 2022 at 19:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Nov 25, 2022 at 5:30 PM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> >
> > Hi Hou,
> > Thanks for the patch. With a simple condition, we can eliminate the
> > need to queueing snapshot change in the current transaction and then
> > applying it. Saves some memory and computation. This looks useful.
> >
> > When the queue snapshot change is processed in
> > ReorderBufferProcessTXN(), we call SetupHistoricSnapshot(). But I
> > didn't find a path through which SetupHistoricSnapshot() is called
> > from SnapBuildCommitTxn().
> >
>
> It will be called after SnapBuildCommitTxn() via ReorderBufferCommit()-
> >ReorderBufferReplay()->ReorderBufferProcessTXN().
> But, I think what I don't see is how the snapshot we build in
> SnapBuildCommitTxn() will be assigned as a historic snapshot. We assign
> base_snapshot as a historic snapshot and the new snapshot we build in
> SnapBuildCommitTxn() is assigned as base_snapshot only if the same is not set
> previously. I might be missing something but if that is true then I don't think the
> patch is correct, OTOH I would expect some existing tests to fail if this change is
> incorrect.

Hi,

I also think that the snapshot we build in SnapBuildCommitTxn() will not be
assigned as a historic snapshot. But I think that when the commandID message is
processed in the function ReorderBufferProcessTXN, the snapshot of the current
transaction will be updated. And I also did some tests and found no problems.

Here is my detailed analysis:

I think that when a transaction internally modifies the catalog, a record of
XLOG_HEAP2_NEW_CID will be inserted into the WAL (see function
log_heap_new_cid). Then during logical decoding, this record will be decoded
into a change of type REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID (see function
ReorderBufferAddNewCommandId). And I think the function ReorderBufferProcessTXN
would update the HistoricSnapshot (member subxip and member curcid of snapshot)
when processing this change.

And here is only one small comment:

+ * We've already done required modifications in snapshot for the
+ * transaction that just committed, so there's no need to add a new
+ * snapshot for the transaction again.
+ */
+ if (xid == txn->xid)
+ continue;

This comment seems a bit inaccurate. How about the comment below?
```
We don't need to add a snapshot to the transaction that just committed as it
will be able to see the new catalog contents after processing the new commandID
in ReorderBufferProcessTXN.
```

Regards,
Wang wei