CREATE INDEX CONCURRENTLY does not index prepared xact's data

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2
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.


v1-0001-Wait-for-prepared-xacts-in-CREATE-INDEX-CONCURREN.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Victor Yegorov
сб, 19 дек. 2020 г. в 18:13, Andrey Borodin <[hidden email]>:
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.

CREATE INDEX CONCURRENTLY isn't supposed to be run inside a transaction?.. 


--
Victor Yegorov
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2


> 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.

Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Victor Yegorov
In reply to this post by Andrey Borodin-2
сб, 19 дек. 2020 г. в 18:13, Andrey Borodin <[hidden email]>:
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.

I agree, that behaviour is unexpected. But getting a notice that requires me
to re-create the index some time later is not better (from DBA perspective).

Maybe it'd be better to wait on prepared xacts like on other open ordinary transactions?


--
Victor Yegorov
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Tom Lane-2
In reply to this post by Andrey Borodin-2
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


Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2
In reply to this post by Victor Yegorov


> 19 дек. 2020 г., в 22:48, Victor Yegorov <[hidden email]> написал(а):
>
> сб, 19 дек. 2020 г. в 18:13, Andrey Borodin <[hidden email]>:
> 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.
>
> I agree, that behaviour is unexpected. But getting a notice that requires me
> to re-create the index some time later is not better (from DBA perspective).
>
> Maybe it'd be better to wait on prepared xacts like on other open ordinary transactions?

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.

Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2


> 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.


v2-0001-Wait-for-prepared-xacts-in-CREATE-INDEX-CONCURREN.patch (7K) Download Attachment
v2-0002-Add-test-for-CIC-with-prepared-xacts.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Michael Paquier-2
In reply to this post by Tom Lane-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2


> 21 дек. 2020 г., в 10:40, Michael Paquier <[hidden email]> написал(а):
>
>> 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.
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.

>  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?
Seems valid. Removed VirtualTransactionIdIsValidOrPreparedXact() from patch.

Thanks!

Best regards, Andrey Borodin.

v3-0001-Wait-for-prepared-xacts-in-CREATE-INDEX-CONCURREN.patch (7K) Download Attachment
v3-0002-Add-test-for-CIC-with-prepared-xacts.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2


> 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.


v4-0002-Add-test-for-CIC-with-prepared-xacts.patch (4K) Download Attachment
v4-0001-Wait-for-prepared-xacts-in-CREATE-INDEX-CONCURREN.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2
In reply to this post by Michael Paquier-2


> 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.

Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2


> 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.

Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Noah Misch-2
In reply to this post by Andrey Borodin-2
On Mon, Dec 21, 2020 at 12:24:55PM +0500, Andrey Borodin wrote:

> > 21 дек. 2020 г., в 10:40, Michael Paquier <[hidden email]> написал(а):
> >> 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.
>
> 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.

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:

> --- /dev/null
> +++ b/src/test/isolation/expected/prepared-transactions-cic.out
> @@ -0,0 +1,18 @@
> +Parsed test spec with 2 sessions
> +
> +starting permutation: w1 p1 cic2 c1 r2
> +step w1: BEGIN; INSERT INTO cic_test VALUES (1);
> +step p1: PREPARE TRANSACTION 's1';
> +step cic2:
> +    CREATE INDEX CONCURRENTLY on cic_test(a);
> +
> +ERROR:  canceling statement due to lock timeout

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.

> --- a/src/backend/storage/lmgr/lock.c
> +++ b/src/backend/storage/lmgr/lock.c
> @@ -2931,15 +2929,17 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
>  
>   /*
>   * Allocate memory to store results, and fill with InvalidVXID.  We only
> - * need enough space for MaxBackends + a terminator, since prepared xacts
> - * don't count. InHotStandby allocate once in TopMemoryContext.
> + * need enough space for MaxBackends + max_prepared_xacts + a
> + * terminator. InHotStandby allocate once in TopMemoryContext.
>   */
>   if (InHotStandby)
>   {
>   if (vxids == NULL)
>   vxids = (VirtualTransactionId *)
>   MemoryContextAlloc(TopMemoryContext,
> -   sizeof(VirtualTransactionId) * (MaxBackends + 1));
> +   sizeof(VirtualTransactionId) * (MaxBackends
> +   + max_prepared_xacts
> +   + 1));

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.

> @@ -4461,9 +4462,21 @@ bool
>  VirtualXactLock(VirtualTransactionId vxid, bool wait)
>  {
>   LOCKTAG tag;
> - PGPROC   *proc;
> + PGPROC   *proc = NULL;
>  
> - Assert(VirtualTransactionIdIsValid(vxid));
> + Assert(VirtualTransactionIdIsValid(vxid) ||
> + VirtualTransactionIdIsPreparedXact(vxid));
> +
> + if (VirtualTransactionIdIsPreparedXact(vxid))
> + {
> + LockAcquireResult lar;
> + /* If it's prepared xact - just wait for it */
> + SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);
> + lar = LockAcquire(&tag, ShareLock, false, !wait);
> + if (lar == LOCKACQUIRE_OK)

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.)

> + LockRelease(&tag, ShareLock, false);
> + return lar != LOCKACQUIRE_NOT_AVAIL;
> + }
>  
>   SET_LOCKTAG_VIRTUALTRANSACTION(tag, vxid);
>  
> diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
> index 1c3e9c1999..cedb9d6d2a 100644
> --- a/src/include/storage/lock.h
> +++ b/src/include/storage/lock.h
> @@ -70,6 +70,8 @@ typedef struct
>  #define VirtualTransactionIdIsValid(vxid) \
>   (((vxid).backendId != InvalidBackendId) && \
>   LocalTransactionIdIsValid((vxid).localTransactionId))
> +#define VirtualTransactionIdIsPreparedXact(vxid) \
> + ((vxid).backendId == InvalidBackendId)

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


Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2
Thanks for looking into this!

> 17 янв. 2021 г., в 12:24, Noah Misch <[hidden email]> написал(а):
>
>> --- a/src/backend/storage/lmgr/lock.c
>> +++ b/src/backend/storage/lmgr/lock.c
>> @@ -2931,15 +2929,17 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
>>
>> /*
>> * Allocate memory to store results, and fill with InvalidVXID.  We only
>> - * need enough space for MaxBackends + a terminator, since prepared xacts
>> - * don't count. InHotStandby allocate once in TopMemoryContext.
>> + * need enough space for MaxBackends + max_prepared_xacts + a
>> + * terminator. InHotStandby allocate once in TopMemoryContext.
>> */
>> if (InHotStandby)
>> {
>> if (vxids == NULL)
>> vxids = (VirtualTransactionId *)
>> MemoryContextAlloc(TopMemoryContext,
>> -   sizeof(VirtualTransactionId) * (MaxBackends + 1));
>> +   sizeof(VirtualTransactionId) * (MaxBackends
>> +   + max_prepared_xacts
>> +   + 1));
>
> PostgreSQL typically puts the operator before the newline.  Also, please note
> the whitespace error that "git diff --check origin/master" reports.
Fixed.
>
>> }
>> else
>> vxids = (VirtualTransactionId *)
>
> This is updating only the InHotStandby branch.  Both branches need the change.
Fixed.

>
>> @@ -4461,9 +4462,21 @@ bool
>> VirtualXactLock(VirtualTransactionId vxid, bool wait)
>> {
>> LOCKTAG tag;
>> - PGPROC   *proc;
>> + PGPROC   *proc = NULL;
>>
>> - Assert(VirtualTransactionIdIsValid(vxid));
>> + Assert(VirtualTransactionIdIsValid(vxid) ||
>> + VirtualTransactionIdIsPreparedXact(vxid));
>> +
>> + if (VirtualTransactionIdIsPreparedXact(vxid))
>> + {
>> + LockAcquireResult lar;
>> + /* If it's prepared xact - just wait for it */
>> + SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);
>> + lar = LockAcquire(&tag, ShareLock, false, !wait);
>> + if (lar == LOCKACQUIRE_OK)
>
> 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.)
I think that code that successfully acquired lock should release it. Other way we risk to release someone's else lock held for a reason. We only acquire lock to release it instantly anyway.

>
>> + LockRelease(&tag, ShareLock, false);
>> + return lar != LOCKACQUIRE_NOT_AVAIL;
>> + }
>>
>> SET_LOCKTAG_VIRTUALTRANSACTION(tag, vxid);
>>
>> diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
>> index 1c3e9c1999..cedb9d6d2a 100644
>> --- a/src/include/storage/lock.h
>> +++ b/src/include/storage/lock.h
>> @@ -70,6 +70,8 @@ typedef struct
>> #define VirtualTransactionIdIsValid(vxid) \
>> (((vxid).backendId != InvalidBackendId) && \
>> LocalTransactionIdIsValid((vxid).localTransactionId))
>> +#define VirtualTransactionIdIsPreparedXact(vxid) \
>> + ((vxid).backendId == InvalidBackendId)
>
> 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.
Makes sense, fixed. I was afraid that there's something I'm not aware about. I've iterated over VirtualTransactionIdIsValid() calls and did not find suspicious cases.

> 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.


v5-0001-Wait-for-prepared-xacts-in-CREATE-INDEX-CONCURREN.patch (7K) Download Attachment
v5-0002-Add-test-for-CIC-with-prepared-xacts.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2


> 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.


v6-0002-Add-test-for-CIC-with-prepared-xacts.patch (4K) Download Attachment
v6-0001-Wait-for-prepared-xacts-in-CREATE-INDEX-CONCURREN.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Noah Misch-2
In reply to this post by Andrey Borodin-2
On Fri, Jan 22, 2021 at 10:44:35AM +0500, Andrey Borodin wrote:

> > 17 янв. 2021 г., в 12:24, Noah Misch <[hidden email]> написал(а):
> >> @@ -4461,9 +4462,21 @@ bool
> >> VirtualXactLock(VirtualTransactionId vxid, bool wait)
> >> {
> >> LOCKTAG tag;
> >> - PGPROC   *proc;
> >> + PGPROC   *proc = NULL;
> >>
> >> - Assert(VirtualTransactionIdIsValid(vxid));
> >> + Assert(VirtualTransactionIdIsValid(vxid) ||
> >> + VirtualTransactionIdIsPreparedXact(vxid));
> >> +
> >> + if (VirtualTransactionIdIsPreparedXact(vxid))
> >> + {
> >> + LockAcquireResult lar;
> >> + /* If it's prepared xact - just wait for it */
> >> + SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);
> >> + lar = LockAcquire(&tag, ShareLock, false, !wait);
> >> + if (lar == LOCKACQUIRE_OK)
> >
> > 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.)
> I think that code that successfully acquired lock should release it. Other way we risk to release someone's else lock held for a reason. We only acquire lock to release it instantly anyway.
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.

prepared-transactions-cic-v7nm.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Andrey Borodin-2


> 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.