On 2018-Dec-19, Tom Lane wrote:
> Alvaro Herrera <alvhe...@2ndquadrant.com> writes:
> > I support this idea. Here's a proof-of-concept patch that corresponds
> > to one of the cases that Ashutosh was on about (specifically, the one
> > that uses the RELKIND_CAN_HAVE_STORAGE macro I just added). If there
> > are no objections to this approach, I'm going to complete it along these
> > lines.
>
> +1
Here's a v2 -- completed patching the other places that needed it, fixed
test expected output, and made the other changes you suggested.
It's a bit scary that none of the backend message wording changes affect
any tests. Only contrib test files had to be patched. (I didn't run
sepgsql tests.)
One (very small IMO) problem here is that Ashutosh started from the
desire of being able to notice when a new relkind needs to be added to
checks spread in the code. This patch doesn't really do that; the new
"errdetail_unsuitable_relkind" can be grepped for but that doesn't
really solve it because some places don't throw errors, but just, say,
"continue". I don't see a solution for this.
But I do see one further problem, which is that each message being
patched was listing the relkinds that are allowed and now they ain't.
It seems user-friendly to preserve that list in some way. An example:
diff --git a/src/backend/commands/indexcmdsindex bd85099c28..7a255176df 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 437,444 **** DefineIndex(Oid relationId,
default:
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg("\"%s\" is not a table or
materialized view",
!
RelationGetRelationName(rel))));
break;
}
--- 437,447 ----
default:
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg("cannot create index on \"%s\"",
!
RelationGetRelationName(rel)),
!
errdetail_unsuitable_relkind(rel->rd_rel->relkind,
!
RelationGetRelationName(rel)),
! errhint("Indexes can only be created
on regular tables, materialized views and partitioned tables.")));
break;
}
This requires inventing a new message for at least some of the patched
sites. I'm not happy about using errhint() for that, since that's not a
hint, but I don't see anything else we could use.
(I'm not sure about using the term "regular table" for RELKIND_RELATION,
but I think using unadorned "table" doesn't cut it anymore -- we have
TOAST tables, foreign tables, and partitioned tables, neither of which
are tables for many of these checks. If you look at DefineIndex you'll
see that we even have a separate message for foreign tables there, which
kinda proves my point.)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 5cfb74bfa200fb34b1ca45546c4285d1886caab9 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 19 Dec 2018 14:33:07 -0300
Subject: [PATCH v2] unsuitable relkind
---
.../pg_visibility/expected/pg_visibility.out | 75 ++++++++++++-------
contrib/pg_visibility/pg_visibility.c | 7 +-
contrib/pgstattuple/expected/pgstattuple.out | 9 ++-
contrib/pgstattuple/pgstatindex.c | 13 ++--
src/backend/catalog/catalog.c | 45 +++++++++++
src/backend/catalog/toasting.c | 6 +-
src/backend/commands/comment.c | 7 +-
src/backend/commands/seclabel.c | 6 +-
src/include/catalog/catalog.h | 2 +
9 files changed, 127 insertions(+), 43 deletions(-)
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
index f0dcb897c4..d72ea2f63e 100644
--- a/contrib/pg_visibility/expected/pg_visibility.out
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -6,66 +6,91 @@ CREATE EXTENSION pg_visibility;
create table test_partitioned (a int) partition by list (a);
-- these should all fail
select pg_visibility('test_partitioned', 0);
-ERROR: "test_partitioned" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_partitioned"
+DETAIL: "test_partitioned" is a partitioned table.
select pg_visibility_map('test_partitioned');
-ERROR: "test_partitioned" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_partitioned"
+DETAIL: "test_partitioned" is a partitioned table.
select pg_visibility_map_summary('test_partitioned');
-ERROR: "test_partitioned" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_partitioned"
+DETAIL: "test_partitioned" is a partitioned table.
select pg_check_frozen('test_partitioned');
-ERROR: "test_partitioned" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_partitioned"
+DETAIL: "test_partitioned" is a partitioned table.
select pg_truncate_visibility_map('test_partitioned');
-ERROR: "test_partitioned" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_partitioned"
+DETAIL: "test_partitioned" is a partitioned table.
create table test_partition partition of test_partitioned for values in (1);
create index test_index on test_partition (a);
-- indexes do not, so these all fail
select pg_visibility('test_index', 0);
-ERROR: "test_index" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_index"
+DETAIL: "test_index" is an index.
select pg_visibility_map('test_index');
-ERROR: "test_index" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_index"
+DETAIL: "test_index" is an index.
select pg_visibility_map_summary('test_index');
-ERROR: "test_index" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_index"
+DETAIL: "test_index" is an index.
select pg_check_frozen('test_index');
-ERROR: "test_index" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_index"
+DETAIL: "test_index" is an index.
select pg_truncate_visibility_map('test_index');
-ERROR: "test_index" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_index"
+DETAIL: "test_index" is an index.
create view test_view as select 1;
-- views do not have VMs, so these all fail
select pg_visibility('test_view', 0);
-ERROR: "test_view" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_view"
+DETAIL: "test_view" is a view.
select pg_visibility_map('test_view');
-ERROR: "test_view" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_view"
+DETAIL: "test_view" is a view.
select pg_visibility_map_summary('test_view');
-ERROR: "test_view" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_view"
+DETAIL: "test_view" is a view.
select pg_check_frozen('test_view');
-ERROR: "test_view" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_view"
+DETAIL: "test_view" is a view.
select pg_truncate_visibility_map('test_view');
-ERROR: "test_view" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_view"
+DETAIL: "test_view" is a view.
create sequence test_sequence;
-- sequences do not have VMs, so these all fail
select pg_visibility('test_sequence', 0);
-ERROR: "test_sequence" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_sequence"
+DETAIL: "test_sequence" is a sequence.
select pg_visibility_map('test_sequence');
-ERROR: "test_sequence" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_sequence"
+DETAIL: "test_sequence" is a sequence.
select pg_visibility_map_summary('test_sequence');
-ERROR: "test_sequence" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_sequence"
+DETAIL: "test_sequence" is a sequence.
select pg_check_frozen('test_sequence');
-ERROR: "test_sequence" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_sequence"
+DETAIL: "test_sequence" is a sequence.
select pg_truncate_visibility_map('test_sequence');
-ERROR: "test_sequence" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_sequence"
+DETAIL: "test_sequence" is a sequence.
create foreign data wrapper dummy;
create server dummy_server foreign data wrapper dummy;
create foreign table test_foreign_table () server dummy_server;
-- foreign tables do not have VMs, so these all fail
select pg_visibility('test_foreign_table', 0);
-ERROR: "test_foreign_table" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_foreign_table"
+DETAIL: "test_foreign_table" is a foreign table.
select pg_visibility_map('test_foreign_table');
-ERROR: "test_foreign_table" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_foreign_table"
+DETAIL: "test_foreign_table" is a foreign table.
select pg_visibility_map_summary('test_foreign_table');
-ERROR: "test_foreign_table" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_foreign_table"
+DETAIL: "test_foreign_table" is a foreign table.
select pg_check_frozen('test_foreign_table');
-ERROR: "test_foreign_table" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_foreign_table"
+DETAIL: "test_foreign_table" is a foreign table.
select pg_truncate_visibility_map('test_foreign_table');
-ERROR: "test_foreign_table" is not a table, materialized view, or TOAST table
+ERROR: cannot examine visibility information for "test_foreign_table"
+DETAIL: "test_foreign_table" is a foreign table.
-- check some of the allowed relkinds
create table regular_table (a int);
insert into regular_table values (1), (2);
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 3282742b80..24f164f8e8 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -13,6 +13,7 @@
#include "access/heapam.h"
#include "access/htup_details.h"
#include "access/visibilitymap.h"
+#include "catalog/catalog.h"
#include "catalog/pg_type.h"
#include "catalog/storage_xlog.h"
#include "funcapi.h"
@@ -776,6 +777,8 @@ check_relation_relkind(Relation rel)
rel->rd_rel->relkind != RELKIND_TOASTVALUE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, materialized view, or TOAST table",
- RelationGetRelationName(rel))));
+ errmsg("cannot examine visibility information for \"%s\"",
+ RelationGetRelationName(rel)),
+ errdetail_unsuitable_relkind(rel->rd_rel->relkind,
+ RelationGetRelationName(rel))));
}
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 9858ea69d4..753087cdc8 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -161,7 +161,8 @@ ERROR: "test_partitioned_index" (partitioned index) is not supported
select pgstattuple_approx('test_partitioned');
ERROR: "test_partitioned" is not a table or materialized view
select pg_relpages('test_partitioned');
-ERROR: "test_partitioned" is not a table, index, materialized view, sequence, or TOAST table
+ERROR: cannot compute relpages of "test_partitioned"
+DETAIL: "test_partitioned" is a partitioned table.
select pgstatindex('test_partitioned');
ERROR: relation "test_partitioned" is not a btree index
select pgstatginindex('test_partitioned');
@@ -175,7 +176,8 @@ ERROR: "test_view" (view) is not supported
select pgstattuple_approx('test_view');
ERROR: "test_view" is not a table or materialized view
select pg_relpages('test_view');
-ERROR: "test_view" is not a table, index, materialized view, sequence, or TOAST table
+ERROR: cannot compute relpages of "test_view"
+DETAIL: "test_view" is a view.
select pgstatindex('test_view');
ERROR: relation "test_view" is not a btree index
select pgstatginindex('test_view');
@@ -191,7 +193,8 @@ ERROR: "test_foreign_table" (foreign table) is not supported
select pgstattuple_approx('test_foreign_table');
ERROR: "test_foreign_table" is not a table or materialized view
select pg_relpages('test_foreign_table');
-ERROR: "test_foreign_table" is not a table, index, materialized view, sequence, or TOAST table
+ERROR: cannot compute relpages of "test_foreign_table"
+DETAIL: "test_foreign_table" is a foreign table.
select pgstatindex('test_foreign_table');
ERROR: relation "test_foreign_table" is not a btree index
select pgstatginindex('test_foreign_table');
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 2c80753726..02dcf77287 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -33,6 +33,7 @@
#include "access/nbtree.h"
#include "access/relation.h"
#include "access/table.h"
+#include "catalog/catalog.h"
#include "catalog/namespace.h"
#include "catalog/pg_am.h"
#include "funcapi.h"
@@ -758,13 +759,11 @@ GetHashPageStats(Page page, HashIndexStat *stats)
static void
check_relation_relkind(Relation rel)
{
- if (rel->rd_rel->relkind != RELKIND_RELATION &&
- rel->rd_rel->relkind != RELKIND_INDEX &&
- rel->rd_rel->relkind != RELKIND_MATVIEW &&
- rel->rd_rel->relkind != RELKIND_SEQUENCE &&
- rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+ if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, index, materialized view, sequence, or TOAST table",
- RelationGetRelationName(rel))));
+ errmsg("cannot compute relpages of \"%s\"",
+ RelationGetRelationName(rel)),
+ errdetail_unsuitable_relkind(rel->rd_rel->relkind,
+ RelationGetRelationName(rel))));
}
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index c39da41d2e..cd096db0a0 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -528,3 +528,48 @@ pg_nextoid(PG_FUNCTION_ARGS)
return newoid;
}
+
+/*
+ * Add an errdetail to the current error message indicating what the relkind of
+ * the relation is. This is useful when an operation is being attempted on a
+ * relation that cannot accept it.
+ */
+int
+errdetail_unsuitable_relkind(char relkind, const char *relname)
+{
+ switch (relkind)
+ {
+ case RELKIND_RELATION:
+ errdetail("\"%s\" is a plain table.", relname);
+ break;
+ case RELKIND_INDEX:
+ errdetail("\"%s\" is an index.", relname);
+ break;
+ case RELKIND_SEQUENCE:
+ errdetail("\"%s\" is a sequence.", relname);
+ break;
+ case RELKIND_TOASTVALUE:
+ errdetail("\"%s\" is a TOAST table.", relname);
+ break;
+ case RELKIND_VIEW:
+ errdetail("\"%s\" is a view.", relname);
+ break;
+ case RELKIND_MATVIEW:
+ errdetail("\"%s\" is a materialized view.", relname);
+ break;
+ case RELKIND_COMPOSITE_TYPE:
+ errdetail("\"%s\" is a composite type.", relname);
+ break;
+ case RELKIND_FOREIGN_TABLE:
+ errdetail("\"%s\" is a foreign table.", relname);
+ break;
+ case RELKIND_PARTITIONED_TABLE:
+ errdetail("\"%s\" is a partitioned table.", relname);
+ break;
+ case RELKIND_PARTITIONED_INDEX:
+ errdetail("\"%s\" is a partitioned index.", relname);
+ break;
+ }
+
+ return 0;
+}
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 77be19175a..5ecb9db90e 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -18,6 +18,7 @@
#include "access/tuptoaster.h"
#include "access/xact.h"
#include "catalog/binary_upgrade.h"
+#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/heap.h"
#include "catalog/index.h"
@@ -103,8 +104,9 @@ BootstrapToastTable(char *relName, Oid toastOid, Oid toastIndexOid)
rel->rd_rel->relkind != RELKIND_MATVIEW)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or materialized view",
- relName)));
+ errmsg("cannot create toast table for \"%s\"", relName),
+ errdetail_unsuitable_relkind(rel->rd_rel->relkind,
+ RelationGetRelationName(rel))));
/* create_toast_table does all the work */
if (!create_toast_table(rel, toastOid, toastIndexOid, (Datum) 0,
diff --git a/src/backend/commands/comment.c b/src/backend/commands/comment.c
index 1859fb628f..f4b274e8d6 100644
--- a/src/backend/commands/comment.c
+++ b/src/backend/commands/comment.c
@@ -18,6 +18,7 @@
#include "access/htup_details.h"
#include "access/relation.h"
#include "access/table.h"
+#include "catalog/catalog.h"
#include "catalog/indexing.h"
#include "catalog/objectaddress.h"
#include "catalog/pg_description.h"
@@ -98,8 +99,10 @@ CommentObject(CommentStmt *stmt)
relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, materialized view, composite type, or foreign table",
- RelationGetRelationName(relation))));
+ errmsg("cannot add comment on \"%s\"",
+ RelationGetRelationName(relation)),
+ errdetail_unsuitable_relkind(relation->rd_rel->relkind,
+ RelationGetRelationName(relation))));
break;
default:
break;
diff --git a/src/backend/commands/seclabel.c b/src/backend/commands/seclabel.c
index 9db8228028..706b6d7610 100644
--- a/src/backend/commands/seclabel.c
+++ b/src/backend/commands/seclabel.c
@@ -114,8 +114,10 @@ ExecSecLabelStmt(SecLabelStmt *stmt)
relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, materialized view, composite type, or foreign table",
- RelationGetRelationName(relation))));
+ errmsg("cannot set security label on \"%s\"",
+ RelationGetRelationName(relation)),
+ errdetail_unsuitable_relkind(relation->rd_rel->relkind,
+ RelationGetRelationName(relation))));
break;
default:
break;
diff --git a/src/include/catalog/catalog.h b/src/include/catalog/catalog.h
index a58d09d6c7..65d8e2b3c7 100644
--- a/src/include/catalog/catalog.h
+++ b/src/include/catalog/catalog.h
@@ -38,4 +38,6 @@ extern Oid GetNewOidWithIndex(Relation relation, Oid indexId,
extern Oid GetNewRelFileNode(Oid reltablespace, Relation pg_class,
char relpersistence);
+extern int errdetail_unsuitable_relkind(char relkind, const char *relname);
+
#endif /* CATALOG_H */
--
2.17.1
