CREATE INDEX CONCURRENTLY does not index prepared xact's data

Hi hackers!

$subj.

Steps to reproduce:
create extension if not exists amcheck;
create table if not exists t1(i int);
begin;
insert into t1 values(1);
prepare transaction 'x';
create index concurrently i1 on t1(i);
commit prepared 'x';
select bt_index_check('i1', true);

I observe:
NOTICE:  heap tuple (1,8) from table "t1" lacks matching index tuple within index "i1"
I expect: awaiting 'x' commit before index is created, correct index after.

This happens because WaitForLockersMultiple() does not take prepared xacts into account. Meanwhile CREATE INDEX CONCURRENTLY expects that locks are dropped only when transaction commit is visible.

This issue affects pg_repack and similar machinery based on CIC.

PFA draft of a fix.

Best regards, Andrey Borodin.

> 19 дек. 2020 г., в 22:22, Victor Yegorov <[hidden email]> написал(а):
>
> CREATE INDEX CONCURRENTLY isn't supposed to be run inside a transaction?..

CREATE INDEX CONCURRENTLY cannot run inside a transaction block.

Best regards, Andrey Borodin.

Andrey Borodin <[hidden email]> writes:
> This happens because WaitForLockersMultiple() does not take prepared
> xacts into account.

Ugh, clearly an oversight.

> Meanwhile CREATE INDEX CONCURRENTLY expects that locks are dropped only
> when transaction commit is visible.

Don't follow your point here --- I'm pretty sure that prepared xacts
continue to hold their locks.

> PFA draft of a fix.

Haven't you completely broken VirtualXactLock()?  Certainly, whether the
target is a normal or prepared transaction shouldn't alter the meaning
of the "wait" flag.

In general, I wonder whether WaitForLockersMultiple and GetLockConflicts
need to gain an additional parameter indicating whether to consider
prepared xacts.  It's not clear to me that their current behavior is wrong
for all possible uses.

                        regards, tom lane

This is not a real NOTICE. I just used a bit altered amcheck to diagnose the problem. It's an incorrect index. It lacks some tuples. It will not find existing data, failing to provide "read committed" consistency guaranties.
Fix waits for prepared xacts just like any other transaction.



> 19 дек. 2020 г., в 22:57, Tom Lane <[hidden email]> написал(а):
>
>> Meanwhile CREATE INDEX CONCURRENTLY expects that locks are dropped only
>> when transaction commit is visible.
>
> Don't follow your point here --- I'm pretty sure that prepared xacts
> continue to hold their locks.
Uhmm, yes, locks are there. Relation is locked with RowExclusiveLock, but this lock is ignored by WaitForLockers(heaplocktag, ShareLock, true). Locking relation with ShareLock works as expected.

>> PFA draft of a fix.
>
> Haven't you completely broken VirtualXactLock()?  Certainly, whether the
> target is a normal or prepared transaction shouldn't alter the meaning
> of the "wait" flag.
You are right, the patch has a bug here.

> In general, I wonder whether WaitForLockersMultiple and GetLockConflicts
> need to gain an additional parameter indicating whether to consider
> prepared xacts.  It's not clear to me that their current behavior is wrong
> for all possible uses.
I don't see usages besides indexing stuff. But maybe it worth to analyse each case...

BTW do we need a test for this? Will isolation test be good at checking this?

Thanks!

Best regards, Andrey Borodin.

> 19 дек. 2020 г., в 23:25, Andrey Borodin <[hidden email]> написал(а):
>
>
> BTW do we need a test for this? Will isolation test be good at checking this?

PFA patch set with isolation test for the $subj and fix for VirtualXactLock() bug.

I think I'll register the thread on January CF.

Thanks!

Best regards, Andrey Borodin.

On Sat, Dec 19, 2020 at 12:57:41PM -0500, Tom Lane wrote:
> Andrey Borodin <[hidden email]> writes:
>> This happens because WaitForLockersMultiple() does not take prepared
>> xacts into account.
>
> Ugh, clearly an oversight.

This looks to be the case since 295e639 where virtual XIDs have been
introduced.  So this is an old bug.

> Don't follow your point here --- I'm pretty sure that prepared xacts
> continue to hold their locks.

Yes, that's what I recall as well.

> Haven't you completely broken VirtualXactLock()?  Certainly, whether the
> target is a normal or prepared transaction shouldn't alter the meaning
> of the "wait" flag.

Yep.

> In general, I wonder whether WaitForLockersMultiple and GetLockConflicts
> need to gain an additional parameter indicating whether to consider
> prepared xacts.  It's not clear to me that their current behavior is wrong
> for all possible uses.

WaitForLockers is used only by REINDEX and CREATE/DROP CONCURRENTLY,
where it seems to me we need to care about all the cases related to
concurrent build, validation and index drop.  The other caller of
GetLockConflicts() is for conflict resolution in standbys where it is
fine to ignore 2PC transactions as these cannot be cancelled.  So I
agree that we are going to need more control with a new option
argument to be able to control if 2PC transactions are ignored or
not.

Hmm.  The approach taken by the patch looks to be back-patchable.
Based on the lack of complaints on the matter, we could consider
instead putting an error in WaitForLockersMultiple() if there is at
least one numPrepXact which would at least avoid inconsistent data.
But I don't think what's proposed here is bad either.

VirtualTransactionIdIsValidOrPreparedXact() is confusing IMO, knowing
that VirtualTransactionIdIsPreparedXact() combined with
LocalTransactionIdIsValid() would be enough to do the job.

-       Assert(VirtualTransactionIdIsValid(vxid));
+       Assert(VirtualTransactionIdIsValidOrPreparedXact(vxid));
+
+       if (VirtualTransactionIdIsPreparedXact(vxid))
[...]
#define VirtualTransactionIdIsPreparedXact(vxid) \
    ((vxid).backendId == InvalidBackendId)
This would allow the case where backendId and localTransactionId are
both invalid.  So it would be better to also check in
VirtualTransactionIdIsPreparedXact() that the XID is not invalid, no?
--
Michael

I don't think that fact that we cannot cancel transaction is sufficient here to ignore prepared transaction. I think there should not exist any prepared transaction that we need to cancel in standby conflict resolution. And if it exists - it's a sign of corruption, we could emit warning or something like that.
But I'm really not an expert here, just a common sense that prepared transaction is just like regular transaction that survives crash. If we wait for any transaction - probably we should wait for prepared too. I'm not insisting on anything though.

Thanks!

Best regards, Andrey Borodin.

> 21 дек. 2020 г., в 12:24, Andrey Borodin <[hidden email]> написал(а):
>
>
> Seems valid. Removed VirtualTransactionIdIsValidOrPreparedXact() from patch.

Sorry for the noise, removal was not complete.

Best regards, Andrey Borodin.

> 21 дек. 2020 г., в 10:40, Michael Paquier <[hidden email]> написал(а):
>
> Hmm.  The approach taken by the patch looks to be back-patchable.
I was checking that patch\test works in supported branches and everything seems to be fine down to REL_10_STABLE.

But my machines (Ubuntu18 and MacOS) report several fails in isolation tests on REL9_6_20\REL9_6_STABLE. Particularly eval-plan-qual-trigger, drop-index-concurrently-1, and async-notify. I do not observe problems with regular isolation tests though.

Do I understand correctly that check-world tests on buildfarm 'make check-prepared-txns' and the problem is somewhere in my machines? Or something is actually broken\outdated?

Thanks!

Best regards, Andrey Borodin.

On Wed, Dec 23, 2020 at 12:23:28PM +0500, Andrey Borodin wrote:
> Do I understand correctly that check-world tests on buildfarm 'make
> check-prepared-txns' and the problem is somewhere in my machines? Or
> something is actually broken\outdated?

The buildfarm code has no trace of check-prepared-txns.  And, FWIW, I
see no failures at the top of REL9_6_STABLE.  Do you mean that this
happens only with your patch?  Or do you mean that you see failures
using the stable branch of upstream?  I have not tested the former,
but the latter works fine on my end.
--
Michael

> 23 дек. 2020 г., в 12:52, Michael Paquier <[hidden email]> написал(а):
>
> On Wed, Dec 23, 2020 at 12:23:28PM +0500, Andrey Borodin wrote:
>> Do I understand correctly that check-world tests on buildfarm 'make
>> check-prepared-txns' and the problem is somewhere in my machines? Or
>> something is actually broken\outdated?
>
> FWIW, I
> see no failures at the top of REL9_6_STABLE.

Thanks, Michael! The problem was indeed within my machines. maintainer-cleanup is not enough for make check-prepared-txns. Fresh real installation is necessary.

I've checked that test works down to REL9_5_STABLE with patch.

Thanks!

Best regards, Andrey Borodin.

On Mon, Dec 21, 2020 at 12:24:55PM +0500, Andrey Borodin wrote:
Agreed.  Based on src/backend/storage/lmgr/README section "Locking during Hot
Standby" and RecoverPreparedTransactions(), prepared-transaction PGPROC
entries get locks at end of recovery, not earlier.  The conflict resolution
callers won't see prepared-transaction locks, and they could assert that,
raise an error, or just assume that implicitly.

> But I'm really not an expert here, just a common sense that prepared transaction is just like regular transaction that survives crash. If we wait for any transaction - probably we should wait for prepared too. I'm not insisting on anything though.

In the startup process of a standby, waiting for a primary-side transaction
(regular or prepared) would succeed instantly, rendering the wait meaningless.
I'm not too worried about it; several parts of the system would change if
these standby-side lock invariants ceased to hold.

On Mon, Dec 21, 2020 at 01:19:59PM +0500, Andrey Borodin wrote:
I wondered if a slow server could ever print "<waiting ...>" in here.  It
can't; this is fine.  pg_isolation_test_session_is_blocked() never reports the
prepared transaction, because that transaction's locks have no pid associated.

PostgreSQL typically puts the operator before the newline.  Also, please note
the whitespace error that "git diff --check origin/master" reports.

>   }
>   else
>   vxids = (VirtualTransactionId *)

This is updating only the InHotStandby branch.  Both branches need the change.

This should instead test "!= LOCKACQUIRE_NOT_AVAIL", lest
LOCKACQUIRE_ALREADY_HELD happen.  (It perhaps can't happen, but skipping the
LockRelease() would be wrong if it did.)

Your patch is introducing VirtualTransactionId values that represent prepared
xacts, and it has VirtualTransactionIdIsValid() return false for those values.
Let's make VirtualTransactionIdIsValid() return true for them, since they're
as meaningful as any other value.  The GetLockConflicts() header comment says
"The result array is palloc'd and is terminated with an invalid VXID."  Patch
v4 falsifies that comment.  The array can contain many of these new "invalid"
VXIDs, and an all-zeroes VXID terminates the array.  Rather than change the
comment, let's change VirtualTransactionIdIsValid() to render the comment true
again.  Most (perhaps all) VirtualTransactionIdIsValid() callers won't need to
distinguish the prepared-transaction case.

An alternative to redefining VXID this way would be to have some new type,
each instance of which holds either a valid VXID or a valid
prepared-transaction XID.  That alternative feels inferior to me, though.
What do you think?

Thanks,
nm

Thanks for looking into this!
Fixed.
> An alternative to redefining VXID this way would be to have some new type,
> each instance of which holds either a valid VXID or a valid
> prepared-transaction XID.  That alternative feels inferior to me, though.
> What do you think?
I think we should not call valid vxids "invalid".

By the way maybe rename "check-prepared-txns" to "check-prepared-xacts" for consistency?

Thanks!

Best regards, Andrey Borodin.

> 22 янв. 2021 г., в 10:44, Andrey Borodin <[hidden email]> написал(а):
>
> <v5-0001-Wait-for-prepared-xacts-in-CREATE-INDEX-CONCURREN.patch><v5-0002-Add-test-for-CIC-with-prepared-xacts.patch>
Uh, vscode did not save files and I've send incorrect version. Disregard v5.
Sorry for the noise.

Best regards, Andrey Borodin.

On Fri, Jan 22, 2021 at 10:44:35AM +0500, Andrey Borodin wrote: LockAcquire() increments the reference count before returning
LOCKACQUIRE_ALREADY_HELD, so it is an acquire.  If this caller doesn't
LockRelease(), the lock will persist until end of transaction.

I changed that, updated comments, and fixed pgindent damage.  I plan to push
the attached version.

> > The array can contain many of these new "invalid"
> > VXIDs, and an all-zeroes VXID terminates the array.  Rather than change the

Correction: the terminator contains (InvalidBackendId,0).

> By the way maybe rename "check-prepared-txns" to "check-prepared-xacts" for consistency?

The "xact" term is about three times as frequent "txn".  I favor "xact" in new
usage.  It's not dominant enough to change old usage unless done pervasively.

> 24 янв. 2021 г., в 07:27, Noah Misch <[hidden email]> написал(а):
>
> I changed that, updated comments, and fixed pgindent damage.  I plan to push
> the attached version.

I see that patch was pushed. I'll flip status of CF entry. Many thanks!

Best regards, Andrey Borodin.