Wired if-statement in gen_partprune_steps_internal

Hi,

On Thu, Oct 8, 2020 at 6:56 PM Andy Fan <zhihui.fan1213@gmail.com> wrote:

Hi:

I found the following code in gen_partprune_steps_internal, which
looks the if-statement to be always true since list_length(results) > 1;
I added an Assert(step_ids != NIL) and all the test cases passed.
if the if-statement is always true, shall we remove it to avoid confusion?

gen_partprune_steps_internal(GeneratePruningStepsContext *context,

if (list_length(result) > 1)
{
List *step_ids = NIL;

foreach(lc, result)
{
PartitionPruneStep *step = lfirst(lc);

step_ids = lappend_int(step_ids, step->step_id);
}
Assert(step_ids != NIL);
if (step_ids != NIL) // This should always be true.
{
PartitionPruneStep *step;

step = gen_prune_step_combine(context, step_ids,
PARTPRUNE_COMBINE_INTERSECT);
result = lappend(result, step);
}
}

That seems fine to me.

Looking at this piece of code, I remembered that exactly the same
piece of logic is also present in gen_prune_steps_from_opexps(), which
looks like this:

/* Lastly, add a combine step to mutually AND these op steps, if needed */
if (list_length(opsteps) > 1)
{
List *opstep_ids = NIL;

foreach(lc, opsteps)
{
PartitionPruneStep *step = lfirst(lc);

opstep_ids = lappend_int(opstep_ids, step->step_id);
}

if (opstep_ids != NIL)
return gen_prune_step_combine(context, opstep_ids,
PARTPRUNE_COMBINE_INTERSECT);
return NULL;
}
else if (opsteps != NIL)
return linitial(opsteps);

I think we should remove this duplicative logic and return the
generated steps in a list from this function, which the code in
gen_partprune_steps_internal() then "combines" using an INTERSECT
step. See attached a patch to show what I mean.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangote09@gmail.com>
wrote:

Hi,

On Thu, Oct 8, 2020 at 6:56 PM Andy Fan <zhihui.fan1213@gmail.com> wrote:

Hi:

I found the following code in gen_partprune_steps_internal, which
looks the if-statement to be always true since list_length(results) > 1;
I added an Assert(step_ids != NIL) and all the test cases passed.
if the if-statement is always true, shall we remove it to avoid

confusion?

gen_partprune_steps_internal(GeneratePruningStepsContext *context,

if (list_length(result) > 1)
{
List *step_ids = NIL;

foreach(lc, result)
{
PartitionPruneStep *step = lfirst(lc);

step_ids = lappend_int(step_ids, step->step_id);
}
Assert(step_ids != NIL);
if (step_ids != NIL) // This should always be true.
{
PartitionPruneStep *step;

step = gen_prune_step_combine(context, step_ids,

PARTPRUNE_COMBINE_INTERSECT);

result = lappend(result, step);
}
}

That seems fine to me.

Looking at this piece of code, I remembered that exactly the same
piece of logic is also present in gen_prune_steps_from_opexps(), which
looks like this:

/* Lastly, add a combine step to mutually AND these op steps, if
needed */
if (list_length(opsteps) > 1)
{
List *opstep_ids = NIL;

foreach(lc, opsteps)
{
PartitionPruneStep *step = lfirst(lc);

opstep_ids = lappend_int(opstep_ids, step->step_id);
}

if (opstep_ids != NIL)
return gen_prune_step_combine(context, opstep_ids,
PARTPRUNE_COMBINE_INTERSECT);
return NULL;
}
else if (opsteps != NIL)
return linitial(opsteps);

I think we should remove this duplicative logic and return the
generated steps in a list from this function, which the code in
gen_partprune_steps_internal() then "combines" using an INTERSECT
step. See attached a patch to show what I mean.

This changes LGTM, and "make check" PASSED, thanks for the patch!

--
Best Regards
Andy Fan

At Wed, 14 Oct 2020 11:26:33 +0800, Andy Fan <zhihui.fan1213@gmail.com> wrote in

On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangote09@gmail.com>
wrote:

I think we should remove this duplicative logic and return the
generated steps in a list from this function, which the code in
gen_partprune_steps_internal() then "combines" using an INTERSECT
step. See attached a patch to show what I mean.

This changes LGTM, and "make check" PASSED, thanks for the patch!

FWIW, both looks fine to me.

By the way, I guess that some of the caller sites of
gen_prune_step_combine(PARTPRUNE_COMBINE_INTERSECT) is useless if we
do that later?

(Diff1 below)

Mmm. I was wrong. *All the other caller site* than that at the end of
gen_partprune_steps_internal is useless?

(Note: The Diff1 alone leads to assertion failure at partprune.c:945@master.
See below.)

By the way, I'm confused to see the following portion in
gen_partprune_steps_internal.

/*
* Finally, results from all entries appearing in result should be
* combined using an INTERSECT combine step, if more than one.
*/
if (list_length(result) > 1)

...

step = gen_prune_step_combine(context, step_ids,
PARTPRUNE_COMBINE_INTERSECT);
result = lappend(result, step);

The result contains both the source terms and the combined term. If I
understand it correctly, we should replace the source terms with
combined one. (With this change the assertion above doesn't fire and
passes all regression tests.)

=====
@@ -1180,13 +1163,9 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
}

 		if (step_ids != NIL)
-		{
-			PartitionPruneStep *step;
-
-			step = gen_prune_step_combine(context, step_ids,
-										  PARTPRUNE_COMBINE_INTERSECT);
-			result = lappend(result, step);
-		}
+			result =
+				list_make1(gen_prune_step_combine(context, step_ids,
+												  PARTPRUNE_COMBINE_INTERSECT));
 	}

return result;
=====

regards.

Diff1
======
@@ -983,9 +983,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 			else if (is_andclause(clause))
 			{
 				List	   *args = ((BoolExpr *) clause)->args;
-				List	   *argsteps,
-						   *arg_stepids = NIL;
-				ListCell   *lc1;
+				List	   *argsteps;

/*
* args may itself contain clauses of arbitrary type, so just
@@ -998,21 +996,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
if (context->contradictory)
return NIL;

-				foreach(lc1, argsteps)
-				{
-					PartitionPruneStep *step = lfirst(lc1);
-
-					arg_stepids = lappend_int(arg_stepids, step->step_id);
-				}
-
-				if (arg_stepids != NIL)
-				{
-					PartitionPruneStep *step;
-
-					step = gen_prune_step_combine(context, arg_stepids,
-												  PARTPRUNE_COMBINE_INTERSECT);
-					result = lappend(result, step);
-				}
+				result = list_concat(result, argsteps);
 				continue;
 			}
==== 

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Oct 14, 2020 at 11:26 AM Andy Fan <zhihui.fan1213@gmail.com> wrote:

On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangote09@gmail.com>
wrote:

Hi,

On Thu, Oct 8, 2020 at 6:56 PM Andy Fan <zhihui.fan1213@gmail.com> wrote:

Hi:

I found the following code in gen_partprune_steps_internal, which
looks the if-statement to be always true since list_length(results) > 1;
I added an Assert(step_ids != NIL) and all the test cases passed.
if the if-statement is always true, shall we remove it to avoid

confusion?

gen_partprune_steps_internal(GeneratePruningStepsContext *context,

if (list_length(result) > 1)
{
List *step_ids = NIL;

foreach(lc, result)
{
PartitionPruneStep *step = lfirst(lc);

step_ids = lappend_int(step_ids, step->step_id);
}
Assert(step_ids != NIL);
if (step_ids != NIL) // This should always be true.
{
PartitionPruneStep *step;

step = gen_prune_step_combine(context, step_ids,

PARTPRUNE_COMBINE_INTERSECT);

result = lappend(result, step);
}
}

That seems fine to me.

Looking at this piece of code, I remembered that exactly the same
piece of logic is also present in gen_prune_steps_from_opexps(), which
looks like this:

/* Lastly, add a combine step to mutually AND these op steps, if
needed */
if (list_length(opsteps) > 1)
{
List *opstep_ids = NIL;

foreach(lc, opsteps)
{
PartitionPruneStep *step = lfirst(lc);

opstep_ids = lappend_int(opstep_ids, step->step_id);
}

if (opstep_ids != NIL)
return gen_prune_step_combine(context, opstep_ids,
PARTPRUNE_COMBINE_INTERSECT);
return NULL;
}
else if (opsteps != NIL)
return linitial(opsteps);

I think we should remove this duplicative logic and return the
generated steps in a list from this function, which the code in
gen_partprune_steps_internal() then "combines" using an INTERSECT
step. See attached a patch to show what I mean.

This changes LGTM, and "make check" PASSED, thanks for the patch!

I created https://commitfest.postgresql.org/30/2771/ so that this patch
will not
be lost. Thanks!

--
Best Regards
Andy Fan

Hi Andy,

On Tue, Oct 20, 2020 at 4:05 PM Andy Fan <zhihui.fan1213@gmail.com> wrote:

On Wed, Oct 14, 2020 at 11:26 AM Andy Fan <zhihui.fan1213@gmail.com> wrote:

On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangote09@gmail.com> wrote:

I think we should remove this duplicative logic and return the
generated steps in a list from this function, which the code in
gen_partprune_steps_internal() then "combines" using an INTERSECT
step. See attached a patch to show what I mean.

This changes LGTM, and "make check" PASSED, thanks for the patch!

I created https://commitfest.postgresql.org/30/2771/ so that this patch will not
be lost. Thanks!

Thanks for doing that.

I had updated the patch last week to address Horiguchi-san's comments
but didn't manage to post a polished-enough version. I will try again
this week.

--
Amit Langote
EDB: http://www.enterprisedb.com

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

The original patch still applies and passes make installcheck-world. An updated patch was mentioned but has not been attached. Updating status to Waiting on Author.

Cheers,

-- Ryan Lambert

The new status of this patch is: Waiting on Author

On Tue, Oct 20, 2020 at 9:46 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Oct 20, 2020 at 4:05 PM Andy Fan <zhihui.fan1213@gmail.com> wrote:

On Wed, Oct 14, 2020 at 11:26 AM Andy Fan <zhihui.fan1213@gmail.com> wrote:

On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangote09@gmail.com> wrote:

I think we should remove this duplicative logic and return the
generated steps in a list from this function, which the code in
gen_partprune_steps_internal() then "combines" using an INTERSECT
step. See attached a patch to show what I mean.

This changes LGTM, and "make check" PASSED, thanks for the patch!

I created https://commitfest.postgresql.org/30/2771/ so that this patch will not
be lost. Thanks!

Thanks for doing that.

I had updated the patch last week to address Horiguchi-san's comments
but didn't manage to post a polished-enough version. I will try again
this week.

Sorry, this seems to have totally slipped my mind.

Attached is the patch I had promised.

Also, I have updated the title of the CF entry to "Some cosmetic
improvements of partition pruning code", which I think is more
appropriate.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Wed, Mar 3, 2021 at 11:03 PM Amit Langote <amitlangote09@gmail.com>
wrote:

Sorry, this seems to have totally slipped my mind.

Attached is the patch I had promised.

Also, I have updated the title of the CF entry to "Some cosmetic
improvements of partition pruning code", which I think is more
appropriate.

--
Amit Langote
EDB: http://www.enterprisedb.com

Thank you. The updated patch passes installcheck-world. I ran a handful
of test queries with a small number of partitions and observed the same
plans before and after the patch. I cannot speak to the quality of the
code, though am happy to test any additional use cases that should be
verified.

Ryan Lambert

On Fri, Mar 5, 2021 at 7:50 AM Ryan Lambert <ryan@rustprooflabs.com> wrote:

On Wed, Mar 3, 2021 at 11:03 PM Amit Langote <amitlangote09@gmail.com> wrote:

Sorry, this seems to have totally slipped my mind.

Attached is the patch I had promised.

Also, I have updated the title of the CF entry to "Some cosmetic
improvements of partition pruning code", which I think is more
appropriate.

Thank you. The updated patch passes installcheck-world. I ran a handful of test queries with a small number of partitions and observed the same plans before and after the patch. I cannot speak to the quality of the code, though am happy to test any additional use cases that should be verified.

Thanks Ryan.

There's no need to test it extensively, because no functionality is
changed with this patch.

--
Amit Langote
EDB: http://www.enterprisedb.com

Should the status of this patch be updated to ready for comitter to get in
line for Pg 14 deadline?

*Ryan Lambert*

On Sun, Mar 7, 2021 at 11:38 PM Amit Langote <amitlangote09@gmail.com>
wrote:

Hi Ryan,

On Tue, Mar 23, 2021 at 2:24 AM Ryan Lambert <ryan@rustprooflabs.com> wrote:

Should the status of this patch be updated to ready for comitter to get in line for Pg 14 deadline?

Yes, I've done that. Thanks for the reminder.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Thu, 4 Mar 2021 at 19:03, Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Oct 20, 2020 at 9:46 PM Amit Langote <amitlangote09@gmail.com> wrote:

I had updated the patch last week to address Horiguchi-san's comments
but didn't manage to post a polished-enough version. I will try again
this week.

Sorry, this seems to have totally slipped my mind.

Attached is the patch I had promised.

I've been looking at this patch today and spent quite a bit of time
staring at the following fragment:

  case PARTCLAUSE_MATCH_STEPS:
- Assert(clause_steps != NIL);
- result = list_concat(result, clause_steps);
+ Assert(clause_step != NULL);
+ steps = lappend(steps, clause_step);
  break;

So here, we used to use list_concat to add the steps that
match_clause_to_partition_key() output, but now we lappend() the
single step that match_clause_to_partition_key set in its output arg.

This appears to be ok as we only return PARTCLAUSE_MATCH_STEPS from
match_clause_to_partition_key() when we process a ScalarArrayOpExpr.
There we just transform the IN(<list of consts>) into a Boolean OR
clause with a set of OpExprs which are equivalent to the
ScalarArrayOpExpr. e.g. "a IN (1,2)" becomes "a = 1 OR a = 2". The
code path which processes the list of OR clauses in
gen_partprune_steps_internal() will always just output a single
PARTPRUNE_COMBINE_UNION combine step. So it does not appear that there
are any behavioural changes there. The list_concat would always have
been just adding a single item to the list before anyway.

However, it does change the meaning of what PARTCLAUSE_MATCH_STEPS
does. If we ever needed to expand what PARTCLAUSE_MATCH_STEPS does,
then we'll have less flexibility with the newly updated code. For
example if we needed to return multiple steps and only combine them at
the top level then we now can't. I feel there's a good possibility
that we'll never need to do that, but I'm not certain of that.

I'm keen to hear your opinion on this.

David

On Wed, Apr 7, 2021 at 4:43 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 4 Mar 2021 at 19:03, Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Oct 20, 2020 at 9:46 PM Amit Langote <amitlangote09@gmail.com> wrote:

I had updated the patch last week to address Horiguchi-san's comments
but didn't manage to post a polished-enough version. I will try again
this week.

Sorry, this seems to have totally slipped my mind.

Attached is the patch I had promised.

I've been looking at this patch today and spent quite a bit of time
staring at the following fragment:

Thanks a lot for looking at this.

case PARTCLAUSE_MATCH_STEPS:
- Assert(clause_steps != NIL);
- result = list_concat(result, clause_steps);
+ Assert(clause_step != NULL);
+ steps = lappend(steps, clause_step);
break;

So here, we used to use list_concat to add the steps that
match_clause_to_partition_key() output, but now we lappend() the
single step that match_clause_to_partition_key set in its output arg.

This appears to be ok as we only return PARTCLAUSE_MATCH_STEPS from
match_clause_to_partition_key() when we process a ScalarArrayOpExpr.
There we just transform the IN(<list of consts>) into a Boolean OR
clause with a set of OpExprs which are equivalent to the
ScalarArrayOpExpr. e.g. "a IN (1,2)" becomes "a = 1 OR a = 2". The
code path which processes the list of OR clauses in
gen_partprune_steps_internal() will always just output a single
PARTPRUNE_COMBINE_UNION combine step. So it does not appear that there
are any behavioural changes there. The list_concat would always have
been just adding a single item to the list before anyway.

Right, that was my observation as well.

However, it does change the meaning of what PARTCLAUSE_MATCH_STEPS
does. If we ever needed to expand what PARTCLAUSE_MATCH_STEPS does,
then we'll have less flexibility with the newly updated code. For
example if we needed to return multiple steps and only combine them at
the top level then we now can't. I feel there's a good possibility
that we'll never need to do that, but I'm not certain of that.

I'm keen to hear your opinion on this.

That's a good point. So maybe gen_partprune_steps_internal() should
continue to return a list of steps, the last of which would be an
intersect step to combine the results of the earlier multiple steps.
We should still fix the originally reported issue that
gen_prune_steps_from_opexps() seems to needlessly add an intersect
step.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Wed, 7 Apr 2021 at 21:04, Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Apr 7, 2021 at 4:43 PM David Rowley <dgrowleyml@gmail.com> wrote:

However, it does change the meaning of what PARTCLAUSE_MATCH_STEPS
does. If we ever needed to expand what PARTCLAUSE_MATCH_STEPS does,
then we'll have less flexibility with the newly updated code. For
example if we needed to return multiple steps and only combine them at
the top level then we now can't. I feel there's a good possibility
that we'll never need to do that, but I'm not certain of that.

I'm keen to hear your opinion on this.

That's a good point. So maybe gen_partprune_steps_internal() should
continue to return a list of steps, the last of which would be an
intersect step to combine the results of the earlier multiple steps.
We should still fix the originally reported issue that
gen_prune_steps_from_opexps() seems to needlessly add an intersect
step.

I was hoping you'd just say that we'll likely not need to do that and
if we ever did we could adapt the code at that time. :)

Thinking more about it, these steps we're talking about are generated
from a recursive call to gen_partprune_steps_internal(). I'm finding
it very hard to imagine that we'd want to combine steps generated in
some recursive call with steps from outside that same call. Right now
we recuse into AND BoolExprs OR BoolExprs. I'm struggling to think of
why we'd want to combine a set of steps we generated processing some
of those with steps from outside that BoolExpr. If we did, we might
want to consider teaching canonicalize_qual() to fix it beforehand.

e.g.

postgres=# explain select * from ab where (a = 1 and b = 1) or (a = 1
and b = 2);
QUERY PLAN
---------------------------------------------------
Seq Scan on ab (cost=0.00..49.55 rows=1 width=8)
Filter: ((a = 1) AND ((b = 1) OR (b = 2)))
(2 rows)

If canonicalize_qual() had been unable to rewrite that WHERE clause
then I could see that we might want to combine steps from other
recursive quals. I'm thinking right now that I'm glad
canonicalize_qual() does that hard work for us. (I think partprune.c
could handle the original WHERE clause as-is in this example
anyway...)

David

On Wed, 7 Apr 2021 at 21:53, David Rowley <dgrowleyml@gmail.com> wrote:

If canonicalize_qual() had been unable to rewrite that WHERE clause
then I could see that we might want to combine steps from other
recursive quals. I'm thinking right now that I'm glad
canonicalize_qual() does that hard work for us. (I think partprune.c
could handle the original WHERE clause as-is in this example
anyway...)

I made a pass over the v2 patch and since it's been a long time since
I'd looked at partprune.c I ended doing further rewriting of the
comments you'd changed.

There's only one small code change as I didn't like the following:

- return result;
+ /* A single step or no pruning possible with the provided clauses. */
+ return steps ? linitial(steps) : NULL;

I ended up breaking that out into an if condition.

All the other changes are around the comments.

Can you look over this and let me know if you're happy with the changes?

David

On Wed, Apr 7, 2021 at 8:44 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Wed, 7 Apr 2021 at 21:53, David Rowley <dgrowleyml@gmail.com> wrote:

If canonicalize_qual() had been unable to rewrite that WHERE clause
then I could see that we might want to combine steps from other
recursive quals. I'm thinking right now that I'm glad
canonicalize_qual() does that hard work for us. (I think partprune.c
could handle the original WHERE clause as-is in this example
anyway...)

I made a pass over the v2 patch and since it's been a long time since
I'd looked at partprune.c I ended doing further rewriting of the
comments you'd changed.

There's only one small code change as I didn't like the following:

- return result;
+ /* A single step or no pruning possible with the provided clauses. */
+ return steps ? linitial(steps) : NULL;

I ended up breaking that out into an if condition.

All the other changes are around the comments.

Can you look over this and let me know if you're happy with the changes?

Thanks David. Actually, I was busy updating the patch to revert to
gen_partprune_steps_internal() returning a list and was almost done
with it when I saw your message.

I read through v3 and can say that it certainly looks better than v2.
If you are happy with gen_partprune_steps_internal() no longer
returning a list, I would not object if you wanted to go ahead and
commit the v3.

I've attached the patch I had ended up with and was about to post as
v3, just in case you wanted to glance.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Wed, Apr 7, 2021 at 6:53 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Wed, 7 Apr 2021 at 21:04, Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Apr 7, 2021 at 4:43 PM David Rowley <dgrowleyml@gmail.com> wrote:

However, it does change the meaning of what PARTCLAUSE_MATCH_STEPS
does. If we ever needed to expand what PARTCLAUSE_MATCH_STEPS does,
then we'll have less flexibility with the newly updated code. For
example if we needed to return multiple steps and only combine them at
the top level then we now can't. I feel there's a good possibility
that we'll never need to do that, but I'm not certain of that.

I'm keen to hear your opinion on this.

That's a good point. So maybe gen_partprune_steps_internal() should
continue to return a list of steps, the last of which would be an
intersect step to combine the results of the earlier multiple steps.
We should still fix the originally reported issue that
gen_prune_steps_from_opexps() seems to needlessly add an intersect
step.

I was hoping you'd just say that we'll likely not need to do that and
if we ever did we could adapt the code at that time. :)

Thinking more about it, these steps we're talking about are generated
from a recursive call to gen_partprune_steps_internal(). I'm finding
it very hard to imagine that we'd want to combine steps generated in
some recursive call with steps from outside that same call. Right now
we recuse into AND BoolExprs OR BoolExprs. I'm struggling to think of
why we'd want to combine a set of steps we generated processing some
of those with steps from outside that BoolExpr. If we did, we might
want to consider teaching canonicalize_qual() to fix it beforehand.

e.g.

postgres=# explain select * from ab where (a = 1 and b = 1) or (a = 1
and b = 2);
QUERY PLAN
---------------------------------------------------
Seq Scan on ab (cost=0.00..49.55 rows=1 width=8)
Filter: ((a = 1) AND ((b = 1) OR (b = 2)))
(2 rows)

If canonicalize_qual() had been unable to rewrite that WHERE clause
then I could see that we might want to combine steps from other
recursive quals. I'm thinking right now that I'm glad
canonicalize_qual() does that hard work for us.
(I think partprune.c
could handle the original WHERE clause as-is in this example
anyway...)

Actually, I am not sure that canonicalization always makes things
better for partprune.c. I can show examples where canonicalization
causes partprune.c as it is today to not be able to prune as optimally
as it could have with the original ones.

create table ab (a int, b int) partition by range (a, b);
create table ab0 partition of ab for values from (1, 1) to (1, 2);
create table ab1 partition of ab for values from (1, 2) to (1, 3);
create table ab2 partition of ab for values from (1, 3) to (1, 4);
create table ab3 partition of ab for values from (2, 1) to (2, 2);

explain select * from ab where (a = 1 and b = 1) or (a = 1 and b = 2);
QUERY PLAN
---------------------------------------------------------------
Append (cost=0.00..148.66 rows=3 width=8)
-> Seq Scan on ab0 ab_1 (cost=0.00..49.55 rows=1 width=8)
Filter: ((a = 1) AND ((b = 1) OR (b = 2)))
-> Seq Scan on ab1 ab_2 (cost=0.00..49.55 rows=1 width=8)
Filter: ((a = 1) AND ((b = 1) OR (b = 2)))
-> Seq Scan on ab2 ab_3 (cost=0.00..49.55 rows=1 width=8)
Filter: ((a = 1) AND ((b = 1) OR (b = 2)))
(7 rows)

explain select * from ab where (a = 1 and b = 1) or (a = 1 and b = 3);
QUERY PLAN
---------------------------------------------------------------
Append (cost=0.00..148.66 rows=3 width=8)
-> Seq Scan on ab0 ab_1 (cost=0.00..49.55 rows=1 width=8)
Filter: ((a = 1) AND ((b = 1) OR (b = 3)))
-> Seq Scan on ab1 ab_2 (cost=0.00..49.55 rows=1 width=8)
Filter: ((a = 1) AND ((b = 1) OR (b = 3)))
-> Seq Scan on ab2 ab_3 (cost=0.00..49.55 rows=1 width=8)
Filter: ((a = 1) AND ((b = 1) OR (b = 3)))
(7 rows)

I would've expected the 1st query to scan ab0 and ab1, whereas the 2nd
query to scan ab0 and ab2. But in the canonicalized version, the
AND's 2nd arm is useless for multi-column range pruning, because it
only provides clauses for the 2nd key. With the original version,
both arms of the OR have ANDed clauses covering both keys, so pruning
with that would have produced the desired result.

So, if I am not entirely wrong, maybe it is exactly because of
canonicalization that partprune.c should be looking to peek across
BoolExprs.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Thu, 8 Apr 2021 at 00:49, Amit Langote <amitlangote09@gmail.com> wrote:

Thanks David. Actually, I was busy updating the patch to revert to
gen_partprune_steps_internal() returning a list and was almost done
with it when I saw your message.

I read through v3 and can say that it certainly looks better than v2.
If you are happy with gen_partprune_steps_internal() no longer
returning a list, I would not object if you wanted to go ahead and
commit the v3.

I've attached the patch I had ended up with and was about to post as
v3, just in case you wanted to glance.

Thanks. I've made a pass over that and just fixed up the places that
were mixing up NIL and NULL.

I applied most of my comments from my last version after adapting them
to account for the variation in the functions return value. I also did
a bit more explaining about op steps and combine steps in the header
comment for gen_partprune_steps_internal.

Patch attached.

David

On Thu, Apr 8, 2021 at 5:34 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 8 Apr 2021 at 00:49, Amit Langote <amitlangote09@gmail.com> wrote:

Thanks David. Actually, I was busy updating the patch to revert to
gen_partprune_steps_internal() returning a list and was almost done
with it when I saw your message.

I read through v3 and can say that it certainly looks better than v2.
If you are happy with gen_partprune_steps_internal() no longer
returning a list, I would not object if you wanted to go ahead and
commit the v3.

I've attached the patch I had ended up with and was about to post as
v3, just in case you wanted to glance.

Thanks. I've made a pass over that and just fixed up the places that
were mixing up NIL and NULL.

I applied most of my comments from my last version after adapting them
to account for the variation in the functions return value. I also did
a bit more explaining about op steps and combine steps in the header
comment for gen_partprune_steps_internal.

Thanks for updating the patch.

+ * These partition pruning steps come in 2 forms; operation steps and combine
+ * steps.

Maybe you meant "operator" steps? IIRC, the reason why we named it
PartitionPruneStepOp is that an op step is built to prune based on the
semantics of the operators that were involved in the matched clause.
Although, they're abused for pruning based on nullness clauses too.
Maybe, we should also updated the description of node struct as
follows to consider that last point:

* PartitionPruneStepOp - Information to prune using a set of mutually ANDed
* OpExpr and any IS [ NOT ] NULL clauses

+ * Combine steps (PartitionPruneStepCombine) instruct the partition pruning
+ * code how it should produce a single set of partitions from multiple input
+ * operation steps.

I think the last part should be: ...from multiple operation/operator
and [ other ] combine steps.

If that sounds fine, likewise adjust the following sentences in the
same paragraph.

--
Amit Langote
EDB: http://www.enterprisedb.com