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.
regards, tom lane