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 г. в 18:13, Andrey Borodin <[hidden email]>: Steps to reproduce: CREATE INDEX CONCURRENTLY isn't supposed to be run inside a transaction?.. Victor Yegorov |
> 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. |
In reply to this post by Andrey Borodin-2
сб, 19 дек. 2020 г. в 18:13, Andrey Borodin <[hidden email]>: I observe: 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 |
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 |
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. |
> 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. ![]() ![]() |
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 |
> 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. 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? 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. ![]() ![]() |
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. |
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. |
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 |
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. > >> } >> 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? 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. ![]() ![]() |
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_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. |
Free forum by Nabble | Edit this page |