Re: Failure in contrib test _int on loach

On 11/04/2019 13:14, Heikki Linnakangas wrote:
> On 11/04/2019 09:10, Andrey Lepikhov wrote:
>> On 10/04/2019 20:25, Heikki Linnakangas wrote:
>>> On 09/04/2019 19:11, Anastasia Lubennikova wrote:
>>>> After introducing GistBuildNSN this code path became unreachable.
>>>> To fix it, I added new flag to detect such splits during indexbuild.
>>>
>>> Isn't it possible that the grandparent page is also split, so that we'd
>>> need to climb further up?
>>
>> Based on Anastasia's idea i prepare alternative solution to fix the bug
>> (see attachment).
>> It utilizes the idea of linear increment of LSN/NSN. WAL write process
>> is used for change NSN value to 1 for each block of index relation.
>> I hope this can be a fairly clear and safe solution.
>
> That's basically the same idea as always using the "fake LSN" during
> index build, like the original version of this patch did. It's got the
> problem that I mentioned at
> https://www.postgresql.org/message-id/090fb3cb-1ca4-e173-ecf7-47d41ebac620@iki.fi:
>
>
>> * Using "fake" unlogged LSNs for GiST index build seemed fishy. I
>> could not convince myself that it was safe in all corner cases. In a
>> recently initdb'd cluster, it's theoretically possible that the fake
>> LSN counter overtakes the real LSN value, and that could lead to
>> strange behavior. For example, how would the buffer manager behave, if
>> there was a dirty page in the buffer cache with an LSN value that's
>> greater than the current WAL flush pointer? I think you'd get "ERROR:
>> xlog flush request %X/%X is not satisfied --- flushed only to %X/%X".
>
> Perhaps the risk is theoretical; the real WAL begins at XLOG_SEG_SIZE,
> so with defaults WAL segment size, the index build would have to do
> about 16 million page splits. The index would have to be at least 150 GB
> for that. But it seems possible, and with non-default segment and page
> size settings more so.
As i see in bufmgr.c, XLogFlush() can't called during index build. In
the log_newpage_range() call we can use mask to set value of NSN (and
LSN) to 1.
>
> Perhaps we could start at 1, but instead of using a global counter,
> whenever a page is split, we take the parent's LSN value and increment
> it by one. So different branches of the tree could use the same values,
> which would reduce the consumption of the counter values.
>
> Yet another idea would be to start the counter at 1, but check that it
> doesn't overtake the WAL insert pointer. If it's about to overtake it,
> just generate some dummy WAL.
>
> But it seems best to deal with this in gistdoinsert(). I think
> Anastasia's approach of adding a flag to GISTInsertStack can be made to
> work, if we set the flag somewhere in gistinserttuples() or
> gistplacetopage(), whenever a page is split. That way, if it needs to
> split multiple levels, the flag is set on all of the corresponding
> GISTInsertStack entries.
>
> Yet another trivial fix would be just always start the tree descend from
> the root in gistdoinsert(), if a page is split. Not as efficient, but
> probably negligible in practice.
Agree

--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company