Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options

[Michael Paquier]

On Tue, Nov 12, 2019 at 01:50:03PM +0900, Michael Paquier wrote:
> We have been through great length to have build_reloptions, so
> wouldn't it be better to also have this code path do so?  Sure you
> need to pass NULL for the parsing table..  But there is a point to
> reduce the code paths using directly parseRelOptions() and the
> follow-up, expected calls to allocate and to fill in the set of
> reloptions.

So I have been looking at this one, and finished with the attached.
It looks much better to use build_reloptions() IMO, taking advantage
of the same sanity checks present for the other relkinds.
--
Michael

diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index db42aa35e0..ee7248ad58 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -306,6 +306,7 @@ extern bytea *default_reloptions(Datum reloptions, bool validate,
 								 relopt_kind kind);
 extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate);
 extern bytea *view_reloptions(Datum reloptions, bool validate);
+extern bytea *partitioned_table_reloptions(Datum reloptions, bool validate);
 extern bytea *index_reloptions(amoptions_function amoptions, Datum reloptions,
 							   bool validate);
 extern bytea *attribute_reloptions(Datum reloptions, bool validate);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 8b8b237f0d..1eb323a5d4 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -277,6 +277,16 @@ typedef struct StdRdOptions
 #define HEAP_MIN_FILLFACTOR			10
 #define HEAP_DEFAULT_FILLFACTOR		100
 
+/*
+ * PartitionedRelOptions
+ *     Binary representation of relation options for partitioned tables.
+ */
+typedef struct PartitionedRelOptions
+{
+   int32       vl_len_;        /* varlena header (do not touch directly!) */
+   /* No options defined yet */
+} PartitionedRelOptions;
+
 /*
  * RelationGetToastTupleTarget
  *		Returns the relation's toast_tuple_target.  Note multiple eval of argument!
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d8790ad7a3..6600a154ab 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1099,9 +1099,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
 		case RELKIND_MATVIEW:
-		case RELKIND_PARTITIONED_TABLE:
 			options = heap_reloptions(classForm->relkind, datum, false);
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			options = partitioned_table_reloptions(datum, false);
+			break;
 		case RELKIND_VIEW:
 			options = view_reloptions(datum, false);
 			break;
@@ -1571,6 +1573,21 @@ build_reloptions(Datum reloptions, bool validate,
 	return rdopts;
 }
 
+/*
+ * Option parser for partitioned tables
+ */
+bytea *
+partitioned_table_reloptions(Datum reloptions, bool validate)
+{
+	/*
+	 * There are no options for partitioned tables yet, but this is
+	 * able to do some validation.
+	 */
+	return (bytea *) build_reloptions(reloptions, validate,
+									  RELOPT_KIND_PARTITIONED,
+									  0, NULL, 0);
+}
+
 /*
  * Option parser for views
  */
@@ -1614,9 +1631,6 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
 		case RELKIND_RELATION:
 		case RELKIND_MATVIEW:
 			return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
-		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/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 496c206332..45aae5955d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -719,10 +719,17 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
 									 true, false);
 
-	if (relkind == RELKIND_VIEW)
-		(void) view_reloptions(reloptions, true);
-	else
-		(void) heap_reloptions(relkind, reloptions, true);
+	switch (relkind)
+	{
+		case RELKIND_VIEW:
+			(void) view_reloptions(reloptions, true);
+			break;
+		case RELKIND_PARTITIONED_TABLE:
+			(void) partitioned_table_reloptions(reloptions, true);
+			break;
+		default:
+			(void) heap_reloptions(relkind, reloptions, true);
+	}
 
 	if (stmt->ofTypename)
 	{
@@ -12187,9 +12194,11 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
 		case RELKIND_MATVIEW:
-		case RELKIND_PARTITIONED_TABLE:
 			(void) heap_reloptions(rel->rd_rel->relkind, newOptions, true);
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			(void) partitioned_table_reloptions(newOptions, true);
+			break;
 		case RELKIND_VIEW:
 			(void) view_reloptions(newOptions, true);
 			break;

signature.asc
Description: PGP signature