On 2018-Dec-07, Michael Paquier wrote:
> A macro makes sense to control that.
I added Ashutosh's RELKIND_HAS_STORAGE, but renamed it to
RELKIND_CAN_HAVE_STORAGE, because some of the relkinds can be mapped and
thus would have relfilenode set to 0. I think this is a bit misleading
either way.
> Now I have to admit that I don't
> like your solution. Wouldn't it be cleaner to assign InvalidOid to
> relfilenode in such cases? The callers of heap_create would need to be
> made smarter when they now pass down a relfilenode (looking at you,
> DefineIndex!), but that seems way more consistent to me.
I don't follow. When a relfilenode is passed by callers, they indicate
that the storage has already been created. Contrariwise, when a
relation kind that *does* have storage but is not yet created, they
pass InvalidOid as relfilenode, and heap_create is in charge of creating
storage. Maybe I'm not quite seeing what problem you mean. Or I could
add a separate boolean, but that seems pointless.
Another possible improvement is to remove the create_storage boolean
> Some tests would also be welcome.
Added a test in sanity_check.sql that there's no relation with the
relkinds that aren't supposed to have storage. Without the code fix it
fails in current regression database, but in the failure result set
there isn't any relation of kinds 'p' or 'I', so this isn't a terribly
comprehensive test -- the query runs too early in the regression
sequence. I'm not sure it's worth bothering further.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 11debaa780..8aeddfa3d3 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -324,17 +324,13 @@ heap_create(const char *relname,
get_namespace_name(relnamespace), relname),
errdetail("System catalog modifications are currently disallowed.")));
- /*
- * Decide if we need storage or not, and handle a couple other special
- * cases for particular relkinds.
- */
+ /* Handle reltablespace for specific relkinds. */
switch (relkind)
{
case RELKIND_VIEW:
case RELKIND_COMPOSITE_TYPE:
case RELKIND_FOREIGN_TABLE:
case RELKIND_PARTITIONED_TABLE:
- create_storage = false;
/*
* Force reltablespace to zero if the relation has no physical
@@ -343,16 +339,7 @@ heap_create(const char *relname,
reltablespace = InvalidOid;
break;
- case RELKIND_PARTITIONED_INDEX:
- /*
- * Preserve tablespace so that it's used as tablespace for indexes
- * on future partitions.
- */
- create_storage = false;
- break;
-
case RELKIND_SEQUENCE:
- create_storage = true;
/*
* Force reltablespace to zero for sequences, since we don't
@@ -361,19 +348,21 @@ heap_create(const char *relname,
reltablespace = InvalidOid;
break;
default:
- create_storage = true;
break;
}
/*
- * Unless otherwise requested, the physical ID (relfilenode) is initially
- * the same as the logical ID (OID). When the caller did specify a
- * relfilenode, it already exists; do not attempt to create it.
+ * Decide whether to create storage. If caller passed a valid relfilenode,
+ * storage is already created, so don't do it here. Also don't create it
+ * for relkinds without physical storage.
*/
- if (OidIsValid(relfilenode))
+ if (!RELKIND_CAN_HAVE_STORAGE(relkind) || OidIsValid(relfilenode))
create_storage = false;
else
+ {
+ create_storage = true;
relfilenode = relid;
+ }
/*
* Never allow a pg_class entry to explicitly specify the database's
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index c3071db1cd..911686e6bb 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1253,6 +1253,10 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
static void
RelationInitPhysicalAddr(Relation relation)
{
+ /* these relations kinds never have storage */
+ if (!RELKIND_CAN_HAVE_STORAGE(relation->rd_rel->relkind))
+ return;
+
if (relation->rd_rel->reltablespace)
relation->rd_node.spcNode = relation->rd_rel->reltablespace;
else
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 84e63c6d06..321c9a1973 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -122,6 +122,19 @@ typedef FormData_pg_class *Form_pg_class;
*/
#define REPLICA_IDENTITY_INDEX 'i'
+/*
+ * Relation kinds that have physical storage. These relations normally have
+ * relfilenode set to non-zero, but it can also be zero if the relation is
+ * mapped.
+ */
+#define RELKIND_CAN_HAVE_STORAGE(relkind) \
+ ((relkind) == RELKIND_RELATION || \
+ (relkind) == RELKIND_INDEX || \
+ (relkind) == RELKIND_SEQUENCE || \
+ (relkind) == RELKIND_TOASTVALUE || \
+ (relkind) == RELKIND_MATVIEW)
+
+
#endif /* EXPOSE_TO_CLIENT_CODE */
#endif /* PG_CLASS_H */
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 2217081dcc..393edd33d2 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -450,13 +450,12 @@ typedef struct ViewOptions
/*
* RelationIsMapped
- * True if the relation uses the relfilenode map.
- *
- * NB: this is only meaningful for relkinds that have storage, else it
- * will misleadingly say "true".
+ * True if the relation uses the relfilenode map. Note multiple eval
+ * of argument!
*/
#define RelationIsMapped(relation) \
- ((relation)->rd_rel->relfilenode == InvalidOid)
+ (RELKIND_CAN_HAVE_STORAGE((relation)->rd_rel->relkind) && \
+ ((relation)->rd_rel->relfilenode == InvalidOid))
/*
* RelationOpenSmgr
diff --git a/src/test/regress/expected/sanity_check.out b/src/test/regress/expected/sanity_check.out
index 009a89fc1a..89537bcfcf 100644
--- a/src/test/regress/expected/sanity_check.out
+++ b/src/test/regress/expected/sanity_check.out
@@ -224,3 +224,12 @@ SELECT relname, nspname
---------+---------
(0 rows)
+-- check that relations without storage don't have relfilenode
+SELECT relname, relkind
+ FROM pg_class
+ WHERE relkind IN ('v', 'c', 'f', 'p', 'I')
+ AND relfilenode <> 0;
+ relname | relkind
+---------+---------
+(0 rows)
+
diff --git a/src/test/regress/sql/sanity_check.sql b/src/test/regress/sql/sanity_check.sql
index a2feebc91b..a4ec00309f 100644
--- a/src/test/regress/sql/sanity_check.sql
+++ b/src/test/regress/sql/sanity_check.sql
@@ -31,3 +31,9 @@ SELECT relname, nspname
AND NOT EXISTS (SELECT 1 FROM pg_index i WHERE indrelid = c.oid
AND indkey[0] = a.attnum AND indnatts = 1
AND indisunique AND indimmediate);
+
+-- check that relations without storage don't have relfilenode
+SELECT relname, relkind
+ FROM pg_class
+ WHERE relkind IN ('v', 'c', 'f', 'p', 'I')
+ AND relfilenode <> 0;
