From: | Kevin Grittner <kgrittn(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Thom Brown <thom(at)linux(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Steve Singer <steve(at)ssinger(dot)info>, Kevin Grittner <kgrittn(at)ymail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: snapshot too old, configured by time |
Date: | 2016-04-18 00:52:33 |
Message-ID: | CACjxUsNCYWpM9F+oTZ7kgMCF3qE19tcjXDt9Gg74rHv3Jx=CSw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2016-04-18 01:32:24 | Re: snapshot too old, configured by time |
Previous Message | Noah Misch | 2016-04-18 00:04:03 | Re: Default Roles |