Re: snapshot too old, configured by time

On Thu, Mar 31, 2016 at 5:09 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> I understand the invasiveness argument, but to me the danger of
>> introducing new bugs trumps that. The problem is not the current code,
>> but future patches: it is just too easy to make the mistake of not
>> checking the snapshot in new additions of BufferGetPage. And you said
>> that the result of missing a check is silent wrong results from queries
>> that should instead be cancelled, which seems fairly bad to me.
>
> Fair point.

That's my main concern after going through the patch, and the patch
written as-is does not help much future users. This could be easily
forgotten by committers as well.

>> I said that we should change BufferGetPage into having the snapshot
>> check built-in, except in the cases where a flag is passed; and the flag
>> would be passed in all cases except those 30-something you identified.
>> In other words, the behavior in all the current callsites would be
>> identical to what's there today; we could have a macro do the first
>> check so that we don't introduce the overhead of a function call in the
>> 450 cases where it's not needed.
>
> In many of the places that BufferGetPage is called there is not a
> snapshot available. I assume that you would be OK with an Assert
> that the flag was passed if the snapshot is NULL? I had been
> picturing what you were requesting as just adding a snapshot
> parameter and assuming that NULL meant not to check; adding two
> parameters where the flag explicitly calls that the check is not
> needed might do more to prevent accidents, but I do wonder how much
> it would help during copy/paste frenzy. Touching all spots to use
> the new function signature would be a mechanical job with the
> compiler catching any errors, so it doesn't seem crazy to refactor
> that now, but I would like to hear what some others think about
> this.

That's better than what the existing patch for sure. When calling
BufferGetPage() one could be tempted to forget to set snapshot to NULL
though. It should be clearly documented in the header of
BufferGetPage() where and for which purpose a snapshot should be set,
and in which code paths it is expected to be used. In our case, that's
mainly when a page is fetched from shared buffers and that it is used
for reading tuples from it.

Just a note: I began looking at the tests, but finished looking at the
patch entirely at the end by curiosity. Regarding the integration of
this patch for 9.6, I think that bumping that to 9.7 would be wiser
because the patch needs to be re-written largely, and that's never a
good sign at this point of the development cycle.
--
Michael