On Mon, Aug 31, 2020 at 06:10:46PM +0300, Anastasia Lubennikova wrote: > I reviewed the patch. It does work and the code is clean and sane. It > implements a logic that we already had in CLUSTER, so I think it was simply > an oversight. Thank you for fixing this.
Thanks Anastasia for the review. > I noticed that REINDEXOPT_MISSING_OK can be passed to the TOAST table > reindex. I think it would be better to reset the flag in this > reindex_relation() call, as we don't expect a concurrent DROP here. Good point, and fixed by resetting the flag here if it is set. I have added some extra comments. There is one in ReindexRelationConcurrently() to mention that there should be no extra use of MISSING_OK once the list of indexes is built as session locks are taken where needed. Does the version attached look fine to you? I have done one round of indentation while on it. -- 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 47d4c07306..23840bb8e6 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3352,6 +3352,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 d0ec9a4b9c..a9d7685021 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3437,8 +3437,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)
{
@@ -3672,7 +3684,17 @@ 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, case possible if REINDEXOPT_MISSING_OK is
+ * set and if the relation has been dropped.
+ */
+ if (!rel)
+ return false;
/*
* This may be useful when implemented someday; but that day is not today.
@@ -3771,7 +3793,14 @@ reindex_relation(Oid relid, int flags, int options)
* still hold the lock on the main table.
*/
if ((flags & REINDEX_REL_PROCESS_TOAST) && OidIsValid(toast_relid))
- result |= reindex_relation(toast_relid, flags, options);
+ {
+ /*
+ * Note that this should fail if the toast relation is missing, so
+ * reset REINDEXOPT_MISSING_OK.
+ */
+ result |= reindex_relation(toast_relid, flags,
+ options & ~(REINDEXOPT_MISSING_OK));
+ }
return result;
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 254dbcdce5..d4798d86f4 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2766,9 +2766,18 @@ 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
@@ -2778,7 +2787,9 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
result = reindex_relation(relid,
REINDEX_REL_PROCESS_TOAST |
REINDEX_REL_CHECK_CONSTRAINTS,
- options | REINDEXOPT_REPORT_PROGRESS);
+ options |
+ REINDEXOPT_REPORT_PROGRESS |
+ REINDEXOPT_MISSING_OK);
if (result && (options & REINDEXOPT_VERBOSE))
ereport(INFO,
@@ -2893,7 +2904,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 +2999,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 +3022,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);
@@ -3025,7 +3070,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
break;
}
- /* Definitely no indexes, so leave */
+ /*
+ * Definitely no indexes, so leave. Any checks based on
+ * REINDEXOPT_MISSING_OK should be done only while the list of indexes to
+ * work on is built as the session locks taken before this transaction
+ * commits will make sure that they cannot be dropped by a concurrent
+ * session until this operation completes.
+ */
if (indexIds == NIL)
{
PopActiveSnapshot();
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
