On Sat, Apr 2, 2016 at 7:12 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Apr 1, 2016 at 11:45 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Kevin Grittner wrote:
>>
>>> 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?
>>
>> Yes, this is what I was thinking, thanks.
>
> A small thing:
> $ git diff master --check
> src/include/storage/bufmgr.h:181: trailing whitespace.
> +#define BufferGetPage(buffer, snapshot, relation, agetest)
> ((Page)BufferGetBlock(buffer))
>
> - Page page = BufferGetPage(buf);
> + Page page = BufferGetPage(buf, NULL, NULL, BGP_NO_SNAPSHOT_TEST);
> Having a BufferGetPageExtended() with some flags and a default
> corresponding to NO_SNAPSHOT_TEST would reduce the diff impact. And as
> long as the check is integrated with BufferGetPage[Extended]() I would
> not complain, the patch as proposed being 174kB...
If you are saying that the 450 places that don't need the check
would remain unchanged, and the only difference would be to use
BufferGetPageExtended() instead of BufferGetPage() followed by
TestForOldSnapshot() in the 30-some places that need the check, I
don't see the point. That would eliminate the "forced choice"
aspect of what Álvaro is asking for, and it seems to me that it
would do next to nothing to prevent the errors of omission that are
the concern here.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
