On Sat, Nov 26, 2022 at 19:50 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > On Fri, Nov 25, 2022 at 5:30 PM Ashutosh Bapat > <ashutosh.bapat....@gmail.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
