REINDEX SCHEMA/DATABASE/SYSTEM weak with dropped relations
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
Attachments:
reindex-schema-fix-v1.patchtext/x-diff; charset=us-asciiDownload
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"
On 13.08.2020 07:38, Michael Paquier wrote:
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
Hi,
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.
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.
��� /*
��� �* If the relation has a secondary toast rel, reindex that too while we
��� �* still hold the lock on the main table.
��� �*/
��� if ((flags & REINDEX_REL_PROCESS_TOAST) && OidIsValid(toast_relid))
��� ��� result |= reindex_relation(toast_relid, flags, options);
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
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
Attachments:
reindex-schema-fix-v2.patchtext/x-diff; charset=us-asciiDownload
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"
On 01.09.2020 04:38, Michael Paquier wrote:
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.
Great, it took me a moment to understand the logic around index list
check at first pass.
Does the version attached look fine to you? I have done one round of
indentation while on it.
Yes, this version is good.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Tue, Sep 01, 2020 at 01:25:27PM +0300, Anastasia Lubennikova wrote:
Yes, this version is good.
Thanks. I have added an extra comment for the case of RELKIND_INDEX
with REINDEXOPT_MISSING_OK while on it, as it was not really obvious
why the parent relation needs to be locked (at least attempted to) at
this stage. And applied the change. Thanks for the review,
Anastasia.
--
Michael
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 */
I think you probably intended to write: 1<<2
Even though it's the same, someone is likely to be confused if they try to use
3<<1 vs 1<<3.
I noticed while resolving merge conflict.
--
Justin