From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: James Coleman <jtc331@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: remove spurious CREATE INDEX CONCURRENTLY wait
Date: Mon, 9 Nov 2020 16:47:43 +0100
Message-ID: <20201109154743.sij6j5ue3j673r7h@localhost> (raw)
In-Reply-To: <20201103181447.54ctwnfdt5wztbqn@localhost>
References: <CAAaqYe_tq_Mtd9tdeGDsgQh+wMvouithAmcOXvCbLaH2PPGHvA@mail.gmail.com>
<20200819181646.GA1559@alvherre.pgsql>
<20200820060929.GB3730@paquier.xyz>
<20201103181447.54ctwnfdt5wztbqn@localhost>
> On Tue, Nov 03, 2020 at 07:14:47PM +0100, Dmitry Dolgov wrote:
> > On Thu, Aug 20, 2020 at 03:11:19PM +0900, Michael Paquier wrote:
> > On Wed, Aug 19, 2020 at 02:16:46PM -0400, Alvaro Herrera wrote:
> > > I did not set the flag in REINDEX CONCURRENTLY, but as I understand it
> > > can be done too, since in essence it's the same thing as a CIC from a
> > > snapshot management point of view.
> >
> > Yes, I see no problems for REINDEX CONCURRENTLY as well as long as
> > there are no predicates and expressions involved. The transactions
> > that should be patched are all started in ReindexRelationConcurrently.
> > The transaction of index_concurrently_swap() cannot set up that
> > though. Only thing to be careful is to make sure that safe_flag is
> > correct depending on the list of indexes worked on.
>
> Hi,
>
> After looking through the thread and reading the patch it seems good,
> and there are only few minor questions:
>
> * Doing the same for REINDEX CONCURRENTLY, which does make sense. In
> fact it's already mentioned in the commentaries as done, which a bit
> confusing.
Just to give it a shot, would the attached change be enough?
Attachments:v3-0001-Avoid-spurious-CREATE-INDEX-CONCURRENTLY-waits.patch (text/x-diff, 9.2K) download
From d30d8acf91679985970334069ab7f1f8f7fc3ec5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 4 Aug 2020 22:04:57 -0400
Subject: [PATCH v3] Avoid spurious CREATE INDEX CONCURRENTLY waits
---
src/backend/commands/indexcmds.c | 122 ++++++++++++++++++++++++++++++-
src/include/storage/proc.h | 6 +-
2 files changed, 124 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.cindex 75552c64ed..5019397d50 100644--- a/src/backend/commands/indexcmds.c+++ b/src/backend/commands/indexcmds.c@@ -385,7 +385,10 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
* lazy VACUUMs, because they won't be fazed by missing index entries
* either. (Manual ANALYZEs, however, can't be excluded because they
* might be within transactions that are going to do arbitrary operations
- * later.)+ * later.) Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY+ * on indexes that are neither expressional nor partial are also safe to+ * ignore, since we know that those processes won't examine any data+ * outside the table they're indexing.
*
* Also, GetCurrentVirtualXIDs never reports our own vxid, so we need not
* check for that.
@@ -406,7 +409,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
VirtualTransactionId *old_snapshots;
old_snapshots = GetCurrentVirtualXIDs(limitXmin, true, false,
- PROC_IS_AUTOVACUUM | PROC_IN_VACUUM,+ PROC_IS_AUTOVACUUM | PROC_IN_VACUUM+ | PROC_IN_SAFE_IC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -426,7 +430,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
newer_snapshots = GetCurrentVirtualXIDs(limitXmin,
true, false,
- PROC_IS_AUTOVACUUM | PROC_IN_VACUUM,+ PROC_IS_AUTOVACUUM | PROC_IN_VACUUM+ | PROC_IN_SAFE_IC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -519,6 +524,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1045,6 +1051,17 @@ DefineIndex(Oid relationId,
}
}
+ /*+ * When doing concurrent index builds, we can set a PGPROC flag to tell+ * concurrent VACUUM, CREATE INDEX CONCURRENTLY and REINDEX CONCURRENTLY+ * to ignore us when waiting for concurrent snapshots. That can only be+ * done for indexes that don't execute any expressions. Determine that.+ * (The flag is reset automatically at transaction end, so it must be+ * set for each transaction.)+ */+ safe_index = indexInfo->ii_Expressions == NIL &&+ indexInfo->ii_Predicate == NIL;+
/*
* Report index creation if appropriate (delay this till after most of the
* error checks)
@@ -1431,6 +1448,15 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */+ if (safe_index)+ {+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;+ LWLockRelease(ProcArrayLock);+ }+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1490,6 +1516,15 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */+ if (safe_index)+ {+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;+ LWLockRelease(ProcArrayLock);+ }+
/*
* Phase 3 of concurrent index build
*
@@ -1546,6 +1581,15 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */+ if (safe_index)+ {+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;+ LWLockRelease(ProcArrayLock);+ }+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
@@ -3021,6 +3065,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
PROGRESS_CREATEIDX_ACCESS_METHOD_OID
};
int64 progress_vals[4];
+ bool safe_index = true;
/*
* Create a memory context that will survive forced transaction commits we
@@ -3324,6 +3369,23 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
newIndexRel = index_open(newIndexId, ShareUpdateExclusiveLock);
+ /*+ * When doing concurrent reindex, we can set a PGPROC flag to tell+ * concurrent VACUUM, CREATE INDEX CONCURRENTLY and REINDEX+ * CONCURRENTLY to ignore us when waiting for concurrent snapshots.+ * That can only be done for indexes that don't execute any+ * expressions. Determine that for all involved indexes together. (The+ * flag is reset automatically at transaction end, so it must be set+ * for each transaction.)+ */+ if (safe_index)+ {+ IndexInfo *newIndexInfo = BuildIndexInfo(newIndexRel);++ safe_index = newIndexInfo->ii_Expressions == NIL &&+ newIndexInfo->ii_Predicate == NIL;+ }+
/*
* Save the list of OIDs and locks in private context
*/
@@ -3393,6 +3455,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */+ if (safe_index)+ {+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;+ LWLockRelease(ProcArrayLock);+ }+
/*
* Phase 2 of REINDEX CONCURRENTLY
*
@@ -3456,6 +3527,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
}
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */+ if (safe_index)+ {+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;+ LWLockRelease(ProcArrayLock);+ }+
/*
* Phase 3 of REINDEX CONCURRENTLY
*
@@ -3559,6 +3639,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */+ if (safe_index)+ {+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;+ LWLockRelease(ProcArrayLock);+ }+
forboth(lc, indexIds, lc2, newIndexIds)
{
char *oldName;
@@ -3609,6 +3698,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */+ if (safe_index)+ {+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;+ LWLockRelease(ProcArrayLock);+ }+
/*
* Phase 5 of REINDEX CONCURRENTLY
*
@@ -3641,6 +3739,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */+ if (safe_index)+ {+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;+ LWLockRelease(ProcArrayLock);+ }+
/*
* Phase 6 of REINDEX CONCURRENTLY
*
@@ -3692,6 +3799,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* Start a new transaction to finish process properly */
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */+ if (safe_index)+ {+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;+ LWLockRelease(ProcArrayLock);+ }+
/* Log what we did */
if (options & REINDEXOPT_VERBOSE)
{
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.hindex 9c9a50ae45..50ce5c8cf2 100644--- a/src/include/storage/proc.h+++ b/src/include/storage/proc.h@@ -53,13 +53,17 @@ struct XidCache
*/
#define PROC_IS_AUTOVACUUM 0x01 /* is it an autovac worker? */
#define PROC_IN_VACUUM 0x02 /* currently running lazy vacuum */
+#define PROC_IN_SAFE_IC 0x04 /* currently running CREATE INDEX+ * CONCURRENTLY or REINDEX+ * CONCURRENTLY on non-expressional,+ * non-partial index */
#define PROC_VACUUM_FOR_WRAPAROUND 0x08 /* set by autovac only */
#define PROC_IN_LOGICAL_DECODING 0x10 /* currently doing logical
* decoding outside xact */
/* flags reset at EOXact */
#define PROC_VACUUM_STATE_MASK \
- (PROC_IN_VACUUM | PROC_VACUUM_FOR_WRAPAROUND)+ (PROC_IN_VACUUM | PROC_IN_SAFE_IC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.21.0
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-hackers@postgresql.org
Cc: 9erthalion6@gmail.com, michael@paquier.xyz, alvherre@2ndquadrant.com, jtc331@gmail.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: remove spurious CREATE INDEX CONCURRENTLY wait
In-Reply-To: <20201109154743.sij6j5ue3j673r7h@localhost>
* 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