Skip to content

Commit e428699

Browse files
nmischnmisch
committed
Avoid race in RelationBuildDesc() affecting CREATE INDEX CONCURRENTLY.
CIC and REINDEX CONCURRENTLY assume backends see their catalog changes no later than each backend's next transaction start. That failed to hold when a backend absorbed a relevant invalidation in the middle of running RelationBuildDesc() on the CIC index. Queries that use the resulting index can silently fail to find rows. Fix this for future index builds by making RelationBuildDesc() loop until it finishes without accepting a relevant invalidation. It may be necessary to reindex to recover from past occurrences; REINDEX CONCURRENTLY suffices. Back-patch to 9.6 (all supported versions). Noah Misch and Andrey Borodin, reviewed (in earlier versions) by Andres Freund. Discussion: https://postgr.es/m/20210730022548.GA1940096@gust.leadboat.com
1 parent b1df061 commit e428699

File tree

8 files changed

+408
-4
lines changed

8 files changed

+408
-4
lines changed

‎src/backend/utils/cache/inval.c

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,12 +631,18 @@ LocalExecuteInvalidationMessage(SharedInvalidationMessage *msg)
631631
*/
632632
void
633633
InvalidateSystemCaches(void)
634+
{
635+
InvalidateSystemCachesExtended(false);
636+
}
637+
638+
void
639+
InvalidateSystemCachesExtended(bool debug_discard)
634640
{
635641
int i;
636642

637643
InvalidateCatalogSnapshot();
638644
ResetCatalogCaches();
639-
RelationCacheInvalidate(); /* gets smgr and relmap too */
645+
RelationCacheInvalidate(debug_discard); /* gets smgr and relmap too */
640646

641647
for (i = 0; i < syscache_callback_count; i++)
642648
{
@@ -707,7 +713,7 @@ AcceptInvalidationMessages(void)
707713
if (recursion_depth < 3)
708714
{
709715
recursion_depth++;
710-
InvalidateSystemCaches();
716+
InvalidateSystemCachesExtended(true);
711717
recursion_depth--;
712718
}
713719
}

‎src/backend/utils/cache/relcache.c

Lines changed: 106 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,24 @@ bool criticalSharedRelcachesBuilt = false;
135135
*/
136136
static long relcacheInvalsReceived = 0L;
137137

138+
/*
139+
* in_progress_list is a stack of ongoing RelationBuildDesc() calls. CREATE
140+
* INDEX CONCURRENTLY makes catalog changes under ShareUpdateExclusiveLock.
141+
* It critically relies on each backend absorbing those changes no later than
142+
* next transaction start. Hence, RelationBuildDesc() loops until it finishes
143+
* without accepting a relevant invalidation. (Most invalidation consumers
144+
* don't do this.)
145+
*/
146+
typedef struct inprogressent
147+
{
148+
Oid reloid; /* OID of relation being built */
149+
bool invalidated; /* whether an invalidation arrived for it */
150+
} InProgressEnt;
151+
152+
static InProgressEnt *in_progress_list;
153+
static int in_progress_list_len;
154+
static int in_progress_list_maxlen;
155+
138156
/*
139157
* eoxact_list[] stores the OIDs of relations that (might) need AtEOXact
140158
* cleanup work. This list intentionally has limited size; if it overflows,
@@ -938,11 +956,27 @@ equalRSDesc(RowSecurityDesc *rsdesc1, RowSecurityDesc *rsdesc2)
938956
static Relation
939957
RelationBuildDesc(Oid targetRelId, bool insertIt)
940958
{
959+
int in_progress_offset;
941960
Relation relation;
942961
Oid relid;
943962
HeapTuple pg_class_tuple;
944963
Form_pg_class relp;
945964

965+
/* Register to catch invalidation messages */
966+
if (in_progress_list_len >= in_progress_list_maxlen)
967+
{
968+
int allocsize;
969+
970+
allocsize = in_progress_list_maxlen * 2;
971+
in_progress_list = repalloc(in_progress_list,
972+
allocsize * sizeof(*in_progress_list));
973+
in_progress_list_maxlen = allocsize;
974+
}
975+
in_progress_offset = in_progress_list_len++;
976+
in_progress_list[in_progress_offset].reloid = targetRelId;
977+
retry:
978+
in_progress_list[in_progress_offset].invalidated = false;
979+
946980
/*
947981
* find the tuple in pg_class corresponding to the given relation id
948982
*/
@@ -952,7 +986,11 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
952986
* if no such tuple exists, return NULL
953987
*/
954988
if (!HeapTupleIsValid(pg_class_tuple))
989+
{
990+
Assert(in_progress_offset + 1 == in_progress_list_len);
991+
in_progress_list_len--;
955992
return NULL;
993+
}
956994

957995
/*
958996
* get information from the pg_class_tuple
@@ -1078,6 +1116,21 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
10781116
*/
10791117
heap_freetuple(pg_class_tuple);
10801118

1119+
/*
1120+
* If an invalidation arrived mid-build, start over. Between here and the
1121+
* end of this function, don't add code that does or reasonably could read
1122+
* system catalogs. That range must be free from invalidation processing
1123+
* for the !insertIt case. For the insertIt case, RelationCacheInsert()
1124+
* will enroll this relation in ordinary relcache invalidation processing,
1125+
*/
1126+
if (in_progress_list[in_progress_offset].invalidated)
1127+
{
1128+
RelationDestroyRelation(relation, false);
1129+
goto retry;
1130+
}
1131+
Assert(in_progress_offset + 1 == in_progress_list_len);
1132+
in_progress_list_len--;
1133+
10811134
/*
10821135
* Insert newly created relation into relcache hash table, if requested.
10831136
*
@@ -2282,6 +2335,14 @@ RelationClearRelation(Relation relation, bool rebuild)
22822335

22832336
/* Build temporary entry, but don't link it into hashtable */
22842337
newrel = RelationBuildDesc(save_relid, false);
2338+
2339+
/*
2340+
* Between here and the end of the swap, don't add code that does or
2341+
* reasonably could read system catalogs. That range must be free
2342+
* from invalidation processing. See RelationBuildDesc() manipulation
2343+
* of in_progress_list.
2344+
*/
2345+
22852346
if (newrel == NULL)
22862347
{
22872348
/*
@@ -2457,6 +2518,14 @@ RelationCacheInvalidateEntry(Oid relationId)
24572518
relcacheInvalsReceived++;
24582519
RelationFlushRelation(relation);
24592520
}
2521+
else
2522+
{
2523+
int i;
2524+
2525+
for (i = 0; i < in_progress_list_len; i++)
2526+
if (in_progress_list[i].reloid == relationId)
2527+
in_progress_list[i].invalidated = true;
2528+
}
24602529
}
24612530

24622531
/*
@@ -2488,16 +2557,22 @@ RelationCacheInvalidateEntry(Oid relationId)
24882557
* second pass processes nailed-in-cache items before other nondeletable
24892558
* items. This should ensure that system catalogs are up to date before
24902559
* we attempt to use them to reload information about other open relations.
2560+
*
2561+
* After those two phases of work having immediate effects, we normally
2562+
* signal any RelationBuildDesc() on the stack to start over. However, we
2563+
* don't do this if called as part of debug_discard_caches. Otherwise,
2564+
* RelationBuildDesc() would become an infinite loop.
24912565
*/
24922566
void
2493-
RelationCacheInvalidate(void)
2567+
RelationCacheInvalidate(bool debug_discard)
24942568
{
24952569
HASH_SEQ_STATUS status;
24962570
RelIdCacheEnt *idhentry;
24972571
Relation relation;
24982572
List *rebuildFirstList = NIL;
24992573
List *rebuildList = NIL;
25002574
ListCell *l;
2575+
int i;
25012576

25022577
/*
25032578
* Reload relation mapping data before starting to reconstruct cache.
@@ -2584,6 +2659,11 @@ RelationCacheInvalidate(void)
25842659
RelationClearRelation(relation, true);
25852660
}
25862661
list_free(rebuildList);
2662+
2663+
if (!debug_discard)
2664+
/* Any RelationBuildDesc() on the stack must start over. */
2665+
for (i = 0; i < in_progress_list_len; i++)
2666+
in_progress_list[i].invalidated = true;
25872667
}
25882668

25892669
/*
@@ -2656,6 +2736,13 @@ AtEOXact_RelationCache(bool isCommit)
26562736
RelIdCacheEnt *idhentry;
26572737
int i;
26582738

2739+
/*
2740+
* Forget in_progress_list. This is relevant when we're aborting due to
2741+
* an error during RelationBuildDesc().
2742+
*/
2743+
Assert(in_progress_list_len == 0 || !isCommit);
2744+
in_progress_list_len = 0;
2745+
26592746
/*
26602747
* Unless the eoxact_list[] overflowed, we only need to examine the rels
26612748
* listed in it. Otherwise fall back on a hash_seq_search scan.
@@ -2804,6 +2891,14 @@ AtEOSubXact_RelationCache(bool isCommit, SubTransactionId mySubid,
28042891
RelIdCacheEnt *idhentry;
28052892
int i;
28062893

2894+
/*
2895+
* Forget in_progress_list. This is relevant when we're aborting due to
2896+
* an error during RelationBuildDesc(). We don't commit subtransactions
2897+
* during RelationBuildDesc().
2898+
*/
2899+
Assert(in_progress_list_len == 0 || !isCommit);
2900+
in_progress_list_len = 0;
2901+
28072902
/*
28082903
* Unless the eoxact_list[] overflowed, we only need to examine the rels
28092904
* listed in it. Otherwise fall back on a hash_seq_search scan. Same
@@ -3287,6 +3382,7 @@ void
32873382
RelationCacheInitialize(void)
32883383
{
32893384
HASHCTL ctl;
3385+
int allocsize;
32903386

32913387
/*
32923388
* make sure cache memory context exists
@@ -3303,6 +3399,15 @@ RelationCacheInitialize(void)
33033399
RelationIdCache = hash_create("Relcache by OID", INITRELCACHESIZE,
33043400
&ctl, HASH_ELEM | HASH_BLOBS);
33053401

3402+
/*
3403+
* reserve enough in_progress_list slots for many cases
3404+
*/
3405+
allocsize = 4;
3406+
in_progress_list =
3407+
MemoryContextAlloc(CacheMemoryContext,
3408+
allocsize * sizeof(*in_progress_list));
3409+
in_progress_list_maxlen = allocsize;
3410+
33063411
/*
33073412
* relation mapper needs to be initialized too
33083413
*/

‎src/bin/pgbench/t/022_cic.pl

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
2+
# Copyright (c) 2021, PostgreSQL Global Development Group
3+
4+
# Test CREATE INDEX CONCURRENTLY with concurrent modifications
5+
use strict;
6+
use warnings;
7+
8+
use Config;
9+
use PostgresNode;
10+
use TestLib;
11+
12+
use Test::More tests => 4;
13+
14+
my ($node, $result);
15+
16+
#
17+
# Test set-up
18+
#
19+
$node = get_new_node('CIC_test');
20+
$node->init;
21+
$node->append_conf('postgresql.conf', 'lock_timeout = 180000');
22+
$node->start;
23+
$node->safe_psql('postgres', q(CREATE TABLE tbl(i int)));
24+
$node->safe_psql('postgres', q(CREATE INDEX idx ON tbl(i)));
25+
$node->safe_psql(
26+
'postgres', q(
27+
CREATE FUNCTION heapallindexed() RETURNS void AS $$
28+
DECLARE
29+
count_seqscan int;
30+
count_idxscan int;
31+
BEGIN
32+
count_seqscan := (SELECT count(*) FROM tbl);
33+
SET enable_seqscan = off;
34+
count_idxscan := (SELECT count(*) FROM tbl);
35+
RESET enable_seqscan;
36+
IF count_seqscan <> count_idxscan THEN
37+
RAISE 'seqscan found % rows, but idxscan found % rows',
38+
count_seqscan, count_idxscan;
39+
END IF;
40+
END
41+
$$ LANGUAGE plpgsql;
42+
));
43+
44+
#
45+
# Stress CIC with pgbench
46+
#
47+
48+
# Run background pgbench with CIC. We cannot mix-in this script into single
49+
# pgbench: CIC will deadlock with itself occasionally.
50+
my $pgbench_out = '';
51+
my $pgbench_timer = IPC::Run::timeout(180);
52+
my $pgbench_h = $node->background_pgbench(
53+
'--no-vacuum --client=1 --transactions=200',
54+
{
55+
'002_pgbench_concurrent_cic' => q(
56+
DROP INDEX CONCURRENTLY idx;
57+
CREATE INDEX CONCURRENTLY idx ON tbl(i);
58+
BEGIN ISOLATION LEVEL REPEATABLE READ;
59+
SELECT heapallindexed();
60+
ROLLBACK;
61+
)
62+
},
63+
\$pgbench_out,
64+
$pgbench_timer);
65+
66+
# Run pgbench.
67+
$node->pgbench(
68+
'--no-vacuum --client=5 --transactions=200',
69+
0,
70+
[qr{actually processed}],
71+
[qr{^$}],
72+
'concurrent INSERTs',
73+
{
74+
'002_pgbench_concurrent_transaction' => q(
75+
BEGIN;
76+
INSERT INTO tbl VALUES(0);
77+
COMMIT;
78+
),
79+
'002_pgbench_concurrent_transaction_savepoints' => q(
80+
BEGIN;
81+
SAVEPOINT s1;
82+
INSERT INTO tbl VALUES(0);
83+
COMMIT;
84+
)
85+
});
86+
87+
$pgbench_h->pump_nb;
88+
$pgbench_h->finish();
89+
unlike($pgbench_out, qr/aborted in/, "pgbench with CIC works");
90+
91+
# done
92+
$node->stop;
93+
done_testing();

‎src/include/utils/inval.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,4 +61,5 @@ extern void CacheRegisterRelcacheCallback(RelcacheCallbackFunction func,
6161
extern void CallSyscacheCallbacks(int cacheid, uint32 hashvalue);
6262

6363
extern void InvalidateSystemCaches(void);
64+
extern void InvalidateSystemCachesExtended(bool debug_discard);
6465
#endif /* INVAL_H */

‎src/include/utils/relcache.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ extern void RelationForgetRelation(Oid rid);
107107

108108
extern void RelationCacheInvalidateEntry(Oid relationId);
109109

110-
extern void RelationCacheInvalidate(void);
110+
extern void RelationCacheInvalidate(bool debug_discard);
111111

112112
extern void RelationCloseSmgrByOid(Oid relationId);
113113

0 commit comments

Comments
 (0)