parse partition strategy string in gram.y
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)
Attachments:
0001-have-gram.y-resolve-partition-strategy-names.patchtext/x-diff; charset=us-asciiDownload
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
On Fri, 21 Oct 2022 at 17:32, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
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.
Does there an error about forget the LIST partition?
+/*
+ * 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) <-- it should be list
+ return PARTITION_STRATEGY_RANGE; <-- PARTITION_STRATEGY_LIST
+ 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)));
+}
+--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On 2022-Oct-21, Japin Li wrote:
Does there an error about forget the LIST partition?
Of course.
https://cirrus-ci.com/build/4721735111540736
This is what you get for moving cases around at the last minute ...
Fixed, thanks.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Attachments:
v2-0001-have-gram.y-resolve-partition-strategy-names.patchtext/x-diff; charset=us-asciiDownload
From dc345f3cb70c335421f29a6867438ed4bb95bd91 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 v2] 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..6ca23f88c4 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, "list") == 0)
+ return PARTITION_STRATEGY_LIST;
+ else if (pg_strcasecmp(strategy, "range") == 0)
+ return PARTITION_STRATEGY_RANGE;
+ else if (pg_strcasecmp(strategy, "hash") == 0)
+ return PARTITION_STRATEGY_HASH;
+ 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
On Fri, 21 Oct 2022 at 18:12, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Oct-21, Japin Li wrote:
Does there an error about forget the LIST partition?
Of course.
https://cirrus-ci.com/build/4721735111540736This is what you get for moving cases around at the last minute ...
Is there any way to get the regression tests diffs from Cirrus CI?
I did not find the diffs in [1]https://cirrus-ci.com/build/4721735111540736.
[1]: https://cirrus-ci.com/build/4721735111540736
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On 2022-Oct-21, Japin Li wrote:
Is there any way to get the regression tests diffs from Cirrus CI?
I did not find the diffs in [1].
I think they should be somewhere in the artifacts, but I'm not sure.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)
headerscheck fail, fixed here.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
#error "Operator lives in the wrong universe"
("Use of cookies in real-time system development", M. Gleixner, M. Mc Guire)
Attachments:
v3-0001-have-gram.y-resolve-partition-strategy-names.patchtext/x-diff; charset=us-asciiDownload
From c434020fc07616cdd13017135819083186d33256 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 v3] have gram.y resolve partition strategy names
---
src/backend/commands/tablecmds.c | 35 +++++++++------------------
src/backend/parser/gram.y | 22 ++++++++++++++++-
src/backend/partitioning/partbounds.c | 27 ++++-----------------
src/backend/utils/cache/partcache.c | 6 +++++
src/include/nodes/parsenodes.h | 15 ++++++------
src/include/partitioning/partbounds.h | 2 +-
src/include/utils/partcache.h | 3 ++-
7 files changed, 54 insertions(+), 56 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..6ca23f88c4 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, "list") == 0)
+ return PARTITION_STRATEGY_LIST;
+ else if (pg_strcasecmp(strategy, "range") == 0)
+ return PARTITION_STRATEGY_RANGE;
+ else if (pg_strcasecmp(strategy, "hash") == 0)
+ return PARTITION_STRATEGY_HASH;
+ 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..29643fb4ab 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,12 +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 */
}
+
+ return NULL;
}
/*
@@ -2892,8 +2880,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 +3228,6 @@ check_new_partition_bound(char *relname, Relation parent,
break;
}
-
- default:
- elog(ERROR, "unexpected partition strategy: %d",
- (int) key->strategy);
}
if (overlap)
@@ -3980,8 +3963,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..88b5bd7b2b 100644
--- a/src/include/utils/partcache.h
+++ b/src/include/utils/partcache.h
@@ -13,6 +13,7 @@
#include "access/attnum.h"
#include "fmgr.h"
+#include "nodes/parsenodes.h"
#include "nodes/pg_list.h"
#include "nodes/primnodes.h"
#include "partitioning/partdefs.h"
@@ -23,7 +24,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
On Fri, Oct 21, 2022 at 06:22:44PM +0800, Japin Li wrote:
Is there any way to get the regression tests diffs from Cirrus CI?
I did not find the diffs in [1].
They're called "main".
I'm planning on submitting a patch to rename it to "regress", someday.
See also: /messages/by-id/20221001161420.GF6256@telsasoft.com
--
Justin
On Fri, 21 Oct 2022 at 20:34, Justin Pryzby <pryzby@telsasoft.com> wrote:
On Fri, Oct 21, 2022 at 06:22:44PM +0800, Japin Li wrote:
Is there any way to get the regression tests diffs from Cirrus CI?
I did not find the diffs in [1].They're called "main".
I'm planning on submitting a patch to rename it to "regress", someday.
See also: /messages/by-id/20221001161420.GF6256@telsasoft.com
Oh, thank you very much! I find it in testrun/build/testrun/main/regress [1]https://api.cirrus-ci.com/v1/artifact/task/6215926717612032/testrun/build/testrun/main/regress/regression.diffs.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Is there a reason why HASH partitioning does not currently support range partition bounds, where the values in the partition bounds would refer to the hashed value?
The advantage of hash partition bounds is that they are not domain-specific, as they are for ordinary RANGE partitions, but they are more flexible than MODULUS/REMAINDER partition bounds.
On 10/21/22, 9:48 AM, "Japin Li" <japinli@hotmail.com> wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
On Fri, 21 Oct 2022 at 20:34, Justin Pryzby <pryzby@telsasoft.com> wrote:
On Fri, Oct 21, 2022 at 06:22:44PM +0800, Japin Li wrote:
Is there any way to get the regression tests diffs from Cirrus CI?
I did not find the diffs in [1].They're called "main".
I'm planning on submitting a patch to rename it to "regress", someday.
See also: /messages/by-id/20221001161420.GF6256@telsasoft.com
Oh, thank you very much! I find it in testrun/build/testrun/main/regress [1]https://api.cirrus-ci.com/v1/artifact/task/6215926717612032/testrun/build/testrun/main/regress/regression.diffs.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On 2022-Oct-24, Finnerty, Jim wrote:
Is there a reason why HASH partitioning does not currently support
range partition bounds, where the values in the partition bounds would
refer to the hashed value?
Just lack of an implementation, I suppose.
The advantage of hash partition bounds is that they are not
domain-specific, as they are for ordinary RANGE partitions, but they
are more flexible than MODULUS/REMAINDER partition bounds.
Well, modulus/remainder is what we have. If you have ideas for a
different implementation, let's hear them. I suppose we would have to
know about both the user interface and how it would internally, from two
perspectives: how does tuple routing work (ie. how to match a tuple's
values to a set of bound values), and how does partition pruning work
(ie. how do partition bounds match a query's restriction clauses).
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2022-Oct-24, Finnerty, Jim wrote:
The advantage of hash partition bounds is that they are not
domain-specific, as they are for ordinary RANGE partitions, but they
are more flexible than MODULUS/REMAINDER partition bounds.
I'm more than a bit skeptical of that claim. Under what
circumstances (other than a really awful hash function,
perhaps) would it make sense to not use equi-sized hash
partitions? If you can predict that more stuff is going
to go into one partition than another, then you need to
fix your hash function, not invent more complication for
the core partitioning logic.
regards, tom lane
It will often happen that some hash keys are more frequently referenced than others. Consider a scenario where customer_id is the hash key, and one customer is very large in terms of their activity, like IBM, and other keys have much less activity. This asymmetry creates a noisy neighbor problem. Some partitions may have more than one noisy neighbor, and in general it would be more flexible to be able to divide the work evenly in terms of activity instead of evenly with respect to the encoding of the keys.
On 10/24/22, 8:50 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2022-Oct-24, Finnerty, Jim wrote:
The advantage of hash partition bounds is that they are not
domain-specific, as they are for ordinary RANGE partitions, but they
are more flexible than MODULUS/REMAINDER partition bounds.
I'm more than a bit skeptical of that claim. Under what
circumstances (other than a really awful hash function,
perhaps) would it make sense to not use equi-sized hash
partitions?
<snip>
regards, tom lane
Or if you know the frequencies of the highly frequent values of the partitioning key at the time the partition bounds are defined, you could define hash ranges that contain approximately the same number of rows in each partition. A parallel sequential scan of all partitions would then perform better because data skew is minimized.
On 2022-Oct-25, Finnerty, Jim wrote:
Or if you know the frequencies of the highly frequent values of the
partitioning key at the time the partition bounds are defined, you
could define hash ranges that contain approximately the same number of
rows in each partition. A parallel sequential scan of all partitions
would then perform better because data skew is minimized.
This sounds very much like list partitioning to me.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"The problem with the future is that it keeps turning into the present"
(Hobbes)
On 2022-Oct-26, Alvaro Herrera wrote:
On 2022-Oct-25, Finnerty, Jim wrote:
Or if you know the frequencies of the highly frequent values of the
partitioning key at the time the partition bounds are defined, you
could define hash ranges that contain approximately the same number of
rows in each partition. A parallel sequential scan of all partitions
would then perform better because data skew is minimized.This sounds very much like list partitioning to me.
... or maybe you mean "if the value is X then use this specific
partition, otherwise use hash partitioning". It's a bit like
multi-level partitioning, but not really.
(You could test this idea by using two levels, list partitioning on top
with a default partition which is in turn partitioned by hash; but this
is unlikely to work well for large scale in practice. Or does it?)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Entristecido, Wutra (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"
Pushed this.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/