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.
