From 69adb627c33d3fe0f76c765969b690e280ca49b5 Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Fri, 22 Mar 2019 20:12:57 -0600 Subject: [PATCH 1/5] Add Assert that will crash before coercion fix is in --- .../distributed/planner/shard_pruning.c | 8 ++++++- .../expected/multi_prune_shard_list.out | 24 +++++++++++++++++++ .../regress/sql/multi_prune_shard_list.sql | 18 ++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/planner/shard_pruning.c b/src/backend/distributed/planner/shard_pruning.c index a0adb7ee0..487685ee5 100644 --- a/src/backend/distributed/planner/shard_pruning.c +++ b/src/backend/distributed/planner/shard_pruning.c @@ -63,6 +63,7 @@ #include "nodes/nodeFuncs.h" #include "nodes/makefuncs.h" #include "optimizer/clauses.h" +#include "parser/parse_coerce.h" #include "utils/arrayaccess.h" #include "utils/catcache.h" #include "utils/lsyscache.h" @@ -715,13 +716,15 @@ AddNewConjuction(ClauseWalkerContext *context, OpExpr *op) */ static void AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opClause, - Var *varClause, Const *constantClause) + Var *partitionColumn, Const *constantClause) { PruningInstance *prune = context->currentPruningInstance; List *btreeInterpretationList = NULL; ListCell *btreeInterpretationCell = NULL; bool matchedOp = false; + Assert(IsBinaryCoercible(constantClause->consttype, partitionColumn->vartype)); + btreeInterpretationList = get_op_btree_interpretation(opClause->opno); foreach(btreeInterpretationCell, btreeInterpretationList) @@ -838,6 +841,9 @@ AddHashRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opClause, List *btreeInterpretationList = NULL; ListCell *btreeInterpretationCell = NULL; + /* be paranoid */ + Assert(IsBinaryCoercible(constantClause->consttype, INT4OID)); + btreeInterpretationList = get_op_btree_interpretation(opClause->opno); foreach(btreeInterpretationCell, btreeInterpretationList) diff --git a/src/test/regress/expected/multi_prune_shard_list.out b/src/test/regress/expected/multi_prune_shard_list.out index f39264127..2bdf12640 100644 --- a/src/test/regress/expected/multi_prune_shard_list.out +++ b/src/test/regress/expected/multi_prune_shard_list.out @@ -203,3 +203,27 @@ SELECT print_sorted_shard_intervals('pruning_range'); {800004,800005,800006,800007} (1 row) +-- =================================================================== +-- test pruning using values whose types are coerced +-- =================================================================== +CREATE TABLE coerce_hash ( + id bigint NOT NULL, + value text NOT NULL +); +SELECT create_distributed_table('coerce_hash', 'id'); + create_distributed_table +-------------------------- + +(1 row) + +-- PostgreSQL will cast id to numeric rather than 1.0 to bigint... +-- We used to blindly pass the RHS' Datum to our comparison func., +-- resulting in inaccurate pruning. An Assert now verifies type- +-- compatibility; the following would crash the server in an Assert- +-- before the underlying issue was addressed. It looks like a boring +-- test now, but if the old behavior is restored, it should crash again. +SELECT * FROM coerce_hash WHERE id = 1.0::numeric; + id | value +----+------- +(0 rows) + diff --git a/src/test/regress/sql/multi_prune_shard_list.sql b/src/test/regress/sql/multi_prune_shard_list.sql index 39f32a502..89c73d0db 100644 --- a/src/test/regress/sql/multi_prune_shard_list.sql +++ b/src/test/regress/sql/multi_prune_shard_list.sql @@ -119,3 +119,21 @@ SELECT print_sorted_shard_intervals('pruning_range'); -- all shard placements are uninitialized UPDATE pg_dist_shard set shardminvalue = NULL, shardmaxvalue = NULL WHERE shardid = 103077; SELECT print_sorted_shard_intervals('pruning_range'); + +-- =================================================================== +-- test pruning using values whose types are coerced +-- =================================================================== + +CREATE TABLE coerce_hash ( + id bigint NOT NULL, + value text NOT NULL +); +SELECT create_distributed_table('coerce_hash', 'id'); + +-- PostgreSQL will cast id to numeric rather than 1.0 to bigint... +-- We used to blindly pass the RHS' Datum to our comparison func., +-- resulting in inaccurate pruning. An Assert now verifies type- +-- compatibility; the following would crash the server in an Assert- +-- before the underlying issue was addressed. It looks like a boring +-- test now, but if the old behavior is restored, it should crash again. +SELECT * FROM coerce_hash WHERE id = 1.0::numeric; From 5baa257c9149329a4e88cd337b1b4531c58d1147 Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Fri, 22 Mar 2019 20:14:20 -0600 Subject: [PATCH 2/5] Add second assert to guard against future changes This isn't entirely necessary but I feel safer with it here. --- src/backend/distributed/planner/shard_pruning.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/backend/distributed/planner/shard_pruning.c b/src/backend/distributed/planner/shard_pruning.c index 487685ee5..d43ab9a9d 100644 --- a/src/backend/distributed/planner/shard_pruning.c +++ b/src/backend/distributed/planner/shard_pruning.c @@ -806,6 +806,19 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla case ROWCOMPARE_NE: { + /* + * This case should only arise when ALL list elements have this + * "strategy" number set. Skipping to the end of the list might + * protect us if that assumption is violated, and an Assert can + * notify us if it ever is... + */ + + /* should see this value immediately */ + Assert(btreeInterpretationCell == btreeInterpretationList->head); + + /* stop processing early, would only see unsupported nodes anyhow */ + btreeInterpretationCell = btreeInterpretationList->tail; + /* TODO: could add support for this, if we feel like it */ matchedOp = false; break; From 6acf52660c549fb8dfd6877bbd66414a1d9652ef Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Fri, 22 Mar 2019 20:15:45 -0600 Subject: [PATCH 3/5] Always coerce RHS of pruning op to part. key type Our assumption that strip_implicit_coercions would leave us with a bi- nary-compatible type to that of the partition key was wrong. Instead, we should ensure the RHS of the comparison we perform is proactively coerced into a compatible type (at least binary compatible). --- .../distributed/planner/shard_pruning.c | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/backend/distributed/planner/shard_pruning.c b/src/backend/distributed/planner/shard_pruning.c index d43ab9a9d..508a835b9 100644 --- a/src/backend/distributed/planner/shard_pruning.c +++ b/src/backend/distributed/planner/shard_pruning.c @@ -63,6 +63,7 @@ #include "nodes/nodeFuncs.h" #include "nodes/makefuncs.h" #include "optimizer/clauses.h" +#include "optimizer/planner.h" #include "parser/parse_coerce.h" #include "utils/arrayaccess.h" #include "utils/catcache.h" @@ -172,6 +173,8 @@ static bool PrunableExpressionsWalker(Node *originalNode, ClauseWalkerContext *c static void AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opClause, Var *varClause, Const *constantClause); +static Const * TransformPartitionRestrictionValue(Var *partitionColumn, + Node *restrictionValue); static void AddSAOPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, ScalarArrayOpExpr * arrayOperatorExpression); @@ -723,6 +726,18 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla ListCell *btreeInterpretationCell = NULL; bool matchedOp = false; + /* we want our restriction value in terms of the type of the partition column */ + constantClause = TransformPartitionRestrictionValue(partitionColumn, + (Node *) constantClause); + if (constantClause == NULL) + { + /* couldn't coerce the value, so we note this as a restriction we don't grok */ + prune->otherRestrictions = lappend(prune->otherRestrictions, opClause); + + return; + } + + /* at this point, we'd better be able to pass binary Datums to comparison functions */ Assert(IsBinaryCoercible(constantClause->consttype, partitionColumn->vartype)); btreeInterpretationList = @@ -841,6 +856,45 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla } +/* + * Sometimes PostgreSQL chooses to try to wrap our Var in a coercion rather + * than the Const; to deal with this, we strip the coercions from both and + * manually coerce the Const into the type of our partition column. It is + * conceivable that in some instances, this may not be possible; in those cases + * we will simply fail to prune partitions based on this clause. + */ +static Const * +TransformPartitionRestrictionValue(Var *partitionColumn, Node *restrictionValue) +{ + Node *transformedValue = coerce_to_target_type(NULL, restrictionValue, + exprType(restrictionValue), + partitionColumn->vartype, + partitionColumn->vartypmod, + COERCION_ASSIGNMENT, + COERCE_IMPLICIT_CAST, -1); + + /* if NULL, no implicit coercion is possible between the types */ + if (transformedValue == NULL) + { + return NULL; + } + + /* if still not a constant, evaluate coercion */ + if (!IsA(transformedValue, Const)) + { + transformedValue = (Node *) expression_planner((Expr *) transformedValue); + } + + /* if still not a constant, no immutable coercion matched */ + if (!IsA(transformedValue, Const)) + { + return NULL; + } + + return (Const *) transformedValue; +} + + /* * AddHashRestrictionToInstance adds information about a * RESERVED_HASHED_COLUMN_ID = Const restriction to the current pruning From 6a0dc7756ed72a3d8132aa0c248cbf6ce6e824b8 Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Fri, 22 Mar 2019 20:18:00 -0600 Subject: [PATCH 4/5] Formatting fixes Noticed a lot of weird lines wrapped at 80; our standard is 90. --- src/backend/distributed/planner/shard_pruning.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/backend/distributed/planner/shard_pruning.c b/src/backend/distributed/planner/shard_pruning.c index 508a835b9..89d6346c1 100644 --- a/src/backend/distributed/planner/shard_pruning.c +++ b/src/backend/distributed/planner/shard_pruning.c @@ -534,8 +534,7 @@ PrunableExpressionsWalker(Node *node, ClauseWalkerContext *context) if (!prune->addedToPruningInstances) { - context->pruningInstances = lappend(context->pruningInstances, - prune); + context->pruningInstances = lappend(context->pruningInstances, prune); prune->addedToPruningInstances = true; } @@ -565,8 +564,8 @@ PrunableExpressionsWalker(Node *node, ClauseWalkerContext *context) * Found a restriction on the partition column itself. Update the * current constraint with the new information. */ - AddPartitionKeyRestrictionToInstance(context, - opClause, varClause, constantClause); + AddPartitionKeyRestrictionToInstance(context, opClause, varClause, + constantClause); } else if (constantClause && varClause && varClause->varattno == RESERVED_HASHED_COLUMN_ID) @@ -597,8 +596,7 @@ PrunableExpressionsWalker(Node *node, ClauseWalkerContext *context) */ if (!prune->addedToPruningInstances) { - context->pruningInstances = lappend(context->pruningInstances, - prune); + context->pruningInstances = lappend(context->pruningInstances, prune); prune->addedToPruningInstances = true; } @@ -740,8 +738,7 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla /* at this point, we'd better be able to pass binary Datums to comparison functions */ Assert(IsBinaryCoercible(constantClause->consttype, partitionColumn->vartype)); - btreeInterpretationList = - get_op_btree_interpretation(opClause->opno); + btreeInterpretationList = get_op_btree_interpretation(opClause->opno); foreach(btreeInterpretationCell, btreeInterpretationList) { OpBtreeInterpretation *btreeInterpretation = @@ -846,8 +843,7 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla if (!matchedOp) { - prune->otherRestrictions = - lappend(prune->otherRestrictions, opClause); + prune->otherRestrictions = lappend(prune->otherRestrictions, opClause); } else { From 4c7f78bd7eee322913c88a369adbf373bee505d3 Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Mon, 25 Mar 2019 22:07:27 -0500 Subject: [PATCH 5/5] Code review feedback --- .../distributed/planner/shard_pruning.c | 26 ++++++++------- .../expected/multi_prune_shard_list.out | 32 ++++++++++++++++--- .../regress/sql/multi_prune_shard_list.sql | 14 ++++++++ 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/src/backend/distributed/planner/shard_pruning.c b/src/backend/distributed/planner/shard_pruning.c index 89d6346c1..328d1618c 100644 --- a/src/backend/distributed/planner/shard_pruning.c +++ b/src/backend/distributed/planner/shard_pruning.c @@ -174,7 +174,7 @@ static void AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opClause, Var *varClause, Const *constantClause); static Const * TransformPartitionRestrictionValue(Var *partitionColumn, - Node *restrictionValue); + Const *restrictionValue); static void AddSAOPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, ScalarArrayOpExpr * arrayOperatorExpression); @@ -724,15 +724,19 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla ListCell *btreeInterpretationCell = NULL; bool matchedOp = false; - /* we want our restriction value in terms of the type of the partition column */ - constantClause = TransformPartitionRestrictionValue(partitionColumn, - (Node *) constantClause); - if (constantClause == NULL) + /* only have extra work to do if const isn't same type as partition column */ + if (constantClause->consttype != partitionColumn->vartype) { - /* couldn't coerce the value, so we note this as a restriction we don't grok */ - prune->otherRestrictions = lappend(prune->otherRestrictions, opClause); + /* we want our restriction value in terms of the type of the partition column */ + constantClause = TransformPartitionRestrictionValue(partitionColumn, + constantClause); + if (constantClause == NULL) + { + /* couldn't coerce value, so we note this as a restriction we don't grok */ + prune->otherRestrictions = lappend(prune->otherRestrictions, opClause); - return; + return; + } } /* at this point, we'd better be able to pass binary Datums to comparison functions */ @@ -860,10 +864,10 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla * we will simply fail to prune partitions based on this clause. */ static Const * -TransformPartitionRestrictionValue(Var *partitionColumn, Node *restrictionValue) +TransformPartitionRestrictionValue(Var *partitionColumn, Const *restrictionValue) { - Node *transformedValue = coerce_to_target_type(NULL, restrictionValue, - exprType(restrictionValue), + Node *transformedValue = coerce_to_target_type(NULL, (Node *) restrictionValue, + restrictionValue->consttype, partitionColumn->vartype, partitionColumn->vartypmod, COERCION_ASSIGNMENT, diff --git a/src/test/regress/expected/multi_prune_shard_list.out b/src/test/regress/expected/multi_prune_shard_list.out index 2bdf12640..0d62956a9 100644 --- a/src/test/regress/expected/multi_prune_shard_list.out +++ b/src/test/regress/expected/multi_prune_shard_list.out @@ -216,14 +216,38 @@ SELECT create_distributed_table('coerce_hash', 'id'); (1 row) +INSERT INTO coerce_hash VALUES (1, 'test value'); +-- All three of the following should return the same results... +-- SELECT with same type as partition column +SELECT * FROM coerce_hash WHERE id = 1::bigint; + id | value +----+------------ + 1 | test value +(1 row) + +-- SELECT with similar type to partition column +SELECT * FROM coerce_hash WHERE id = 1; + id | value +----+------------ + 1 | test value +(1 row) + +-- SELECT with numeric type (original impetus for this change) -- PostgreSQL will cast id to numeric rather than 1.0 to bigint... -- We used to blindly pass the RHS' Datum to our comparison func., -- resulting in inaccurate pruning. An Assert now verifies type- -- compatibility; the following would crash the server in an Assert- -- before the underlying issue was addressed. It looks like a boring -- test now, but if the old behavior is restored, it should crash again. -SELECT * FROM coerce_hash WHERE id = 1.0::numeric; - id | value -----+------- -(0 rows) +SELECT * FROM coerce_hash WHERE id = 1.0; + id | value +----+------------ + 1 | test value +(1 row) + +SELECT * FROM coerce_hash WHERE id = 1.0::numeric; + id | value +----+------------ + 1 | test value +(1 row) diff --git a/src/test/regress/sql/multi_prune_shard_list.sql b/src/test/regress/sql/multi_prune_shard_list.sql index 89c73d0db..daf648621 100644 --- a/src/test/regress/sql/multi_prune_shard_list.sql +++ b/src/test/regress/sql/multi_prune_shard_list.sql @@ -130,10 +130,24 @@ CREATE TABLE coerce_hash ( ); SELECT create_distributed_table('coerce_hash', 'id'); +INSERT INTO coerce_hash VALUES (1, 'test value'); + +-- All three of the following should return the same results... + +-- SELECT with same type as partition column +SELECT * FROM coerce_hash WHERE id = 1::bigint; + +-- SELECT with similar type to partition column +SELECT * FROM coerce_hash WHERE id = 1; + +-- SELECT with numeric type (original impetus for this change) + -- PostgreSQL will cast id to numeric rather than 1.0 to bigint... -- We used to blindly pass the RHS' Datum to our comparison func., -- resulting in inaccurate pruning. An Assert now verifies type- -- compatibility; the following would crash the server in an Assert- -- before the underlying issue was addressed. It looks like a boring -- test now, but if the old behavior is restored, it should crash again. +SELECT * FROM coerce_hash WHERE id = 1.0; + SELECT * FROM coerce_hash WHERE id = 1.0::numeric;