don't create storage when unnecessary

Started by Alvaro Herreraabout 7 years ago11 messages
#1Alvaro Herrera
alvherre@2ndquadrant.com
1 attachment(s)

Some time ago, after partitioned indexes had been pushed, I realized
that even though I didn't want them to have relfilenodes, they did. And
looking closer I noticed that *a lot* of relation kinds that didn't need
relfilenodes, had them anyway.

This patch fixes that; if no relfilenode is needed, it's not created.

I didn't verify pg_upgrade behavior across this commit. Maybe something
needs tweaking there.

PS: I think it'd be worth following up with this ...
/messages/by-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=AvCYh55Q@mail.gmail.com

--
�lvaro Herrera

Attachments:

0001-don-t-create-storage-when-not-necessary.patchtext/x-diff; charset=us-asciiDownload
From 25d80040ffe2fa4fa173b4db7a6e91461542fef3 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 9 Aug 2018 16:18:08 -0400
Subject: [PATCH] don't create storage when not necessary

---
 src/backend/catalog/heap.c         |  2 +-
 src/backend/utils/cache/relcache.c |  8 ++++++++
 src/include/utils/rel.h            | 10 ++++++----
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 11debaa780..6f73ff4fcf 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -372,7 +372,7 @@ heap_create(const char *relname,
 	 */
 	if (OidIsValid(relfilenode))
 		create_storage = false;
-	else
+	else if (create_storage)
 		relfilenode = relid;
 
 	/*
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index c3071db1cd..76a4cc65be 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1253,6 +1253,14 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 static void
 RelationInitPhysicalAddr(Relation relation)
 {
+	/* these relations kinds have no storage */
+	if (relation->rd_rel->relkind == RELKIND_VIEW ||
+		relation->rd_rel->relkind == RELKIND_COMPOSITE_TYPE ||
+		relation->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
+		relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
+		relation->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+		return;
+
 	if (relation->rd_rel->reltablespace)
 		relation->rd_node.spcNode = relation->rd_rel->reltablespace;
 	else
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 2217081dcc..6ac40fd84c 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -451,12 +451,14 @@ 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".
  */
 #define RelationIsMapped(relation) \
-	((relation)->rd_rel->relfilenode == InvalidOid)
+	(((relation)->rd_rel->relfilenode == InvalidOid) && \
+	 ((relation)->rd_rel->relkind == RELKIND_RELATION || \
+	  (relation)->rd_rel->relkind == RELKIND_INDEX ||  \
+	  (relation)->rd_rel->relkind == RELKIND_SEQUENCE || \
+	  (relation)->rd_rel->relkind == RELKIND_TOASTVALUE || \
+	  (relation)->rd_rel->relkind == RELKIND_MATVIEW))
 
 /*
  * RelationOpenSmgr
-- 
2.11.0

#2Andres Freund
andres@anarazel.de
In reply to: Alvaro Herrera (#1)
Re: don't create storage when unnecessary

Hi,

On 2018-12-06 18:55:52 -0300, Alvaro Herrera wrote:

Some time ago, after partitioned indexes had been pushed, I realized
that even though I didn't want them to have relfilenodes, they did. And
looking closer I noticed that *a lot* of relation kinds that didn't need
relfilenodes, had them anyway.

This patch fixes that; if no relfilenode is needed, it's not created.

I didn't verify pg_upgrade behavior across this commit. Maybe something
needs tweaking there.

Hm, that generally sounds like a good plan. Could we back this up with
tests in misc_sanity.sql or such?

- Andres

#3Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#1)
Re: don't create storage when unnecessary

On Thu, Dec 06, 2018 at 06:55:52PM -0300, Alvaro Herrera wrote:

Some time ago, after partitioned indexes had been pushed, I realized
that even though I didn't want them to have relfilenodes, they did. And
looking closer I noticed that *a lot* of relation kinds that didn't need
relfilenodes, had them anyway.

This patch fixes that; if no relfilenode is needed, it's not created.

I didn't verify pg_upgrade behavior across this commit. Maybe something
needs tweaking there.

PS: I think it'd be worth following up with this ...
/messages/by-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=AvCYh55Q@mail.gmail.com

A macro makes sense to control that. 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. Some tests
would also be welcome.
--
Michael

#4Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Michael Paquier (#3)
1 attachment(s)
Re: don't create storage when unnecessary

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

Attachments:

v2-0001-don-t-create-storage-when-not-necessary.patchtext/x-diff; charset=us-asciiDownload
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;
#5Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#4)
1 attachment(s)
Re: don't create storage when unnecessary

Rebased over today's conflicting commits.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

v3-0001-don-t-create-storage-when-not-necessary.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 4d5b82aaa9..b128959cb7 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -324,16 +324,12 @@ 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:
-			create_storage = false;
 
 			/*
 			 * Force reltablespace to zero if the relation has no physical
@@ -342,17 +338,7 @@ heap_create(const char *relname,
 			reltablespace = InvalidOid;
 			break;
 
-		case RELKIND_PARTITIONED_TABLE:
-		case RELKIND_PARTITIONED_INDEX:
-			/*
-			 * For partitioned tables and indexes, preserve tablespace so that
-			 * it's used as the tablespace for future partitions.
-			 */
-			create_storage = false;
-			break;
-
 		case RELKIND_SEQUENCE:
-			create_storage = true;
 
 			/*
 			 * Force reltablespace to zero for sequences, since we don't
@@ -361,19 +347,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..a4c8a9e385 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/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;
#6Kyotaro HORIGUCHI
horiguchi.kyotaro@lab.ntt.co.jp
In reply to: Alvaro Herrera (#4)
Re: don't create storage when unnecessary

Hello.

At Sun, 16 Dec 2018 17:47:16 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20181216204716.jddzijk3fb2dpdk4@alvherre.pgsql>

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.

FWIW.. I RELKIND_HAS_STORAGE looks better to me.

# Since it's a bit too late, I don't insist on that.

Mapped relations have storage, which is signalled by relfilenode
= 0 and the real file node is given by relation mapper. And it is
actually created at the boostrap time. In this sense,
(RELKIND_HAS_STORAGE && relfilenode == 0) in heap_craete makes
sense.

Assertion (..->relNode != InvalidOid) at the end of
RelationInitPhysicalAddr doesn't fail. I think RELKIND_HAS_STORGE
is preferable also in this sense.

ATExecSetTableSpaceNoStorage assumes that the relation is
actually having storage.

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.

Actual files were not created even in the past, create_heap has
been just refactored, which looks fine. pg_class and relcache
entries no longer have false relfilenode, which looks fine. The
test should check only relfilenode won't be given to such
relation, which were given in the past, which looks fine.

The patch applies cleanly and passed the regression test.

regares.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#7Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#4)
Re: don't create storage when unnecessary

On Sun, Dec 16, 2018 at 05:47:16PM -0300, Alvaro Herrera wrote:

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.

I have been double-checking my thoughts on the matter, and one take is
to allow the reuse of relfilenodes for indexes like in TryReuseIndex().
I did not recall that case. Sorry for the noise.

Another possible improvement is to remove the create_storage boolean

Yes, this could go away as this is linked with relfilenode. I let it up
to you if you want to remove it or keep it. I think that I would just
remove it.

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.

+-- check that relations without storage don't have relfilenode
It could be an idea to add a comment mentioning that the set of relkinds
in RELKIND_CAN_HAVE_STORAGE are linked with the relkinds of this query.
--
Michael
#8Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Kyotaro HORIGUCHI (#6)
Re: don't create storage when unnecessary

Hi,

On 2018/12/18 14:56, Kyotaro HORIGUCHI wrote:

Hello.

At Sun, 16 Dec 2018 17:47:16 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20181216204716.jddzijk3fb2dpdk4@alvherre.pgsql>

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.

FWIW.. I RELKIND_HAS_STORAGE looks better to me.

# Since it's a bit too late, I don't insist on that.

Mapped relations have storage, which is signalled by relfilenode
= 0 and the real file node is given by relation mapper. And it is
actually created at the boostrap time. In this sense,
(RELKIND_HAS_STORAGE && relfilenode == 0) in heap_craete makes
sense.

Assertion (..->relNode != InvalidOid) at the end of
RelationInitPhysicalAddr doesn't fail. I think RELKIND_HAS_STORGE
is preferable also in this sense.

Sorry to be saying it late, but I have to agree with Horiguchi-san here
that RELKIND_HAS_STORAGE sounds better and is clear. Looking at what's
committed:

/*
* 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)

So, they all *do have* storage, as the comment's first sentence also says.
Mixing the bit about mapped relations in the naming of this macro will
make it hard to reason about using it in other parts of the backend code
(although, in admittedly not too far off places), where it shouldn't
matter if the relation's file is determined using relfilenode or via
relation to file mapper.

ATExecSetTableSpaceNoStorage assumes that the relation is
actually having storage.

Hmm, it has the following Assert:

/*
* Shouldn't be called on relations having storage; these are processed
* in phase 3.
*/
Assert(!RELKIND_CAN_HAVE_STORAGE(rel->rd_rel->relkind));

So, it actually assumes that it's called for relations that don't have
storage (...but do have a tablespace property that applies to its children.)

Thanks,
Amit

#9Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Amit Langote (#8)
Re: don't create storage when unnecessary

On 18/12/2018 08:18, Amit Langote wrote:

Sorry to be saying it late, but I have to agree with Horiguchi-san here
that RELKIND_HAS_STORAGE sounds better and is clear.

I think I agree. Does someone want to send a patch?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#10Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Peter Eisentraut (#9)
Re: don't create storage when unnecessary

On 2019-Jan-04, Peter Eisentraut wrote:

On 18/12/2018 08:18, Amit Langote wrote:

Sorry to be saying it late, but I have to agree with Horiguchi-san here
that RELKIND_HAS_STORAGE sounds better and is clear.

I think I agree. Does someone want to send a patch?

I'll do it.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#11Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#10)
Re: don't create storage when unnecessary

On Fri, Jan 04, 2019 at 09:19:25AM -0300, Alvaro Herrera wrote:

I'll do it.

Thanks for taking care of it. Please note that commands/copy.c also
makes use of RELKIND_CAN_HAVE_STORAGE() as of bf491a9. Could you
split the renaming with a commit independent on what is being
discussed on this thread? These are separate issues in my opinion.
--
Michael