pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
12+ messages / 6 participants
[nested] [flat]
* BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
@ 2024-02-15 09:00 PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: PG Bug reporting form @ 2024-02-15 09:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 18344
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 16.2
Operating system: Ubuntu 22.04
Description:
The following query:
CREATE TABLE t (b bool, i int) PARTITION BY RANGE (b, i);
CREATE TABLE tp PARTITION OF t FOR VALUES FROM (false, 0) TO (false, 1);
SELECT * FROM t WHERE b IS NOT true;
fails with ERROR: invalid strategy number 0.
Reproduced on REL_12_STABLE .. master.
The first bad commit for this anomaly is e0693faf7.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
@ 2024-02-15 16:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 02:56 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-16 12:32 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
0 siblings, 2 replies; 12+ messages in thread
From: Tom Lane @ 2024-02-15 16:28 UTC (permalink / raw)
To: exclusion@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org, David Rowley <dgrowleyml@gmail.com>
PG Bug reporting form <noreply@postgresql.org> writes:
> The following query:
> CREATE TABLE t (b bool, i int) PARTITION BY RANGE (b, i);
> CREATE TABLE tp PARTITION OF t FOR VALUES FROM (false, 0) TO (false, 1);
> SELECT * FROM t WHERE b IS NOT true;
> fails with ERROR: invalid strategy number 0.
> Reproduced on REL_12_STABLE .. master.
> The first bad commit for this anomaly is e0693faf7.
What seems to be happening is that gen_prune_step_op is getting
op_is_ne = true and doing this:
/*
* For clauses that contain an <> operator, set opstrategy to
* InvalidStrategy to signal get_matching_list_bounds to do the right
* thing.
*/
opstep->opstrategy = op_is_ne ? InvalidStrategy : opstrategy;
but then we're failing in get_matching_range_bounds, ie somebody
taught get_matching_list_bounds to do the right thing but not
any of the other code paths.
I'm also wondering how we got there in the first place. It looks like
match_boolean_partition_clause thinks it can translate "b IS NOT true"
to "b <> true", which is flat wrong --- it gives the wrong result for
null.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-02-16 02:56 ` David Rowley <dgrowleyml@gmail.com>
1 sibling, 0 replies; 12+ messages in thread
From: David Rowley @ 2024-02-16 02:56 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, 16 Feb 2024 at 05:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> What seems to be happening is that gen_prune_step_op is getting
> op_is_ne = true and doing this:
>
> /*
> * For clauses that contain an <> operator, set opstrategy to
> * InvalidStrategy to signal get_matching_list_bounds to do the right
> * thing.
> */
> opstep->opstrategy = op_is_ne ? InvalidStrategy : opstrategy;
>
> but then we're failing in get_matching_range_bounds, ie somebody
> taught get_matching_list_bounds to do the right thing but not
> any of the other code paths.
hmm, yeah. I'm just trying to wrap my head around if this can even
work for RANGE partitioned tables.
> I'm also wondering how we got there in the first place. It looks like
> match_boolean_partition_clause thinks it can translate "b IS NOT true"
> to "b <> true", which is flat wrong --- it gives the wrong result for
> null.
Thought I'd fixed that in e0693faf7, but looks like I only tested it
with DEFAULT partitions, not NULL partitions. A fairly simple fix for
that part:
/* Always include the default partition if any. */
result->scan_default = partition_bound_has_default(boundinfo);
+ /* Likewise for the NULL partition, if any */
+ result->scan_null = partition_bound_accepts_nulls(boundinfo);
I'll look at the RANGE <> bool stuff.
David
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-02-16 12:32 ` David Rowley <dgrowleyml@gmail.com>
2024-02-18 12:53 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 12+ messages in thread
From: David Rowley @ 2024-02-16 12:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlan@postgresql.org>; Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, 16 Feb 2024 at 05:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> What seems to be happening is that gen_prune_step_op is getting
> op_is_ne = true and doing this:
>
> /*
> * For clauses that contain an <> operator, set opstrategy to
> * InvalidStrategy to signal get_matching_list_bounds to do the right
> * thing.
> */
> opstep->opstrategy = op_is_ne ? InvalidStrategy : opstrategy;
>
> but then we're failing in get_matching_range_bounds, ie somebody
> taught get_matching_list_bounds to do the right thing but not
> any of the other code paths.
Yeah, prior to e0693faf7, we always did:
partclause->op_is_ne = false;
so the code you quoted would always use the equality opstrategy,
therefore wouldn't hit the "if (opstrategy == InvalidStrategy)" block
in get_matching_list_bounds().
The old code wrongly assumed "bool IS NOT true" was the same as "bool
IS false" and vice-versa. I had thought I could fix this by making it
use the same code that we do for cases like int4col <> 123, but:
a) only get_matching_list_bounds() knows how to handle those, not the
equivalent hash and range versions and;
b) bool is not true matches NULLs, but int4col <> 123 does not.
So, I'm a little unsure if we should try and make bool IS NOT clauses
prune partitions at all. It was a bug in the original code that
thought we could do that, and it looks like I didn't do a good job of
fixing that.
I see three options:
1) Make get_matching_list_bounds() work for bool IS NOT clauses by
properly including the NULL partition when handling a bool IS NOT
clause.
2) Just don't generate a pruning step for bool IS NOT clauses.
3) Just always include the NULL partition in
get_matching_list_bounds's "if (opstrategy == InvalidStrategy)" block.
I don't quite see how to do #1 as we don't have an easy way to tell if
we're handling bool IS NOT clauses inside get_matching_list_bounds().
Maybe we could check the comparison function is btboolcmp. That's
kinda cruddy. We don't have oid_symbol for pg_proc.dat as we do in
pg_operator.dat, so there's no #define for the pg_proc entry's Oid.
If we do #2, I'm concerned we'll regress someone's workload by
including the other bool partition. Likewise, for #3, we'll include
the NULL partition for non-boolean cases, which could cause someone
problems.
The attached does #3 plus disables "bool IS NOT" pruning for RANGE and
HASH to avoid the reported "invalid strategy number 0." error.
Maybe we could do #1 by checking for partsupfunc.fn_oid == 1693 in the
back branches and come up with a cleaner way in master by adding a new
field to PartitionPruneStepOp.
Keen to get feedback on this one. Also included Amit and Álvaro as
they might have another idea.
I also noticed that the following code returns PARTCLAUSE_NOMATCH:
if (part_scheme->strategy != PARTITION_STRATEGY_LIST)
return PARTCLAUSE_NOMATCH;
I think that should return PARTCLAUSE_UNSUPPORTED. But it's really
only an inefficiency rather than a bug, I think.
David
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 6b635e8ad1..2b582a0e72 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1810,6 +1810,13 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
{
PartClauseInfo *partclause;
+ /*
+ * Boolean exprs in the form of IS NOT false, IS NOT true are only supported
+ * for LIST partitioned tables.
+ */
+ if (noteq && part_scheme->strategy != PARTITION_STRATEGY_LIST)
+ return PARTCLAUSE_UNSUPPORTED;
+
partclause = (PartClauseInfo *) palloc(sizeof(PartClauseInfo));
partclause->keyno = partkeyidx;
/* Do pruning with the Boolean equality operator. */
@@ -2757,6 +2764,15 @@ get_matching_list_bounds(PartitionPruneContext *context,
/* Always include the default partition if any. */
result->scan_default = partition_bound_has_default(boundinfo);
+ /*
+ * XXX this line will cause is to match to the NULL partition for
+ * non-boolean cases too. That's a problem as int4 <> const will
+ * not match NULLs, so we'll scan the NULL partition needlessly for
+ * those cases. We'd need a way to check if we're handling bools
+ * here.
+ */
+ result->scan_null = partition_bound_accepts_nulls(boundinfo);
+
return result;
}
Attachments:
[text/plain] prune_on_bool_clause_experiements.patch (1.3K, ../../CAApHDvqriy8mPOFJ_Bd66YGXJ4+XULpv-4YdB+ePdCQFztyisA@mail.gmail.com/2-prune_on_bool_clause_experiements.patch)
download | inline diff:
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 6b635e8ad1..2b582a0e72 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1810,6 +1810,13 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
{
PartClauseInfo *partclause;
+ /*
+ * Boolean exprs in the form of IS NOT false, IS NOT true are only supported
+ * for LIST partitioned tables.
+ */
+ if (noteq && part_scheme->strategy != PARTITION_STRATEGY_LIST)
+ return PARTCLAUSE_UNSUPPORTED;
+
partclause = (PartClauseInfo *) palloc(sizeof(PartClauseInfo));
partclause->keyno = partkeyidx;
/* Do pruning with the Boolean equality operator. */
@@ -2757,6 +2764,15 @@ get_matching_list_bounds(PartitionPruneContext *context,
/* Always include the default partition if any. */
result->scan_default = partition_bound_has_default(boundinfo);
+ /*
+ * XXX this line will cause is to match to the NULL partition for
+ * non-boolean cases too. That's a problem as int4 <> const will
+ * not match NULLs, so we'll scan the NULL partition needlessly for
+ * those cases. We'd need a way to check if we're handling bools
+ * here.
+ */
+ result->scan_null = partition_bound_accepts_nulls(boundinfo);
+
return result;
}
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 12:32 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
@ 2024-02-18 12:53 ` David Rowley <dgrowleyml@gmail.com>
2024-02-18 16:24 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: David Rowley @ 2024-02-18 12:53 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlan@postgresql.org>; Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, 17 Feb 2024 at 01:32, David Rowley <dgrowleyml@gmail.com> wrote:
> I see three options:
>
> 1) Make get_matching_list_bounds() work for bool IS NOT clauses by
> properly including the NULL partition when handling a bool IS NOT
> clause.
> 2) Just don't generate a pruning step for bool IS NOT clauses.
> 3) Just always include the NULL partition in
> get_matching_list_bounds's "if (opstrategy == InvalidStrategy)" block.
It turns out there's a 4th, and much better option that allows this
just to work without any weirdness.
The method used in partprune.c to handle "partkey IN ('const1',
'const2')" is to transform that into "partkey = 'const1' OR partkey =
'const2'". Whenever we see a ScalarArrayOpExpr with consts, we just
form such an OR clause and recursively generate pruning steps for the
OR clause. That'll end up creating two pruning steps and combining
them with a PARTPRUNE_COMBINE_UNION.
We can do the same for BooleanTests. Given a clause such as: "partkey
IS NOT false", we can just generate the clause "partkey IS true OR
partkey IS NULL" and recursively generate steps for that.
I've attached a draft patch. I'll work on this more after I sleep.
I'm tempted to go a bit further in master only and add support for
bool IS NOT UNKNOWN and bool IS UNKNOWN using the same method.
David
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 6b635e8ad1..94cfe5167b 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1810,11 +1810,60 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
{
PartClauseInfo *partclause;
+ /*
+ * For bool tests in the form of partkey IS NOT true and IS NOT false,
+ * we invert these clauses. Effectively, "partkey IS NOT true" becomes
+ * "partkey IS false OR partkey IS NULL". We do this by building an OR
+ * BoolExpr and forming a clause just like that and punt it off to
+ * gen_partprune_steps_internal() to generate pruning steps.
+ */
+ if (noteq)
+ {
+ List *bool_clauses;
+ BooleanTest *new_booltest = copyObject(clause);
+ NullTest *nulltest;
+
+ /* We expect 'noteq' to only be set to true for BooleanTests */
+ Assert(IsA(clause, BooleanTest));
+
+ /* reverse the bool test */
+ if (new_booltest->booltesttype == IS_NOT_TRUE)
+ new_booltest->booltesttype = IS_FALSE;
+ else if (new_booltest->booltesttype == IS_NOT_FALSE)
+ new_booltest->booltesttype = IS_TRUE;
+ else
+ {
+ /*
+ * We only expect match_boolean_partition_clause to match for
+ * IS_NOT_TRUE and IS_NOT_FALSE. IS_NOT_UNKNOWN is not
+ * supported.
+ */
+ Assert(false);
+ }
+
+ nulltest = makeNode(NullTest);
+ nulltest->arg = copyObject(partkey);
+ nulltest->nulltesttype = IS_NULL;
+ nulltest->argisrow = false;
+ nulltest->location = -1;
+
+ bool_clauses = list_make1(makeBoolExpr(OR_EXPR, list_make2(new_booltest, nulltest), -1));
+
+ /* Finally, generate steps */
+ *clause_steps = gen_partprune_steps_internal(context, bool_clauses);
+
+ if (context->contradictory)
+ return PARTCLAUSE_MATCH_CONTRADICT; /* shouldn't happen */
+ else if (*clause_steps == NIL)
+ return PARTCLAUSE_UNSUPPORTED; /* step generation failed */
+ return PARTCLAUSE_MATCH_STEPS;
+ }
+
partclause = (PartClauseInfo *) palloc(sizeof(PartClauseInfo));
partclause->keyno = partkeyidx;
/* Do pruning with the Boolean equality operator. */
partclause->opno = BooleanEqualOperator;
- partclause->op_is_ne = noteq;
+ partclause->op_is_ne = false;
partclause->expr = expr;
/* We know that expr is of Boolean type. */
partclause->cmpfn = part_scheme->partsupfunc[partkeyidx].fn_oid;
@@ -2358,7 +2407,7 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
* For LIST and RANGE partitioned tables, callers must ensure that
* step_nullkeys is NULL, and that prefix contains at least one clause for
* each of the partition keys prior to the key that 'step_lastexpr' and
- * 'step_lastcmpfn'belong to.
+ * 'step_lastcmpfn' belong to.
*
* For HASH partitioned tables, callers must ensure that 'prefix' contains at
* least one clause for each of the partition keys apart from the final key
Attachments:
[text/plain] fix_partprune_BooleanTests_draft.patch (2.9K, ../../CAApHDvoFbt0fne1C_prK5j0MvxHccJD7NxUY0UDaTtAZuEyxrg@mail.gmail.com/2-fix_partprune_BooleanTests_draft.patch)
download | inline diff:
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 6b635e8ad1..94cfe5167b 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1810,11 +1810,60 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
{
PartClauseInfo *partclause;
+ /*
+ * For bool tests in the form of partkey IS NOT true and IS NOT false,
+ * we invert these clauses. Effectively, "partkey IS NOT true" becomes
+ * "partkey IS false OR partkey IS NULL". We do this by building an OR
+ * BoolExpr and forming a clause just like that and punt it off to
+ * gen_partprune_steps_internal() to generate pruning steps.
+ */
+ if (noteq)
+ {
+ List *bool_clauses;
+ BooleanTest *new_booltest = copyObject(clause);
+ NullTest *nulltest;
+
+ /* We expect 'noteq' to only be set to true for BooleanTests */
+ Assert(IsA(clause, BooleanTest));
+
+ /* reverse the bool test */
+ if (new_booltest->booltesttype == IS_NOT_TRUE)
+ new_booltest->booltesttype = IS_FALSE;
+ else if (new_booltest->booltesttype == IS_NOT_FALSE)
+ new_booltest->booltesttype = IS_TRUE;
+ else
+ {
+ /*
+ * We only expect match_boolean_partition_clause to match for
+ * IS_NOT_TRUE and IS_NOT_FALSE. IS_NOT_UNKNOWN is not
+ * supported.
+ */
+ Assert(false);
+ }
+
+ nulltest = makeNode(NullTest);
+ nulltest->arg = copyObject(partkey);
+ nulltest->nulltesttype = IS_NULL;
+ nulltest->argisrow = false;
+ nulltest->location = -1;
+
+ bool_clauses = list_make1(makeBoolExpr(OR_EXPR, list_make2(new_booltest, nulltest), -1));
+
+ /* Finally, generate steps */
+ *clause_steps = gen_partprune_steps_internal(context, bool_clauses);
+
+ if (context->contradictory)
+ return PARTCLAUSE_MATCH_CONTRADICT; /* shouldn't happen */
+ else if (*clause_steps == NIL)
+ return PARTCLAUSE_UNSUPPORTED; /* step generation failed */
+ return PARTCLAUSE_MATCH_STEPS;
+ }
+
partclause = (PartClauseInfo *) palloc(sizeof(PartClauseInfo));
partclause->keyno = partkeyidx;
/* Do pruning with the Boolean equality operator. */
partclause->opno = BooleanEqualOperator;
- partclause->op_is_ne = noteq;
+ partclause->op_is_ne = false;
partclause->expr = expr;
/* We know that expr is of Boolean type. */
partclause->cmpfn = part_scheme->partsupfunc[partkeyidx].fn_oid;
@@ -2358,7 +2407,7 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
* For LIST and RANGE partitioned tables, callers must ensure that
* step_nullkeys is NULL, and that prefix contains at least one clause for
* each of the partition keys prior to the key that 'step_lastexpr' and
- * 'step_lastcmpfn'belong to.
+ * 'step_lastcmpfn' belong to.
*
* For HASH partitioned tables, callers must ensure that 'prefix' contains at
* least one clause for each of the partition keys apart from the final key
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 12:32 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 12:53 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
@ 2024-02-18 16:24 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-02-18 23:49 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2024-02-18 16:24 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Amit Langote <amitlan@postgresql.org>; Alvaro Herrera <alvherre@alvh.no-ip.org>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
David Rowley <dgrowleyml@gmail.com> writes:
> We can do the same for BooleanTests. Given a clause such as: "partkey
> IS NOT false", we can just generate the clause "partkey IS true OR
> partkey IS NULL" and recursively generate steps for that.
+1 ... sounds clean and clearly correct.
> I'm tempted to go a bit further in master only and add support for
> bool IS NOT UNKNOWN and bool IS UNKNOWN using the same method.
These are the same as IS NOT NULL and IS NULL, so I don't see the
need for an OR?
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 12:32 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 12:53 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 16:24 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-02-18 23:49 ` David Rowley <dgrowleyml@gmail.com>
2024-02-19 08:21 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tender Wang <tndrwang@gmail.com>
2024-02-19 09:45 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Amit Langote <amitlangote09@gmail.com>
2024-02-20 03:00 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Alexander Lakhin <exclusion@gmail.com>
0 siblings, 3 replies; 12+ messages in thread
From: David Rowley @ 2024-02-18 23:49 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Langote <amitlan@postgresql.org>; Alvaro Herrera <alvherre@alvh.no-ip.org>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, 19 Feb 2024 at 05:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > We can do the same for BooleanTests. Given a clause such as: "partkey
> > IS NOT false", we can just generate the clause "partkey IS true OR
> > partkey IS NULL" and recursively generate steps for that.
>
> +1 ... sounds clean and clearly correct.
Here's a more complete patch for this. I included some tests for LIST
and RANGE partitioned tables. I did manual testing for HASH, and was
on the fence about covering that too.
I did try the following using the table from the tests:
select * from boolrangep where a is not true and not b and c = 25 and
a is not null;
When will be effectively transformed into:
select * from boolrangep where (a is false or a is null) and not b and
c = 25 and a is not null;
It seems that's unable to prune the NULL partition but that mostly
seems to be due to a limitation of the current design. I'm not sure
it's worth going to any additional trouble to make that work. It
seems a bit unlikely, especially so given how long the BooleanTest
pruning stuff was broken for before anyone noticed.
> > I'm tempted to go a bit further in master only and add support for
> > bool IS NOT UNKNOWN and bool IS UNKNOWN using the same method.
>
> These are the same as IS NOT NULL and IS NULL, so I don't see the
> need for an OR?
Uh, yeah. True. That makes it even more simple. Just use
PARTCLAUSE_MATCH_NULLNESS.
David
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 6b635e8ad1..0c445a9bbe 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1810,11 +1810,62 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
{
PartClauseInfo *partclause;
+ /*
+ * For bool tests in the form of partkey IS NOT true and IS NOT false,
+ * we invert these clauses. Effectively, "partkey IS NOT true" becomes
+ * "partkey IS false OR partkey IS NULL". We do this by building an OR
+ * BoolExpr and forming a clause just like that and punt it off to
+ * gen_partprune_steps_internal() to generate pruning steps.
+ */
+ if (noteq)
+ {
+ List *new_clauses;
+ List *or_clause;
+ BooleanTest *new_booltest = copyObject(clause);
+ NullTest *nulltest;
+
+ /* We expect 'noteq' to only be set to true for BooleanTests */
+ Assert(IsA(clause, BooleanTest));
+
+ /* reverse the bool test */
+ if (new_booltest->booltesttype == IS_NOT_TRUE)
+ new_booltest->booltesttype = IS_FALSE;
+ else if (new_booltest->booltesttype == IS_NOT_FALSE)
+ new_booltest->booltesttype = IS_TRUE;
+ else
+ {
+ /*
+ * We only expect match_boolean_partition_clause to match for
+ * IS_NOT_TRUE and IS_NOT_FALSE. IS_NOT_UNKNOWN is not
+ * supported.
+ */
+ Assert(false);
+ }
+
+ nulltest = makeNode(NullTest);
+ nulltest->arg = copyObject(partkey);
+ nulltest->nulltesttype = IS_NULL;
+ nulltest->argisrow = false;
+ nulltest->location = -1;
+
+ new_clauses = list_make2(new_booltest, nulltest);
+ or_clause = list_make1(makeBoolExpr(OR_EXPR, new_clauses, -1));
+
+ /* Finally, generate steps */
+ *clause_steps = gen_partprune_steps_internal(context, or_clause);
+
+ if (context->contradictory)
+ return PARTCLAUSE_MATCH_CONTRADICT; /* shouldn't happen */
+ else if (*clause_steps == NIL)
+ return PARTCLAUSE_UNSUPPORTED; /* step generation failed */
+ return PARTCLAUSE_MATCH_STEPS;
+ }
+
partclause = (PartClauseInfo *) palloc(sizeof(PartClauseInfo));
partclause->keyno = partkeyidx;
/* Do pruning with the Boolean equality operator. */
partclause->opno = BooleanEqualOperator;
- partclause->op_is_ne = noteq;
+ partclause->op_is_ne = false;
partclause->expr = expr;
/* We know that expr is of Boolean type. */
partclause->cmpfn = part_scheme->partsupfunc[partkeyidx].fn_oid;
@@ -2358,7 +2409,7 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
* For LIST and RANGE partitioned tables, callers must ensure that
* step_nullkeys is NULL, and that prefix contains at least one clause for
* each of the partition keys prior to the key that 'step_lastexpr' and
- * 'step_lastcmpfn'belong to.
+ * 'step_lastcmpfn' belong to.
*
* For HASH partitioned tables, callers must ensure that 'prefix' contains at
* least one clause for each of the partition keys apart from the final key
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index 9a4c48c055..b41950d923 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1169,6 +1169,57 @@ select * from boolpart where a is not unknown;
t
(2 rows)
+-- try some other permutations with a NULL partition instead of a DEFAULT
+delete from boolpart where a is null;
+create table boolpart_null partition of boolpart for values in (null);
+insert into boolpart values(null);
+explain (costs off) select * from boolpart where a is not true;
+ QUERY PLAN
+--------------------------------------------
+ Append
+ -> Seq Scan on boolpart_f boolpart_1
+ Filter: (a IS NOT TRUE)
+ -> Seq Scan on boolpart_null boolpart_2
+ 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
+--------------------------------------------------
+ Seq Scan on boolpart_null boolpart
+ Filter: ((a IS NOT TRUE) AND (a IS NOT FALSE))
+(2 rows)
+
+explain (costs off) select * from boolpart where a is not false;
+ QUERY PLAN
+--------------------------------------------
+ Append
+ -> Seq Scan on boolpart_t boolpart_1
+ Filter: (a IS NOT FALSE)
+ -> Seq Scan on boolpart_null boolpart_2
+ Filter: (a IS NOT FALSE)
+(5 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 not false;
+ a
+---
+ 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));
@@ -1315,11 +1366,37 @@ select * from iboolpart where a is not unknown;
f
(2 rows)
+-- Try some other permutations with a NULL partition instead of a DEFAULT
+delete from iboolpart where a is null;
+create table iboolpart_null partition of iboolpart for values in (null);
+insert into iboolpart values(null);
+-- Pruning shouldn't take place for these. Just check the result is correct
+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 not false;
+ a
+---
+ t
+
+(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);
create table boolrangep_ff1 partition of boolrangep for values from ('false', 'false', 0) to ('false', 'false', 50);
create table boolrangep_ff2 partition of boolrangep for values from ('false', 'false', 50) to ('false', 'false', 100);
+create table boolrangep_null partition of boolrangep default;
-- try a more complex case that's been known to trip up pruning in the past
explain (costs off) select * from boolrangep where not a and not b and c = 25;
QUERY PLAN
@@ -1328,6 +1405,32 @@ explain (costs off) select * from boolrangep where not a and not b and c = 25;
Filter: ((NOT a) AND (NOT b) AND (c = 25))
(2 rows)
+-- ensure we prune boolrangep_tf
+explain (costs off) select * from boolrangep where a is not true and not b and c = 25;
+ QUERY PLAN
+------------------------------------------------------------
+ Append
+ -> Seq Scan on boolrangep_ff1 boolrangep_1
+ Filter: ((a IS NOT TRUE) AND (NOT b) AND (c = 25))
+ -> Seq Scan on boolrangep_ff2 boolrangep_2
+ Filter: ((a IS NOT TRUE) AND (NOT b) AND (c = 25))
+ -> Seq Scan on boolrangep_ft boolrangep_3
+ Filter: ((a IS NOT TRUE) AND (NOT b) AND (c = 25))
+ -> Seq Scan on boolrangep_null boolrangep_4
+ Filter: ((a IS NOT TRUE) AND (NOT b) AND (c = 25))
+(9 rows)
+
+-- ensure we prune everything apart from boolrangep_tf and boolrangep_null
+explain (costs off) select * from boolrangep where a is not false and not b and c = 25;
+ QUERY PLAN
+-------------------------------------------------------------
+ Append
+ -> Seq Scan on boolrangep_tf boolrangep_1
+ Filter: ((a IS NOT FALSE) AND (NOT b) AND (c = 25))
+ -> Seq Scan on boolrangep_null boolrangep_2
+ Filter: ((a IS NOT FALSE) AND (NOT b) AND (c = 25))
+(5 rows)
+
-- test scalar-to-array operators
create table coercepart (a varchar) partition by list (a);
create table coercepart_ab partition of coercepart for values in ('ab');
diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql
index 7bf3920827..7ba6a9ff37 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -178,6 +178,19 @@ 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;
+-- try some other permutations with a NULL partition instead of a DEFAULT
+delete from boolpart where a is null;
+create table boolpart_null partition of boolpart for values in (null);
+insert into boolpart values(null);
+
+explain (costs off) select * from boolpart where a is not true;
+explain (costs off) select * from boolpart where a is not true and a is not false;
+explain (costs off) select * from boolpart where a is not false;
+
+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 not false;
+
-- 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));
@@ -204,15 +217,32 @@ 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;
+-- Try some other permutations with a NULL partition instead of a DEFAULT
+delete from iboolpart where a is null;
+create table iboolpart_null partition of iboolpart for values in (null);
+insert into iboolpart values(null);
+
+-- Pruning shouldn't take place for these. Just check the result is correct
+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 not false;
+
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);
create table boolrangep_ff1 partition of boolrangep for values from ('false', 'false', 0) to ('false', 'false', 50);
create table boolrangep_ff2 partition of boolrangep for values from ('false', 'false', 50) to ('false', 'false', 100);
+create table boolrangep_null partition of boolrangep default;
-- try a more complex case that's been known to trip up pruning in the past
explain (costs off) select * from boolrangep where not a and not b and c = 25;
+-- ensure we prune boolrangep_tf
+explain (costs off) select * from boolrangep where a is not true and not b and c = 25;
+
+-- ensure we prune everything apart from boolrangep_tf and boolrangep_null
+explain (costs off) select * from boolrangep where a is not false and not b and c = 25;
+
-- test scalar-to-array operators
create table coercepart (a varchar) partition by list (a);
create table coercepart_ab partition of coercepart for values in ('ab');
Attachments:
[text/plain] fix_partprune_BooleanTests.patch (10.7K, ../../CAApHDvpW6N38TerNpHuvXSZ3NHMpQr+G0R8fqvj1wD9ocCeRsA@mail.gmail.com/2-fix_partprune_BooleanTests.patch)
download | inline diff:
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 6b635e8ad1..0c445a9bbe 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1810,11 +1810,62 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
{
PartClauseInfo *partclause;
+ /*
+ * For bool tests in the form of partkey IS NOT true and IS NOT false,
+ * we invert these clauses. Effectively, "partkey IS NOT true" becomes
+ * "partkey IS false OR partkey IS NULL". We do this by building an OR
+ * BoolExpr and forming a clause just like that and punt it off to
+ * gen_partprune_steps_internal() to generate pruning steps.
+ */
+ if (noteq)
+ {
+ List *new_clauses;
+ List *or_clause;
+ BooleanTest *new_booltest = copyObject(clause);
+ NullTest *nulltest;
+
+ /* We expect 'noteq' to only be set to true for BooleanTests */
+ Assert(IsA(clause, BooleanTest));
+
+ /* reverse the bool test */
+ if (new_booltest->booltesttype == IS_NOT_TRUE)
+ new_booltest->booltesttype = IS_FALSE;
+ else if (new_booltest->booltesttype == IS_NOT_FALSE)
+ new_booltest->booltesttype = IS_TRUE;
+ else
+ {
+ /*
+ * We only expect match_boolean_partition_clause to match for
+ * IS_NOT_TRUE and IS_NOT_FALSE. IS_NOT_UNKNOWN is not
+ * supported.
+ */
+ Assert(false);
+ }
+
+ nulltest = makeNode(NullTest);
+ nulltest->arg = copyObject(partkey);
+ nulltest->nulltesttype = IS_NULL;
+ nulltest->argisrow = false;
+ nulltest->location = -1;
+
+ new_clauses = list_make2(new_booltest, nulltest);
+ or_clause = list_make1(makeBoolExpr(OR_EXPR, new_clauses, -1));
+
+ /* Finally, generate steps */
+ *clause_steps = gen_partprune_steps_internal(context, or_clause);
+
+ if (context->contradictory)
+ return PARTCLAUSE_MATCH_CONTRADICT; /* shouldn't happen */
+ else if (*clause_steps == NIL)
+ return PARTCLAUSE_UNSUPPORTED; /* step generation failed */
+ return PARTCLAUSE_MATCH_STEPS;
+ }
+
partclause = (PartClauseInfo *) palloc(sizeof(PartClauseInfo));
partclause->keyno = partkeyidx;
/* Do pruning with the Boolean equality operator. */
partclause->opno = BooleanEqualOperator;
- partclause->op_is_ne = noteq;
+ partclause->op_is_ne = false;
partclause->expr = expr;
/* We know that expr is of Boolean type. */
partclause->cmpfn = part_scheme->partsupfunc[partkeyidx].fn_oid;
@@ -2358,7 +2409,7 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
* For LIST and RANGE partitioned tables, callers must ensure that
* step_nullkeys is NULL, and that prefix contains at least one clause for
* each of the partition keys prior to the key that 'step_lastexpr' and
- * 'step_lastcmpfn'belong to.
+ * 'step_lastcmpfn' belong to.
*
* For HASH partitioned tables, callers must ensure that 'prefix' contains at
* least one clause for each of the partition keys apart from the final key
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index 9a4c48c055..b41950d923 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1169,6 +1169,57 @@ select * from boolpart where a is not unknown;
t
(2 rows)
+-- try some other permutations with a NULL partition instead of a DEFAULT
+delete from boolpart where a is null;
+create table boolpart_null partition of boolpart for values in (null);
+insert into boolpart values(null);
+explain (costs off) select * from boolpart where a is not true;
+ QUERY PLAN
+--------------------------------------------
+ Append
+ -> Seq Scan on boolpart_f boolpart_1
+ Filter: (a IS NOT TRUE)
+ -> Seq Scan on boolpart_null boolpart_2
+ 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
+--------------------------------------------------
+ Seq Scan on boolpart_null boolpart
+ Filter: ((a IS NOT TRUE) AND (a IS NOT FALSE))
+(2 rows)
+
+explain (costs off) select * from boolpart where a is not false;
+ QUERY PLAN
+--------------------------------------------
+ Append
+ -> Seq Scan on boolpart_t boolpart_1
+ Filter: (a IS NOT FALSE)
+ -> Seq Scan on boolpart_null boolpart_2
+ Filter: (a IS NOT FALSE)
+(5 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 not false;
+ a
+---
+ 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));
@@ -1315,11 +1366,37 @@ select * from iboolpart where a is not unknown;
f
(2 rows)
+-- Try some other permutations with a NULL partition instead of a DEFAULT
+delete from iboolpart where a is null;
+create table iboolpart_null partition of iboolpart for values in (null);
+insert into iboolpart values(null);
+-- Pruning shouldn't take place for these. Just check the result is correct
+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 not false;
+ a
+---
+ t
+
+(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);
create table boolrangep_ff1 partition of boolrangep for values from ('false', 'false', 0) to ('false', 'false', 50);
create table boolrangep_ff2 partition of boolrangep for values from ('false', 'false', 50) to ('false', 'false', 100);
+create table boolrangep_null partition of boolrangep default;
-- try a more complex case that's been known to trip up pruning in the past
explain (costs off) select * from boolrangep where not a and not b and c = 25;
QUERY PLAN
@@ -1328,6 +1405,32 @@ explain (costs off) select * from boolrangep where not a and not b and c = 25;
Filter: ((NOT a) AND (NOT b) AND (c = 25))
(2 rows)
+-- ensure we prune boolrangep_tf
+explain (costs off) select * from boolrangep where a is not true and not b and c = 25;
+ QUERY PLAN
+------------------------------------------------------------
+ Append
+ -> Seq Scan on boolrangep_ff1 boolrangep_1
+ Filter: ((a IS NOT TRUE) AND (NOT b) AND (c = 25))
+ -> Seq Scan on boolrangep_ff2 boolrangep_2
+ Filter: ((a IS NOT TRUE) AND (NOT b) AND (c = 25))
+ -> Seq Scan on boolrangep_ft boolrangep_3
+ Filter: ((a IS NOT TRUE) AND (NOT b) AND (c = 25))
+ -> Seq Scan on boolrangep_null boolrangep_4
+ Filter: ((a IS NOT TRUE) AND (NOT b) AND (c = 25))
+(9 rows)
+
+-- ensure we prune everything apart from boolrangep_tf and boolrangep_null
+explain (costs off) select * from boolrangep where a is not false and not b and c = 25;
+ QUERY PLAN
+-------------------------------------------------------------
+ Append
+ -> Seq Scan on boolrangep_tf boolrangep_1
+ Filter: ((a IS NOT FALSE) AND (NOT b) AND (c = 25))
+ -> Seq Scan on boolrangep_null boolrangep_2
+ Filter: ((a IS NOT FALSE) AND (NOT b) AND (c = 25))
+(5 rows)
+
-- test scalar-to-array operators
create table coercepart (a varchar) partition by list (a);
create table coercepart_ab partition of coercepart for values in ('ab');
diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql
index 7bf3920827..7ba6a9ff37 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -178,6 +178,19 @@ 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;
+-- try some other permutations with a NULL partition instead of a DEFAULT
+delete from boolpart where a is null;
+create table boolpart_null partition of boolpart for values in (null);
+insert into boolpart values(null);
+
+explain (costs off) select * from boolpart where a is not true;
+explain (costs off) select * from boolpart where a is not true and a is not false;
+explain (costs off) select * from boolpart where a is not false;
+
+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 not false;
+
-- 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));
@@ -204,15 +217,32 @@ 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;
+-- Try some other permutations with a NULL partition instead of a DEFAULT
+delete from iboolpart where a is null;
+create table iboolpart_null partition of iboolpart for values in (null);
+insert into iboolpart values(null);
+
+-- Pruning shouldn't take place for these. Just check the result is correct
+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 not false;
+
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);
create table boolrangep_ff1 partition of boolrangep for values from ('false', 'false', 0) to ('false', 'false', 50);
create table boolrangep_ff2 partition of boolrangep for values from ('false', 'false', 50) to ('false', 'false', 100);
+create table boolrangep_null partition of boolrangep default;
-- try a more complex case that's been known to trip up pruning in the past
explain (costs off) select * from boolrangep where not a and not b and c = 25;
+-- ensure we prune boolrangep_tf
+explain (costs off) select * from boolrangep where a is not true and not b and c = 25;
+
+-- ensure we prune everything apart from boolrangep_tf and boolrangep_null
+explain (costs off) select * from boolrangep where a is not false and not b and c = 25;
+
-- test scalar-to-array operators
create table coercepart (a varchar) partition by list (a);
create table coercepart_ab partition of coercepart for values in ('ab');
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 12:32 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 12:53 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 16:24 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-18 23:49 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
@ 2024-02-19 08:21 ` Tender Wang <tndrwang@gmail.com>
2 siblings, 0 replies; 12+ messages in thread
From: Tender Wang @ 2024-02-19 08:21 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlan@postgresql.org>; Alvaro Herrera <alvherre@alvh.no-ip.org>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
David Rowley <dgrowleyml@gmail.com> 于2024年2月19日周一 07:49写道：
> On Mon, 19 Feb 2024 at 05:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > David Rowley <dgrowleyml@gmail.com> writes:
> > > We can do the same for BooleanTests. Given a clause such as: "partkey
> > > IS NOT false", we can just generate the clause "partkey IS true OR
> > > partkey IS NULL" and recursively generate steps for that.
> >
> > +1 ... sounds clean and clearly correct.
>
> Here's a more complete patch for this. I included some tests for LIST
> and RANGE partitioned tables. I did manual testing for HASH, and was
> on the fence about covering that too.
>
> I did try the following using the table from the tests:
>
> select * from boolrangep where a is not true and not b and c = 25 and
> a is not null;
>
> When will be effectively transformed into:
>
> select * from boolrangep where (a is false or a is null) and not b and
> c = 25 and a is not null;
>
> It seems that's unable to prune the NULL partition but that mostly
> seems to be due to a limitation of the current design. I'm not sure
> it's worth going to any additional trouble to make that work. It
> seems a bit unlikely, especially so given how long the BooleanTest
> pruning stuff was broken for before anyone noticed.
>
> > > I'm tempted to go a bit further in master only and add support for
> > > bool IS NOT UNKNOWN and bool IS UNKNOWN using the same method.
> >
> > These are the same as IS NOT NULL and IS NULL, so I don't see the
> > need for an OR?
>
> Uh, yeah. True. That makes it even more simple. Just use
> PARTCLAUSE_MATCH_NULLNESS.
>
> David
>
After git apply fix_partprune_BooleanTests.patch on master, I got below
warnings:
partprune.c: In function ‘match_clause_to_partition_key’:
../../../src/include/nodes/nodes.h:221:25: warning: initialization of
‘BooleanTest *’ {aka ‘struct BooleanTest *’} from incompatible pointer type
‘Expr *’ {aka ‘struct Expr *’} [-Wincompatible-pointer-types]
221 | #define copyObject(obj) ((typeof(obj)) copyObjectImpl(obj))
| ^
partprune.c:1824:32: note: in expansion of macro ‘copyObject’
1824 | BooleanTest *new_booltest = copyObject(clause);
Maybe this: BooleanTest *new_booltest = (BooleanTest *) copyObject(clause);
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 12:32 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 12:53 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 16:24 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-18 23:49 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
@ 2024-02-19 09:45 ` Amit Langote <amitlangote09@gmail.com>
2 siblings, 0 replies; 12+ messages in thread
From: Amit Langote @ 2024-02-19 09:45 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlan@postgresql.org>; Alvaro Herrera <alvherre@alvh.no-ip.org>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Hi David,
On Mon, Feb 19, 2024 at 8:49 AM David Rowley <dgrowleyml@gmail.com> wrote:
> On Mon, 19 Feb 2024 at 05:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > David Rowley <dgrowleyml@gmail.com> writes:
> > > We can do the same for BooleanTests. Given a clause such as: "partkey
> > > IS NOT false", we can just generate the clause "partkey IS true OR
> > > partkey IS NULL" and recursively generate steps for that.
> >
> > +1 ... sounds clean and clearly correct.
>
> Here's a more complete patch for this.
Thanks for working on this.
Overall, I too like this idea.
Though I noticed that this approach will effectively disable pruning
with a clause on the 2nd key column, if any, present in the query:
CREATE TABLE t (b bool, i int) PARTITION BY RANGE (b, i);
CREATE TABLE tp PARTITION OF t FOR VALUES FROM (false, 0) TO (false, 1);
CREATE TABLE tp2 PARTITION OF t FOR VALUES FROM (false, 1) TO (false, 2);
CREATE TABLE tp3 PARTITION OF t FOR VALUES FROM (true, 0) TO (true, 1);
CREATE TABLE tp4 PARTITION OF t FOR VALUES FROM (true, 1) TO (true, 2);
-- tp2 should be pruned, but is not.
explain SELECT * FROM t WHERE b IS NOT true and i = 0;
QUERY PLAN
--------------------------------------------------------------
Append (cost=0.00..81.81 rows=12 width=5)
-> Seq Scan on tp t_1 (cost=0.00..40.88 rows=6 width=5)
Filter: ((b IS NOT TRUE) AND (i = 0))
-> Seq Scan on tp2 t_2 (cost=0.00..40.88 rows=6 width=5)
Filter: ((b IS NOT TRUE) AND (i = 0))
(5 rows)
-- like it is in this case
explain SELECT * FROM t WHERE b IS false and i = 0;
QUERY PLAN
-----------------------------------------------------
Seq Scan on tp t (cost=0.00..40.88 rows=6 width=5)
Filter: ((b IS FALSE) AND (i = 0))
(2 rows)
I guess we'll have to live with that, because the generate_opsteps
code that generates multi-column pruning steps only supports scenarios
where each key's matched clause is a simple comparison, not, for
example, where it is an OR expression.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 12:32 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 12:53 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 16:24 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-18 23:49 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
@ 2024-02-20 03:00 ` Alexander Lakhin <exclusion@gmail.com>
2024-02-20 03:50 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2 siblings, 1 reply; 12+ messages in thread
From: Alexander Lakhin @ 2024-02-20 03:00 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Langote <amitlan@postgresql.org>; Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-bugs@lists.postgresql.org
Hello David,
19.02.2024 02:49, David Rowley wrote:
>
> Here's a more complete patch for this. I included some tests for LIST
> and RANGE partitioned tables. I did manual testing for HASH, and was
> on the fence about covering that too.
>
Thank you for the fix!
Beside that, I'm a bit confused by the opstrategy description for
get_matching_range_bounds().
Above that function we have:
* 'opstrategy' if non-zero must be a btree strategy number.
But as we could see, zero opstrategy is not valid for the function (so
"if non-zero" is meaningless here?), unlike opstrategy for
get_matching_list_bounds(), which has the same description, but the latter
function contains:
/* Special case handling of values coming from a <> operator clause. */
if (opstrategy == InvalidStrategy)
...
Best regards,
Alexander
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 12:32 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 12:53 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 16:24 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-18 23:49 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-20 03:00 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Alexander Lakhin <exclusion@gmail.com>
@ 2024-02-20 03:50 ` David Rowley <dgrowleyml@gmail.com>
2024-02-20 05:39 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: David Rowley @ 2024-02-20 03:50 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlan@postgresql.org>; Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-bugs@lists.postgresql.org
On Tue, 20 Feb 2024 at 16:00, Alexander Lakhin <exclusion@gmail.com> wrote:
> Beside that, I'm a bit confused by the opstrategy description for
> get_matching_range_bounds().
> Above that function we have:
> * 'opstrategy' if non-zero must be a btree strategy number.
>
> But as we could see, zero opstrategy is not valid for the function (so
> "if non-zero" is meaningless here?), unlike opstrategy for
> get_matching_list_bounds(), which has the same description, but the latter
> function contains:
> /* Special case handling of values coming from a <> operator clause. */
> if (opstrategy == InvalidStrategy)
Yeah, that seems worth fixing in master as, I agree, the comment is
wrong. Possibly, we considered supporting <> for RANGE partitioning
at some point, I don't recall.
I was also working on a fix for what I mentioned in [1], which, I
think, is master-only material. I'd say we can fix the comment as
part of that.
The patch for both is attached.
David
[1] https://postgr.es/m/CAApHDvqriy8mPOFJ_Bd66YGXJ4+XULpv-4YdB+ePdCQFztyisA@mail.gmail.com
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index ccb9a579b3..a19f596d4d 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1936,9 +1936,11 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
* whatsoever, but their negators (equality) are. We can use one of
* those if we find it, but only for list partitioning.
*
- * Note: we report NOMATCH on failure, in case a later partkey has the
- * same expression but different opfamily. That's unlikely, but not
- * much more so than duplicate expressions with different collations.
+ * Note: we report NOMATCH on failure if the negator isn't the
+ * equality operator for the partkey's opfamily as other partkeys may
+ * have the same expression but different opfamily. That's unlikely,
+ * but not much more so than duplicate expressions with different
+ * collations.
*/
if (op_in_opfamily(opno, partopfamily))
{
@@ -1948,8 +1950,9 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
}
else
{
+ /* not supported for anything apart from LIST partitioned tables */
if (part_scheme->strategy != PARTITION_STRATEGY_LIST)
- return PARTCLAUSE_NOMATCH;
+ return PARTCLAUSE_UNSUPPORTED;
/* See if the negator is equality */
negator = get_negator(opno);
@@ -2924,7 +2927,7 @@ get_matching_list_bounds(PartitionPruneContext *context,
* multiple pruning steps might exclude it, so we infer its inclusion
* elsewhere.
*
- * 'opstrategy' if non-zero must be a btree strategy number.
+ * 'opstrategy' must be a btree strategy number.
*
* 'values' contains Datums indexed by the partition key to use for pruning.
*
Attachments:
[text/plain] minor_partpruning_fixes.patch (1.7K, ../../CAApHDvqcY53LqR4bH2Cz3XHjCc7ZbbX0UptBCMhuwqbN-e8SRA@mail.gmail.com/2-minor_partpruning_fixes.patch)
download | inline diff:
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index ccb9a579b3..a19f596d4d 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1936,9 +1936,11 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
* whatsoever, but their negators (equality) are. We can use one of
* those if we find it, but only for list partitioning.
*
- * Note: we report NOMATCH on failure, in case a later partkey has the
- * same expression but different opfamily. That's unlikely, but not
- * much more so than duplicate expressions with different collations.
+ * Note: we report NOMATCH on failure if the negator isn't the
+ * equality operator for the partkey's opfamily as other partkeys may
+ * have the same expression but different opfamily. That's unlikely,
+ * but not much more so than duplicate expressions with different
+ * collations.
*/
if (op_in_opfamily(opno, partopfamily))
{
@@ -1948,8 +1950,9 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
}
else
{
+ /* not supported for anything apart from LIST partitioned tables */
if (part_scheme->strategy != PARTITION_STRATEGY_LIST)
- return PARTCLAUSE_NOMATCH;
+ return PARTCLAUSE_UNSUPPORTED;
/* See if the negator is equality */
negator = get_negator(opno);
@@ -2924,7 +2927,7 @@ get_matching_list_bounds(PartitionPruneContext *context,
* multiple pruning steps might exclude it, so we infer its inclusion
* elsewhere.
*
- * 'opstrategy' if non-zero must be a btree strategy number.
+ * 'opstrategy' must be a btree strategy number.
*
* 'values' contains Datums indexed by the partition key to use for pruning.
*
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 12:32 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 12:53 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-18 16:24 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Tom Lane <tgl@sss.pgh.pa.us>
2024-02-18 23:49 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
2024-02-20 03:00 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy Alexander Lakhin <exclusion@gmail.com>
2024-02-20 03:50 ` Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy David Rowley <dgrowleyml@gmail.com>
@ 2024-02-20 05:39 ` David Rowley <dgrowleyml@gmail.com>
0 siblings, 0 replies; 12+ messages in thread
From: David Rowley @ 2024-02-20 05:39 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlan@postgresql.org>; Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-bugs@lists.postgresql.org
On Tue, 20 Feb 2024 at 16:50, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Tue, 20 Feb 2024 at 16:00, Alexander Lakhin <exclusion@gmail.com> wrote:
> > Beside that, I'm a bit confused by the opstrategy description for
> > get_matching_range_bounds().
> > Above that function we have:
> > * 'opstrategy' if non-zero must be a btree strategy number.
> >
> Yeah, that seems worth fixing in master as, I agree, the comment is
> wrong. Possibly, we considered supporting <> for RANGE partitioning
> at some point, I don't recall.
>
> I was also working on a fix for what I mentioned in [1], which, I
> think, is master-only material. I'd say we can fix the comment as
> part of that.
>
> The patch for both is attached.
I've pushed this patch.
David
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2024-02-20 05:39 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-02-15 09:00 BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy PG Bug reporting form <noreply@postgresql.org>
2024-02-15 16:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-02-16 02:56 ` David Rowley <dgrowleyml@gmail.com>
2024-02-16 12:32 ` David Rowley <dgrowleyml@gmail.com>
2024-02-18 12:53 ` David Rowley <dgrowleyml@gmail.com>
2024-02-18 16:24 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-02-18 23:49 ` David Rowley <dgrowleyml@gmail.com>
2024-02-19 08:21 ` Tender Wang <tndrwang@gmail.com>
2024-02-19 09:45 ` Amit Langote <amitlangote09@gmail.com>
2024-02-20 03:00 ` Alexander Lakhin <exclusion@gmail.com>
2024-02-20 03:50 ` David Rowley <dgrowleyml@gmail.com>
2024-02-20 05:39 ` David Rowley <dgrowleyml@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox