From 1c19e2863994ec15e496d21a9320aff95a389d0b Mon Sep 17 00:00:00 2001 From: David Rowley Date: Fri, 14 Apr 2023 16:23:11 +1200 Subject: [PATCH] Fix incorrect partition pruning logic for boolean partitioned tables The partition pruning logic assumed that "b IS NOT true" was exactly the same as "b IS FALSE". This is not the case when considering NULL values. Fix this so we correctly include any partition which could hold NULL values for the NOT case. Additionally, this fixes a bug in the partition pruning code which handles partitioned tables partitioned like ((NOT boolcol)). This is a seemingly unlikely schema design, and it was untested and also broken. Here we add tests for the ((NOT boolcol)) case and insert some actual data into those tables and verify we do get the correct rows back when running queries. I've also adjusted the existing boolpart tests to include some data and verify we get the correct results too. Both the bugs being fixed here could lead to incorrect query results with fewer rows being returned than expected. No additional rows could have been returned accidentally. In passing, remove needless ternary expression. It's more simple just to pass !is_not_clause to makeBoolConst(). It makes sense to do this so the code is consistent with the bug fix in the "else if" condition just below. David Kimura did submit a patch to fix the first of the issues here, but that's not what's being committed here. Reported-by: David Kimura Reviewed-by: Richard Guo, David Kimura Discussion: https://postgr.es/m/CAHnPFjQ5qxs6J_p+g8=ww7GQvfn71_JE+Tygj0S7RdRci1uwPw@mail.gmail.com Backpatch-through: 11, all supported versions --- src/backend/partitioning/partprune.c | 54 +++-- src/test/regress/expected/partition_prune.out | 227 +++++++++++++++++- src/test/regress/sql/partition_prune.sql | 38 ++- 3 files changed, 289 insertions(+), 30 deletions(-) diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c index f3864d2992..86a4ca497d 100644 --- a/src/backend/partitioning/partprune.c +++ b/src/backend/partitioning/partprune.c @@ -198,7 +198,8 @@ static PruneStepResult *perform_pruning_combine_step(PartitionPruneContext *cont static PartClauseMatchStatus match_boolean_partition_clause(Oid partopfamily, Expr *clause, Expr *partkey, - Expr **outconst); + Expr **outconst, + bool *noteq); static void partkey_datum_from_expr(PartitionPruneContext *context, Expr *expr, int stateidx, Datum *value, bool *isnull); @@ -1695,12 +1696,13 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context, Oid partopfamily = part_scheme->partopfamily[partkeyidx], partcoll = part_scheme->partcollation[partkeyidx]; Expr *expr; + bool noteq; /* * Recognize specially shaped clauses that match a Boolean partition key. */ boolmatchstatus = match_boolean_partition_clause(partopfamily, clause, - partkey, &expr); + partkey, &expr, ¬eq); if (boolmatchstatus == PARTCLAUSE_MATCH_CLAUSE) { @@ -1710,7 +1712,7 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context, partclause->keyno = partkeyidx; /* Do pruning with the Boolean equality operator. */ partclause->opno = BooleanEqualOperator; - partclause->op_is_ne = false; + partclause->op_is_ne = noteq; partclause->expr = expr; /* We know that expr is of Boolean type. */ partclause->cmpfn = part_scheme->partsupfunc[partkeyidx].fn_oid; @@ -3468,20 +3470,22 @@ perform_pruning_combine_step(PartitionPruneContext *context, * match_boolean_partition_clause * * If we're able to match the clause to the partition key as specially-shaped - * boolean clause, set *outconst to a Const containing a true or false value - * and return PARTCLAUSE_MATCH_CLAUSE. Returns PARTCLAUSE_UNSUPPORTED if the - * clause is not a boolean clause or if the boolean clause is unsuitable for - * partition pruning. Returns PARTCLAUSE_NOMATCH if it's a bool quals but - * just does not match this partition key. *outconst is set to NULL in the - * latter two cases. + * boolean clause, set *outconst to a Const containing a true or false value, + * set *noteq according to if the clause was in the "not" form, i.e. "is not + * true" or "is not false", and return PARTCLAUSE_MATCH_CLAUSE. Returns + * PARTCLAUSE_UNSUPPORTED if the clause is not a boolean clause or if the + * boolean clause is unsuitable for partition pruning. Returns + * PARTCLAUSE_NOMATCH if it's a bool quals but just does not match this + * partition key. *outconst is set to NULL in the latter two cases. */ static PartClauseMatchStatus match_boolean_partition_clause(Oid partopfamily, Expr *clause, Expr *partkey, - Expr **outconst) + Expr **outconst, bool *noteq) { Expr *leftop; *outconst = NULL; + *noteq = false; if (!IsBooleanOpfamily(partopfamily)) return PARTCLAUSE_UNSUPPORTED; @@ -3500,11 +3504,25 @@ match_boolean_partition_clause(Oid partopfamily, Expr *clause, Expr *partkey, leftop = ((RelabelType *) leftop)->arg; if (equal(leftop, partkey)) - *outconst = (btest->booltesttype == IS_TRUE || - btest->booltesttype == IS_NOT_FALSE) - ? (Expr *) makeBoolConst(true, false) - : (Expr *) makeBoolConst(false, false); - + { + switch (btest->booltesttype) + { + case IS_NOT_TRUE: + *noteq = true; + /* fall through */ + case IS_TRUE: + *outconst = (Expr *) makeBoolConst(true, false); + break; + case IS_NOT_FALSE: + *noteq = true; + /* fall through */ + case IS_FALSE: + *outconst = (Expr *) makeBoolConst(false, false); + break; + default: + return PARTCLAUSE_UNSUPPORTED; + } + } if (*outconst) return PARTCLAUSE_MATCH_CLAUSE; } @@ -3519,11 +3537,9 @@ match_boolean_partition_clause(Oid partopfamily, Expr *clause, Expr *partkey, /* Compare to the partition key, and make up a clause ... */ if (equal(leftop, partkey)) - *outconst = is_not_clause ? - (Expr *) makeBoolConst(false, false) : - (Expr *) makeBoolConst(true, false); + *outconst = (Expr *) makeBoolConst(!is_not_clause, false); else if (equal(negate_clause((Node *) leftop), partkey)) - *outconst = (Expr *) makeBoolConst(false, false); + *outconst = (Expr *) makeBoolConst(is_not_clause, false); if (*outconst) return PARTCLAUSE_MATCH_CLAUSE; diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out index 28ce692eea..83148a5d14 100644 --- a/src/test/regress/expected/partition_prune.out +++ b/src/test/regress/expected/partition_prune.out @@ -1043,6 +1043,7 @@ create table boolpart (a bool) partition by list (a); create table boolpart_default partition of boolpart default; create table boolpart_t partition of boolpart for values in ('true'); create table boolpart_f partition of boolpart for values in ('false'); +insert into boolpart values (true), (false), (null); explain (costs off) select * from boolpart where a in (true, false); QUERY PLAN ------------------------------------------------ @@ -1077,22 +1078,27 @@ explain (costs off) select * from boolpart where a is true or a is not true; Filter: ((a IS TRUE) OR (a IS NOT TRUE)) -> Seq Scan on boolpart_t Filter: ((a IS TRUE) OR (a IS NOT TRUE)) -(5 rows) + -> Seq Scan on boolpart_default + Filter: ((a IS TRUE) OR (a IS NOT TRUE)) +(7 rows) explain (costs off) select * from boolpart where a is not true; - QUERY PLAN ---------------------------------- + QUERY PLAN +------------------------------------ Append -> Seq Scan on boolpart_f Filter: (a IS NOT TRUE) -(3 rows) + -> Seq Scan on boolpart_default + Filter: (a IS NOT TRUE) +(5 rows) explain (costs off) select * from boolpart where a is not true and a is not false; - QUERY PLAN --------------------------- - Result - One-Time Filter: false -(2 rows) + QUERY PLAN +-------------------------------------------------------- + Append + -> Seq Scan on boolpart_default + Filter: ((a IS NOT TRUE) AND (a IS NOT FALSE)) +(3 rows) explain (costs off) select * from boolpart where a is unknown; QUERY PLAN @@ -1118,6 +1124,207 @@ explain (costs off) select * from boolpart where a is not unknown; Filter: (a IS NOT UNKNOWN) (7 rows) +select * from boolpart where a in (true, false); + a +--- + f + t +(2 rows) + +select * from boolpart where a = false; + a +--- + f +(1 row) + +select * from boolpart where not a = false; + a +--- + t +(1 row) + +select * from boolpart where a is true or a is not true; + a +--- + f + t + +(3 rows) + +select * from boolpart where a is not true; + a +--- + f + +(2 rows) + +select * from boolpart where a is not true and a is not false; + a +--- + +(1 row) + +select * from boolpart where a is unknown; + a +--- + +(1 row) + +select * from boolpart where a is not unknown; + a +--- + f + t +(2 rows) + +-- inverse boolean partitioning - a seemingly unlikely design, but we've got +-- code for it, so we'd better test it. +create table iboolpart (a bool) partition by list ((not a)); +create table iboolpart_default partition of iboolpart default; +create table iboolpart_f partition of iboolpart for values in ('true'); +create table iboolpart_t partition of iboolpart for values in ('false'); +insert into iboolpart values (true), (false), (null); +explain (costs off) select * from iboolpart where a in (true, false); + QUERY PLAN +------------------------------------------------ + Append + -> Seq Scan on iboolpart_t + Filter: (a = ANY ('{t,f}'::boolean[])) + -> Seq Scan on iboolpart_f + Filter: (a = ANY ('{t,f}'::boolean[])) + -> Seq Scan on iboolpart_default + Filter: (a = ANY ('{t,f}'::boolean[])) +(7 rows) + +explain (costs off) select * from iboolpart where a = false; + QUERY PLAN +------------------------------- + Append + -> Seq Scan on iboolpart_f + Filter: (NOT a) +(3 rows) + +explain (costs off) select * from iboolpart where not a = false; + QUERY PLAN +------------------------------- + Append + -> Seq Scan on iboolpart_t + Filter: a +(3 rows) + +explain (costs off) select * from iboolpart where a is true or a is not true; + QUERY PLAN +-------------------------------------------------- + Append + -> Seq Scan on iboolpart_t + Filter: ((a IS TRUE) OR (a IS NOT TRUE)) + -> Seq Scan on iboolpart_f + Filter: ((a IS TRUE) OR (a IS NOT TRUE)) + -> Seq Scan on iboolpart_default + Filter: ((a IS TRUE) OR (a IS NOT TRUE)) +(7 rows) + +explain (costs off) select * from iboolpart where a is not true; + QUERY PLAN +------------------------------------- + Append + -> Seq Scan on iboolpart_t + Filter: (a IS NOT TRUE) + -> Seq Scan on iboolpart_f + Filter: (a IS NOT TRUE) + -> Seq Scan on iboolpart_default + Filter: (a IS NOT TRUE) +(7 rows) + +explain (costs off) select * from iboolpart where a is not true and a is not false; + QUERY PLAN +-------------------------------------------------------- + Append + -> Seq Scan on iboolpart_t + Filter: ((a IS NOT TRUE) AND (a IS NOT FALSE)) + -> Seq Scan on iboolpart_f + Filter: ((a IS NOT TRUE) AND (a IS NOT FALSE)) + -> Seq Scan on iboolpart_default + Filter: ((a IS NOT TRUE) AND (a IS NOT FALSE)) +(7 rows) + +explain (costs off) select * from iboolpart where a is unknown; + QUERY PLAN +------------------------------------- + Append + -> Seq Scan on iboolpart_t + Filter: (a IS UNKNOWN) + -> Seq Scan on iboolpart_f + Filter: (a IS UNKNOWN) + -> Seq Scan on iboolpart_default + Filter: (a IS UNKNOWN) +(7 rows) + +explain (costs off) select * from iboolpart where a is not unknown; + QUERY PLAN +------------------------------------- + Append + -> Seq Scan on iboolpart_t + Filter: (a IS NOT UNKNOWN) + -> Seq Scan on iboolpart_f + Filter: (a IS NOT UNKNOWN) + -> Seq Scan on iboolpart_default + Filter: (a IS NOT UNKNOWN) +(7 rows) + +select * from iboolpart where a in (true, false); + a +--- + t + f +(2 rows) + +select * from iboolpart where a = false; + a +--- + f +(1 row) + +select * from iboolpart where not a = false; + a +--- + t +(1 row) + +select * from iboolpart where a is true or a is not true; + a +--- + t + f + +(3 rows) + +select * from iboolpart where a is not true; + a +--- + f + +(2 rows) + +select * from iboolpart where a is not true and a is not false; + a +--- + +(1 row) + +select * from iboolpart where a is unknown; + a +--- + +(1 row) + +select * from iboolpart where a is not unknown; + a +--- + t + f +(2 rows) + create table boolrangep (a bool, b bool, c int) partition by range (a,b,c); create table boolrangep_tf partition of boolrangep for values from ('true', 'false', 0) to ('true', 'false', 100); create table boolrangep_ft partition of boolrangep for values from ('false', 'true', 0) to ('false', 'true', 100); @@ -1524,7 +1731,7 @@ explain (costs off) select * from rparted_by_int2 where a > 100000000000000; Filter: (a > '100000000000000'::bigint) (3 rows) -drop table lp, coll_pruning, rlp, mc3p, mc2p, boolpart, boolrangep, rp, coll_pruning_multi, like_op_noprune, lparted_by_int2, rparted_by_int2; +drop table lp, coll_pruning, rlp, mc3p, mc2p, boolpart, iboolpart, boolrangep, rp, coll_pruning_multi, like_op_noprune, lparted_by_int2, rparted_by_int2; -- -- Test Partition pruning for HASH partitioning -- diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql index 9b94ac50af..a1ad757bfb 100644 --- a/src/test/regress/sql/partition_prune.sql +++ b/src/test/regress/sql/partition_prune.sql @@ -150,6 +150,7 @@ create table boolpart (a bool) partition by list (a); create table boolpart_default partition of boolpart default; create table boolpart_t partition of boolpart for values in ('true'); create table boolpart_f partition of boolpart for values in ('false'); +insert into boolpart values (true), (false), (null); explain (costs off) select * from boolpart where a in (true, false); explain (costs off) select * from boolpart where a = false; @@ -160,6 +161,41 @@ explain (costs off) select * from boolpart where a is not true and a is not fals explain (costs off) select * from boolpart where a is unknown; explain (costs off) select * from boolpart where a is not unknown; +select * from boolpart where a in (true, false); +select * from boolpart where a = false; +select * from boolpart where not a = false; +select * from boolpart where a is true or a is not true; +select * from boolpart where a is not true; +select * from boolpart where a is not true and a is not false; +select * from boolpart where a is unknown; +select * from boolpart where a is not unknown; + +-- inverse boolean partitioning - a seemingly unlikely design, but we've got +-- code for it, so we'd better test it. +create table iboolpart (a bool) partition by list ((not a)); +create table iboolpart_default partition of iboolpart default; +create table iboolpart_f partition of iboolpart for values in ('true'); +create table iboolpart_t partition of iboolpart for values in ('false'); +insert into iboolpart values (true), (false), (null); + +explain (costs off) select * from iboolpart where a in (true, false); +explain (costs off) select * from iboolpart where a = false; +explain (costs off) select * from iboolpart where not a = false; +explain (costs off) select * from iboolpart where a is true or a is not true; +explain (costs off) select * from iboolpart where a is not true; +explain (costs off) select * from iboolpart where a is not true and a is not false; +explain (costs off) select * from iboolpart where a is unknown; +explain (costs off) select * from iboolpart where a is not unknown; + +select * from iboolpart where a in (true, false); +select * from iboolpart where a = false; +select * from iboolpart where not a = false; +select * from iboolpart where a is true or a is not true; +select * from iboolpart where a is not true; +select * from iboolpart where a is not true and a is not false; +select * from iboolpart where a is unknown; +select * from iboolpart where a is not unknown; + create table boolrangep (a bool, b bool, c int) partition by range (a,b,c); create table boolrangep_tf partition of boolrangep for values from ('true', 'false', 0) to ('true', 'false', 100); create table boolrangep_ft partition of boolrangep for values from ('false', 'true', 0) to ('false', 'true', 100); @@ -281,7 +317,7 @@ create table rparted_by_int2_maxvalue partition of rparted_by_int2 for values fr -- all partitions but rparted_by_int2_maxvalue pruned explain (costs off) select * from rparted_by_int2 where a > 100000000000000; -drop table lp, coll_pruning, rlp, mc3p, mc2p, boolpart, boolrangep, rp, coll_pruning_multi, like_op_noprune, lparted_by_int2, rparted_by_int2; +drop table lp, coll_pruning, rlp, mc3p, mc2p, boolpart, iboolpart, boolrangep, rp, coll_pruning_multi, like_op_noprune, lparted_by_int2, rparted_by_int2; -- -- Test Partition pruning for HASH partitioning