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

Hi,

When doing some other related work, I noticed that when decoding the COMMIT
record via SnapBuildCommitTxn()-> SnapBuildDistributeNewCatalogSnapshot() we
will add a new snapshot to all transactions including the one being decoded(just committed one).

But since we've already done required modifications in the snapshot for the
current transaction being decoded(in SnapBuildCommitTxn()), so I think we can
avoid adding a snapshot for it again.

Here is the patch to improve this.
Thoughts ?

Best regards,
Hou zhijie

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(). Either I missed some code path or it's not
needed. Can you please enlighten me?

-- 
Best Wishes,
Ashutosh Bapat

On Fri, Nov 25, 2022 at 2:28 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> Hi,
>
> When doing some other related work, I noticed that when decoding the COMMIT
> record via SnapBuildCommitTxn()-> SnapBuildDistributeNewCatalogSnapshot() we
> will add a new snapshot to all transactions including the one being decoded(just committed one).
>
> But since we've already done required modifications in the snapshot for the
> current transaction being decoded(in SnapBuildCommitTxn()), so I think we can
> avoid adding a snapshot for it again.
>
> Here is the patch to improve this.
> Thoughts ?
>
> Best regards,
> Hou zhijie
>

On Fri, Nov 25, 2022 at 5:30 PM Ashutosh Bapat
<ashutosh.bapat.oss@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.

-- 
With Regards,
Amit Kapila.

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