agora inbox for [email protected]
help / color / mirror / Atom feedRe: [PATCH]: fix bug in SP-GiST box_ops
30+ messages / 8 participants
[nested] [flat]
* Re: [PATCH]: fix bug in SP-GiST box_ops
@ 2017-03-09 14:04 Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Alexander Korotkov @ 2017-03-09 14:04 UTC (permalink / raw)
To: Emre Hasegeli <[email protected]>; +Cc: Kyotaro HORIGUCHI <[email protected]>; Nikita Glukhov <[email protected]>; pgsql-hackers
On Wed, Feb 1, 2017 at 2:43 PM, Emre Hasegeli <[email protected]> wrote:
> > I think this patch is already in a good shape.
>
> I am sorry for introducing this bug. This fix looks good to me as well.
I checked this patch too. And it seems good to me as well.
Should we mark it as "ready for committer"?
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: [PATCH]: fix bug in SP-GiST box_ops
@ 2017-03-09 23:13 Tels <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Tels @ 2017-03-09 23:13 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Emre Hasegeli <[email protected]>; Kyotaro HORIGUCHI <[email protected]>; Nikita Glukhov <[email protected]>; pgsql-hackers
Hello,
On Thu, March 9, 2017 9:04 am, Alexander Korotkov wrote:
> On Wed, Feb 1, 2017 at 2:43 PM, Emre Hasegeli <[email protected]> wrote:
>
>> > I think this patch is already in a good shape.
>>
>> I am sorry for introducing this bug. This fix looks good to me as well.
>
>
> I checked this patch too. And it seems good to me as well.
> Should we mark it as "ready for committer"?
I can't comment on the code, but the grammar on the comments caught my eye:
> +/* Can any range from range_box does not extend higher than this
argument? */
>
>+static bool
>+overLower2D(RangeBox *range_box, Range *query)
>+{
>+ return FPle(range_box->left.low, query->high) &&
>+ FPle(range_box->right.low, query->high);
>+}
The sentence sounds quite garbled in English. I'm not entirely sure what
it should be, but given the comment below "/* Can any range from range_box
to be higher than this argument? */" maybe something like:
/* Does any range from range_box extend to the right side of the query? */
If used, an analog wording should be used for overHigher2D's comment like:
/* Does any range from range_box extend to the left side of the query? */
Also:
/* Can any range from range_box to be higher than this argument? */
should be:
/* Can any range from range_box be higher than this argument? */
Another question: Does it make sense to add the "minimal bad example for
the '&<' case" as test case, too? After all, it should pass the test after
the patch.
Bets regards,
Tels
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: [PATCH]: fix bug in SP-GiST box_ops
@ 2017-03-10 09:15 Nikita Glukhov <[email protected]>
parent: Tels <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Nikita Glukhov @ 2017-03-10 09:15 UTC (permalink / raw)
To: Tels <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Emre Hasegeli <[email protected]>; Kyotaro HORIGUCHI <[email protected]>; pgsql-hackers
On 10.03.2017 02:13, Tels wrote:
> I can't comment on the code, but the grammar on the comments caught my eye:
>> +/* Can any range from range_box does not extend higher than this argument? */
>> +static bool
>> +overLower2D(RangeBox *range_box, Range *query)
>> +{
>> + return FPle(range_box->left.low, query->high) &&
>> + FPle(range_box->right.low, query->high);
>> +}
> The sentence sounds quite garbled in English. I'm not entirely sure what
> it should be, but given the comment below "/* Can any range from range_box
> to be higher than this argument? */" maybe something like:
>
> /* Does any range from range_box extend to the right side of the query? */
>
> If used, an analog wording should be used for overHigher2D's comment like:
>
> /* Does any range from range_box extend to the left side of the query? */
I've changed comments as you proposed, but I've added missing "not" and left "Can":
/* Can any range from range_box not extend to the right side of the query? */
/* Can any range from range_box not extend to the left side of the query? */
See also comments on overhigher and overlower operators from documentation:
&< Does not extend to the right of?
&> Does not extend to the left of?
> Another question: Does it make sense to add the "minimal bad example for
> the '&<' case" as test case, too? After all, it should pass the test after
> the patch.
Yes, it will make sense, but the Kyotaro's test case doesn't work for me and
I still don't know how to force SP-GiST to create inner leaves without
inserting hundreds of rows.
--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
[text/x-patch] fix-bug-in-SP-GiST-box_ops-v04.patch (7.0K, ../../[email protected]/2-fix-bug-in-SP-GiST-box_ops-v04.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/geo_spgist.c b/src/backend/utils/adt/geo_spgist.c
index aacb340..c95ec1c 100644
--- a/src/backend/utils/adt/geo_spgist.c
+++ b/src/backend/utils/adt/geo_spgist.c
@@ -286,6 +286,14 @@ lower2D(RangeBox *range_box, Range *query)
FPlt(range_box->right.low, query->low);
}
+/* Can any range from range_box not extend to the right side of the query? */
+static bool
+overLower2D(RangeBox *range_box, Range *query)
+{
+ return FPle(range_box->left.low, query->high) &&
+ FPle(range_box->right.low, query->high);
+}
+
/* Can any range from range_box to be higher than this argument? */
static bool
higher2D(RangeBox *range_box, Range *query)
@@ -294,6 +302,14 @@ higher2D(RangeBox *range_box, Range *query)
FPgt(range_box->right.high, query->high);
}
+/* Can any range from range_box not extend to the left side of the query? */
+static bool
+overHigher2D(RangeBox *range_box, Range *query)
+{
+ return FPge(range_box->left.high, query->low) &&
+ FPge(range_box->right.high, query->low);
+}
+
/* Can any rectangle from rect_box be left of this argument? */
static bool
left4D(RectBox *rect_box, RangeBox *query)
@@ -305,7 +321,7 @@ left4D(RectBox *rect_box, RangeBox *query)
static bool
overLeft4D(RectBox *rect_box, RangeBox *query)
{
- return lower2D(&rect_box->range_box_x, &query->right);
+ return overLower2D(&rect_box->range_box_x, &query->left);
}
/* Can any rectangle from rect_box be right of this argument? */
@@ -319,7 +335,7 @@ right4D(RectBox *rect_box, RangeBox *query)
static bool
overRight4D(RectBox *rect_box, RangeBox *query)
{
- return higher2D(&rect_box->range_box_x, &query->right);
+ return overHigher2D(&rect_box->range_box_x, &query->left);
}
/* Can any rectangle from rect_box be below of this argument? */
@@ -333,7 +349,7 @@ below4D(RectBox *rect_box, RangeBox *query)
static bool
overBelow4D(RectBox *rect_box, RangeBox *query)
{
- return lower2D(&rect_box->range_box_y, &query->left);
+ return overLower2D(&rect_box->range_box_y, &query->right);
}
/* Can any rectangle from rect_box be above of this argument? */
@@ -347,7 +363,7 @@ above4D(RectBox *rect_box, RangeBox *query)
static bool
overAbove4D(RectBox *rect_box, RangeBox *query)
{
- return higher2D(&rect_box->range_box_y, &query->right);
+ return overHigher2D(&rect_box->range_box_y, &query->right);
}
/*
diff --git a/src/test/regress/expected/box.out b/src/test/regress/expected/box.out
index 5f8b945..251df93 100644
--- a/src/test/regress/expected/box.out
+++ b/src/test/regress/expected/box.out
@@ -455,3 +455,108 @@ EXPLAIN (COSTS OFF) SELECT * FROM box_temp WHERE f1 ~= '(20,20),(40,40)';
RESET enable_seqscan;
DROP INDEX box_spgist;
+--
+-- Test the SP-GiST index on the larger volume of data
+--
+CREATE TEMPORARY TABLE quad_box_tbl (b box);
+INSERT INTO quad_box_tbl
+ SELECT box(point(x * 10, y * 10), point(x * 10 + 5, y * 10 + 5))
+ FROM generate_series(1, 100) x,
+ generate_series(1, 100) y;
+-- insert repeating data to test allTheSame
+INSERT INTO quad_box_tbl
+ SELECT '((200, 300),(210, 310))'
+ FROM generate_series(1, 1000);
+INSERT INTO quad_box_tbl
+ VALUES
+ (NULL),
+ (NULL),
+ ('((-infinity,-infinity),(infinity,infinity))'),
+ ('((-infinity,100),(-infinity,500))'),
+ ('((-infinity,-infinity),(700,infinity))');
+CREATE INDEX quad_box_tbl_idx ON quad_box_tbl USING spgist(b);
+SET enable_seqscan = OFF;
+SET enable_indexscan = ON;
+SET enable_bitmapscan = ON;
+SELECT count(*) FROM quad_box_tbl WHERE b << box '((100,200),(300,500))';
+ count
+-------
+ 901
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b &< box '((100,200),(300,500))';
+ count
+-------
+ 3901
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b && box '((100,200),(300,500))';
+ count
+-------
+ 1653
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b &> box '((100,200),(300,500))';
+ count
+-------
+ 10100
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b >> box '((100,200),(300,500))';
+ count
+-------
+ 7000
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b >> box '((100,200),(300,500))';
+ count
+-------
+ 7000
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b <<| box '((100,200),(300,500))';
+ count
+-------
+ 1900
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b &<| box '((100,200),(300,500))';
+ count
+-------
+ 5901
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b |&> box '((100,200),(300,500))';
+ count
+-------
+ 9100
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b |>> box '((100,200),(300,500))';
+ count
+-------
+ 5000
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b @> box '((201,301),(202,303))';
+ count
+-------
+ 1003
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b <@ box '((100,200),(300,500))';
+ count
+-------
+ 1600
+(1 row)
+
+SELECT count(*) FROM quad_box_tbl WHERE b ~= box '((200,300),(205,305))';
+ count
+-------
+ 1
+(1 row)
+
+RESET enable_seqscan;
+RESET enable_indexscan;
+RESET enable_bitmapscan;
+DROP INDEX quad_box_tbl_idx;
diff --git a/src/test/regress/sql/box.sql b/src/test/regress/sql/box.sql
index 128a016..37f05cb 100644
--- a/src/test/regress/sql/box.sql
+++ b/src/test/regress/sql/box.sql
@@ -179,3 +179,52 @@ EXPLAIN (COSTS OFF) SELECT * FROM box_temp WHERE f1 ~= '(20,20),(40,40)';
RESET enable_seqscan;
DROP INDEX box_spgist;
+
+--
+-- Test the SP-GiST index on the larger volume of data
+--
+CREATE TEMPORARY TABLE quad_box_tbl (b box);
+
+INSERT INTO quad_box_tbl
+ SELECT box(point(x * 10, y * 10), point(x * 10 + 5, y * 10 + 5))
+ FROM generate_series(1, 100) x,
+ generate_series(1, 100) y;
+
+-- insert repeating data to test allTheSame
+INSERT INTO quad_box_tbl
+ SELECT '((200, 300),(210, 310))'
+ FROM generate_series(1, 1000);
+
+INSERT INTO quad_box_tbl
+ VALUES
+ (NULL),
+ (NULL),
+ ('((-infinity,-infinity),(infinity,infinity))'),
+ ('((-infinity,100),(-infinity,500))'),
+ ('((-infinity,-infinity),(700,infinity))');
+
+CREATE INDEX quad_box_tbl_idx ON quad_box_tbl USING spgist(b);
+
+SET enable_seqscan = OFF;
+SET enable_indexscan = ON;
+SET enable_bitmapscan = ON;
+
+SELECT count(*) FROM quad_box_tbl WHERE b << box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b &< box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b && box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b &> box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b >> box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b >> box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b <<| box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b &<| box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b |&> box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b |>> box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b @> box '((201,301),(202,303))';
+SELECT count(*) FROM quad_box_tbl WHERE b <@ box '((100,200),(300,500))';
+SELECT count(*) FROM quad_box_tbl WHERE b ~= box '((200,300),(205,305))';
+
+RESET enable_seqscan;
+RESET enable_indexscan;
+RESET enable_bitmapscan;
+
+DROP INDEX quad_box_tbl_idx;
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: [PATCH]: fix bug in SP-GiST box_ops
@ 2017-03-17 06:16 Kyotaro HORIGUCHI <[email protected]>
parent: Nikita Glukhov <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Kyotaro HORIGUCHI @ 2017-03-17 06:16 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers
At Fri, 10 Mar 2017 12:15:52 +0300, Nikita Glukhov <[email protected]> wrote in <[email protected]>
> On 10.03.2017 02:13, Tels wrote:
>
> > I can't comment on the code, but the grammar on the comments caught my
> > eye:
> >> +/* Can any range from range_box does not extend higher than this
> >> argument? */
> >> +static bool
> >> +overLower2D(RangeBox *range_box, Range *query)
> >> +{
> >> + return FPle(range_box->left.low, query->high) &&
> >> + FPle(range_box->right.low, query->high);
> >> +}
> > The sentence sounds quite garbled in English. I'm not entirely sure
> > what
> > it should be, but given the comment below "/* Can any range from
> > range_box
> > to be higher than this argument? */" maybe something like:
> >
> > /* Does any range from range_box extend to the right side of the
> > query? */
> >
> > If used, an analog wording should be used for overHigher2D's comment
> > like:
> >
> > /* Does any range from range_box extend to the left side of the query?
> > */
>
> I've changed comments as you proposed, but I've added missing "not"
> and left "Can":
>
> /* Can any range from range_box not extend to the right side of the
> query? */
> /* Can any range from range_box not extend to the left side of the
> query? */
>
> See also comments on overhigher and overlower operators from
> documentation:
>
> &< Does not extend to the right of?
> &> Does not extend to the left of?
>
> > Another question: Does it make sense to add the "minimal bad example
> > for
> > the '&<' case" as test case, too? After all, it should pass the test
> > after
> > the patch.
>
> Yes, it will make sense, but the Kyotaro's test case doesn't work for
> me and
> I still don't know how to force SP-GiST to create inner leaves without
> inserting hundreds of rows.
I admit that the case is quite unstable, or environment-dependent
and the minimal bad case no longer replays even for me. On the
other hand, inserting some hundreds of boxes makes it more stable
than only one box. I'm not sure that it is enough but it seems to
be the best we can. As I mentioned previously, box cannot be used
as the key for outer join so comparing one-by-one is out of hand
of regression test.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: [PATCH]: fix bug in SP-GiST box_ops
@ 2017-03-21 08:49 Anastasia Lubennikova <[email protected]>
parent: Kyotaro HORIGUCHI <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Anastasia Lubennikova @ 2017-03-21 08:49 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Nikita Glukhov <[email protected]>
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
As I can see, this bugfix was already discussed and reviewed.
All mentioned issues are fixed in the latest version of the patch
So I mark it "Ready for committer".
Thank you for fixing it!
The new status of this patch is: Ready for Committer
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: [PATCH]: fix bug in SP-GiST box_ops
@ 2017-03-21 13:26 Teodor Sigaev <[email protected]>
parent: Anastasia Lubennikova <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Teodor Sigaev @ 2017-03-21 13:26 UTC (permalink / raw)
To: Anastasia Lubennikova <[email protected]>; pgsql-hackers; +Cc: Nikita Glukhov <[email protected]>
Thank you, pushed. I just make test table permanent.
Anastasia Lubennikova wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: tested, passed
> Documentation: not tested
>
> As I can see, this bugfix was already discussed and reviewed.
> All mentioned issues are fixed in the latest version of the patch
> So I mark it "Ready for committer".
> Thank you for fixing it!
>
> The new status of this patch is: Ready for Committer
>
--
Teodor Sigaev E-mail: [email protected]
WWW: http://www.sigaev.ru/
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v3] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
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.c
index 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.h
index 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
--5k2hsgqhdfnmvlms--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v4 2/2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ 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] 30+ messages in thread
* [PATCH v5] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 85 +++++++++++++++++++++++++++++---
src/include/storage/proc.h | 6 ++-
2 files changed, 83 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 35696f9f75..44ea84c54d 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@
/* non-export function prototypes */
+static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
Oid *typeOidP,
@@ -87,13 +88,12 @@ static char *ChooseIndexNameAddition(List *colnames);
static List *ChooseIndexColumnNames(List *indexElems);
static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
-static bool ReindexRelationConcurrently(Oid relationOid, int options);
-
+static void reindex_error_callback(void *args);
static void ReindexPartitions(Oid relid, int options, bool isTopLevel);
static void ReindexMultipleInternal(List *relids, int options);
-static void reindex_error_callback(void *args);
+static bool ReindexRelationConcurrently(Oid relationOid, int options);
static void update_relispartition(Oid relationId, bool newval);
-static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
+static inline void set_safe_index_flag(void);
/*
* callback argument type for RangeVarCallbackForReindexIndex()
@@ -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,10 @@ DefineIndex(Oid relationId,
}
}
+ /* Determine whether we can call set_safe_index_flag */
+ 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 +1441,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 +1504,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 +1564,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 +3043,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
PROGRESS_CREATEIDX_ACCESS_METHOD_OID
};
int64 progress_vals[4];
+ bool all_indexes_safe = true;
/*
* Create a memory context that will survive forced transaction commits we
@@ -3325,6 +3348,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
newIndexRel = index_open(newIndexId, ShareUpdateExclusiveLock);
+ /* consider safety of this index for set_safe_index_flag */
+ if (all_indexes_safe &&
+ (newIndexRel->rd_indexprs != NIL ||
+ newIndexRel->rd_indpred != NIL))
+ all_indexes_safe = false;
+
/*
* Save the list of OIDs and locks in private context
*/
@@ -3394,6 +3423,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (all_indexes_safe)
+ set_safe_index_flag();
+
/*
* Phase 2 of REINDEX CONCURRENTLY
*
@@ -3457,6 +3490,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
}
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (all_indexes_safe)
+ set_safe_index_flag();
+
/*
* Phase 3 of REINDEX CONCURRENTLY
*
@@ -3610,6 +3647,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (all_indexes_safe)
+ set_safe_index_flag();
+
/*
* Phase 5 of REINDEX CONCURRENTLY
*
@@ -3642,6 +3683,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (all_indexes_safe)
+ set_safe_index_flag();
+
/*
* Phase 6 of REINDEX CONCURRENTLY
*
@@ -3897,3 +3942,29 @@ update_relispartition(Oid relationId, bool newval)
heap_freetuple(tup);
table_close(classRel, RowExclusiveLock);
}
+
+/*
+ * Set the PROC_IN_SAFE_IC flag in my PGPROC entry.
+ *
+ * When doing concurrent index builds, we can set this flag
+ * to tell other processes concurrently running VACUUM, CREATE
+ * INDEX CONCURRENTLY and REINDEX CONCURRENTLY to ignore us when
+ * doing their waits for concurrent snapshots. On one hand it
+ * avoids pointlessly waiting for a process that's not interesting
+ * anyway, but more importantly it avoids deadlocks in some cases.
+ *
+ * This can only be done for indexes that don't execute any expressions.
+ * Caller is responsible for only calling this routine when that
+ * assumption holds true.
+ *
+ * (The flag is reset automatically at transaction end, so it must be
+ * set for each transaction.)
+ */
+static inline void
+set_safe_index_flag(void)
+{
+ 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 1067f58f51..b2347ffd79 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.20.1
--rwEMma7ioTxnRzrJ--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits
@ 2020-08-05 02:04 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Alvaro Herrera @ 2020-08-05 02:04 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++++++++++--
src/include/storage/proc.h | 6 +++-
2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..459f6fa5db 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -372,7 +372,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.
@@ -393,7 +396,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_CIC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -413,7 +417,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_CIC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -506,6 +511,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1033,6 +1039,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)
@@ -1419,6 +1436,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1478,6 +1504,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Phase 3 of concurrent index build
*
@@ -1534,6 +1569,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_CIC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae45..d91e199a60 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_CIC 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_CIC | PROC_VACUUM_FOR_WRAPAROUND)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
--
2.20.1
--nFreZHaLTZJo0R7j--
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Avoiding superfluous buffer locking during nbtree backwards scans
@ 2024-10-08 16:52 Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Peter Geoghegan @ 2024-10-08 16:52 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
On Fri, Jul 5, 2024 at 12:47 PM Peter Geoghegan <[email protected]> wrote:
> On HEAD, the following query requires 24 buffer hits (I'm getting a
> standard/forward index scan for this):
>
> select
> abalance
> from
> pgbench_accounts
> where
> aid in (1, 500, 1000, 1500, 2000, 3000) order by aid asc;
>
> However, we don't see that with the backwards scan variant:
>
> select
> abalance
> from
> pgbench_accounts
> where
> aid in (1, 500, 1000, 1500, 2000, 3000) order by aid desc;
>
> We actually see 30 buffer hits for this on HEAD. Whereas with the
> patch, both variants get only 24 buffer hits -- there's parity, at
> least in cases like these two.
I'll now present a similar example that shows the remaining issue that
my patch (which became my commit 3f44959f) didn't address, and how the
issue if fixed by Matthias' more recent follow-up patch:
$ pgbench -i -s 1
Now run:
explain (analyze, buffers, costs off, summary off)
select
abalance
from
pgbench_accounts order by aid;
With a warm cache, this query gets a full index scan requiring 1915
buffer hits. However, we still see worse performance for an equivalent
backwards scan:
explain (analyze, buffers, costs off, summary off)
select
abalance
from
pgbench_accounts order by aid desc;
On master, the latter query gets 2188 buffer hits -- so clearly my
commit 3f44959f left money on the table. These extra buffer hits just
don't make sense.
Matthias' patch (I tested
v3-0001-Avoid-unneeded-nbtree-backwards-scan-buffer-locks.patch) will
result in both variants requiring 1915 buffer hits. (Plus, of course,
query execution should be faster with Matthias' new approach.)
Though we do need special extra steps for dealing with concurrent page
splits during backwards scans (steps involving a reread of the
original page when its sibling link turns out to be stale), there
hasn't been a concurrent split here -- which is why I find the
inconsistency to be unnatural. Matthias' patch fixes the problem by
passing _bt_walk_left the left link that will now have been stashed in
the local scan state back when the page was read by _bt_readpage, so
that it (_bt_walk_left) can optimistically *start* there (and *not*
start at the page that has already been read, as on the master
branch).
Of course, the left link might be stale, but it was always inherently
necessary for _bt_walk_left to be able to deal with that. Technically
we're being more optimistic about it now, but that optimism is
extremely well justified (concurrent page splits are rare). Arguably,
this latest patch from Matthias actually makes the code simpler. It
makes the backwards scan case more similar to the forwards scan case.
This is another missed opportunity for commit 2ed5b87f96, I'd say --
that 2015 commit left things in a slightly odd in-between state, which
this patch fully corrects.
Note: my example was deliberately constructed to use an index scan
backwards, rather than an index-only scan backwards. While both types
of backwards scan will benefit in the same way (less buffer lock
traffic), you'll only see a reduction in buffers hit for an
EXPLAIN(ANALYZE, BUFFERS) involving a plain index scan backwards. This
is due to the fact that the mechanism added by commit 2ed5b87f96 will
only drop a leaf page buffer pin early for plain index scans, never
for index-only scans. My example also wouldn't make the difference
apparent with an unlogged table, for the same reason -- this
difference isn't really important, but seems worth noting to avoid any
confusion.
A nice side benefit of this approach is that it'll make it easier to
add better coverage for _bt_walk_left. The case where _bt_walk_left
notices a stale left link will still be very rare (we're really not
being all that optimistic in doing things this way), but it'll now be
easier to hit on purpose.
The code in v3-0001-Avoid-unneeded-nbtree-backwards-scan-buffer-locks.patch
looks reasonable to me. I don't think that there are any impediments
to committing it sometime this week.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: Avoiding superfluous buffer locking during nbtree backwards scans
@ 2024-10-10 17:41 Peter Geoghegan <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Peter Geoghegan @ 2024-10-10 17:41 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Tomas Vondra <[email protected]>
On Tue, Oct 8, 2024 at 12:52 PM Peter Geoghegan <[email protected]> wrote:
> The code in v3-0001-Avoid-unneeded-nbtree-backwards-scan-buffer-locks.patch
> looks reasonable to me. I don't think that there are any impediments
> to committing it sometime this week.
I see significant benefits for parallel index-only backwards scans
with this patch. It's not that hard to see about a 10% decrease in
execution time. I imagine that there are disproportionate benefits for
the parallel case because the work we're avoiding is work that takes
place while a backend has seized the scan. Not bad!
I've been looking at this patch in more detail. I think that you
(Matthias) have gone too far in the direction of making the backwards
scan handling within _bt_readnextpage match the forwards scan
handling. You fail to update blkno when we have to deal with a
concurrent split/deletion that necessitates reading from another blkno
(when the correct left sibling page to read from changes) -- v3 still
used the potentially-stale blkno passed by _bt_readnextpage's caller.
Attached v4 deals with that issue. It also goes further in the
direction of simplifying the code in and around _bt_readnextpage.
The code in this area has always seemed to me to be quite a lot more
complicated than it really needed to be. Now seems like a good time to
refactor and simplify. It's likely that we'll need to do more work
nearby to enable prefetching during plain index scans, without
regressing the kill_prior_tuple/_bt_killitems mechanism -- Tomas
Vondra is currently working on that. The mechanism in question is
closely tied to the stuff that I'm simplifying now, particularly the
_bt_drop_lock_and_maybe_pin/TID-recycle-safety business.
The main simplification new to v4 is that v4 isolates the need to call
_bt_drop_lock_and_maybe_pin to only 2 functions: _bt_readnextpage, and
its new _bt_readfirstpage "sibling" function. The functions have
similar preconditions, and identical postconditions -- all of which
are now clearly documented.
It's not just _bt_drop_lock_and_maybe_pin that's only called from
these 2 functions in v4. All calls to _bt_readpage (plus their
associated PredicateLockPage calls) are now also isolated to those
same 2 functions. This shift enables removing the
_bt_parallel_readpage function entirely -- the point at the top of
_bt_first where _bt_parallel_readpage used to be called can now call
_bt_readnextpage directly instead. ISTM that having a separate
_bt_parallel_readpage function never made much sense -- it was
papering over the fact that the interfaces it uses are so confusing.
v4 also demotes _bt_steppage to a helper function that sets up a call
to _bt_readnextpage. Note that the 2017 parallel index scan commit
split _bt_steppage in two: it became the current _bt_steppage code,
plus the current _bt_readnextpage code. Making that relationship
explicit seems much clearer. I'm essentially finishing off the process
of splitting _bt_steppage up.
Side note: all of these simplifications were enabled by making the
preconditions for calling _bt_readnextpage work independently of the
scan direction (which was present in earlier versions of the patch
from Matthias). We now always do a
"BTScanPosUnpinIfPinned(so->currPos)" within _bt_steppage, so it's
easy to talk clearly about the preconditions/postconditions for
_bt_readnextpage without directly considering its _bt_steppage caller
(still the main caller, including during parallel index scans).
Performance improvement bonus: We do that
"BTScanPosUnpinIfPinned(so->currPos)" at a slightly different point in
_bt_steppage in v4, as compared to master, even during forwards scans:
we consistently unpin *before* the required next call to
_bt_parallel_seize takes place (unless, of course, it's a serial scan,
which'll never need to call _bt_parallel_seize). Presumably this is
why I see a ~5% increase in throughput for parallel index-only forward
(standard) scans with this v4 -- all parallel scans (backwards and
forwards alike) now receive at least some benefit from this patch. It
makes a certain amount of sense; we should do as little as possible
during any window where our backend has seized the scan.
I do need to do more testing, but the direction of v4 seems promising.
--
Peter Geoghegan
Attachments:
[application/octet-stream] v4-0001-Optimize-and-simplify-nbtree-backwards-scans.patch (33.3K, ../../CAH2-Wzmq9NHGbx4zuP6Yn4LO2cJK=gQuK318QuXq4QMqmQh4WA@mail.gmail.com/2-v4-0001-Optimize-and-simplify-nbtree-backwards-scans.patch)
download | inline diff:
From d864b395f0c0012312d518c28787ae40dcdbe5b1 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <[email protected]>
Date: Tue, 8 Oct 2024 11:40:54 -0400
Subject: [PATCH v4] Optimize and simplify nbtree backwards scans.
---
src/include/access/nbtree.h | 11 +-
src/backend/access/nbtree/nbtree.c | 40 ++-
src/backend/access/nbtree/nbtsearch.c | 463 +++++++++++++-------------
src/backend/access/nbtree/nbtutils.c | 2 +-
4 files changed, 267 insertions(+), 249 deletions(-)
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index d64300fb9..c082bdf64 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -954,6 +954,7 @@ typedef struct BTScanPosData
XLogRecPtr lsn; /* pos in the WAL stream when page was read */
BlockNumber currPage; /* page referenced by items array */
+ BlockNumber prevPage; /* page's left link when we scanned it */
BlockNumber nextPage; /* page's right link when we scanned it */
/*
@@ -1022,6 +1023,7 @@ typedef BTScanPosData *BTScanPos;
#define BTScanPosInvalidate(scanpos) \
do { \
(scanpos).currPage = InvalidBlockNumber; \
+ (scanpos).prevPage = InvalidBlockNumber; \
(scanpos).nextPage = InvalidBlockNumber; \
(scanpos).buf = InvalidBuffer; \
(scanpos).lsn = InvalidXLogRecPtr; \
@@ -1090,7 +1092,6 @@ typedef struct BTReadPageState
OffsetNumber minoff; /* Lowest non-pivot tuple's offset */
OffsetNumber maxoff; /* Highest non-pivot tuple's offset */
IndexTuple finaltup; /* Needed by scans with array keys */
- BlockNumber prev_scan_page; /* previous _bt_parallel_release block */
Page page; /* Page being read */
/* Per-tuple input parameters, set by _bt_readpage for _bt_checkkeys */
@@ -1192,11 +1193,13 @@ extern int btgettreeheight(Relation rel);
* prototypes for internal functions in nbtree.c
*/
extern bool _bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno,
- bool first);
-extern void _bt_parallel_release(IndexScanDesc scan, BlockNumber scan_page);
+ BlockNumber *currblkno, bool first);
+extern void _bt_parallel_release(IndexScanDesc scan,
+ BlockNumber next_scan_page,
+ BlockNumber curr_page);
extern void _bt_parallel_done(IndexScanDesc scan);
extern void _bt_parallel_primscan_schedule(IndexScanDesc scan,
- BlockNumber prev_scan_page);
+ BlockNumber curr_page);
/*
* prototypes for functions in nbtdedup.c
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 56e502c4f..b926b99ba 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -66,7 +66,9 @@ typedef enum
*/
typedef struct BTParallelScanDescData
{
- BlockNumber btps_scanPage; /* latest or next page to be scanned */
+ BlockNumber btps_nextScanPage; /* next page to be scanned */
+ BlockNumber btps_lastCurrPage; /* page whose sibling link was copied into
+ * btps_scanPage */
BTPS_State btps_pageStatus; /* indicates whether next page is
* available for scan. see above for
* possible states of parallel scan. */
@@ -548,7 +550,8 @@ btinitparallelscan(void *target)
BTParallelScanDesc bt_target = (BTParallelScanDesc) target;
SpinLockInit(&bt_target->btps_mutex);
- bt_target->btps_scanPage = InvalidBlockNumber;
+ bt_target->btps_nextScanPage = InvalidBlockNumber;
+ bt_target->btps_lastCurrPage = InvalidBlockNumber;
bt_target->btps_pageStatus = BTPARALLEL_NOT_INITIALIZED;
ConditionVariableInit(&bt_target->btps_cv);
}
@@ -573,7 +576,8 @@ btparallelrescan(IndexScanDesc scan)
* consistency.
*/
SpinLockAcquire(&btscan->btps_mutex);
- btscan->btps_scanPage = InvalidBlockNumber;
+ btscan->btps_nextScanPage = InvalidBlockNumber;
+ btscan->btps_lastCurrPage = InvalidBlockNumber;
btscan->btps_pageStatus = BTPARALLEL_NOT_INITIALIZED;
SpinLockRelease(&btscan->btps_mutex);
}
@@ -600,7 +604,8 @@ btparallelrescan(IndexScanDesc scan)
* Callers should ignore the value of pageno if the return value is false.
*/
bool
-_bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno, bool first)
+_bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno,
+ BlockNumber *currblkno, bool first)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
bool exit_loop = false;
@@ -609,6 +614,7 @@ _bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno, bool first)
BTParallelScanDesc btscan;
*pageno = P_NONE;
+ *currblkno = InvalidBlockNumber;
if (first)
{
@@ -684,7 +690,8 @@ _bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno, bool first)
* of advancing it to a new page!
*/
btscan->btps_pageStatus = BTPARALLEL_ADVANCING;
- *pageno = btscan->btps_scanPage;
+ *pageno = btscan->btps_nextScanPage;
+ *currblkno = btscan->btps_lastCurrPage;
exit_loop = true;
}
SpinLockRelease(&btscan->btps_mutex);
@@ -699,17 +706,22 @@ _bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno, bool first)
/*
* _bt_parallel_release() -- Complete the process of advancing the scan to a
- * new page. We now have the new value btps_scanPage; some other backend
+ * new page. We now have the new value btps_nextScanPage; another backend
* can now begin advancing the scan.
*
- * Callers whose scan uses array keys must save their scan_page argument so
+ * Callers whose scan uses array keys must save their curr_page argument so
* that it can be passed to _bt_parallel_primscan_schedule, should caller
* determine that another primitive index scan is required. If that happens,
* scan_page won't be scanned by any backend (unless the next primitive index
* scan lands on scan_page).
+ *
+ * Note: unlike the serial case, parallel scans don't need to remember both
+ * sibling links. Caller can just pass us whichever link is next for the
+ * current scan direction because it cannot change within a parallel scan.
*/
void
-_bt_parallel_release(IndexScanDesc scan, BlockNumber scan_page)
+_bt_parallel_release(IndexScanDesc scan, BlockNumber next_scan_page,
+ BlockNumber curr_page)
{
ParallelIndexScanDesc parallel_scan = scan->parallel_scan;
BTParallelScanDesc btscan;
@@ -718,7 +730,8 @@ _bt_parallel_release(IndexScanDesc scan, BlockNumber scan_page)
parallel_scan->ps_offset);
SpinLockAcquire(&btscan->btps_mutex);
- btscan->btps_scanPage = scan_page;
+ btscan->btps_nextScanPage = next_scan_page;
+ btscan->btps_lastCurrPage = curr_page;
btscan->btps_pageStatus = BTPARALLEL_IDLE;
SpinLockRelease(&btscan->btps_mutex);
ConditionVariableSignal(&btscan->btps_cv);
@@ -774,13 +787,13 @@ _bt_parallel_done(IndexScanDesc scan)
/*
* _bt_parallel_primscan_schedule() -- Schedule another primitive index scan.
*
- * Caller passes the block number most recently passed to _bt_parallel_release
+ * Caller passes the curr_page most recently passed to _bt_parallel_release
* by its backend. Caller successfully schedules the next primitive index scan
* if the shared parallel state hasn't been seized since caller's backend last
* advanced the scan.
*/
void
-_bt_parallel_primscan_schedule(IndexScanDesc scan, BlockNumber prev_scan_page)
+_bt_parallel_primscan_schedule(IndexScanDesc scan, BlockNumber curr_page)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
ParallelIndexScanDesc parallel_scan = scan->parallel_scan;
@@ -792,10 +805,11 @@ _bt_parallel_primscan_schedule(IndexScanDesc scan, BlockNumber prev_scan_page)
parallel_scan->ps_offset);
SpinLockAcquire(&btscan->btps_mutex);
- if (btscan->btps_scanPage == prev_scan_page &&
+ if (btscan->btps_lastCurrPage == curr_page &&
btscan->btps_pageStatus == BTPARALLEL_IDLE)
{
- btscan->btps_scanPage = InvalidBlockNumber;
+ btscan->btps_nextScanPage = InvalidBlockNumber;
+ btscan->btps_lastCurrPage = InvalidBlockNumber;
btscan->btps_pageStatus = BTPARALLEL_NEED_PRIMSCAN;
/* Serialize scan's current array keys */
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index fff7c89ea..047c891d8 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -43,10 +43,12 @@ static inline void _bt_savepostingitem(BTScanOpaque so, int itemIndex,
OffsetNumber offnum,
ItemPointer heapTid, int tupleOffset);
static bool _bt_steppage(IndexScanDesc scan, ScanDirection dir);
-static bool _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir);
-static bool _bt_parallel_readpage(IndexScanDesc scan, BlockNumber blkno,
- ScanDirection dir);
-static Buffer _bt_walk_left(Relation rel, Buffer buf);
+static bool _bt_readfirstpage(IndexScanDesc scan, ScanDirection dir,
+ OffsetNumber offnum);
+static bool _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno,
+ BlockNumber lastcurrblkno, ScanDirection dir);
+static Buffer _bt_lock_and_validate_left(Relation rel, BlockNumber *blkno,
+ BlockNumber lastcurrblkno);
static bool _bt_endpoint(IndexScanDesc scan, ScanDirection dir);
static inline void _bt_initialize_more_data(BTScanOpaque so, ScanDirection dir);
@@ -924,7 +926,9 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
*/
if (scan->parallel_scan != NULL)
{
- status = _bt_parallel_seize(scan, &blkno, true);
+ BlockNumber lastcurrblkno;
+
+ status = _bt_parallel_seize(scan, &blkno, &lastcurrblkno, true);
/*
* Initialize arrays (when _bt_parallel_seize didn't already set up
@@ -942,7 +946,11 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
}
else if (blkno != InvalidBlockNumber)
{
- if (!_bt_parallel_readpage(scan, blkno, dir))
+ Assert(!so->needPrimScan);
+
+ _bt_initialize_more_data(so, dir);
+
+ if (!_bt_readnextpage(scan, blkno, lastcurrblkno, dir))
return false;
goto readcomplete;
}
@@ -1427,8 +1435,6 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
}
}
- PredicateLockPage(rel, BufferGetBlockNumber(buf), scan->xs_snapshot);
-
_bt_initialize_more_data(so, dir);
/* position to the precise item on the page */
@@ -1455,21 +1461,8 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
* for the page. For example, when inskey is both < the leaf page's high
* key and > all of its non-pivot tuples, offnum will be "maxoff + 1".
*/
- if (!_bt_readpage(scan, dir, offnum, true))
- {
- /*
- * There's no actually-matching data on this page. Try to advance to
- * the next page. Return false if there's no matching data at all.
- */
- _bt_unlockbuf(scan->indexRelation, so->currPos.buf);
- if (!_bt_steppage(scan, dir))
- return false;
- }
- else
- {
- /* We have at least one item to return as scan's first item */
- _bt_drop_lock_and_maybe_pin(scan, &so->currPos);
- }
+ if (!_bt_readfirstpage(scan, dir, offnum))
+ return false;
readcomplete:
/* OK, itemIndex says what to return */
@@ -1582,17 +1575,6 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
page = BufferGetPage(so->currPos.buf);
opaque = BTPageGetOpaque(page);
- /* allow next page be processed by parallel worker */
- if (scan->parallel_scan)
- {
- if (ScanDirectionIsForward(dir))
- pstate.prev_scan_page = opaque->btpo_next;
- else
- pstate.prev_scan_page = BufferGetBlockNumber(so->currPos.buf);
-
- _bt_parallel_release(scan, pstate.prev_scan_page);
- }
-
indnatts = IndexRelationGetNumberOfAttributes(scan->indexRelation);
arrayKeys = so->numArrayKeys != 0;
minoff = P_FIRSTDATAKEY(opaque);
@@ -1626,10 +1608,10 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
so->currPos.lsn = BufferGetLSNAtomic(so->currPos.buf);
/*
- * we must save the page's right-link while scanning it; this tells us
- * where to step right to after we're done with these items. There is no
- * corresponding need for the left-link, since splits always go right.
+ * we must save the page's right-link and left-link; this tells us where
+ * to step right/left to after we're done with these items
*/
+ so->currPos.prevPage = opaque->btpo_prev;
so->currPos.nextPage = opaque->btpo_next;
/* initialize tuple workspace to empty */
@@ -1640,6 +1622,18 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
* good.
*/
Assert(BTScanPosIsPinned(so->currPos));
+ Assert(!P_IGNORE(opaque));
+
+ /* allow next page to be processed by parallel worker */
+ if (scan->parallel_scan)
+ {
+ if (ScanDirectionIsForward(dir))
+ _bt_parallel_release(scan, so->currPos.nextPage,
+ so->currPos.currPage);
+ else
+ _bt_parallel_release(scan, so->currPos.prevPage,
+ so->currPos.currPage);
+ }
/*
* Prechecking the value of the continuescan flag for the last item on the
@@ -1934,7 +1928,7 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
* We don't need to visit page to the left when no more matches will
* be found there
*/
- if (!pstate.continuescan || P_LEFTMOST(opaque))
+ if (!pstate.continuescan)
so->currPos.moreLeft = false;
Assert(itemIndex >= 0);
@@ -2035,19 +2029,21 @@ _bt_savepostingitem(BTScanOpaque so, int itemIndex, OffsetNumber offnum,
/*
* _bt_steppage() -- Step to next page containing valid data for scan
*
- * On entry, if so->currPos.buf is valid the buffer is pinned but not locked;
- * if pinned, we'll drop the pin before moving to next page. The buffer is
- * not locked on entry.
+ * On entry, if so->currPos.buf is valid the buffer is pinned but not locked.
+ * This is a wrapper on _bt_readnextpage that performs final steps for the
+ * current page (including dropping its pin). It sets up the _bt_readnextpage
+ * call using either local state saved by the last _bt_readpage call, or using
+ * shared state consumed by seizing the parallel scan.
*
- * For success on a scan using a non-MVCC snapshot we hold a pin, but not a
- * read lock, on that page. If we do not hold the pin, we set so->currPos.buf
- * to InvalidBuffer. We return true to indicate success.
+ * Parallel scan callers that have already seized the scan should directly
+ * call _bt_readnextpage, rather than calling here.
*/
static bool
_bt_steppage(IndexScanDesc scan, ScanDirection dir)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
BlockNumber blkno = InvalidBlockNumber;
+ BlockNumber lastcurrblkno;
bool status;
Assert(BTScanPosIsValid(so->currPos));
@@ -2098,85 +2094,139 @@ _bt_steppage(IndexScanDesc scan, ScanDirection dir)
else
so->markPos.moreLeft = true;
}
+
+ /* mark/restore not supported by parallel scans */
+ Assert(!scan->parallel_scan);
}
- if (ScanDirectionIsForward(dir))
+ /* release the previously pinned leaf page buffer, if any */
+ BTScanPosUnpinIfPinned(so->currPos);
+
+ /* Walk to the next page with data */
+ if (!scan->parallel_scan)
{
- /* Walk right to the next page with data */
- if (scan->parallel_scan != NULL)
+ /* Not parallel, so use nextPage/prevPage from local scan state */
+ if (ScanDirectionIsForward(dir))
{
- /*
- * Seize the scan to get the next block number; if the scan has
- * ended already, bail out.
- */
- status = _bt_parallel_seize(scan, &blkno, false);
- if (!status)
- {
- /* release the previous buffer, if pinned */
- BTScanPosUnpinIfPinned(so->currPos);
- BTScanPosInvalidate(so->currPos);
- return false;
- }
+ blkno = so->currPos.nextPage;
+ /* Remember we left a page with data */
+ so->currPos.moreLeft = true;
}
else
{
- /* Not parallel, so use the previously-saved nextPage link. */
- blkno = so->currPos.nextPage;
+ blkno = so->currPos.prevPage;
+ /* Remember we left a page with data */
+ so->currPos.moreRight = true;
}
-
- /* Remember we left a page with data */
- so->currPos.moreLeft = true;
-
- /* release the previous buffer, if pinned */
- BTScanPosUnpinIfPinned(so->currPos);
+ lastcurrblkno = so->currPos.currPage;
}
else
{
- /* Remember we left a page with data */
- so->currPos.moreRight = true;
+ /*
+ * Seize the scan to get the nextPage and currPage from shared
+ * parallel state
+ */
+ status = _bt_parallel_seize(scan, &blkno, &lastcurrblkno, false);
+ if (!status)
+ {
+ BTScanPosInvalidate(so->currPos);
+ return false;
+ }
- if (scan->parallel_scan != NULL)
- {
- /*
- * Seize the scan to get the current block number; if the scan has
- * ended already, bail out.
- */
- status = _bt_parallel_seize(scan, &blkno, false);
- BTScanPosUnpinIfPinned(so->currPos);
- if (!status)
- {
- BTScanPosInvalidate(so->currPos);
- return false;
- }
- }
- else
- {
- /* Not parallel, so just use our own notion of the current page */
- blkno = so->currPos.currPage;
- }
+ /* no point in remembering we left a page with data here */
}
- if (!_bt_readnextpage(scan, blkno, dir))
+ if (!_bt_readnextpage(scan, blkno, lastcurrblkno, dir))
return false;
- /* We have at least one item to return as scan's next item */
- _bt_drop_lock_and_maybe_pin(scan, &so->currPos);
+ return true;
+}
+/*
+ * _bt_readfirstpage() -- Read first page containing valid data for scan
+ *
+ * Caller passes us a page offset number that's the first tuple that'll be
+ * read by _bt_readpage. This offset typically comes from _bt_binsrch.
+ *
+ * On entry, so->currPos.buf must be pinned and locked. Also, parallel scan
+ * callers must have seized the scan before calling here.
+ *
+ * On success exit, so->currPos is updated to contain data from the next
+ * interesting page, and we return true. We hold a pin on the buffer on
+ * success exit, except for scans that _bt_drop_lock_and_maybe_pin indicates
+ * can safely have their pin dropped to avoid blocking progress by VACUUM.
+ *
+ * If there are no more matching records in the given direction, we set
+ * so->currPos.buf to InvalidBuffer, and return false. Note that this could
+ * just signal the end of the current primitive index scan.
+ *
+ * We always release the scan for a parallel scan caller, regardless of
+ * success or failure; we do this by calling _bt_parallel_release at the
+ * earliest opportunity.
+ */
+static bool
+_bt_readfirstpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum)
+{
+ Relation rel = scan->indexRelation;
+ BTScanOpaque so = (BTScanOpaque) scan->opaque;
+
+ Assert(BufferIsValid(so->currPos.buf));
+ Assert(!P_IGNORE(BTPageGetOpaque(BufferGetPage(so->currPos.buf))));
+
+ PredicateLockPage(rel, BufferGetBlockNumber(so->currPos.buf),
+ scan->xs_snapshot);
+
+ if (_bt_readpage(scan, dir, offnum, true))
+ {
+ /* We have at least one item to return as scan's first item */
+ _bt_drop_lock_and_maybe_pin(scan, &so->currPos);
+ return true;
+ }
+
+ /*
+ * There's no actually-matching data on this page. Try to advance to the
+ * next page.
+ */
+ _bt_unlockbuf(scan->indexRelation, so->currPos.buf);
+ if (!_bt_steppage(scan, dir))
+ return false;
+
+ /*
+ * We have at least one item to return as scan's first item (just not from
+ * the first page it read). No need to call _bt_drop_lock_and_maybe_pin
+ * here; _bt_readnextpage already did that for us.
+ */
return true;
}
/*
* _bt_readnextpage() -- Read next page containing valid data for scan
*
- * On success exit, so->currPos is updated to contain data from the next
- * interesting page, and we return true. Caller must release the lock (and
- * maybe the pin) on the buffer on success exit.
+ * caller's blkno is the next interesting page's link, taken from either the
+ * previously-saved right link or left link. lastcurrblkno is the page that
+ * was current at the point where the blkno link was saved, which we somtimes
+ * use to reason about concurrent page splits/page deletions (this is only
+ * required by backwards scans, but we always have it, just to be consistent).
*
- * If there are no more matching records in the given direction, we drop all
- * locks and pins, set so->currPos.buf to InvalidBuffer, and return false.
+ * On entry, so->currPos.buf must not be pinned or locked. Also, parallel
+ * scan callers must have seized the scan before calling here.
+ *
+ * On success exit, so->currPos is updated to contain data from the next
+ * interesting page, and we return true. We hold a pin on the buffer on
+ * success exit, except for scans that _bt_drop_lock_and_maybe_pin indicates
+ * can safely have their pin dropped to avoid blocking progress by VACUUM.
+ *
+ * If there are no more matching records in the given direction, we set
+ * so->currPos.buf to InvalidBuffer, and return false. Note that this could
+ * just signal the end of the current primitive index scan.
+ *
+ * We always release the scan for a parallel scan caller, regardless of
+ * success or failure; we do this by calling _bt_parallel_release at the
+ * earliest opportunity.
*/
static bool
-_bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
+_bt_readnextpage(IndexScanDesc scan, BlockNumber blkno,
+ BlockNumber lastcurrblkno, ScanDirection dir)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
Relation rel;
@@ -2186,6 +2236,8 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
rel = scan->indexRelation;
+ Assert(!BTScanPosIsPinned(so->currPos));
+
if (ScanDirectionIsForward(dir))
{
for (;;)
@@ -2217,84 +2269,54 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
}
else if (scan->parallel_scan != NULL)
{
- /* allow next page be processed by parallel worker */
- _bt_parallel_release(scan, opaque->btpo_next);
+ /* _bt_readpage not called, so do this for ourselves */
+ _bt_parallel_release(scan, opaque->btpo_next, blkno);
}
- /* nope, keep going */
if (scan->parallel_scan != NULL)
{
+ /* no matching tuples, so seize a page to the right */
_bt_relbuf(rel, so->currPos.buf);
- status = _bt_parallel_seize(scan, &blkno, false);
+ status = _bt_parallel_seize(scan, &blkno, &lastcurrblkno, false);
if (!status)
{
+ /* don't call _bt_parallel_done */
BTScanPosInvalidate(so->currPos);
return false;
}
}
else
{
+ /* no matching tuples, so consider following right link */
+ lastcurrblkno = blkno;
blkno = opaque->btpo_next;
_bt_relbuf(rel, so->currPos.buf);
+ so->currPos.buf = InvalidBuffer;
}
}
}
else
{
- /*
- * Should only happen in parallel cases, when some other backend
- * advanced the scan.
- */
- if (so->currPos.currPage != blkno)
- {
- BTScanPosUnpinIfPinned(so->currPos);
- so->currPos.currPage = blkno;
- }
-
- /* Done if we know that the left sibling link isn't of interest */
- if (!so->currPos.moreLeft)
- {
- BTScanPosUnpinIfPinned(so->currPos);
- _bt_parallel_done(scan);
- BTScanPosInvalidate(so->currPos);
- return false;
- }
-
- /*
- * Walk left to the next page with data. This is much more complex
- * than the walk-right case because of the possibility that the page
- * to our left splits while we are in flight to it, plus the
- * possibility that the page we were on gets deleted after we leave
- * it. See nbtree/README for details.
- *
- * It might be possible to rearrange this code to have less overhead
- * in pinning and locking, but that would require capturing the left
- * sibling block number when the page is initially read, and then
- * optimistically starting there (rather than pinning the page twice).
- * It is not clear that this would be worth the complexity.
- */
- if (BTScanPosIsPinned(so->currPos))
- _bt_lockbuf(rel, so->currPos.buf, BT_READ);
- else
- so->currPos.buf = _bt_getbuf(rel, so->currPos.currPage, BT_READ);
-
for (;;)
{
- /* Done if we know that the left sibling link isn't of interest */
- if (!so->currPos.moreLeft)
+ /*
+ * if we're at end of scan, give up and mark parallel scan as
+ * done, so that all the workers can finish their scan
+ */
+ if (blkno == P_NONE || !so->currPos.moreLeft)
{
- _bt_relbuf(rel, so->currPos.buf);
_bt_parallel_done(scan);
BTScanPosInvalidate(so->currPos);
return false;
}
- /* Step to next physical page */
- so->currPos.buf = _bt_walk_left(rel, so->currPos.buf);
+ /* move left at least one page (checks for interrupts, too) */
+ so->currPos.buf = _bt_lock_and_validate_left(rel, &blkno,
+ lastcurrblkno);
- /* if we're physically at end of index, return failure */
if (so->currPos.buf == InvalidBuffer)
{
+ /* must have been a concurrent deletion of leftmost page */
_bt_parallel_done(scan);
BTScanPosInvalidate(so->currPos);
return false;
@@ -2309,7 +2331,7 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
opaque = BTPageGetOpaque(page);
if (!P_IGNORE(opaque))
{
- PredicateLockPage(rel, BufferGetBlockNumber(so->currPos.buf), scan->xs_snapshot);
+ PredicateLockPage(rel, blkno, scan->xs_snapshot);
/* see if there are any matches on this page */
/* note that this will clear moreLeft if we can stop */
if (_bt_readpage(scan, dir, PageGetMaxOffsetNumber(page), false))
@@ -2317,51 +2339,34 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
}
else if (scan->parallel_scan != NULL)
{
- /* allow next page be processed by parallel worker */
- _bt_parallel_release(scan, BufferGetBlockNumber(so->currPos.buf));
+ /* _bt_readpage not called, so do this for ourselves */
+ Assert(P_ISHALFDEAD(opaque));
+ _bt_parallel_release(scan, opaque->btpo_prev, blkno);
}
- /*
- * For parallel scans, get the last page scanned as it is quite
- * possible that by the time we try to seize the scan, some other
- * worker has already advanced the scan to a different page. We
- * must continue based on the latest page scanned by any worker.
- */
if (scan->parallel_scan != NULL)
{
+ /* no matching tuples, so seize a page to the left */
_bt_relbuf(rel, so->currPos.buf);
- status = _bt_parallel_seize(scan, &blkno, false);
+ status = _bt_parallel_seize(scan, &blkno, &lastcurrblkno, false);
if (!status)
{
+ /* don't call _bt_parallel_done */
BTScanPosInvalidate(so->currPos);
return false;
}
- so->currPos.buf = _bt_getbuf(rel, blkno, BT_READ);
+ }
+ else
+ {
+ /* no matching tuples, so consider following left link */
+ lastcurrblkno = blkno;
+ blkno = opaque->btpo_prev;
+ _bt_relbuf(rel, so->currPos.buf);
+ so->currPos.buf = InvalidBuffer;
}
}
}
- return true;
-}
-
-/*
- * _bt_parallel_readpage() -- Read current page containing valid data for scan
- *
- * On success, release lock and maybe pin on buffer. We return true to
- * indicate success.
- */
-static bool
-_bt_parallel_readpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
-{
- BTScanOpaque so = (BTScanOpaque) scan->opaque;
-
- Assert(!so->needPrimScan);
-
- _bt_initialize_more_data(so, dir);
-
- if (!_bt_readnextpage(scan, blkno, dir))
- return false;
-
/* We have at least one item to return as scan's next item */
_bt_drop_lock_and_maybe_pin(scan, &so->currPos);
@@ -2369,48 +2374,41 @@ _bt_parallel_readpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
}
/*
- * _bt_walk_left() -- step left one page, if possible
+ * _bt_lock_and_validate_left() -- lock caller's left sibling blkno,
+ * recovering from concurrent page splits/page deletions when necessary
*
- * The given buffer must be pinned and read-locked. This will be dropped
- * before stepping left. On return, we have pin and read lock on the
- * returned page, instead.
+ * Called during backwards scans, to deal with their unique concurrency rules.
*
- * Returns InvalidBuffer if there is no page to the left (no lock is held
- * in that case).
+ * blkno points to the block number of the page that we expect to move the
+ * scan to. We'll successfully move the scan there when we find that its
+ * right sibling link points to lastcurrblkno, the page that we just finished
+ * reading. Otherwise, we have to figure out which page is the correct one to
+ * visit next the hard way, which involves reasoning about both concurrent
+ * page splits and concurrent page deletions. See nbtree/README for details.
+ *
+ * On return, we have both a pin and a read lock on the returned page, whose
+ * block number will be set in *blkno. Returns InvalidBuffer if there is no
+ * page to the left (no lock is held in that case).
*
* It is possible for the returned leaf page to be half-dead; caller must
* check that condition and step left again when required.
*/
static Buffer
-_bt_walk_left(Relation rel, Buffer buf)
+_bt_lock_and_validate_left(Relation rel, BlockNumber *blkno,
+ BlockNumber lastcurrblkno)
{
- Page page;
- BTPageOpaque opaque;
-
- page = BufferGetPage(buf);
- opaque = BTPageGetOpaque(page);
+ BlockNumber origblkno = *blkno; /* detects circular links */
for (;;)
{
- BlockNumber obknum;
- BlockNumber lblkno;
- BlockNumber blkno;
+ Buffer buf;
+ Page page;
+ BTPageOpaque opaque;
int tries;
- /* if we're at end of tree, release buf and return failure */
- if (P_LEFTMOST(opaque))
- {
- _bt_relbuf(rel, buf);
- break;
- }
- /* remember original page we are stepping left from */
- obknum = BufferGetBlockNumber(buf);
- /* step left */
- blkno = lblkno = opaque->btpo_prev;
- _bt_relbuf(rel, buf);
/* check for interrupts while we're not holding any buffer lock */
CHECK_FOR_INTERRUPTS();
- buf = _bt_getbuf(rel, blkno, BT_READ);
+ buf = _bt_getbuf(rel, *blkno, BT_READ);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
@@ -2427,21 +2425,26 @@ _bt_walk_left(Relation rel, Buffer buf)
tries = 0;
for (;;)
{
- if (!P_ISDELETED(opaque) && opaque->btpo_next == obknum)
+ if (!P_ISDELETED(opaque) && opaque->btpo_next == lastcurrblkno)
{
/* Found desired page, return it */
return buf;
}
if (P_RIGHTMOST(opaque) || ++tries > 4)
break;
- blkno = opaque->btpo_next;
- buf = _bt_relandgetbuf(rel, buf, blkno, BT_READ);
+ /* step right */
+ *blkno = opaque->btpo_next;
+ buf = _bt_relandgetbuf(rel, buf, *blkno, BT_READ);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
}
- /* Return to the original page to see what's up */
- buf = _bt_relandgetbuf(rel, buf, obknum, BT_READ);
+ /*
+ * Return to the original page (usually the page most recently read by
+ * _bt_readpage, which is passed by caller as lastcurrblkno) to see
+ * what's up with sibling links
+ */
+ buf = _bt_relandgetbuf(rel, buf, lastcurrblkno, BT_READ);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
if (P_ISDELETED(opaque))
@@ -2457,8 +2460,8 @@ _bt_walk_left(Relation rel, Buffer buf)
if (P_RIGHTMOST(opaque))
elog(ERROR, "fell off the end of index \"%s\"",
RelationGetRelationName(rel));
- blkno = opaque->btpo_next;
- buf = _bt_relandgetbuf(rel, buf, blkno, BT_READ);
+ *blkno = opaque->btpo_next;
+ buf = _bt_relandgetbuf(rel, buf, *blkno, BT_READ);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
if (!P_ISDELETED(opaque))
@@ -2466,9 +2469,10 @@ _bt_walk_left(Relation rel, Buffer buf)
}
/*
- * Now return to top of loop, resetting obknum to point to this
- * nondeleted page, and try again.
+ * remember that this page is actually now the leftmost one whose
+ * key space we've already read all tuples from
*/
+ lastcurrblkno = BufferGetBlockNumber(buf);
}
else
{
@@ -2477,11 +2481,22 @@ _bt_walk_left(Relation rel, Buffer buf)
* to the left got split or deleted. Without this check, we'd go
* into an infinite loop if there's anything wrong.
*/
- if (opaque->btpo_prev == lblkno)
+ if (opaque->btpo_prev == origblkno)
elog(ERROR, "could not find left sibling of block %u in index \"%s\"",
- obknum, RelationGetRelationName(rel));
- /* Okay to try again with new lblkno value */
+ lastcurrblkno, RelationGetRelationName(rel));
+ /* Okay to try again, since left sibling link changed */
}
+
+ if (P_LEFTMOST(opaque))
+ {
+ /* previous leftmost page must have been concurrently deleted */
+ _bt_relbuf(rel, buf);
+ break;
+ }
+
+ /* next loop iteration will start over with a clean slate */
+ *blkno = origblkno = opaque->btpo_prev;
+ _bt_relbuf(rel, buf);
}
return InvalidBuffer;
@@ -2605,7 +2620,6 @@ _bt_endpoint(IndexScanDesc scan, ScanDirection dir)
return false;
}
- PredicateLockPage(rel, BufferGetBlockNumber(buf), scan->xs_snapshot);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
Assert(P_ISLEAF(opaque));
@@ -2637,21 +2651,8 @@ _bt_endpoint(IndexScanDesc scan, ScanDirection dir)
/*
* Now load data from the first page of the scan.
*/
- if (!_bt_readpage(scan, dir, start, true))
- {
- /*
- * There's no actually-matching data on this page. Try to advance to
- * the next page. Return false if there's no matching data at all.
- */
- _bt_unlockbuf(scan->indexRelation, so->currPos.buf);
- if (!_bt_steppage(scan, dir))
- return false;
- }
- else
- {
- /* We have at least one item to return as scan's first item */
- _bt_drop_lock_and_maybe_pin(scan, &so->currPos);
- }
+ if (!_bt_readfirstpage(scan, dir, start))
+ return false;
/* OK, itemIndex says what to return */
currItem = &so->currPos.items[so->currPos.itemIndex];
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index b4ba51357..03d5c671f 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -2442,7 +2442,7 @@ new_prim_scan:
so->needPrimScan = true; /* ...but call _bt_first again */
if (scan->parallel_scan)
- _bt_parallel_primscan_schedule(scan, pstate->prev_scan_page);
+ _bt_parallel_primscan_schedule(scan, so->currPos.currPage);
/* Caller's tuple doesn't match the new qual */
return false;
--
2.45.2
^ permalink raw reply [nested|flat] 30+ messages in thread
end of thread, other threads:[~2024-10-10 17:41 UTC | newest]
Thread overview: 30+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-03-09 14:04 Re: [PATCH]: fix bug in SP-GiST box_ops Alexander Korotkov <[email protected]>
2017-03-09 23:13 ` Tels <[email protected]>
2017-03-10 09:15 ` Nikita Glukhov <[email protected]>
2017-03-17 06:16 ` Kyotaro HORIGUCHI <[email protected]>
2017-03-21 08:49 ` Anastasia Lubennikova <[email protected]>
2017-03-21 13:26 ` Teodor Sigaev <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v3] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v5] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v4 2/2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2020-08-05 02:04 [PATCH v2] Avoid spurious CREATE INDEX CONCURRENTLY waits Alvaro Herrera <[email protected]>
2024-10-08 16:52 Re: Avoiding superfluous buffer locking during nbtree backwards scans Peter Geoghegan <[email protected]>
2024-10-10 17:41 ` Re: Avoiding superfluous buffer locking during nbtree backwards scans Peter Geoghegan <[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