Re: parse partition strategy string in gram.y
Pushed this. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: parse partition strategy string in gram.y
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"
Re: parse partition strategy string in gram.y
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 HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "The problem with the future is that it keeps turning into the present" (Hobbes)
Re: parse partition strategy string in gram.y
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.
Re: parse partition strategy string in gram.y
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" 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 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? regards, tom lane
Re: parse partition strategy string in gram.y
Alvaro Herrera 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
Re: parse partition strategy string in gram.y
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/
Re: parse partition strategy string in gram.y
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" 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 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]. >> >> [1] https://cirrus-ci.com/build/4721735111540736 > > They're called "main". > I'm planning on submitting a patch to rename it to "regress", someday. > See also: https://www.postgresql.org/message-id/20221001161420.GF6256%40telsasoft.com Oh, thank you very much! I find it in testrun/build/testrun/main/regress [1]. [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.
Re: parse partition strategy string in gram.y
On Fri, 21 Oct 2022 at 20:34, Justin Pryzby 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]. >> >> [1] https://cirrus-ci.com/build/4721735111540736 > > They're called "main". > I'm planning on submitting a patch to rename it to "regress", someday. > See also: > https://www.postgresql.org/message-id/20221001161420.GF6256%40telsasoft.com Oh, thank you very much! I find it in testrun/build/testrun/main/regress [1]. [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.
Re: parse partition strategy string in gram.y
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]. > > [1] https://cirrus-ci.com/build/4721735111540736 They're called "main". I'm planning on submitting a patch to rename it to "regress", someday. See also: https://www.postgresql.org/message-id/20221001161420.GF6256%40telsasoft.com -- Justin
Re: parse partition strategy string in gram.y
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) >From c434020fc07616cdd13017135819083186d33256 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera 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/par
Re: parse partition strategy string in gram.y
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)
Re: parse partition strategy string in gram.y
On Fri, 21 Oct 2022 at 18:12, Alvaro Herrera wrote: > 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 ... > Is there any way to get the regression tests diffs from Cirrus CI? I did not find the diffs in [1]. [1] https://cirrus-ci.com/build/4721735111540736 -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: parse partition strategy string in gram.y
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/ >From dc345f3cb70c335421f29a6867438ed4bb95bd91 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera 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.
Re: parse partition strategy string in gram.y
On Fri, 21 Oct 2022 at 17:32, Alvaro Herrera 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.