From: Andrey Borodin <x4mmm@yandex-team.ru>
To: pgsql-bugs@lists.postgresql.org
Subject: CREATE INDEX CONCURRENTLY does not index prepared xact's data
Date: Sat, 19 Dec 2020 22:13:39 +0500
Message-ID: <2E712143-97F7-4890-B470-4A35142ABC82@yandex-team.ru> (raw)
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.
Attachments:v1-0001-Wait-for-prepared-xacts-in-CREATE-INDEX-CONCURREN.patch (application/octet-stream, 5.1K) download
From 02a3baa7f963d5116b3e4506a0f1dcac2c3c16a9 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <amborodin@acm.org>
Date: Sat, 19 Dec 2020 21:53:00 +0500
Subject: [PATCH v1] Wait for prepared xacts in CREATE INDEX CONCURRENTLY locks
infrastructure
---
src/backend/storage/lmgr/lmgr.c | 5 ++---
src/backend/storage/lmgr/lock.c | 33 +++++++++++++++++++++------------
src/include/storage/lock.h | 5 +++++
3 files changed, 28 insertions(+), 15 deletions(-)
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.cindex 7409de9405..c1e8e76f41 100644--- a/src/backend/storage/lmgr/lmgr.c+++ b/src/backend/storage/lmgr/lmgr.c@@ -904,8 +904,7 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode, bool progress)
/*
* Note: GetLockConflicts() never reports our own xid, hence we need not
- * check for that. Also, prepared xacts are not reported, which is fine- * since they certainly aren't going to do anything anymore.+ * check for that. Also, prepared xacts are reported and awaited.
*/
/* Finally wait for each such transaction to complete */
@@ -913,7 +912,7 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode, bool progress)
{
VirtualTransactionId *lockholders = lfirst(lc);
- while (VirtualTransactionIdIsValid(*lockholders))+ while (VirtualTransactionIdIsValidOrPreparedXact(*lockholders))
{
/* If requested, publish who we're going to wait for. */
if (progress)
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.cindex d86566f455..9681fcabc8 100644--- a/src/backend/storage/lmgr/lock.c+++ b/src/backend/storage/lmgr/lock.c@@ -2904,9 +2904,7 @@ FastPathGetRelationLockEntry(LOCALLOCK *locallock)
* so use of this function has to be thought about carefully.
*
* Note we never include the current xact's vxid in the result array,
- * since an xact never blocks itself. Also, prepared transactions are- * ignored, which is a bit more debatable but is appropriate for current- * uses of the result.+ * since an xact never blocks itself.
*/
VirtualTransactionId *
GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
@@ -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));
}
else
vxids = (VirtualTransactionId *)
@@ -3085,10 +3085,10 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
/*
* If we see an invalid VXID, then either the xact has already
- * committed (or aborted), or it's a prepared xact. In either- * case we may ignore it.+ * committed (or aborted), or it's a prepared xact. We take+ * prepared xacts into account too.
*/
- if (VirtualTransactionIdIsValid(vxid))+ if (VirtualTransactionIdIsValidOrPreparedXact(vxid))
{
int i;
@@ -3108,7 +3108,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
LWLockRelease(partitionLock);
- if (count > MaxBackends) /* should never happen */+ if (count > MaxBackends + max_prepared_xacts) /* should never happen */
elog(PANIC, "too many conflicting locks found");
vxids[count].backendId = InvalidBackendId;
@@ -4461,9 +4461,18 @@ bool
VirtualXactLock(VirtualTransactionId vxid, bool wait)
{
LOCKTAG tag;
- PGPROC *proc;+ PGPROC *proc = NULL;- Assert(VirtualTransactionIdIsValid(vxid));+ Assert(VirtualTransactionIdIsValidOrPreparedXact(vxid));++ if (VirtualTransactionIdIsPreparedXact(vxid))+ {+ /* If it's prepared xact - just wait for it */+ SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);+ (void) LockAcquire(&tag, ShareLock, false, false);+ LockRelease(&tag, ShareLock, false);+ return true;+ }
SET_LOCKTAG_VIRTUALTRANSACTION(tag, vxid);
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.hindex 1c3e9c1999..dbd2a32678 100644--- a/src/include/storage/lock.h+++ b/src/include/storage/lock.h@@ -70,6 +70,11 @@ typedef struct
#define VirtualTransactionIdIsValid(vxid) \
(((vxid).backendId != InvalidBackendId) && \
LocalTransactionIdIsValid((vxid).localTransactionId))
+#define VirtualTransactionIdIsValidOrPreparedXact(vxid) \+ (((vxid).backendId == InvalidBackendId) || \+ LocalTransactionIdIsValid((vxid).localTransactionId))+#define VirtualTransactionIdIsPreparedXact(vxid) \+ ((vxid).backendId == InvalidBackendId)
#define VirtualTransactionIdEquals(vxid1, vxid2) \
((vxid1).backendId == (vxid2).backendId && \
(vxid1).localTransactionId == (vxid2).localTransactionId)
--
2.24.3 (Apple Git-128)
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: x4mmm@yandex-team.ru, pgsql-bugs@lists.postgresql.org
Subject: Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data
In-Reply-To: <2E712143-97F7-4890-B470-4A35142ABC82@yandex-team.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox