Feedback-ID: ia2694551:Fastmail
Date: Fri, 21 Oct 2022 11:32:16 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: parse partition strategy string in gram.y
Message-ID: <20221021093216.ffupd7epy2mytkux@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="xrws3pp5u7pwsoei"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/20221021093216.ffupd7epy2mytkux%40alvherre.pgsql>
Precedence: bulk


--xrws3pp5u7pwsoei
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hello

I've had this patch sitting in a local branch for way too long.  It's a
trivial thing but for some reason it bothered me: we let the partition 
strategy flow into the backend as a string and only parse it into the
catalog 1-char version quite late.

This patch makes gram.y responsible for parsing it and passing it down
as a value from a new enum, which looks more neat.  Because it's an
enum, some "default:" cases can be removed in a couple of places.  I
also added a new elog() in case the catalog contents becomes broken.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Estoy de acuerdo contigo en que la verdad absoluta no existe...
El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)

--xrws3pp5u7pwsoei
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-have-gram.y-resolve-partition-strategy-names.patch"

From e6067eee889a6636eb013f2f8d85efaf2112232b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 20 Oct 2022 12:29:18 +0200
Subject: [PATCH] have gram.y resolve partition strategy names

---
 src/backend/commands/tablecmds.c      | 35 +++++++++------------------
 src/backend/parser/gram.y             | 22 ++++++++++++++++-
 src/backend/partitioning/partbounds.c | 26 ++++----------------
 src/backend/utils/cache/partcache.c   |  6 +++++
 src/include/nodes/parsenodes.h        | 15 ++++++------
 src/include/partitioning/partbounds.h |  2 +-
 src/include/utils/partcache.h         |  2 +-
 7 files changed, 53 insertions(+), 55 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 20135ef1b0..e07fd747f7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -605,9 +605,10 @@ static void RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid,
 											Oid oldRelOid, void *arg);
 static void RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid,
 											 Oid oldrelid, void *arg);
-static PartitionSpec *transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy);
+static PartitionSpec *transformPartitionSpec(Relation rel, PartitionSpec *partspec);
 static void ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNumber *partattrs,
-								  List **partexprs, Oid *partopclass, Oid *partcollation, char strategy);
+								  List **partexprs, Oid *partopclass, Oid *partcollation,
+								  PartitionStrategy strategy);
 static void CreateInheritance(Relation child_rel, Relation parent_rel);
 static void RemoveInheritance(Relation child_rel, Relation parent_rel,
 							  bool expect_detached);
@@ -1122,7 +1123,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	if (partitioned)
 	{
 		ParseState *pstate;
-		char		strategy;
 		int			partnatts;
 		AttrNumber	partattrs[PARTITION_MAX_KEYS];
 		Oid			partopclass[PARTITION_MAX_KEYS];
@@ -1147,14 +1147,14 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		 * and CHECK constraints, we could not have done the transformation
 		 * earlier.
 		 */
-		stmt->partspec = transformPartitionSpec(rel, stmt->partspec,
-												&strategy);
+		stmt->partspec = transformPartitionSpec(rel, stmt->partspec);
 
 		ComputePartitionAttrs(pstate, rel, stmt->partspec->partParams,
 							  partattrs, &partexprs, partopclass,
-							  partcollation, strategy);
+							  partcollation, stmt->partspec->strategy);
 
-		StorePartitionKey(rel, strategy, partnatts, partattrs, partexprs,
+		StorePartitionKey(rel, stmt->partspec->strategy, partnatts, partattrs,
+						  partexprs,
 						  partopclass, partcollation);
 
 		/* make it all visible */
@@ -17132,10 +17132,10 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid,
 /*
  * Transform any expressions present in the partition key
  *
- * Returns a transformed PartitionSpec, as well as the strategy code
+ * Returns a transformed PartitionSpec.
  */
 static PartitionSpec *
-transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
+transformPartitionSpec(Relation rel, PartitionSpec *partspec)
 {
 	PartitionSpec *newspec;
 	ParseState *pstate;
@@ -17148,21 +17148,8 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
 	newspec->partParams = NIL;
 	newspec->location = partspec->location;
 
-	/* Parse partitioning strategy name */
-	if (pg_strcasecmp(partspec->strategy, "hash") == 0)
-		*strategy = PARTITION_STRATEGY_HASH;
-	else if (pg_strcasecmp(partspec->strategy, "list") == 0)
-		*strategy = PARTITION_STRATEGY_LIST;
-	else if (pg_strcasecmp(partspec->strategy, "range") == 0)
-		*strategy = PARTITION_STRATEGY_RANGE;
-	else
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("unrecognized partitioning strategy \"%s\"",
-						partspec->strategy)));
-
 	/* Check valid number of columns for strategy */
-	if (*strategy == PARTITION_STRATEGY_LIST &&
+	if (partspec->strategy == PARTITION_STRATEGY_LIST &&
 		list_length(partspec->partParams) != 1)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -17208,7 +17195,7 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
 static void
 ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNumber *partattrs,
 					  List **partexprs, Oid *partopclass, Oid *partcollation,
-					  char strategy)
+					  PartitionStrategy strategy)
 {
 	int			attn;
 	ListCell   *lc;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 737bd2d06d..51cff3cd8a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -213,6 +213,7 @@ static void SplitColQualList(List *qualList,
 static void processCASbits(int cas_bits, int location, const char *constrType,
 			   bool *deferrable, bool *initdeferred, bool *not_valid,
 			   bool *no_inherit, core_yyscan_t yyscanner);
+static PartitionStrategy parsePartitionStrategy(char *strategy);
 static void preprocess_pubobj_list(List *pubobjspec_list,
 								   core_yyscan_t yyscanner);
 static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
@@ -4357,7 +4358,7 @@ PartitionSpec: PARTITION BY ColId '(' part_params ')'
 				{
 					PartitionSpec *n = makeNode(PartitionSpec);
 
-					n->strategy = $3;
+					n->strategy = parsePartitionStrategy($3);
 					n->partParams = $5;
 					n->location = @1;
 
@@ -18414,6 +18415,25 @@ processCASbits(int cas_bits, int location, const char *constrType,
 	}
 }
 
+/*
+ * Parse a user-supplied partition strategy string into parse node
+ * PartitionStrategy representation, or die trying.
+ */
+static PartitionStrategy
+parsePartitionStrategy(char *strategy)
+{
+	if (pg_strcasecmp(strategy, "range") == 0)
+		return PARTITION_STRATEGY_RANGE;
+	else if (pg_strcasecmp(strategy, "hash") == 0)
+		return PARTITION_STRATEGY_HASH;
+	else if (pg_strcasecmp(strategy, "range") == 0)
+		return PARTITION_STRATEGY_RANGE;
+	ereport(ERROR,
+			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+			 errmsg("unrecognized partitioning strategy \"%s\"",
+					strategy)));
+}
+
 /*
  * Process pubobjspec_list to check for errors in any of the objects and
  * convert PUBLICATIONOBJ_CONTINUATION into appropriate PublicationObjSpecType.
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 0823fa7b1d..7b5cd55e80 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -270,10 +270,6 @@ get_qual_from_partbound(Relation parent, PartitionBoundSpec *spec)
 			Assert(spec->strategy == PARTITION_STRATEGY_RANGE);
 			my_qual = get_qual_for_range(parent, spec, false);
 			break;
-
-		default:
-			elog(ERROR, "unexpected partition strategy: %d",
-				 (int) key->strategy);
 	}
 
 	return my_qual;
@@ -338,11 +334,6 @@ partition_bounds_create(PartitionBoundSpec **boundspecs, int nparts,
 
 		case PARTITION_STRATEGY_RANGE:
 			return create_range_bounds(boundspecs, nparts, key, mapping);
-
-		default:
-			elog(ERROR, "unexpected partition strategy: %d",
-				 (int) key->strategy);
-			break;
 	}
 
 	Assert(false);
@@ -1181,11 +1172,9 @@ partition_bounds_merge(int partnatts,
 									  jointype,
 									  outer_parts,
 									  inner_parts);
-
 		default:
-			elog(ERROR, "unexpected partition strategy: %d",
-				 (int) outer_rel->boundinfo->strategy);
-			return NULL;		/* keep compiler quiet */
+			Assert(false);
+			return NULL;
 	}
 }
 
@@ -2892,8 +2881,7 @@ partitions_are_ordered(PartitionBoundInfo boundinfo, Bitmapset *live_parts)
 				return true;
 
 			break;
-		default:
-			/* HASH, or some other strategy */
+		case PARTITION_STRATEGY_HASH:
 			break;
 	}
 
@@ -3241,10 +3229,6 @@ check_new_partition_bound(char *relname, Relation parent,
 
 				break;
 			}
-
-		default:
-			elog(ERROR, "unexpected partition strategy: %d",
-				 (int) key->strategy);
 	}
 
 	if (overlap)
@@ -3980,8 +3964,8 @@ make_partition_op_expr(PartitionKey key, int keynum,
 								   key->partcollation[keynum]);
 			break;
 
-		default:
-			elog(ERROR, "invalid partitioning strategy");
+		case PARTITION_STRATEGY_HASH:
+			Assert(false);
 			break;
 	}
 
diff --git a/src/backend/utils/cache/partcache.c b/src/backend/utils/cache/partcache.c
index afa99c5d03..8f3e411476 100644
--- a/src/backend/utils/cache/partcache.c
+++ b/src/backend/utils/cache/partcache.c
@@ -115,6 +115,12 @@ RelationBuildPartitionKey(Relation relation)
 	key->strategy = form->partstrat;
 	key->partnatts = form->partnatts;
 
+	/* Validate partition strategy code */
+	if (key->strategy != PARTITION_STRATEGY_HASH &&
+		key->strategy != PARTITION_STRATEGY_LIST &&
+		key->strategy != PARTITION_STRATEGY_RANGE)
+		elog(ERROR, "invalid partition strategy \"%c\"", key->strategy);
+
 	/*
 	 * We can rely on the first variable-length attribute being mapped to the
 	 * relevant field of the catalog's C struct, because all previous
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 633e7671b3..99cba300f3 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -827,6 +827,13 @@ typedef struct PartitionElem
 	int			location;		/* token location, or -1 if unknown */
 } PartitionElem;
 
+typedef enum PartitionStrategy
+{
+   PARTITION_STRATEGY_LIST = 'l',
+   PARTITION_STRATEGY_RANGE = 'r',
+   PARTITION_STRATEGY_HASH = 'h'
+} PartitionStrategy;
+
 /*
  * PartitionSpec - parse-time representation of a partition key specification
  *
@@ -835,17 +842,11 @@ typedef struct PartitionElem
 typedef struct PartitionSpec
 {
 	NodeTag		type;
-	char	   *strategy;		/* partitioning strategy ('hash', 'list' or
-								 * 'range') */
+	PartitionStrategy strategy;
 	List	   *partParams;		/* List of PartitionElems */
 	int			location;		/* token location, or -1 if unknown */
 } PartitionSpec;
 
-/* Internal codes for partitioning strategies */
-#define PARTITION_STRATEGY_HASH		'h'
-#define PARTITION_STRATEGY_LIST		'l'
-#define PARTITION_STRATEGY_RANGE	'r'
-
 /*
  * PartitionBoundSpec - a partition bound specification
  *
diff --git a/src/include/partitioning/partbounds.h b/src/include/partitioning/partbounds.h
index 1f5b706d83..9af4b1b5c7 100644
--- a/src/include/partitioning/partbounds.h
+++ b/src/include/partitioning/partbounds.h
@@ -78,7 +78,7 @@ struct RelOptInfo;				/* avoid including pathnodes.h here */
  */
 typedef struct PartitionBoundInfoData
 {
-	char		strategy;		/* hash, list or range? */
+	PartitionStrategy	strategy;		/* hash, list or range? */
 	int			ndatums;		/* Length of the datums[] array */
 	Datum	  **datums;
 	PartitionRangeDatumKind **kind; /* The kind of each range bound datum;
diff --git a/src/include/utils/partcache.h b/src/include/utils/partcache.h
index 3394e1fcdb..c01ce48241 100644
--- a/src/include/utils/partcache.h
+++ b/src/include/utils/partcache.h
@@ -23,7 +23,7 @@
  */
 typedef struct PartitionKeyData
 {
-	char		strategy;		/* partitioning strategy */
+	PartitionStrategy strategy;		/* partitioning strategy */
 	int16		partnatts;		/* number of columns in the partition key */
 	AttrNumber *partattrs;		/* attribute numbers of columns in the
 								 * partition key or 0 if it's an expr */
-- 
2.30.2


--xrws3pp5u7pwsoei--