On Sun, May 05, 2019 at 05:45:53PM -0400, Tom Lane wrote: > In the other place, checking IsSystemNamespace isn't even > approximately the correct way to proceed, since it fails to reject > reindexing system catalogs' toast tables.
Good point. I overlooked that part. It is easy enough to have a test which fails for a catalog index, a catalog table, a toast table on a system catalog and a toast index on a system catalog. However I don't see a way to test directly that a toast relation or index on a non-catalog relation works as we cannot run REINDEX CONCURRENTLY within a function, and it is not possible to save the toast relation name as a psql variable. Perhaps somebody has a trick?x > After looking around a bit, I propose that we invent > "IsCatalogRelationOid(Oid reloid)" (not wedded to that name), which > is a wrapper around IsCatalogClass() that does the needful syscache > lookup for you. Aside from this use-case, it could be used in > sepgsql/dml.c, which I see is also using > IsSystemNamespace(get_rel_namespace(oid)) for the wrong thing. Hmmm. A wrapper on top of IsCatalogClass() implies that we would need to open the related relation directly in the new function so as it is possible to grab its pg_class entry. We could imply that the function takes a ShareLock all the time, but that's not going to be true most of the time and the recent discussions around lock upgrades stress me a bit, and I'd rather not add new race conditions or upgrade hazards. We should have an extra argument with the lock mode, but we have nothing in catalog.c of that kind, and that does not feel consistent with the current interface. At the end I have made the choice to not reinvent the world, and just get a Relation from the parent table when looking after an index relkind so as IsCatalogRelation() is used for the check. What do you think about the updated patch attached? I have removed the tests from reindex_catalog.sql, and added more coverage into create_index.sql. -- Michael
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 663407c65d..bcc9a09370 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2729,6 +2729,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* Open relation to get its indexes */
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
+ /* A system catalog cannot be reindexed concurrently */
+ if (IsCatalogRelation(heapRelation))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex a system catalog concurrently")));
+
/* Add all the valid indexes of relation to list */
foreach(lc, RelationGetIndexList(heapRelation))
{
@@ -2813,18 +2819,18 @@ ReindexRelationConcurrently(Oid relationOid, int options)
case RELKIND_INDEX:
{
Oid heapId = IndexGetRelation(relationOid, false);
+ Relation heapRelation;
- /* A shared relation cannot be reindexed concurrently */
- if (IsSharedRelation(heapId))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("concurrent reindex is not supported for shared relations")));
+ /* Open relation to check it */
+ heapRelation = table_open(heapId, ShareUpdateExclusiveLock);
/* A system catalog cannot be reindexed concurrently */
- if (IsSystemNamespace(get_rel_namespace(heapId)))
+ if (IsCatalogRelation(heapRelation))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("concurrent reindex is not supported for catalog relations")));
+ errmsg("cannot reindex a system catalog concurrently")));
+
+ table_close(heapRelation, NoLock);
/* Save the list of relation OIDs in private context */
oldcontext = MemoryContextSwitchTo(private_context);
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 326dc44177..7447a1d196 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2092,10 +2092,15 @@ BEGIN;
REINDEX TABLE CONCURRENTLY concur_reindex_tab;
ERROR: REINDEX CONCURRENTLY cannot run inside a transaction block
COMMIT;
-REINDEX TABLE CONCURRENTLY pg_database; -- no shared relation
-ERROR: concurrent index creation on system catalog tables is not supported
-REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relations
-ERROR: concurrent index creation on system catalog tables is not supported
+REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
+ERROR: cannot reindex a system catalog concurrently
+REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
+ERROR: cannot reindex a system catalog concurrently
+-- These are the toast table and index from pg_authid
+REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no toast table
+ERROR: cannot reindex a system catalog concurrently
+REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no toast index
+ERROR: cannot reindex a system catalog concurrently
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
ERROR: concurrent reindex of system catalogs is not supported
-- Warns about catalog relations
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f29b8ca826..ad806a09d8 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -838,8 +838,11 @@ DROP TABLE concur_reindex_part;
BEGIN;
REINDEX TABLE CONCURRENTLY concur_reindex_tab;
COMMIT;
-REINDEX TABLE CONCURRENTLY pg_database; -- no shared relation
-REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relations
+REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
+REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
+-- These are the toast table and index from pg_authid
+REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no toast table
+REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no toast index
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
-- Warns about catalog relations
REINDEX SCHEMA CONCURRENTLY pg_catalog;
signature.asc
Description: PGP signature
