Re: BUG #17949: Adding an index introduces serialisation anomalies.

On 26/06/2023 03:25, Thomas Munro wrote:
> On Sun, Jun 25, 2023 at 1:59 AM Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>> I've managed to resolve it, or at least reduce the chances for the issue to
>> appear, via semi-randomly adding more CheckForSerializableConflictIn /
>> PredicateLock around the new sublist that has to be created in
>> ginHeapTupleFastInsert. I haven't seen the reproducer failing with this
>> changeset after running it multiple times for a couple of minutes, where on the
>> main branch, with the two fixes from Thomas included, it was failing within a
>> couple of seconds.
>
> Ahh, right, thanks. I don't think we need to lock all those pages as
> you showed, as this whole "fast" path is supposed to be covered by the
> meta page (in other words, GIN is expected to have the highest
> possible serialisation failure rate under SSI unless you turn fast
> updates off). But there is an ordering bug with the existing
> predicate lock in that code, which allows this to happen:
>
> S1: CheckForSerializableConflictIn(meta)
>
> S2: PredicateLockPage(meta)
> S2: scan, find no tuples
>
> S1: BufferLock(EXCLUSIVE
> S1: modify stuff...
>
> CheckForSerializableConflictIn() was written with the assumption that
> you're inserting a tuple (ie you have the page containing the tuple
> locked), so you'll either conflict with a reader who already has a
> predicate lock at that point OR you'll insert first and then the
> reader will see your (invisible-to-snapshot) tuples, but here we're
> doing some fancy footwork with a meta page, and we screwed up the
> ordering and left a window where neither of those things happens.
> Perhaps it was coded that way because there is drop-then-reacquire
> dance, but it's easy enough to move the check in both branches. Does
> that make sense?

Yes, +1 on the patches. Any chance of constructing test cases for these?
The above race condition is hard to reach, but some of these other bugs
seem more testable.

Some minor nits: In
v3-0001-Fix-race-in-SSI-interaction-with-empty-btrees.patch:

> /*
> - * We only get here if the index is completely empty. Lock relation
> - * because nothing finer to lock exists.
> + * Since we have no pages locked, it's possible for another
> + * transaction to insert data between _bt_search() and
> + * PredicateLockRelation(). We have to try again after taking a
> + * relation-level predicate lock, to close a narrow window where we
> + * wouldn't scan concurrently inserted tuples, but the writer wouldn't
> + * see our predicate lock.
> */

I'd like to keep the old comment here, it's good context, and add the
new text in addition to the old.

v3-0002-Fix-race-in-SSI-interaction-with-bitmap-heap-scan.patch: Can we
keep the optimization when not using SSI?

--
Heikki Linnakangas
Neon (https://neon.tech)