Message-ID: <4638fef6-ea51-0e0e-d463-d5269575b021@iki.fi>
Date: Mon, 26 Jun 2023 11:04:26 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: BUG #17949: Adding an index introduces serialisation anomalies.
Content-Language: en-US
To: Thomas Munro <thomas.munro@gmail.com>,
 Dmitry Dolgov <9erthalion6@gmail.com>
Cc: artem.anisimov.255@gmail.com, pgsql-bugs@lists.postgresql.org,
 Teodor Sigaev <teodor@sigaev.ru>
References: 
 <CA+hUKG+zAbVgU91ZjkaCd=1AFOaMRbOp0=E9DP0BwippqqGx5g@mail.gmail.com>
 <CA+hUKG+3B7uBkJd4rr2FXhTc+95F3RvRovbhBM2UDt68aL5a-g@mail.gmail.com>
 <CA+hUKGJATT51T0Vkg0FDkQa5boQv-WoqL1_KM=313ySu0MVqig@mail.gmail.com>
 <20230619121757.gjvuvypp25ngwkux@ddolgov.remote.csb>
 <CA+hUKGJP3g6PF4vES0X0zy34uuSvHxHUhoq65A_WtzqxPpJ_6g@mail.gmail.com>
 <CA+hUKGJQ5ijziJtnrZfo8-8dmfiCU8nJBTnpkVweBp2krN_6Ng@mail.gmail.com>
 <CA+hUKGKRD5UM6mEDkre-yhm9Smr4ampf-5=NeFkAqi-jRt4RYQ@mail.gmail.com>
 <20230621090421.4rr55ybndttng7xl@ddolgov.remote.csb>
 <CA+hUKGJKTCGShnj0ZRhHsmOuHtZ-udZu8jN+GJ2-7YCSGnDYzg@mail.gmail.com>
 <20230623140542.5tbeg5ikz3cupag3@ddolgov.remote.csb>
 <20230624135910.zufuollwp57otawo@ddolgov.remote.csb>
 <CA+hUKGKmR57CgZAPRjeSQfXb_oxjSUx7673SqDRw9RF2FfCw=A@mail.gmail.com>
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: 
 <CA+hUKGKmR57CgZAPRjeSQfXb_oxjSUx7673SqDRw9RF2FfCw=A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/4638fef6-ea51-0e0e-d463-d5269575b021%40iki.fi>
Precedence: bulk

On 26/06/2023 03:25, Thomas Munro wrote:
> On Sun, Jun 25, 2023 at 1:59 AM Dmitry Dolgov <9erthalion6@gmail.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)