Thread

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

   ocean_li_996 <ocean_li_996@163.com> — 2025-11-17T17:18:12Z

   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.
   
   
   
   
   Looking forward to your comments.
   
   Best regards,
   
   Haiyang Li

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

   Yuefei Shi <shiyuefei1004@gmail.com> — 2025-11-18T01:13:12Z

   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
   

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

   ocean_li_996 <ocean_li_996@163.com> — 2025-11-18T02:05:18Z

   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
   
   
   
   

4. Re: minor improvement in snapbuild: use existing interface and removefake code

   cca5507 <cca5507@qq.com> — 2025-11-18T02:47:20Z

   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

5. Re: minor improvement in snapbuild: use existing interface and removefake code

   ocean_li_996 <ocean_li_996@163.com> — 2025-11-18T16:26:19Z

   
   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], 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].
   
   
   As for the comment about this path, I can revert it currently.
   Please find the updated patch attached.
   
   
   [1] https://www.postgresql.org/message-id/2b9e5ac8.136f.19a8f7297ee.Coremail.ocean_li_996%40163.com
   
   
   Regards,
   Haiyang Li