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
