Re: snapshot too old, configured by time - Mailing list pgsql-hackers

On Sun, Apr 17, 2016 at 5:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@gmail.com> writes:
>> On Wed, Mar 30, 2016 at 3:26 PM, Alvaro Herrera> <alvherre@2ndquadrant.com> wrote:
>>> Kevin Grittner wrote:
>>>> On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera <alvherre@2ndquadrant.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