Re: snapshot too old, configured by time

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.

> 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.

> Tom said that my proposal would be performance-killing, not that your
> patch would be performance-killing. But as I argue above, with my
> proposal performance would stay the same, so we're actually okay.
>
> I don't think nobody disputes that your patch is good in general.
> I would be happy with it in 9.6, but I have my reservations about the
> aforementioned problem.

We have a lot of places in our code where people need to know
things that they are not reminded of by the surrounding code, but
I'm not about to argue that's a good thing; if the consensus is
that this would help prevent future bugs when new BufferGetPage
calls are added, I can go with the flow.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company