Skip to content

Commit 1d65416

Browse files
michaelpqmichaelpq
committed
Improve handling of dropped relations for REINDEX DATABASE/SCHEMA/SYSTEM
When multiple relations are reindexed, a scan of pg_class is done first to build the list of relations to work on. However the REINDEX logic has never checked if a relation listed still exists when beginning the work on it, causing for example sudden cache lookup failures. This commit adds safeguards against dropped relations for REINDEX, similarly to VACUUM or CLUSTER where we try to open the relation, ignoring it if it is missing. A new option is added to the REINDEX routines to control if a missed relation is OK to ignore or not. An isolation test, based on REINDEX SCHEMA, is added for the concurrent and non-concurrent cases. Author: Michael Paquier Reviewed-by: Anastasia Lubennikova Discussion: https://postgr.es/m/20200813043805.GE11663@paquier.xyz
1 parent 4c51a2d commit 1d65416

File tree

8 files changed

+174
-9
lines changed

8 files changed

+174
-9
lines changed

‎src/backend/access/table/table.c

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,40 @@ table_open(Oid relationId, LOCKMODE lockmode)
5757
return r;
5858
}
5959

60+
61+
/* ----------------
62+
* try_table_open - open a table relation by relation OID
63+
*
64+
* Same as table_open, except return NULL instead of failing
65+
* if the relation does not exist.
66+
* ----------------
67+
*/
68+
Relation
69+
try_table_open(Oid relationId, LOCKMODE lockmode)
70+
{
71+
Relation r;
72+
73+
r = try_relation_open(relationId, lockmode);
74+
75+
/* leave if table does not exist */
76+
if (!r)
77+
return NULL;
78+
79+
if (r->rd_rel->relkind == RELKIND_INDEX ||
80+
r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
81+
ereport(ERROR,
82+
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
83+
errmsg("\"%s\" is an index",
84+
RelationGetRelationName(r))));
85+
else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
86+
ereport(ERROR,
87+
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
88+
errmsg("\"%s\" is a composite type",
89+
RelationGetRelationName(r))));
90+
91+
return r;
92+
}
93+
6094
/* ----------------
6195
* table_openrv - open a table relation specified
6296
* by a RangeVar node

‎src/backend/catalog/index.c

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3437,8 +3437,20 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
34373437
* Open and lock the parent heap relation. ShareLock is sufficient since
34383438
* we only need to be sure no schema or data changes are going on.
34393439
*/
3440-
heapId = IndexGetRelation(indexId, false);
3441-
heapRelation = table_open(heapId, ShareLock);
3440+
heapId = IndexGetRelation(indexId,
3441+
(options & REINDEXOPT_MISSING_OK) != 0);
3442+
/* if relation is missing, leave */
3443+
if (!OidIsValid(heapId))
3444+
return;
3445+
3446+
if ((options & REINDEXOPT_MISSING_OK) != 0)
3447+
heapRelation = try_table_open(heapId, ShareLock);
3448+
else
3449+
heapRelation = table_open(heapId, ShareLock);
3450+
3451+
/* if relation is gone, leave */
3452+
if (!heapRelation)
3453+
return;
34423454

34433455
if (progress)
34443456
{
@@ -3672,7 +3684,14 @@ reindex_relation(Oid relid, int flags, int options)
36723684
* to prevent schema and data changes in it. The lock level used here
36733685
* should match ReindexTable().
36743686
*/
3675-
rel = table_open(relid, ShareLock);
3687+
if ((options & REINDEXOPT_MISSING_OK) != 0)
3688+
rel = try_table_open(relid, ShareLock);
3689+
else
3690+
rel = table_open(relid, ShareLock);
3691+
3692+
/* if relation is gone, leave */
3693+
if (!rel)
3694+
return false;
36763695

36773696
/*
36783697
* This may be useful when implemented someday; but that day is not today.
@@ -3771,7 +3790,14 @@ reindex_relation(Oid relid, int flags, int options)
37713790
* still hold the lock on the main table.
37723791
*/
37733792
if ((flags & REINDEX_REL_PROCESS_TOAST) && OidIsValid(toast_relid))
3774-
result |= reindex_relation(toast_relid, flags, options);
3793+
{
3794+
/*
3795+
* Note that this should fail if the toast relation is missing, so
3796+
* reset REINDEXOPT_MISSING_OK.
3797+
*/
3798+
result |= reindex_relation(toast_relid, flags,
3799+
options & ~(REINDEXOPT_MISSING_OK));
3800+
}
37753801

37763802
return result;
37773803
}

‎src/backend/commands/indexcmds.c

Lines changed: 58 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2766,9 +2766,19 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
27662766
/* functions in indexes may want a snapshot set */
27672767
PushActiveSnapshot(GetTransactionSnapshot());
27682768

2769+
/* check if the relation still exists */
2770+
if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(relid)))
2771+
{
2772+
PopActiveSnapshot();
2773+
CommitTransactionCommand();
2774+
continue;
2775+
}
2776+
27692777
if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
27702778
{
2771-
(void) ReindexRelationConcurrently(relid, options);
2779+
(void) ReindexRelationConcurrently(relid,
2780+
options |
2781+
REINDEXOPT_MISSING_OK);
27722782
/* ReindexRelationConcurrently() does the verbose output */
27732783
}
27742784
else
@@ -2778,7 +2788,9 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
27782788
result = reindex_relation(relid,
27792789
REINDEX_REL_PROCESS_TOAST |
27802790
REINDEX_REL_CHECK_CONSTRAINTS,
2781-
options | REINDEXOPT_REPORT_PROGRESS);
2791+
options |
2792+
REINDEXOPT_REPORT_PROGRESS |
2793+
REINDEXOPT_MISSING_OK);
27822794

27832795
if (result && (options & REINDEXOPT_VERBOSE))
27842796
ereport(INFO,
@@ -2893,7 +2905,17 @@ ReindexRelationConcurrently(Oid relationOid, int options)
28932905
errmsg("cannot reindex system catalogs concurrently")));
28942906

28952907
/* Open relation to get its indexes */
2896-
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
2908+
if ((options & REINDEXOPT_MISSING_OK) != 0)
2909+
{
2910+
heapRelation = try_table_open(relationOid,
2911+
ShareUpdateExclusiveLock);
2912+
/* leave if relation does not exist */
2913+
if (!heapRelation)
2914+
break;
2915+
}
2916+
else
2917+
heapRelation = table_open(relationOid,
2918+
ShareUpdateExclusiveLock);
28972919

28982920
/* Add all the valid indexes of relation to list */
28992921
foreach(lc, RelationGetIndexList(heapRelation))
@@ -2978,7 +3000,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
29783000
}
29793001
case RELKIND_INDEX:
29803002
{
2981-
Oid heapId = IndexGetRelation(relationOid, false);
3003+
Oid heapId = IndexGetRelation(relationOid,
3004+
(options & REINDEXOPT_MISSING_OK) != 0);
3005+
Relation heapRelation;
3006+
3007+
/* if relation is missing, leave */
3008+
if (!OidIsValid(heapId))
3009+
break;
29823010

29833011
if (IsCatalogRelationOid(heapId))
29843012
ereport(ERROR,
@@ -2995,6 +3023,25 @@ ReindexRelationConcurrently(Oid relationOid, int options)
29953023
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
29963024
errmsg("cannot reindex invalid index on TOAST table concurrently")));
29973025

3026+
/*
3027+
* Check if parent relation can be locked and if it exists,
3028+
* this needs to be done at this stage as the list of indexes
3029+
* to rebuild is not complete yet, and REINDEXOPT_MISSING_OK
3030+
* should not be used once all the session locks are taken.
3031+
*/
3032+
if ((options & REINDEXOPT_MISSING_OK) != 0)
3033+
{
3034+
heapRelation = try_table_open(heapId,
3035+
ShareUpdateExclusiveLock);
3036+
/* leave if relation does not exist */
3037+
if (!heapRelation)
3038+
break;
3039+
}
3040+
else
3041+
heapRelation = table_open(heapId,
3042+
ShareUpdateExclusiveLock);
3043+
table_close(heapRelation, NoLock);
3044+
29983045
/* Save the list of relation OIDs in private context */
29993046
oldcontext = MemoryContextSwitchTo(private_context);
30003047

@@ -3025,7 +3072,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
30253072
break;
30263073
}
30273074

3028-
/* Definitely no indexes, so leave */
3075+
/*
3076+
* Definitely no indexes, so leave. Any checks based on
3077+
* REINDEXOPT_MISSING_OK should be done only while the list of indexes to
3078+
* work on is built as the session locks taken before this transaction
3079+
* commits will make sure that they cannot be dropped by a concurrent
3080+
* session until this operation completes.
3081+
*/
30293082
if (indexIds == NIL)
30303083
{
30313084
PopActiveSnapshot();

‎src/include/access/table.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ extern Relation table_open(Oid relationId, LOCKMODE lockmode);
2222
extern Relation table_openrv(const RangeVar *relation, LOCKMODE lockmode);
2323
extern Relation table_openrv_extended(const RangeVar *relation,
2424
LOCKMODE lockmode, bool missing_ok);
25+
extern Relation try_table_open(Oid relationId, LOCKMODE lockmode);
2526
extern void table_close(Relation relation, LOCKMODE lockmode);
2627

2728
#endif /* TABLE_H */

‎src/include/nodes/parsenodes.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3352,6 +3352,7 @@ typedef struct ConstraintsSetStmt
33523352
/* Reindex options */
33533353
#define REINDEXOPT_VERBOSE (1 << 0) /* print progress info */
33543354
#define REINDEXOPT_REPORT_PROGRESS (1 << 1) /* report pgstat progress */
3355+
#define REINDEXOPT_MISSING_OK (2 << 1) /* skip missing relations */
33553356

33563357
typedef enum ReindexObjectType
33573358
{

‎src/test/isolation/expected/reindex-schema.out

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
Parsed test spec with 3 sessions
2+
3+
starting permutation: begin1 lock1 reindex2 drop3 end1
4+
step begin1: BEGIN;
5+
step lock1: LOCK reindex_schema.tab_locked IN SHARE UPDATE EXCLUSIVE MODE;
6+
step reindex2: REINDEX SCHEMA reindex_schema; <waiting ...>
7+
step drop3: DROP TABLE reindex_schema.tab_dropped;
8+
step end1: COMMIT;
9+
step reindex2: <... completed>
10+
11+
starting permutation: begin1 lock1 reindex_conc2 drop3 end1
12+
step begin1: BEGIN;
13+
step lock1: LOCK reindex_schema.tab_locked IN SHARE UPDATE EXCLUSIVE MODE;
14+
step reindex_conc2: REINDEX SCHEMA CONCURRENTLY reindex_schema; <waiting ...>
15+
step drop3: DROP TABLE reindex_schema.tab_dropped;
16+
step end1: COMMIT;
17+
step reindex_conc2: <... completed>

‎src/test/isolation/isolation_schedule

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ test: lock-committed-update
4949
test: lock-committed-keyupdate
5050
test: update-locked-tuple
5151
test: reindex-concurrently
52+
test: reindex-schema
5253
test: propagate-lock-delete
5354
test: tuplelock-conflict
5455
test: tuplelock-update

‎src/test/isolation/specs/reindex-schema.spec

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# REINDEX with schemas
2+
#
3+
# Check that concurrent drop of relations while doing a REINDEX
4+
# SCHEMA allows the command to work.
5+
6+
setup
7+
{
8+
CREATE SCHEMA reindex_schema;
9+
CREATE TABLE reindex_schema.tab_locked (a int PRIMARY KEY);
10+
CREATE TABLE reindex_schema.tab_dropped (a int PRIMARY KEY);
11+
}
12+
13+
teardown
14+
{
15+
DROP SCHEMA reindex_schema CASCADE;
16+
}
17+
18+
session "s1"
19+
step "begin1" { BEGIN; }
20+
step "lock1" { LOCK reindex_schema.tab_locked IN SHARE UPDATE EXCLUSIVE MODE; }
21+
step "end1" { COMMIT; }
22+
23+
session "s2"
24+
step "reindex2" { REINDEX SCHEMA reindex_schema; }
25+
step "reindex_conc2" { REINDEX SCHEMA CONCURRENTLY reindex_schema; }
26+
27+
session "s3"
28+
step "drop3" { DROP TABLE reindex_schema.tab_dropped; }
29+
30+
# The table can be dropped while reindex is waiting.
31+
permutation "begin1" "lock1" "reindex2" "drop3" "end1"
32+
permutation "begin1" "lock1" "reindex_conc2" "drop3" "end1"

0 commit comments

Comments
 (0)