Inconsistent error message wording for REINDEX CONCURRENTLY
In view of the REINDEX-on-pg_class kerfuffle that we're currently
sorting through, I was very glad to see that the concurrent reindex
code doesn't even try:
regression=# reindex index concurrently pg_class_oid_index;
psql: ERROR: concurrent reindex is not supported for catalog relations
regression=# reindex table concurrently pg_class;
psql: ERROR: concurrent index creation on system catalog tables is not supported
It'd be nice though if those error messages gave the impression of having
been written on the same planet.
(It might be worth comparing wording of other errors-in-common between
what are evidently two completely different code paths...)
regards, tom lane
On Thu, May 02, 2019 at 10:06:42AM -0400, Tom Lane wrote:
In view of the REINDEX-on-pg_class kerfuffle that we're currently
sorting through, I was very glad to see that the concurrent reindex
code doesn't even try:regression=# reindex index concurrently pg_class_oid_index;
psql: ERROR: concurrent reindex is not supported for catalog relations
regression=# reindex table concurrently pg_class;
psql: ERROR: concurrent index creation on system catalog tables is not supportedIt'd be nice though if those error messages gave the impression of having
been written on the same planet.
We could do a larger brush-up of error messages in this area, as these
are full sentences which is not a style allowed, no? The second error
message can be used as well by both CREATE INDEX CONCURRENTLY and
REINDEX CONCURRENTLY, but not the first one, so the first one needs to
be more generic than the second one. How about the following changes
for at least these two?
"cannot use REINDEX CONCURRENTLY on system catalogs"
"cannot create index on system catalog concurrently"
Then we have some other messages in index.c which could be cleaned
up.. For example at the beginning of index_constraint_create(), there
are two them, but there is much more which could be improved. Do you
think this is worth having a look and fixing?
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Thu, May 02, 2019 at 10:06:42AM -0400, Tom Lane wrote:
regression=# reindex index concurrently pg_class_oid_index;
psql: ERROR: concurrent reindex is not supported for catalog relations
regression=# reindex table concurrently pg_class;
psql: ERROR: concurrent index creation on system catalog tables is not supportedIt'd be nice though if those error messages gave the impression of having
been written on the same planet.
We could do a larger brush-up of error messages in this area, as these
are full sentences which is not a style allowed, no?
I wouldn't object to either one in isolation, it's the inconsistency
that irks me.
How about the following changes
for at least these two?
"cannot use REINDEX CONCURRENTLY on system catalogs"
"cannot create index on system catalog concurrently"
I'd suggest something like "cannot reindex a system catalog concurrently"
for both cases. The "cannot create index" wording doesn't seem to me to
be very relevant, because if you try that you'll get
regression=# create index on pg_class(relchecks);
psql: ERROR: permission denied: "pg_class" is a system catalog
Then we have some other messages in index.c which could be cleaned
up.. For example at the beginning of index_constraint_create(), there
are two them, but there is much more which could be improved. Do you
think this is worth having a look and fixing?
I'm not excited about rewording longstanding errors. These two are
new though (aren't they?)
regards, tom lane
On Sat, May 04, 2019 at 11:00:11AM -0400, Tom Lane wrote:
I'm not excited about rewording longstanding errors. These two are
new though (aren't they?)
The message you are referring to in index_create() has been introduced
as of e093dcdd with the introduction of CREATE INDEX CONCURRENTLY, and
it can be perfectly hit without REINDEX:
=# show allow_system_table_mods;
allow_system_table_mods
-------------------------
on
(1 row)
=# create index CONCURRENTLY popo on pg_class (relname);
ERROR: 0A000: concurrent index creation on system catalog tables is
not supported
LOCATION: index_create, index.c:830
So I don't agree with switching the existing error message in
index_create(). What we could do instead is to add a REINDEX-specific
error in ReindexRelationConcurrently() as done for index relkinds,
using your proposed wording.
What do you think about something like the attached then? HEAD does
not check after system indexes with REINDEX INDEX CONCURRENTLY, and I
have moved all the catalog-related tests to reindex_catalog.sql.
--
Michael
Attachments:
reindex-catalog-errors.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index fabba3bacd..519673a2e3 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 (IsSystemRelation(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))
{
@@ -2814,17 +2820,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
{
Oid heapId = IndexGetRelation(relationOid, false);
- /* 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")));
-
/* A system catalog cannot be reindexed concurrently */
if (IsSystemNamespace(get_rel_namespace(heapId)))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("concurrent reindex is not supported for catalog relations")));
+ errmsg("cannot reindex a system catalog 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 326dc44177..ab8b1eb3fe 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2092,10 +2092,6 @@ 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 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/expected/reindex_catalog.out b/src/test/regress/expected/reindex_catalog.out
index 142616fccb..70a3f906da 100644
--- a/src/test/regress/expected/reindex_catalog.out
+++ b/src/test/regress/expected/reindex_catalog.out
@@ -31,3 +31,28 @@ REINDEX INDEX pg_index_indexrelid_index; -- non-mapped, non-shared, critical
REINDEX INDEX pg_index_indrelid_index; -- non-mapped, non-shared, non-critical
REINDEX INDEX pg_database_oid_index; -- mapped, shared, critical
REINDEX INDEX pg_shdescription_o_c_index; -- mapped, shared, non-critical
+-- Check that REINDEX CONCURRENTLY is forbidden with system catalogs.
+-- For whole tables.
+REINDEX TABLE CONCURRENTLY pg_class; -- mapped, non-shared, critical
+ERROR: cannot reindex a system catalog concurrently
+REINDEX TABLE CONCURRENTLY pg_index; -- non-mapped, non-shared, critical
+ERROR: cannot reindex a system catalog concurrently
+REINDEX TABLE CONCURRENTLY pg_operator; -- non-mapped, non-shared, critical
+ERROR: cannot reindex a system catalog concurrently
+REINDEX TABLE CONCURRENTLY pg_database; -- mapped, shared, critical
+ERROR: cannot reindex a system catalog concurrently
+REINDEX TABLE CONCURRENTLY pg_shdescription; -- mapped, shared non-critical
+ERROR: cannot reindex a system catalog concurrently
+-- For system indexes
+REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- mapped, non-shared, critical
+ERROR: cannot reindex a system catalog concurrently
+REINDEX INDEX CONCURRENTLY pg_class_relname_nsp_index; -- mapped, non-shared, non-critical
+ERROR: cannot reindex a system catalog concurrently
+REINDEX INDEX CONCURRENTLY pg_index_indexrelid_index; -- non-mapped, non-shared, critical
+ERROR: cannot reindex a system catalog concurrently
+REINDEX INDEX CONCURRENTLY pg_index_indrelid_index; -- non-mapped, non-shared, non-critical
+ERROR: cannot reindex a system catalog concurrently
+REINDEX INDEX CONCURRENTLY pg_database_oid_index; -- mapped, shared, critical
+ERROR: cannot reindex a system catalog concurrently
+REINDEX INDEX CONCURRENTLY pg_shdescription_o_c_index; -- mapped, shared, non-critical
+ERROR: cannot reindex a system catalog concurrently
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f29b8ca826..7bcc79b458 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -838,8 +838,6 @@ 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 SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
-- Warns about catalog relations
REINDEX SCHEMA CONCURRENTLY pg_catalog;
diff --git a/src/test/regress/sql/reindex_catalog.sql b/src/test/regress/sql/reindex_catalog.sql
index 2180ee5791..72d2996132 100644
--- a/src/test/regress/sql/reindex_catalog.sql
+++ b/src/test/regress/sql/reindex_catalog.sql
@@ -34,3 +34,18 @@ REINDEX INDEX pg_index_indexrelid_index; -- non-mapped, non-shared, critical
REINDEX INDEX pg_index_indrelid_index; -- non-mapped, non-shared, non-critical
REINDEX INDEX pg_database_oid_index; -- mapped, shared, critical
REINDEX INDEX pg_shdescription_o_c_index; -- mapped, shared, non-critical
+
+-- Check that REINDEX CONCURRENTLY is forbidden with system catalogs.
+-- For whole tables.
+REINDEX TABLE CONCURRENTLY pg_class; -- mapped, non-shared, critical
+REINDEX TABLE CONCURRENTLY pg_index; -- non-mapped, non-shared, critical
+REINDEX TABLE CONCURRENTLY pg_operator; -- non-mapped, non-shared, critical
+REINDEX TABLE CONCURRENTLY pg_database; -- mapped, shared, critical
+REINDEX TABLE CONCURRENTLY pg_shdescription; -- mapped, shared non-critical
+-- For system indexes
+REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- mapped, non-shared, critical
+REINDEX INDEX CONCURRENTLY pg_class_relname_nsp_index; -- mapped, non-shared, non-critical
+REINDEX INDEX CONCURRENTLY pg_index_indexrelid_index; -- non-mapped, non-shared, critical
+REINDEX INDEX CONCURRENTLY pg_index_indrelid_index; -- non-mapped, non-shared, non-critical
+REINDEX INDEX CONCURRENTLY pg_database_oid_index; -- mapped, shared, critical
+REINDEX INDEX CONCURRENTLY pg_shdescription_o_c_index; -- mapped, shared, non-critical
Michael Paquier <michael@paquier.xyz> writes:
The message you are referring to in index_create() has been introduced
as of e093dcdd with the introduction of CREATE INDEX CONCURRENTLY, and
it can be perfectly hit without REINDEX:
=# show allow_system_table_mods;
allow_system_table_mods
-------------------------
on
(1 row)
Oh, yeah, if you do that you can get to it.
What do you think about something like the attached then? HEAD does
not check after system indexes with REINDEX INDEX CONCURRENTLY, and I
have moved all the catalog-related tests to reindex_catalog.sql.
OK as far as the wording goes, but now that I look at the specific tests
that are being applied, they seem rather loony, as well as inconsistent
between the two cases. IsSystemRelation *sounds* like the right thing,
but it's not, because it forbids user-relation toast tables which seems
like a restriction we need not make. I think IsCatalogRelation is the
test we actually want there. 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.
We should be doing the equivalent of IsCatalogRelation there too.
(It's a bit of a pain that catalog.c doesn't offer a function that
makes that determination from just an OID. Should we add one?
There might be other callers for it.)
I concur that we shouldn't need a separate check for relisshared,
since all shared rels should be system catalogs.
I"m not sure I'd move these error-case tests to reindex_catalog.sql ---
bear in mind that later today, that test is either going away entirely
or at least not getting run by default anymore.
regards, tom lane
I wrote:
Michael Paquier <michael@paquier.xyz> writes:
What do you think about something like the attached then? HEAD does
not check after system indexes with REINDEX INDEX CONCURRENTLY, and I
have moved all the catalog-related tests to reindex_catalog.sql.
OK as far as the wording goes, but now that I look at the specific tests
that are being applied, they seem rather loony, as well as inconsistent
between the two cases. IsSystemRelation *sounds* like the right thing,
but it's not, because it forbids user-relation toast tables which seems
like a restriction we need not make. I think IsCatalogRelation is the
test we actually want there. 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.
We should be doing the equivalent of IsCatalogRelation there too.
(It's a bit of a pain that catalog.c doesn't offer a function that
makes that determination from just an OID. Should we add one?
There might be other callers for it.)
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.
I'm also thinking that it'd be a good idea to rename IsSystemNamespace
to IsCatalogNamespace. The existing naming is totally confusing given
that it doesn't square with the distinction between IsSystemRelation
and IsCatalogRelation (ie that the former includes user toast tables).
There are only five external callers of it, and per this discussion
at least two of them are wrong anyway.
I was thinking about also proposing that we rename IsSystemRelation
to IsCatalogOrToastRelation (likewise for IsSystemClass), which would
be clearer as to its semantics. However, after looking through the
code, it seems that 90% of the callers are using those functions to
decide whether to apply !allow_system_table_mods restrictions, and
indeed it's likely that some of the other 10% are wrong and should be
testing IsCatalogRelation/Class instead. So unless we want to rename
that GUC, I think the existing names of these functions are fine but
we should adjust their comments to explain that this is the primary
if not sole use-case. Another idea is to make IsSystemRelation/Class
be macros for IsCatalogOrToastRelation/Class, with the intention that
we use the former names specifically for allow_system_table_mods tests
and the latter names for anything else that happens to really want
those semantics.
There's some other cleanup I want to do in catalog.c --- many of the
comments are desperately in need of copy-editing, to start with ---
but I don't think any of the rest of it would be controversial.
regards, tom lane
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
Attachments:
reindex-catalog-errors-v2.patchtext/x-diff; charset=us-asciiDownload
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;
I wrote:
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.
I'm also thinking that it'd be a good idea to rename IsSystemNamespace
to IsCatalogNamespace. The existing naming is totally confusing given
that it doesn't square with the distinction between IsSystemRelation
and IsCatalogRelation (ie that the former includes user toast tables).
There are only five external callers of it, and per this discussion
at least two of them are wrong anyway.
After studying the callers of these catalog.c functions for awhile,
I realized that IsCatalogRelation/Class are really fundamentally wrong,
and have been so for a long time. The reason is that while they will
return FALSE for tables in the information_schema, they will return
TRUE for toast tables attached to the information_schema tables.
(They're toast tables, and they have OIDs below FirstNormalObjectId,
so there you have it.) This is wrong on its face: if those tables don't
need to be protected as catalogs, why should their TOAST appendages
need it? Moreover, if you drop and recreate information_schema, you'll
start getting different behavior for them, which is even sillier.
I was driven to this realization by the following very confused (and
confusing) bit in ReindexMultipleTables:
/*
* Skip system tables that index_create() would reject to index
* concurrently. XXX We need the additional check for
* FirstNormalObjectId to skip information_schema tables, because
* IsCatalogClass() here does not cover information_schema, but the
* check in index_create() will error on the TOAST tables of
* information_schema tables.
*/
if (concurrent &&
(IsCatalogClass(relid, classtuple) || relid < FirstNormalObjectId))
{
That's nothing but a hack, and the reason it's necessary is that
index_create will throw error if IsCatalogRelation is true, which
it will be for information_schema TOAST tables --- but not for their
parent tables that are being examined here.
After looking around, it seems to me that the correct definition for
IsCatalogRelation is just "is the OID less than FirstBootstrapObjectId?".
Currently we could actually restrict it to "less than
FirstGenbkiObjectId", because all the catalogs, indexes, and TOAST tables
have hand-assigned OIDs --- but perhaps someday we'll let genbki.pl
assign some of those OIDs, so I prefer the weaker constraint. In any
case, this gives us a correct separation between objects that are
traceable to the bootstrap data and those that are created by plain SQL
later in initdb.
With this, the Form_pg_class argument to IsCatalogClass becomes
vestigial. I'm tempted to get rid of that function altogether in
favor of direct calls to IsCatalogRelationOid, but haven't done so
in the attached.
Comments?
regards, tom lane
Attachments:
clean-up-catalog.c-functions-1.patchtext/x-diff; charset=us-ascii; name=clean-up-catalog.c-functions-1.patchDownload
diff --git a/contrib/sepgsql/dml.c b/contrib/sepgsql/dml.c
index cc934b5..2892346 100644
--- a/contrib/sepgsql/dml.c
+++ b/contrib/sepgsql/dml.c
@@ -161,12 +161,10 @@ check_relation_privileges(Oid relOid,
*/
if (sepgsql_getenforce() > 0)
{
- Oid relnamespace = get_rel_namespace(relOid);
-
- if (IsSystemNamespace(relnamespace) &&
- (required & (SEPG_DB_TABLE__UPDATE |
+ if ((required & (SEPG_DB_TABLE__UPDATE |
SEPG_DB_TABLE__INSERT |
- SEPG_DB_TABLE__DELETE)) != 0)
+ SEPG_DB_TABLE__DELETE)) != 0 &&
+ IsCatalogRelationOid(relOid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("SELinux: hardwired security policy violation")));
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 6d8c746..b5e554f 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -53,15 +53,18 @@
/*
* IsSystemRelation
- * True iff the relation is either a system catalog or toast table.
- * By a system catalog, we mean one that created in the pg_catalog schema
- * during initdb. User-created relations in pg_catalog don't count as
- * system catalogs.
+ * True iff the relation is either a system catalog or a toast table.
+ * See IsCatalogRelation for the exact definition of a system catalog.
*
- * NB: TOAST relations are considered system relations by this test
- * for compatibility with the old IsSystemRelationName function.
- * This is appropriate in many places but not all. Where it's not,
- * also check IsToastRelation or use IsCatalogRelation().
+ * We treat toast tables of user relations as "system relations" for
+ * protection purposes, e.g. you can't change their schemas without
+ * special permissions. Therefore, most uses of this function are
+ * checking whether allow_system_table_mods restrictions apply.
+ * For other purposes, consider whether you shouldn't be using
+ * IsCatalogRelation instead.
+ *
+ * This function does not perform any catalog accesses.
+ * Some callers rely on that!
*/
bool
IsSystemRelation(Relation relation)
@@ -78,67 +81,86 @@ IsSystemRelation(Relation relation)
bool
IsSystemClass(Oid relid, Form_pg_class reltuple)
{
- return IsToastClass(reltuple) || IsCatalogClass(relid, reltuple);
+ /* IsCatalogRelationOid is a bit faster, so test that first */
+ return (IsCatalogRelationOid(relid) || IsToastClass(reltuple));
}
/*
* IsCatalogRelation
- * True iff the relation is a system catalog, or the toast table for
- * a system catalog. By a system catalog, we mean one that created
- * in the pg_catalog schema during initdb. As with IsSystemRelation(),
- * user-created relations in pg_catalog don't count as system catalogs.
+ * True iff the relation is a system catalog.
+ *
+ * By a system catalog, we mean one that is created during the bootstrap
+ * phase of initdb. That includes not just the catalogs per se, but
+ * also their indexes, and TOAST tables and indexes if any.
*
- * Note that IsSystemRelation() returns true for ALL toast relations,
- * but this function returns true only for toast relations of system
- * catalogs.
+ * This function does not perform any catalog accesses.
+ * Some callers rely on that!
*/
bool
IsCatalogRelation(Relation relation)
{
- return IsCatalogClass(RelationGetRelid(relation), relation->rd_rel);
+ return IsCatalogRelationOid(RelationGetRelid(relation));
}
/*
* IsCatalogClass
- * True iff the relation is a system catalog relation.
- *
- * Check IsCatalogRelation() for details.
+ * Like the above, but takes a Form_pg_class as argument.
+ * Used when we do not want to open the relation and have to
+ * search pg_class directly.
*/
bool
IsCatalogClass(Oid relid, Form_pg_class reltuple)
{
- Oid relnamespace = reltuple->relnamespace;
+ return IsCatalogRelationOid(relid);
+}
+/*
+ * IsCatalogRelationOid
+ * True iff the relation identified by this OID is a system catalog.
+ *
+ * By a system catalog, we mean one that is created during the bootstrap
+ * phase of initdb. That includes not just the catalogs per se, but
+ * also their indexes, and TOAST tables and indexes if any.
+ *
+ * This function does not perform any catalog accesses.
+ * Some callers rely on that!
+ */
+bool
+IsCatalogRelationOid(Oid relid)
+{
/*
- * Never consider relations outside pg_catalog/pg_toast to be catalog
- * relations.
- */
- if (!IsSystemNamespace(relnamespace) && !IsToastNamespace(relnamespace))
- return false;
-
- /* ----
- * Check whether the oid was assigned during initdb, when creating the
- * initial template database. Minus the relations in information_schema
- * excluded above, these are integral part of the system.
- * We could instead check whether the relation is pinned in pg_depend, but
- * this is noticeably cheaper and doesn't require catalog access.
+ * We consider a relation to be a system catalog if it has an OID that was
+ * manually assigned or assigned by genbki.pl. This includes all the
+ * defined catalogs, their indexes, and their TOAST tables and indexes.
+ *
+ * This rule excludes the relations in information_schema, which are not
+ * integral to the system and can be treated the same as user relations.
+ * (Since it's valid to drop and recreate information_schema, any rule
+ * that did not act this way would be wrong.)
*
- * This test is safe since even an oid wraparound will preserve this
- * property (cf. GetNewObjectId()) and it has the advantage that it works
- * correctly even if a user decides to create a relation in the pg_catalog
- * namespace.
- * ----
+ * This test is reliable since an OID wraparound will skip this range of
+ * OIDs; see GetNewObjectId().
*/
- return relid < FirstNormalObjectId;
+ return (relid < (Oid) FirstBootstrapObjectId);
}
/*
* IsToastRelation
* True iff relation is a TOAST support relation (or index).
+ *
+ * Does not perform any catalog accesses.
*/
bool
IsToastRelation(Relation relation)
{
+ /*
+ * What we actually check is whether the relation belongs to a pg_toast
+ * namespace. This should be equivalent because of restrictions that are
+ * enforced elsewhere against creating user relations in, or moving
+ * relations into/out of, a pg_toast namespace. Notice also that this
+ * will not say "true" for toast tables belonging to other sessions' temp
+ * tables; we expect that other mechanisms will prevent access to those.
+ */
return IsToastNamespace(RelationGetNamespace(relation));
}
@@ -157,14 +179,16 @@ IsToastClass(Form_pg_class reltuple)
}
/*
- * IsSystemNamespace
+ * IsCatalogNamespace
* True iff namespace is pg_catalog.
*
+ * Does not perform any catalog accesses.
+ *
* NOTE: the reason this isn't a macro is to avoid having to include
* catalog/pg_namespace.h in a lot of places.
*/
bool
-IsSystemNamespace(Oid namespaceId)
+IsCatalogNamespace(Oid namespaceId)
{
return namespaceId == PG_CATALOG_NAMESPACE;
}
@@ -173,9 +197,13 @@ IsSystemNamespace(Oid namespaceId)
* IsToastNamespace
* True iff namespace is pg_toast or my temporary-toast-table namespace.
*
+ * Does not perform any catalog accesses.
+ *
* Note: this will return false for temporary-toast-table namespaces belonging
* to other backends. Those are treated the same as other backends' regular
* temp table namespaces, and access is prevented where appropriate.
+ * If you need to check for those, you may be able to use isAnyTempNamespace,
+ * but beware that that does involve a catalog access.
*/
bool
IsToastNamespace(Oid namespaceId)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index ee6b72e..6cffe55 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -324,7 +324,7 @@ heap_create(const char *relname,
* user defined relation, not a system one.
*/
if (!allow_system_table_mods &&
- ((IsSystemNamespace(relnamespace) && relkind != RELKIND_INDEX) ||
+ ((IsCatalogNamespace(relnamespace) && relkind != RELKIND_INDEX) ||
IsToastNamespace(relnamespace)) &&
IsNormalProcessingMode())
ereport(ERROR,
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 663407c..ccbe08c 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2569,15 +2569,11 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
continue;
/*
- * Skip system tables that index_create() would reject to index
- * concurrently. XXX We need the additional check for
- * FirstNormalObjectId to skip information_schema tables, because
- * IsCatalogClass() here does not cover information_schema, but the
- * check in index_create() will error on the TOAST tables of
- * information_schema tables.
+ * Skip system tables, since index_create() would reject indexing them
+ * concurrently (and it would likely fail if we tried).
*/
if (concurrent &&
- (IsCatalogClass(relid, classtuple) || relid < FirstNormalObjectId))
+ IsCatalogClass(relid, classtuple))
{
if (!concurrent_warning)
ereport(WARNING,
@@ -2821,7 +2817,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
errmsg("concurrent reindex is not supported for shared relations")));
/* A system catalog cannot be reindexed concurrently */
- if (IsSystemNamespace(get_rel_namespace(heapId)))
+ if (IsCatalogRelationOid(heapId))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("concurrent reindex is not supported for catalog relations")));
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8e4743d..97c58c8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12453,9 +12453,10 @@ AlterTableMoveAll(AlterTableMoveAllStmt *stmt)
* Also, explicitly avoid any shared tables, temp tables, or TOAST
* (TOAST will be moved with the main table).
*/
- if (IsSystemNamespace(relForm->relnamespace) || relForm->relisshared ||
+ if (IsCatalogNamespace(relForm->relnamespace) ||
+ relForm->relisshared ||
isAnyTempNamespace(relForm->relnamespace) ||
- relForm->relnamespace == PG_TOAST_NAMESPACE)
+ IsToastNamespace(relForm->relnamespace))
continue;
/* Only move the object type requested */
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index eaf2b18..d0f6f71 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3316,8 +3316,8 @@ RelationBuildLocalRelation(const char *relname,
else
rel->rd_rel->relispopulated = true;
- /* system relations and non-table objects don't have one */
- if (!IsSystemNamespace(relnamespace) &&
+ /* set replica identity -- system catalogs and non-tables don't have one */
+ if (!IsCatalogNamespace(relnamespace) &&
(relkind == RELKIND_RELATION ||
relkind == RELKIND_MATVIEW ||
relkind == RELKIND_PARTITIONED_TABLE))
diff --git a/src/include/catalog/catalog.h b/src/include/catalog/catalog.h
index a58d09d..47c48b8 100644
--- a/src/include/catalog/catalog.h
+++ b/src/include/catalog/catalog.h
@@ -26,7 +26,9 @@ extern bool IsSystemClass(Oid relid, Form_pg_class reltuple);
extern bool IsToastClass(Form_pg_class reltuple);
extern bool IsCatalogClass(Oid relid, Form_pg_class reltuple);
-extern bool IsSystemNamespace(Oid namespaceId);
+extern bool IsCatalogRelationOid(Oid relid);
+
+extern bool IsCatalogNamespace(Oid namespaceId);
extern bool IsToastNamespace(Oid namespaceId);
extern bool IsReservedName(const char *name);
On Tue, May 07, 2019 at 05:19:38PM -0400, Tom Lane wrote:
That's nothing but a hack, and the reason it's necessary is that
index_create will throw error if IsCatalogRelation is true, which
it will be for information_schema TOAST tables --- but not for their
parent tables that are being examined here.
Oh. Good catch. That's indeed crazy now that I look closer at that.
After looking around, it seems to me that the correct definition for
IsCatalogRelation is just "is the OID less than FirstBootstrapObjectId?".
Currently we could actually restrict it to "less than
FirstGenbkiObjectId", because all the catalogs, indexes, and TOAST tables
have hand-assigned OIDs --- but perhaps someday we'll let genbki.pl
assign some of those OIDs, so I prefer the weaker constraint. In any
case, this gives us a correct separation between objects that are
traceable to the bootstrap data and those that are created by plain SQL
later in initdb.With this, the Form_pg_class argument to IsCatalogClass becomes
vestigial. I'm tempted to get rid of that function altogether in
favor of direct calls to IsCatalogRelationOid, but haven't done so
in the attached.
I think that removing entirely IsCatalogClass() is just better as if
any extension uses this routine, then it could potentially simplify
its code because needing Form_pg_class means usually opening a
Relation, and this can be removed.
With IsCatalogClass() removed, the only dependency with Form_pg_class
comes from IsToastClass() which is not used at all except in
IsSystemClass(). Wouldn't it be better to remove entirely
IsToastClass() and switch IsSystemClass() to use a namespace OID
instead of Form_pg_class?
With your patch, ReindexRelationConcurrently() does not complain for
REINDEX TABLE CONCURRENTLY for a catalog table and would trigger the
error from index_create(), which is at the origin of this thread. The
check with IsSharedRelation() for REINDEX INDEX CONCURRENTLY is
useless and the error message generated for IsCatalogRelationOid()
still needs to be improved. Would you prefer to include those changes
in your patch? Or should I work on top of what you are proposing
(your patch does not include negative tests for toast index and
tables on catalogs either).
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Tue, May 07, 2019 at 05:19:38PM -0400, Tom Lane wrote:
With this, the Form_pg_class argument to IsCatalogClass becomes
vestigial. I'm tempted to get rid of that function altogether in
favor of direct calls to IsCatalogRelationOid, but haven't done so
in the attached.
I think that removing entirely IsCatalogClass() is just better as if
any extension uses this routine, then it could potentially simplify
its code because needing Form_pg_class means usually opening a
Relation, and this can be removed.
Yeah, it's clearly easier to use without the extra argument.
With IsCatalogClass() removed, the only dependency with Form_pg_class
comes from IsToastClass() which is not used at all except in
IsSystemClass(). Wouldn't it be better to remove entirely
IsToastClass() and switch IsSystemClass() to use a namespace OID
instead of Form_pg_class?
Not sure. The way it's defined has the advantage of being more
independent of exactly what the implementation of the "is a toast table"
check is. Also, I looked around to see if any callers could really be
simplified if they only had to pass the table OID, and didn't find much;
almost all of them are looking at the pg_class tuple themselves, typically
to check the relkind too. So we'd not make any net savings in syscache
lookups by changing IsSystemClass's API. I'm kind of inclined to leave
it alone.
With your patch, ReindexRelationConcurrently() does not complain for
REINDEX TABLE CONCURRENTLY for a catalog table and would trigger the
error from index_create(), which is at the origin of this thread. The
check with IsSharedRelation() for REINDEX INDEX CONCURRENTLY is
useless and the error message generated for IsCatalogRelationOid()
still needs to be improved. Would you prefer to include those changes
in your patch? Or should I work on top of what you are proposing
(your patch does not include negative tests for toast index and
tables on catalogs either).
Yes, we still need to do your patch on top of this one (or really
either order would do). I think keeping them separate is good.
BTW, when I was looking at this I got dissatisfied about another
aspect of the wording of the relevant error messages: a lot of them
are like, for example
errmsg("cannot reindex concurrently this type of relation")));
While that matches the command syntax we're using, it's just horrid
English grammar. Better would be
errmsg("cannot reindex this type of relation concurrently")));
Can we change that while we're at it?
regards, tom lane
On Wed, May 08, 2019 at 08:31:54AM -0400, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
With IsCatalogClass() removed, the only dependency with Form_pg_class
comes from IsToastClass() which is not used at all except in
IsSystemClass(). Wouldn't it be better to remove entirely
IsToastClass() and switch IsSystemClass() to use a namespace OID
instead of Form_pg_class?Not sure. The way it's defined has the advantage of being more
independent of exactly what the implementation of the "is a toast table"
check is. Also, I looked around to see if any callers could really be
simplified if they only had to pass the table OID, and didn't find much;
almost all of them are looking at the pg_class tuple themselves, typically
to check the relkind too. So we'd not make any net savings in syscache
lookups by changing IsSystemClass's API. I'm kind of inclined to leave
it alone.
Hmm. Okay. It would have been nice to remove completely the
dependency to Form_pg_class from this set of APIs, but I can see your
point.
Yes, we still need to do your patch on top of this one (or really
either order would do). I think keeping them separate is good.
Okay, glad to hear. That's what I wanted to do.
While that matches the command syntax we're using, it's just horrid
English grammar. Better would beerrmsg("cannot reindex this type of relation concurrently")));
Can we change that while we're at it?
No problem to do that. I'll brush up all that once you commit the
first piece you have come up with, and reuse the new API of catalog.c
you are introducing based on the table OID.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
No problem to do that. I'll brush up all that once you commit the
first piece you have come up with, and reuse the new API of catalog.c
you are introducing based on the table OID.
Pushed my stuff, have at it.
regards, tom lane
On Wed, May 08, 2019 at 11:28:35PM -0400, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
No problem to do that. I'll brush up all that once you commit the
first piece you have come up with, and reuse the new API of catalog.c
you are introducing based on the table OID.Pushed my stuff, have at it.
Thanks. Attached is what I get to after scanning the error messages
in indexcmds.c and index.c. Perhaps you have more comments about it?
--
Michael
Attachments:
reindex-catalog-errors-v3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index dab06e20a8..7e7c03ef12 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2743,6 +2743,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
MemoryContextSwitchTo(oldcontext);
+ /* A system catalog cannot be reindexed concurrently */
+ if (IsCatalogRelationOid(relationOid))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex a system catalog concurrently")));
+
/* Open relation to get its indexes */
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
@@ -2756,13 +2762,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
if (!indexRelation->rd_index->indisvalid)
ereport(WARNING,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex concurrently invalid index \"%s.%s\", skipping",
+ errmsg("cannot reindex invalid index \"%s.%s\" concurrently, skipping",
get_namespace_name(get_rel_namespace(cellOid)),
get_rel_name(cellOid))));
else if (indexRelation->rd_index->indisexclusion)
ereport(WARNING,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex concurrently exclusion constraint index \"%s.%s\", skipping",
+ errmsg("cannot reindex exclusion constraint index \"%s.%s\" concurrently, skipping",
get_namespace_name(get_rel_namespace(cellOid)),
get_rel_name(cellOid))));
else
@@ -2802,7 +2808,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
if (!indexRelation->rd_index->indisvalid)
ereport(WARNING,
(errcode(ERRCODE_INDEX_CORRUPTED),
- errmsg("cannot reindex concurrently invalid index \"%s.%s\", skipping",
+ errmsg("cannot reindex invalid index \"%s.%s\" concurrently, skipping",
get_namespace_name(get_rel_namespace(cellOid)),
get_rel_name(cellOid))));
else
@@ -2831,17 +2837,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
{
Oid heapId = IndexGetRelation(relationOid, false);
- /* 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")));
-
/* A system catalog cannot be reindexed concurrently */
if (IsCatalogRelationOid(heapId))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("concurrent reindex is not supported for catalog relations")));
+ errmsg("cannot reindex a system catalog concurrently")));
/* Save the list of relation OIDs in private context */
oldcontext = MemoryContextSwitchTo(private_context);
@@ -2869,7 +2869,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* Return error if type of relation is not supported */
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot reindex concurrently this type of relation")));
+ errmsg("cannot reindex this type of relation concurrently")));
break;
}
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 326dc44177..c8bc6be061 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1967,7 +1967,7 @@ INSERT INTO concur_reindex_tab3 VALUES (3, '[1,2]');
REINDEX INDEX CONCURRENTLY concur_reindex_tab3_c2_excl; -- error
ERROR: concurrent index creation for exclusion constraints is not supported
REINDEX TABLE CONCURRENTLY concur_reindex_tab3; -- succeeds with warning
-WARNING: cannot reindex concurrently exclusion constraint index "public.concur_reindex_tab3_c2_excl", skipping
+WARNING: cannot reindex exclusion constraint index "public.concur_reindex_tab3_c2_excl" concurrently, skipping
INSERT INTO concur_reindex_tab3 VALUES (4, '[2,4]');
ERROR: conflicting key value violates exclusion constraint "concur_reindex_tab3_c2_excl"
DETAIL: Key (c2)=([2,5)) conflicts with existing key (c2)=([1,3)).
@@ -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 of pg_authid.
+REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
+ERROR: cannot reindex a system catalog concurrently
+REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog 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
@@ -2144,7 +2149,7 @@ DROP INDEX concur_reindex_ind5_ccnew;
DELETE FROM concur_reindex_tab4 WHERE c1 = 1;
-- The invalid index is not processed when running REINDEX TABLE.
REINDEX TABLE CONCURRENTLY concur_reindex_tab4;
-WARNING: cannot reindex concurrently invalid index "public.concur_reindex_ind5", skipping
+WARNING: cannot reindex invalid index "public.concur_reindex_ind5" concurrently, skipping
NOTICE: table "concur_reindex_tab4" has no indexes
\d concur_reindex_tab4
Table "public.concur_reindex_tab4"
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f29b8ca826..cd46f071bd 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 of pg_authid.
+REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
+REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
-- Warns about catalog relations
REINDEX SCHEMA CONCURRENTLY pg_catalog;
Michael Paquier <michael@paquier.xyz> writes:
On Wed, May 08, 2019 at 11:28:35PM -0400, Tom Lane wrote:
Pushed my stuff, have at it.
Thanks. Attached is what I get to after scanning the error messages
in indexcmds.c and index.c. Perhaps you have more comments about it?
LGTM, thanks.
regards, tom lane
On Thu, May 09, 2019 at 02:08:39PM -0400, Tom Lane wrote:
LGTM, thanks.
Thanks for double-checking, committed. I am closing the open item.
--
Michael
On 2019-May-09, Michael Paquier wrote:
On Wed, May 08, 2019 at 11:28:35PM -0400, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
No problem to do that. I'll brush up all that once you commit the
first piece you have come up with, and reuse the new API of catalog.c
you are introducing based on the table OID.Pushed my stuff, have at it.
Thanks. Attached is what I get to after scanning the error messages
in indexcmds.c and index.c. Perhaps you have more comments about it?
I do :-) There are a couple of "is not supported" messages that are
annoyingly similar but different:
git grep --show-function 'reindex.*supported' -- *.c
src/backend/commands/indexcmds.c=ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
src/backend/commands/indexcmds.c: errmsg("concurrent reindex of system catalogs is not supported")));
src/backend/commands/indexcmds.c: errmsg("concurrent reindex is not supported for catalog relations, skipping all")));
src/backend/commands/indexcmds.c=ReindexRelationConcurrently(Oid relationOid, int options)
src/backend/commands/indexcmds.c: errmsg("concurrent reindex is not supported for shared relations")));
src/backend/commands/indexcmds.c: errmsg("concurrent reindex is not supported for catalog relations")));
It seems strange to have some cases say "cannot do foo" and other cases
say "foo is not supported". However, I think having
ReindexMultipleTables say "cannot reindex a system catalog" would be
slightly wrong (since we're not reindexing one but many) -- so it would
have to be "cannot reindex system catalogs". And in order to avoid
having two messages that are essentially identical except in number, I
propose to change the others to use the plural too. So the one you just
committed
+ /* A system catalog cannot be reindexed concurrently */ + if (IsCatalogRelationOid(relationOid)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot reindex a system catalog concurrently")));
would become "cannot reindex system catalogs concurrently", identical to
the one in ReindexMultipleTables.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, May 14, 2019 at 11:32:52AM -0400, Alvaro Herrera wrote:
I do :-)
And actually I am happy to see somebody raising that point. The
current log messages are quite inconsistent for a couple of years now
but I did not bother changing anything other than the new strings per
the feedback I got until, well, yesterday.
It seems strange to have some cases say "cannot do foo" and other cases
say "foo is not supported". However, I think having
ReindexMultipleTables say "cannot reindex a system catalog" would be
slightly wrong (since we're not reindexing one but many) -- so it would
have to be "cannot reindex system catalogs". And in order to avoid
having two messages that are essentially identical except in number, I
propose to change the others to use the plural too. So the one you just
committedwould become "cannot reindex system catalogs concurrently", identical to
the one in ReindexMultipleTables.
There are also a couple of similar, much older, error messages in
index_create() for concurrent creation. Do you think that these
should be changed? I can see benefits for translators to unify things
a bit more, but these do not directly apply to REINDEX, and all
messages are a bit different depending on the context. One argument
to change them is that they don't comply with the project style as
they use full sentences.
Perhaps something like the attached for the REINDEX portion would make
the world a better place? What do you think?
--
Michael
Attachments:
reindex-error-strings.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 7e7c03ef12..8501b29b0a 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);
@@ -2862,7 +2862,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* see reindex_relation() */
ereport(WARNING,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
+ errmsg("cannot reindex partitioned tables, skipping \"%s\"",
get_rel_name(relationOid))));
return false;
default:
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index c8bc6be061..fda9878f25 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2045,7 +2045,7 @@ REINDEX TABLE concur_reindex_part_10;
WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
NOTICE: table "concur_reindex_part_10" has no indexes
REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
-WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
+WARNING: cannot reindex partitioned tables, skipping "concur_reindex_part_10"
NOTICE: table "concur_reindex_part_10" has no indexes
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
@@ -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"
On Wed, May 15, 2019 at 11:17:51AM +0900, Michael Paquier wrote:
Perhaps something like the attached for the REINDEX portion would make
the world a better place? What do you think?
Alvaro, do you have extra thoughts about this patch improving the
error message consistency for REINDEX CONCURRENTLY. I quite like the
suggestions you made and this makes the error strings more
project-like, so I would like to apply it.
--
Michael
On 2019-May-27, Michael Paquier wrote:
On Wed, May 15, 2019 at 11:17:51AM +0900, Michael Paquier wrote:
Perhaps something like the attached for the REINDEX portion would make
the world a better place? What do you think?Alvaro, do you have extra thoughts about this patch improving the
error message consistency for REINDEX CONCURRENTLY. I quite like the
suggestions you made and this makes the error strings more
project-like, so I would like to apply it.
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
/messages/by-id/CA+TgmoYS8jKhETyhGYTYMcbvGPwYY=qA6yYp9B47MX7MweE25w@mail.gmail.com
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.)
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. 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".
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
/messages/by-id/CA+TgmoYS8jKhETyhGYTYMcbvGPwYY=qA6yYp9B47MX7MweE25w@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
Attachments:
reindex-error-strings-v2.patchtext/x-diff; charset=us-asciiDownload
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"
On Mon, May 27, 2019 at 4:02 AM Michael Paquier <michael@paquier.xyz> wrote:
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
/messages/by-id/CA+TgmoYS8jKhETyhGYTYMcbvGPwYY=qA6yYp9B47MX7MweE25w@mail.gmail.comIncorrect URL?
That's one of my messages that never made it through to the list.
Try /messages/by-id/CA+TgmoZ0HZuLGVLkF_LRTNYDijic4nqd-EpCDf_NgtMksfNL1g@mail.gmail.com
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2019-May-27, Michael Paquier wrote:
On Mon, May 27, 2019 at 12:20:58AM -0400, Alvaro Herrera wrote:
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.
Sure.
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?
Looks good.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jun 19, 2019 at 11:29:37PM -0400, Alvaro Herrera wrote:
Looks good.
Thanks for the review, and reminding me about it :)
While on it, I have removed some comments around the error messages
because they actually don't bring more information.
--
Michael