Обсуждение: minor improvement in snapbuild: use existing interface and remove fake code
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
Вложения
Hi，
ocean_li_996 <ocean_li_996@163.com> 于2025年11月18日周二 08:48写道：
Hi hackers,While reviewing thesnapbuildimplementation, I noticed several small changes that could improve code clarity, correctness, and reuse.I have prepared a patch with these modifications (attached):1. Removed theAssertinSnapBuildGetOrBuildSnapshot(). When called fromlogicalmsg_decode(), thisAssertmay not hold, which looks like a bug.2. InSnapBuildProcessChange(), now reuseSnapBuildGetOrBuildSnapshot()to obtain the snapshot.
3. Removed handling ofSNAPBUILD_STARTandSNAPBUILD_BUILDING_SNAPSHOTstates inSnapBuildCommitTxn(). When entering this function,builder->stateis alwaysSNAPBUILD_FULL_SNAPSHOTorSNAPBUILD_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);
- 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
Re: minor improvement in snapbuild: use existing interface and remove fake code
От
ocean_li_996
Дата:
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);
> - 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 snapshotis 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
Re: minor improvement in snapbuild: use existing interface and removefake code
От
ocean_li_996
Дата:
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.
Regards,
Haiyang Li