public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4 2/2] Avoid spurious CREATE INDEX CONCURRENTLY waits
6+ messages / 3 participants
[nested] [flat]
* [PATCH v4 2/2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 92 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 ++-
2 files changed, 94 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 75552c64ed..4abb60ea44 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -94,6 +94,7 @@ static void ReindexMultipleInternal(List *relids, int options);
static void reindex_error_callback(void *args);
static void update_relispartition(Oid relationId, bool newval);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
+static void set_safe_index_flag(void);
/*
* callback argument type for RangeVarCallbackForReindexIndex()
@@ -385,7 +386,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 +410,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 +431,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 +525,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1045,6 +1052,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 +1449,10 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1490,6 +1512,10 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 3 of concurrent index build
*
@@ -1546,6 +1572,10 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
@@ -3021,6 +3051,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 +3355,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 +3441,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 2 of REINDEX CONCURRENTLY
*
@@ -3456,6 +3508,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
}
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 3 of REINDEX CONCURRENTLY
*
@@ -3559,6 +3615,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
forboth(lc, indexIds, lc2, newIndexIds)
{
char *oldName;
@@ -3609,6 +3669,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 5 of REINDEX CONCURRENTLY
*
@@ -3641,6 +3705,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ set_safe_index_flag();
+
/*
* Phase 6 of REINDEX CONCURRENTLY
*
@@ -3692,6 +3760,10 @@ 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)
+ set_safe_index_flag();
+
/* Log what we did */
if (options & REINDEXOPT_VERBOSE)
{
@@ -3896,3 +3968,17 @@ update_relispartition(Oid relationId, bool newval)
heap_freetuple(tup);
table_close(classRel, RowExclusiveLock);
}
+
+/*
+ * Set a PGPROC flag to tell concurrent VACUUM, CREATE INDEX CONCURRENTLY and
+ * REINDEX CONCURRENTLY to ignore us when waiting for concurrent snapshots.
+ * Should be called just after starting a transaction.
+ */
+static void
+set_safe_index_flag()
+{
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc->statusFlags |= PROC_IN_SAFE_IC;
+ ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
+ LWLockRelease(ProcArrayLock);
+}
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 63aea0e253..6664803e2a 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
--52xkrdy5w45ewuyh--
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pg17 issues with not-null contraints
@ 2024-04-18 16:23 Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Alvaro Herrera @ 2024-04-18 16:23 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]
On 2024-Apr-18, Justin Pryzby wrote:
> That seems like it could be important. I considered but never actually
> test your patch by pg_upgrading across major versions.
It would be a welcome contribution for sure. I've been doing it rather
haphazardly, which is not great.
> BTW, this works up to v16 (although maybe it should not):
>
> | CREATE TABLE ip(id int PRIMARY KEY); CREATE TABLE ic(id int) INHERITS (ip); ALTER TABLE ic ALTER id DROP NOT NULL;
>
> Under v17, this fails. Maybe that's okay, but it should probably be
> called out in the release notes.
Sure, we should mention that.
> | ERROR: cannot drop inherited constraint "ic_id_not_null" of relation "ic"
>
> That's the issue that I mentioned in the 6 year old thread. In the
> future (upgrading *from* v17) it won't be possible anymore, right?
Yeah, trying to drop the constraint in 17 fails as it should; it was one
of the goals of this whole thing in fact.
> It'd still be nice to detect the issue in advance rather than failing
> halfway through the upgrade.
Maybe we can have pg_upgrade --check look for cases we might have
trouble upgrading. (I mean: such cases would fail if you have rows with
nulls in the affected columns, but the schema upgrade should be
successful. Is that what you have in mind?)
> I have a rebased patch while I'll send on that thread. I guess it's
> mostly unrelated to your patch but it'd be nice if you could take a
> look.
Okay.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees." (E. Dijkstra)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pg17 issues with not-null contraints
@ 2024-04-18 16:52 Justin Pryzby <[email protected]>
parent: Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Justin Pryzby @ 2024-04-18 16:52 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]
On Thu, Apr 18, 2024 at 06:23:30PM +0200, Alvaro Herrera wrote:
> On 2024-Apr-18, Justin Pryzby wrote:
>
> > BTW, this works up to v16 (although maybe it should not):
> >
> > | CREATE TABLE ip(id int PRIMARY KEY); CREATE TABLE ic(id int) INHERITS (ip); ALTER TABLE ic ALTER id DROP NOT NULL;
>
> > It'd still be nice to detect the issue in advance rather than failing
> > halfway through the upgrade.
>
> Maybe we can have pg_upgrade --check look for cases we might have
> trouble upgrading. (I mean: such cases would fail if you have rows with
> nulls in the affected columns, but the schema upgrade should be
> successful. Is that what you have in mind?)
Before v16, pg_upgrade failed in the middle of restoring the schema,
without being caught during --check. The patch to implement that was
forgotten and never progressed.
I'm not totally clear on what's intended in v17 - maybe it'd be dead
code, and maybe it shouldn't even be applied to master branch. But I do
think it's worth patching earlier versions (even though it'll be less
useful than having done so 5 years ago).
--
Justin
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pg17 issues with not-null contraints
@ 2024-04-30 17:52 Robert Haas <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Robert Haas @ 2024-04-30 17:52 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Thu, Apr 18, 2024 at 12:52 PM Justin Pryzby <[email protected]> wrote:
> I'm not totally clear on what's intended in v17 - maybe it'd be dead
> code, and maybe it shouldn't even be applied to master branch. But I do
> think it's worth patching earlier versions (even though it'll be less
> useful than having done so 5 years ago).
This thread is still on the open items list, but I'm not sure whether
there's still stuff here that needs to be fixed for the current
release. If not, this thread should be moved to the "resolved before
17beta1" section. If so, we should try to reach consensus on what the
remaining issues are and what we're going to do about them.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pg17 issues with not-null contraints
@ 2024-04-30 18:52 Justin Pryzby <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Justin Pryzby @ 2024-04-30 18:52 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Tue, Apr 30, 2024 at 01:52:02PM -0400, Robert Haas wrote:
> On Thu, Apr 18, 2024 at 12:52 PM Justin Pryzby <[email protected]> wrote:
> > I'm not totally clear on what's intended in v17 - maybe it'd be dead
> > code, and maybe it shouldn't even be applied to master branch. But I do
> > think it's worth patching earlier versions (even though it'll be less
> > useful than having done so 5 years ago).
>
> This thread is still on the open items list, but I'm not sure whether
> there's still stuff here that needs to be fixed for the current
> release. If not, this thread should be moved to the "resolved before
> 17beta1" section. If so, we should try to reach consensus on what the
> remaining issues are and what we're going to do about them.
I think the only thing that's relevant for v17 is this:
On Tue, Apr 16, 2024 at 08:11:49PM +0200, Alvaro Herrera wrote:
> Speaking of which, I wonder if I should modify pg16's tests so that they
> leave behind tables set up in this way, to immortalize pg_upgrade testing.
The patch on the other thread for pg_upgrade --check is an old issue
affecting all stable releases.
--
Justin
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pg17 issues with not-null contraints
@ 2024-05-03 14:05 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Justin Pryzby @ 2024-05-03 14:05 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; Alexander Lakhin <[email protected]>; Dmitry Koval <[email protected]>
On another thread [0], Alexander Lakhin pointed out, indirectly, that
partitions created using LIKE+ATTACH now have different not-null constraints
from partitions created using PARTITION OF.
postgres=# CREATE TABLE t (i int PRIMARY KEY) PARTITION BY RANGE (i);
postgres=# CREATE TABLE t1 PARTITION OF t DEFAULT ;
postgres=# \d+ t1
...
Partition of: t DEFAULT
No partition constraint
Indexes:
"t1_pkey" PRIMARY KEY, btree (i)
Access method: heap
But if it's created with LIKE:
postgres=# CREATE TABLE t1 (LIKE t);
postgres=# ALTER TABLE t ATTACH PARTITION t1 DEFAULT ;
..one also sees:
Not-null constraints:
"t1_i_not_null" NOT NULL "i"
It looks like ATTACH may have an issue with constraints implied by pkey.
[0] https://www.postgresql.org/message-id/8034d1c6-5f0e-e858-9af9-45d5e246515e%40gmail.com
--
Justin
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-05-03 14:05 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-05 02:04 [PATCH v4 2/2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2024-04-18 16:23 Re: pg17 issues with not-null contraints Alvaro Herrera <[email protected]>
2024-04-18 16:52 ` Re: pg17 issues with not-null contraints Justin Pryzby <[email protected]>
2024-04-30 17:52 ` Re: pg17 issues with not-null contraints Robert Haas <[email protected]>
2024-04-30 18:52 ` Re: pg17 issues with not-null contraints Justin Pryzby <[email protected]>
2024-05-03 14:05 ` Re: pg17 issues with not-null contraints Justin Pryzby <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox