Horiguchi-san,

On 2017/03/31 15:50, Kyotaro HORIGUCHI wrote:
> At Thu, 30 Mar 2017 20:58:35 +0900, Amit Langote wrote:
>> Updated patch attached.
> 
> Thank you.
> 
> - Applies cleanly on master (f90d23d)
> - Compiled without error
> - Code seems fine.
> - Documentaion seems fine.. for me.
> - Passes regression test.
> - Leaving the ALTER-on-toast.* problem is fine for me.

Thanks for the review.

> The regression contains the tests to fail with several reloptions
> only for partitioned tables. Are they still required even though
> it is now in the same framework with other kind of reloptions?

Hmm, maybe we don't really need those tests, so removed.

Thanks,
Amit
>From bc80db31f218d299ad4ccfa5140088382ac947db Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 24 Jan 2017 14:22:34 +0900
Subject: [PATCH] Do not allocate storage for partitioned tables.

Currently, it is not possible to insert any data into a partitioned
table.  So they're empty at all times, which means it is wasteful to
allocate relfilenode and related storage objects.

Also, because there is no storage, it doesn't make sense to allow
specifying storage parameters such as fillfactor, etc.

Patch by: Amit Langote & Maksim Milyutin
Reviewed by: Kyotaro Horiguchi
---
 doc/src/sgml/ref/create_table.sgml     |  6 ++++++
 src/backend/access/common/reloptions.c | 33 ++++++++++++++++++++++++---------
 src/backend/catalog/heap.c             | 20 ++++++++++++--------
 src/include/access/reloptions.h        |  3 ++-
 4 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 283d53e203..5f13f240f5 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1040,6 +1040,12 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
     will use the table's parameter value.
    </para>
 
+   <para>
+    Note that specifying the following parameters for partitioned tables is
+    not supported.  You must specify them for individual leaf partitions if
+    necessary.
+   </para>
+
    <variablelist>
 
    <varlistentry>
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 72e12532ab..f3f5ade38f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1024,21 +1024,28 @@ parseRelOptions(Datum options, bool validate, relopt_kind kind,
 		if (relOpts[i]->kinds & kind)
 			numoptions++;
 
-	if (numoptions == 0)
+	/*
+	 * Even if we found no options of kind, it's better to raise the
+	 * "unrecognized parameter %s" error here, instead of at the caller.
+	 */
+	if (numoptions == 0 && !validate)
 	{
 		*numrelopts = 0;
 		return NULL;
 	}
 
-	reloptions = palloc(numoptions * sizeof(relopt_value));
-
-	for (i = 0, j = 0; relOpts[i]; i++)
+	if (numoptions > 0)
 	{
-		if (relOpts[i]->kinds & kind)
+		reloptions = palloc(numoptions * sizeof(relopt_value));
+
+		for (i = 0, j = 0; relOpts[i]; i++)
 		{
-			reloptions[j].gen = relOpts[i];
-			reloptions[j].isset = false;
-			j++;
+			if (relOpts[i]->kinds & kind)
+			{
+				reloptions[j].gen = relOpts[i];
+				reloptions[j].isset = false;
+				j++;
+			}
 		}
 	}
 
@@ -1418,8 +1425,16 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
 			return (bytea *) rdopts;
 		case RELKIND_RELATION:
 		case RELKIND_MATVIEW:
-		case RELKIND_PARTITIONED_TABLE:
 			return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
+
+		/*
+		 * There aren't any partitioned table options yet really, so the
+		 * following simply serves to reject whatever's specified as being
+		 * "unrecognized storage parameter" if validate is true.
+		 */
+		case RELKIND_PARTITIONED_TABLE:
+			return default_reloptions(reloptions, validate,
+									  RELOPT_KIND_PARTITIONED);
 		default:
 			/* other relkinds are not supported */
 			return NULL;
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index eee5e2f6ca..ff2f2e8efb 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -293,6 +293,7 @@ heap_create(const char *relname,
 		case RELKIND_VIEW:
 		case RELKIND_COMPOSITE_TYPE:
 		case RELKIND_FOREIGN_TABLE:
+		case RELKIND_PARTITIONED_TABLE:
 			create_storage = false;
 
 			/*
@@ -1347,14 +1348,13 @@ heap_create_with_catalog(const char *relname,
 	if (oncommit != ONCOMMIT_NOOP)
 		register_on_commit_action(relid, oncommit);
 
-	if (relpersistence == RELPERSISTENCE_UNLOGGED)
-	{
-		Assert(relkind == RELKIND_RELATION || relkind == RELKIND_MATVIEW ||
-			   relkind == RELKIND_TOASTVALUE ||
-			   relkind == RELKIND_PARTITIONED_TABLE);
-
+	/*
+	 * We do not want to create any storage objects for a partitioned
+	 * table, including the init fork.
+	 */
+	if (relpersistence == RELPERSISTENCE_UNLOGGED &&
+		relkind != RELKIND_PARTITIONED_TABLE)
 		heap_create_init_fork(new_rel_desc);
-	}
 
 	/*
 	 * ok, the relation has been cataloged, so close our relations and return
@@ -1378,6 +1378,9 @@ heap_create_with_catalog(const char *relname,
 void
 heap_create_init_fork(Relation rel)
 {
+	Assert(rel->rd_rel->relkind == RELKIND_RELATION ||
+		   rel->rd_rel->relkind == RELKIND_MATVIEW ||
+		   rel->rd_rel->relkind == RELKIND_TOASTVALUE);
 	RelationOpenSmgr(rel);
 	smgrcreate(rel->rd_smgr, INIT_FORKNUM, false);
 	log_smgrcreate(&rel->rd_smgr->smgr_rnode.node, INIT_FORKNUM);
@@ -1824,7 +1827,8 @@ heap_drop_with_catalog(Oid relid)
 	 */
 	if (rel->rd_rel->relkind != RELKIND_VIEW &&
 		rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE &&
-		rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
+		rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
+		rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
 	{
 		RelationDropStorage(rel);
 	}
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 861977a608..91b2cd7bb2 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -48,8 +48,9 @@ typedef enum relopt_kind
 	RELOPT_KIND_SPGIST = (1 << 8),
 	RELOPT_KIND_VIEW = (1 << 9),
 	RELOPT_KIND_BRIN = (1 << 10),
+	RELOPT_KIND_PARTITIONED = (1 << 11),
 	/* if you add a new kind, make sure you update "last_default" too */
-	RELOPT_KIND_LAST_DEFAULT = RELOPT_KIND_BRIN,
+	RELOPT_KIND_LAST_DEFAULT = RELOPT_KIND_PARTITIONED,
 	/* some compilers treat enums as signed ints, so we can't use 1 << 31 */
 	RELOPT_KIND_MAX = (1 << 30)
 } relopt_kind;
-- 
2.11.0

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to