On Sun, Apr 17, 2016 at 5:15 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Kevin Grittner <kgri...@gmail.com> writes: >> On Wed, Mar 30, 2016 at 3:26 PM, Alvaro Herrera> <alvhe...@2ndquadrant.com> >> wrote: >>> Kevin Grittner wrote: >>>> On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera <alvhe...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers