Hackers, The return value of RegisterSnapshot is being ignored in a few places in indexam.c and tableam.c, suggesting an intimate knowledge of the inner workings of the snapshot manager from these two files. I don't think that is particularly wise, and I don't see a performance justification for the way it is presently coded. There are no live bugs caused by this that I can see, but I would still like it cleaned up.
Inside index_beginscan_parallel: snapshot = RestoreSnapshot(pscan->ps_snapshot_data); RegisterSnapshot(snapshot); scan = index_beginscan_internal(indexrel, nkeys, norderbys, snapshot, pscan, true); It happens to be true in the current implementation of the snapshot manager that restored snapshots will have their 'copied' field set to true, and that the RegisterSnapshot function will in that case return the same snapshot that it was handed, so the snapshot handed to index_beginscan_internal turns out to be the right one. But if RegisterSnapshot were changed to return a different copy of the snapshot, this code would break. There is a similar level of knowledge in table_beginscan_parallel, which for brevity I won't quote here. The code in table_scan_update_snapshot appears even more brittle to me. The only function in the core code base that calls table_scan_update_snapshot is ExecBitmapHeapInitializeWorker, and it does so right after restoring the snapshot that it hands to table_scan_update_snapshot, so the fact that table_scan_update_snapshot then ignores the return value of RegisterSnapshot on that snapshot happens to be ok. If some other code were changed to call this function, it is not clear that it would work out so well. I propose that attached patch. mark
snapshot.patch.1
Description: Binary data