Re: snapshot too old, configured by time

On Sun, Apr 17, 2016 at 5:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)gmail(dot)com> writes:
>> On Wed, Mar 30, 2016 at 3:26 PM, Alvaro Herrera> <alvherre(at)2ndquadrant(dot)com> wrote:
>>> Kevin Grittner wrote:
>>>> On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>>>> 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.
>
>> Attached is what I think you're talking about for the first patch.
>> AFAICS this should generate identical executable code to unpatched.
>> Then the patch to actually implement the feature would, instead
>> of adding 30-some lines with TestForOldSnapshot() would implement
>> that as the behavior for the other enum value, and alter those
>> 30-some BufferGetPage() calls.
>
>> Álvaro and Michael, is this what you were looking for?
>
>> Is everyone else OK with this approach?
>
> After struggling with back-patching a GIN bug fix, I wish to offer up the
> considered opinion that this was an impressively bad idea. It's inserted
> 450 or so pain points for back-patching, which we will have to deal with
> for the next five years. Moreover, I do not believe that it will do a
> damn thing for ensuring that future calls of BufferGetPage think about
> what to do; they'll most likely be copied-and-pasted from nearby calls,
> just as people have always done. With luck, the nearby calls will have
> the right semantics, but this change won't help very much at all if they
> don't.
>
> I think we should revert BufferGetPage to be what it was before (with
> no snapshot test) and invent BufferGetPageExtended or similar to be
> used in the small number of places that need a snapshot test.

I'm not sure what BufferGetPageExtended() buys us over simply
inserting TestForOldSnapshot() where it is needed. Other than that
question, I have no objections to the course outlined, but figure I
should not jump on it without allowing at least a couple days for
discussion. That also may give me time to perform the benchmarks I
wanted -- VPN issues have blocked me from the big test machines so
far. I think I see where the time may be going when the feature is
disabled, and if I'm right I have a fix; but without a big NUMA
machine there is no way to confirm it.

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