On Mon, Apr 18, 2016 at 9:52 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> 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:
>> 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.
TBH, BufferGetPageExtended() still looks like a good idea to me.
Backpatching those code paths is going to make the maintenance far
harder, on top of the compilation of extensions for perhaps no good
reason. Even if this is a low-level change, if this feature goes in
with 9.6, it would be really good to mention as well that callers of
BufferGetPage should update their calls accordingly if they care about
the checks with the old snapshot. This is a routine used a lot in many
plugins and extensions. Usually such low-level things are not
mentioned in the release notes, but this time I think that's really
important to say it loudly.
--
Michael