Hi all, While working on support for REINDEX for partitioned relations, I have noticed an old bug in the logic of ReindexMultipleTables(): the list of relations to process is built in a first transaction, and then each table is done in an independent transaction, but we don't actually check that the relation still exists when doing the real work. I think that a complete fix involves two things: 1) Addition of one SearchSysCacheExists1() at the beginning of the loop processing each item in the list in ReindexMultipleTables(). This would protect from a relation dropped, but that would not be enough if ReindexMultipleTables() is looking at a relation dropped before we lock it in reindex_table() or ReindexRelationConcurrently(). Still that's simple, cheaper, and would protect from most problems. 2) Be completely water-proof and adopt a logic close to what we do for VACUUM where we try to open a relation, but leave if it does not exist. This can be achieved with just some try_relation_open() calls with the correct lock used, and we also need to have a new REINDEXOPT_* flag to control this behavior conditionally.
2) and 1) are complementary, but 2) is invasive, so based on the lack of complaints we have seen that does not seem really worth backpatching to me, and I think that we could also just have 1) on stable branches as a minimal fix, to take care of most of the problems that could show up to users. Attached is a patch to fix all that, with a cheap isolation test that fails on HEAD with a cache lookup failure. I am adding that to the next CF for now, and I would rather fix this issue before moving on with REINDEX for partitioned relations as fixing this issue reduces the use of session locks for partition trees. Any thoughts? -- Michael
diff --git a/src/include/access/table.h b/src/include/access/table.h
index cf0ef7b337..68dc4d62c0 100644
--- a/src/include/access/table.h
+++ b/src/include/access/table.h
@@ -22,6 +22,7 @@ extern Relation table_open(Oid relationId, LOCKMODE lockmode);
extern Relation table_openrv(const RangeVar *relation, LOCKMODE lockmode);
extern Relation table_openrv_extended(const RangeVar *relation,
LOCKMODE lockmode, bool missing_ok);
+extern Relation try_table_open(Oid relationId, LOCKMODE lockmode);
extern void table_close(Relation relation, LOCKMODE lockmode);
#endif /* TABLE_H */
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 151bcdb7ef..f6ffee7e53 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3351,6 +3351,7 @@ typedef struct ConstraintsSetStmt
/* Reindex options */
#define REINDEXOPT_VERBOSE (1 << 0) /* print progress info */
#define REINDEXOPT_REPORT_PROGRESS (1 << 1) /* report pgstat progress */
+#define REINDEXOPT_MISSING_OK (2 << 1) /* skip missing relations */
typedef enum ReindexObjectType
{
diff --git a/src/backend/access/table/table.c b/src/backend/access/table/table.c
index 1aa01a54b3..7c29091e6c 100644
--- a/src/backend/access/table/table.c
+++ b/src/backend/access/table/table.c
@@ -57,6 +57,40 @@ table_open(Oid relationId, LOCKMODE lockmode)
return r;
}
+
+/* ----------------
+ * try_table_open - open a table relation by relation OID
+ *
+ * Same as table_open, except return NULL instead of failing
+ * if the relation does not exist.
+ * ----------------
+ */
+Relation
+try_table_open(Oid relationId, LOCKMODE lockmode)
+{
+ Relation r;
+
+ r = try_relation_open(relationId, lockmode);
+
+ /* leave if table does not exist */
+ if (!r)
+ return NULL;
+
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is an index",
+ RelationGetRelationName(r))));
+ else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is a composite type",
+ RelationGetRelationName(r))));
+
+ return r;
+}
+
/* ----------------
* table_openrv - open a table relation specified
* by a RangeVar node
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1be27eec52..4e7b3eb6a2 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3423,8 +3423,20 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
* Open and lock the parent heap relation. ShareLock is sufficient since
* we only need to be sure no schema or data changes are going on.
*/
- heapId = IndexGetRelation(indexId, false);
- heapRelation = table_open(heapId, ShareLock);
+ heapId = IndexGetRelation(indexId,
+ (options & REINDEXOPT_MISSING_OK) != 0);
+ /* if relation is missing, leave */
+ if (!OidIsValid(heapId))
+ return;
+
+ if ((options & REINDEXOPT_MISSING_OK) != 0)
+ heapRelation = try_table_open(heapId, ShareLock);
+ else
+ heapRelation = table_open(heapId, ShareLock);
+
+ /* if relation is gone, leave */
+ if (!heapRelation)
+ return;
if (progress)
{
@@ -3658,7 +3670,14 @@ reindex_relation(Oid relid, int flags, int options)
* to prevent schema and data changes in it. The lock level used here
* should match ReindexTable().
*/
- rel = table_open(relid, ShareLock);
+ if ((options & REINDEXOPT_MISSING_OK) != 0)
+ rel = try_table_open(relid, ShareLock);
+ else
+ rel = table_open(relid, ShareLock);
+
+ /* leave if relation is missing */
+ if (!rel)
+ return false;
/*
* This may be useful when implemented someday; but that day is not today.
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 7819266a63..a98fe0a1fd 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2766,9 +2766,17 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
+ /* check if the relation still exists */
+ if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(relid)))
+ {
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+ continue;
+ }
+
if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
{
- (void) ReindexRelationConcurrently(relid, options);
+ (void) ReindexRelationConcurrently(relid, options | REINDEXOPT_MISSING_OK);
/* ReindexRelationConcurrently() does the verbose output */
}
else
@@ -2777,8 +2785,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
result = reindex_relation(relid,
REINDEX_REL_PROCESS_TOAST |
- REINDEX_REL_CHECK_CONSTRAINTS,
- options | REINDEXOPT_REPORT_PROGRESS);
+ REINDEX_REL_CHECK_CONSTRAINTS ,
+ options | REINDEXOPT_REPORT_PROGRESS | REINDEXOPT_MISSING_OK);
if (result && (options & REINDEXOPT_VERBOSE))
ereport(INFO,
@@ -2893,7 +2901,17 @@ ReindexRelationConcurrently(Oid relationOid, int options)
errmsg("cannot reindex system catalogs concurrently")));
/* Open relation to get its indexes */
- heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
+ if ((options & REINDEXOPT_MISSING_OK) != 0)
+ {
+ heapRelation = try_table_open(relationOid,
+ ShareUpdateExclusiveLock);
+ /* leave if relation does not exist */
+ if (!heapRelation)
+ break;
+ }
+ else
+ heapRelation = table_open(relationOid,
+ ShareUpdateExclusiveLock);
/* Add all the valid indexes of relation to list */
foreach(lc, RelationGetIndexList(heapRelation))
@@ -2978,7 +2996,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
}
case RELKIND_INDEX:
{
- Oid heapId = IndexGetRelation(relationOid, false);
+ Oid heapId = IndexGetRelation(relationOid,
+ (options & REINDEXOPT_MISSING_OK) != 0);
+ Relation heapRelation;
+
+ /* if relation is missing, leave */
+ if (!OidIsValid(heapId))
+ break;
if (IsCatalogRelationOid(heapId))
ereport(ERROR,
@@ -2995,6 +3019,24 @@ ReindexRelationConcurrently(Oid relationOid, int options)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex invalid index on TOAST table concurrently")));
+ /*
+ * Check if parent relation can be locked and if it exists,
+ * this needs to be done at this stage as the list of
+ * indexes to rebuild is not complete yet.
+ */
+ if ((options & REINDEXOPT_MISSING_OK) != 0)
+ {
+ heapRelation = try_table_open(heapId,
+ ShareUpdateExclusiveLock);
+ /* leave if relation does not exist */
+ if (!heapRelation)
+ break;
+ }
+ else
+ heapRelation = table_open(heapId,
+ ShareUpdateExclusiveLock);
+ table_close(heapRelation, NoLock);
+
/* Save the list of relation OIDs in private context */
oldcontext = MemoryContextSwitchTo(private_context);
diff --git a/src/test/isolation/expected/reindex-schema.out b/src/test/isolation/expected/reindex-schema.out
new file mode 100644
index 0000000000..0884e75412
--- /dev/null
+++ b/src/test/isolation/expected/reindex-schema.out
@@ -0,0 +1,17 @@
+Parsed test spec with 3 sessions
+
+starting permutation: begin1 lock1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lock1: LOCK reindex_schema.tab_locked IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex2: REINDEX SCHEMA reindex_schema; <waiting ...>
+step drop3: DROP TABLE reindex_schema.tab_dropped;
+step end1: COMMIT;
+step reindex2: <... completed>
+
+starting permutation: begin1 lock1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lock1: LOCK reindex_schema.tab_locked IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex_conc2: REINDEX SCHEMA CONCURRENTLY reindex_schema; <waiting ...>
+step drop3: DROP TABLE reindex_schema.tab_dropped;
+step end1: COMMIT;
+step reindex_conc2: <... completed>
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 218c87b24b..6acbb695ec 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -49,6 +49,7 @@ test: lock-committed-update
test: lock-committed-keyupdate
test: update-locked-tuple
test: reindex-concurrently
+test: reindex-schema
test: propagate-lock-delete
test: tuplelock-conflict
test: tuplelock-update
diff --git a/src/test/isolation/specs/reindex-schema.spec b/src/test/isolation/specs/reindex-schema.spec
new file mode 100644
index 0000000000..feff8a5ec0
--- /dev/null
+++ b/src/test/isolation/specs/reindex-schema.spec
@@ -0,0 +1,32 @@
+# REINDEX with schemas
+#
+# Check that concurrent drop of relations while doing a REINDEX
+# SCHEMA allows the command to work.
+
+setup
+{
+ CREATE SCHEMA reindex_schema;
+ CREATE TABLE reindex_schema.tab_locked (a int PRIMARY KEY);
+ CREATE TABLE reindex_schema.tab_dropped (a int PRIMARY KEY);
+}
+
+teardown
+{
+ DROP SCHEMA reindex_schema CASCADE;
+}
+
+session "s1"
+step "begin1" { BEGIN; }
+step "lock1" { LOCK reindex_schema.tab_locked IN SHARE UPDATE EXCLUSIVE MODE; }
+step "end1" { COMMIT; }
+
+session "s2"
+step "reindex2" { REINDEX SCHEMA reindex_schema; }
+step "reindex_conc2" { REINDEX SCHEMA CONCURRENTLY reindex_schema; }
+
+session "s3"
+step "drop3" { DROP TABLE reindex_schema.tab_dropped; }
+
+# The table can be dropped while reindex is waiting.
+permutation "begin1" "lock1" "reindex2" "drop3" "end1"
+permutation "begin1" "lock1" "reindex_conc2" "drop3" "end1"
signature.asc
Description: PGP signature
