Re: minor improvement in snapbuild: use existing interface and remove fake code
Yuefei Shi <shiyuefei1004@gmail.com>
From: Yuefei Shi <shiyuefei1004@gmail.com>
To: ocean_li_996 <ocean_li_996@163.com>
Cc: pgsql-hackers@lists.postgresql.org, "mohen.lhy@alibaba-inc.com" <mohen.lhy@alibaba-inc.com>
Date: 2025-11-18T01:13:12Z
Lists: pgsql-hackers
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