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
