Re: getting rid of SnapshotNow

On Thu, Jul 18, 2013 at 08:46:48AM -0400, Robert Haas wrote:
> 1. snapshot-error-v1.patch introduces a new special snapshot, called
> SnapshotError. In the cases where we set SnapshotNow as a sort of
> default snapshot, this patch changes the code to use SnapshotError
> instead. This affects scan->xs_snapshot in genam.c and
> estate->es_snapshot in execUtils.c. This passes make check-world, so
> apparently there is no code in the core distribution that does this.
> However, this is safer for third-party code, which will ERROR instead
> of seg faulting. The alternative approach would be to use
> InvalidSnapshot, which I think would be OK too if people dislike this
> approach.

I don't have a strong opinion. Anything it diagnoses is a code bug, probably
one that makes the affected extension useless until it's fixed. But the patch
is small and self-contained. I think the benefit, more than making things
safer in production, would be reducing the amount of time the developer needs
to zero in on the problem. It wouldn't be the first time we've done that;
compare AtEOXact_Buffers(). Does this particular class of bug deserve that
aid? I don't know.

> 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow
> to use SnapshotSelf instead. These include pgrowlocks(),
> pgstat_heap(), and get_actual_variable_range(). In all of those
> cases, only an approximately-correct answer is needed, so the change
> should be fine. I'd also generally expect that it's very unlikely for
> any of these things to get called in a context where the table being
> scanned has been updated by the current transaction after the most
> recent command-counter increment, so in practice the change in
> semantics will probably not be noticeable at all.

SnapshotSelf is awfully special; currently, you can grep for all uses of it
and find a collection of callers with highly-technical needs. Diluting that
with a handful of callers that legitimately preferred SnapshotNow but don't
care enough to mind SnapshotSelf in its place brings a minor loss of clarity.

From an accuracy perspective, GetActiveSnapshot() does seem ideal for
get_actual_variable_range(). That's independent of any hurry to remove
SnapshotNow. A possible disadvantage is that older snapshots could waste time
scanning back through newer index entries, when a more-accessible value would
be good enough for estimation purposes.

To me, the major advantage of removing SnapshotNow is to force all third-party
code to reevaluate. But that could be just as well achieved by renaming it
to, say, SnapshotImmediate. If there are borderline-legitimate SnapshotNow
uses in our code base, I'd lean toward a rename instead. Even if we decide to
remove every core use, third-party code might legitimately reach a different
conclusion on similar borderline cases.

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com