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)
>From e6067eee889a6636eb013f2f8d85efaf2112232b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@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
