On Mon, May 27, 2019 at 12:20:58AM -0400, Alvaro Herrera wrote: > I wonder if we really want to abolish all distinction between "cannot do > X" and "Y is not supported". I take the former to mean that the > operation is impossible to do for some reason, while the latter means we > just haven't implemented it yet and it seems likely to get implemented > in a reasonable timeframe. See some excellent commentary about about > the "can not" wording at > https://postgr.es/m/CA+TgmoYS8jKhETyhGYTYMcbvGPwYY=qa6yyp9b47mx7mwee...@mail.gmail.com
Incorrect URL?
> I notice your patch changes "catalog relations" to "system catalogs".
> I think we predominantly prefer the latter, so that part of your change
> seems OK. (In passing, I noticed we have a couple of places using
> "system catalog tables", which is weird.)
Good point. These are not new though, so I would prefer not touch
those parts for this patch.
src/backend/catalog/index.c: errmsg("user-defined
indexes on system catalog tables are not supported")));
src/backend/catalog/index.c: errmsg("concurrent index
creation on system catalog tables is not supported")));
src/backend/catalog/index.c: errmsg("user-defined
indexes on system catalog tables are not supported")));
src/backend/parser/parse_clause.c: errmsg("ON CONFLICT
is not supported with system catalog tables"),
> I think reindexing system catalogs concurrently is a complex enough
> undertaking that implementing it is far enough in the future that the
> "cannot" wording is okay; but reindexing partitioned tables is not so
> obviously out of the question.
I am not sure that we actually can without much complication, as
technically locks on catalogs may get released before commit if I
recall correctly.
> We do have "is not yet implemented" in a
> couple of other places, so all things considered I'm not so sure about
> changing that one to "cannot".
Okay. I can live with this difference. Not changing the string in
ReindexRelationConcurrently() has the merit to be consistent with the
existing ones in reindex_relation() and ReindexPartitionedIndex().
Please find attached an updated version. What do you think?
--
Michael
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 40ea629ffe..217f668d15 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2485,7 +2485,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
if (objectKind == REINDEX_OBJECT_SYSTEM && concurrent)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("concurrent reindex of system catalogs is not supported")));
+ errmsg("cannot reindex system catalogs concurrently")));
/*
* Get OID of object to reindex, being the database currently being used
@@ -2599,7 +2599,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
if (!concurrent_warning)
ereport(WARNING,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("concurrent reindex is not supported for catalog relations, skipping all")));
+ errmsg("cannot reindex system catalogs concurrently, skipping all")));
concurrent_warning = true;
continue;
}
@@ -2747,7 +2747,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
if (IsCatalogRelationOid(relationOid))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex a system catalog concurrently")));
+ errmsg("cannot reindex system catalogs concurrently")));
/* Open relation to get its indexes */
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
@@ -2841,7 +2841,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
if (IsCatalogRelationOid(heapId))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex a system catalog concurrently")));
+ errmsg("cannot reindex system catalogs concurrently")));
/* 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 c8bc6be061..bf39d51f52 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2093,19 +2093,19 @@ REINDEX TABLE CONCURRENTLY concur_reindex_tab;
ERROR: REINDEX CONCURRENTLY cannot run inside a transaction block
COMMIT;
REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
-ERROR: cannot reindex a system catalog concurrently
+ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
-ERROR: cannot reindex a system catalog concurrently
+ERROR: cannot reindex system catalogs concurrently
-- These are the toast table and index of pg_authid.
REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
-ERROR: cannot reindex a system catalog concurrently
+ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
-ERROR: cannot reindex a system catalog concurrently
+ERROR: cannot reindex system catalogs concurrently
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
-ERROR: concurrent reindex of system catalogs is not supported
+ERROR: cannot reindex system catalogs concurrently
-- Warns about catalog relations
REINDEX SCHEMA CONCURRENTLY pg_catalog;
-WARNING: concurrent reindex is not supported for catalog relations, skipping all
+WARNING: cannot reindex system catalogs concurrently, skipping all
-- Check the relation status, there should not be invalid indexes
\d concur_reindex_tab
Table "public.concur_reindex_tab"
signature.asc
Description: PGP signature
