Обсуждение: 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

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

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

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

On Thu, 8 Apr 2021 at 21:04, Amit Langote <amitlangote09@gmail.com> wrote:
> + * 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:

Oh right. Thanks. I fixed that.

>  * 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 didn't add that. I wasn't really sure if I understood why we'd talk
about PartitionPruneStepCombine in the PartitionPruneStepOp. I thought
the overview in gen_partprune_steps_internal was ok to link the two
together and explain why they're both needed.

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

Change that and pushed.

David

On Thu, Apr 8, 2021 at 7:41 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Thu, 8 Apr 2021 at 21:04, Amit Langote <amitlangote09@gmail.com> wrote:
> > 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
>
> I didn't add that. I wasn't really sure if I understood why we'd talk
> about PartitionPruneStepCombine in the PartitionPruneStepOp. I thought
> the overview in gen_partprune_steps_internal was ok to link the two
> together and explain why they're both needed.

Sorry, maybe the way I wrote it was a bit confusing, but I meant to
suggest that we do what I have quoted above from my last email.  That
is, we should clarify in the description of PartitionPruneStepOp that
it contains information derived from OpExprs and in some cases also IS
[ NOT ] NULL clauses.

Thanks for the commit.

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