minor improvement in snapbuild: use existing interface and remove fake code

Hi，
ocean_li_996 <ocean_li_996@163.com> 于2025年11月18日周二 08:48写道：

Hi hackers,
While reviewing the snapbuild implementation, I noticed several small
changes that could improve code clarity, correctness, and reuse.
I have prepared a patch with these modifications (attached):

1. Removed the Assert in SnapBuildGetOrBuildSnapshot(). When called from
logicalmsg_decode(), this Assert may not hold, which looks like a bug.

2. In SnapBuildProcessChange(), now reuse SnapBuildGetOrBuildSnapshot() to
obtain the snapshot.

3. Removed handling of SNAPBUILD_START and SNAPBUILD_BUILDING_SNAPSHOT states
in SnapBuildCommitTxn(). When entering this function,
builder->state is always SNAPBUILD_FULL_SNAPSHOT or SNAPBUILD_CONSISTENT.

- /* only build a new snapshot if we don't have a prebuilt one */
- if (builder->snapshot == NULL)
- {
- builder->snapshot = SnapBuildBuildSnapshot(builder);
- /* increase refcount for the snapshot builder */
- SnapBuildSnapIncRefcount(builder->snapshot);
- }
+ Snapshot snapshot =  SnapBuildGetOrBuildSnapshot(builder);

  /*
  * Increase refcount for the transaction we're handing the snapshot
  * out to.
  */
- SnapBuildSnapIncRefcount(builder->snapshot);
+ SnapBuildSnapIncRefcount(snapshot);
  ReorderBufferSetBaseSnapshot(builder->reorder, xid, lsn,
- builder->snapshot);
+ snapshot);

The snapshot created above is a temporary variable and is not recorded into
builder->snapshot, which may cause a leak.

Best regards,
Yuefei Shi

Hi yuefei,

Thanks for your review.

At 2025-11-18 09:13:12, "Yuefei Shi" <shiyuefei1004@gmail.com> wrote:

- /* only build a new snapshot if we don't have a prebuilt one */
- if (builder->snapshot == NULL)
- {
- builder->snapshot = SnapBuildBuildSnapshot(builder);
- /* increase refcount for the snapshot builder */
- SnapBuildSnapIncRefcount(builder->snapshot);
- }
+ Snapshot snapshot =  SnapBuildGetOrBuildSnapshot(builder);

/*
* Increase refcount for the transaction we're handing the snapshot
* out to.
*/
- SnapBuildSnapIncRefcount(builder->snapshot);
+ SnapBuildSnapIncRefcount(snapshot);
ReorderBufferSetBaseSnapshot(builder->reorder, xid, lsn,
- builder->snapshot);
+ snapshot);

The snapshot created above is a temporary variable and is not recorded into builder->snapshot, which may cause a leak.

AFAICT, the snapshot is created and recorded into builder->snapshot in SnapBuildGetOrBuildSnapshot() if needed. And the local variable snapshot
is just a poniter which actually pointing to builder->snapshot. Did I missed something?

Regards,
Haiyang Li

Hi,

Some comments:

1===

Is there a test case can reproduce the assert fail in SnapBuildGetOrBuildSnapshot()?

2===

We skip xact_decode() when SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT, but it's a bug, so your NO.3 modification might be wrong.

See threads in this for details: https://commitfest.postgresql.org/patch/5029/

--
Regards,
ChangAo Chen

Hi ChangAo,

Thanks for your comments.

At 2025-11-18 10:47:20, "cca5507" <cca5507@qq.com> wrote:

Is there a test case can reproduce the assert fail in SnapBuildGetOrBuildSnapshot()?

After exploring the logicalmsg_decode()，I think the Assert in SnapBuildGetOrBuildSnapshot() will not fail.
But the assert in SnapBuildGetOrBuildSnapshot() can be removed if we want to reuse it.

We skip xact_decode() when SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT, but it's a bug, so your NO.3 modification might be wrong.

See threads in this for details: https://commitfest.postgresql.org/patch/5029/

Wow, what a nice surprise! A few days ago, I reported a bug in [1]/messages/by-id/2b9e5ac8.136f.19a8f7297ee.Coremail.ocean_li_996@163.com, which I believe is the same issue you have described here.
Unfortunately, I was not able to provide a simple reproduction case at that time — but now we seem to have one :).
Regarding the fix for that issue: I will review the patch you have provided. My current idea is to update the snapshot using
builder->xmin once we reach the SNAPBUILD_CONSISTENT state. If you have any thoughts, please feel free to join the
discussion in [1]/messages/by-id/2b9e5ac8.136f.19a8f7297ee.Coremail.ocean_li_996@163.com.

As for the comment about this path, I can revert it currently.
Please find the updated patch attached.

[1]: /messages/by-id/2b9e5ac8.136f.19a8f7297ee.Coremail.ocean_li_996@163.com

Regards,
Haiyang Li