Horton

Thread

Commits

  1. Teach planner to account for HAVING quals in aggregation plan nodes.

  1. Account for cost and selectivity of HAVING quals

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-31T21:45:37Z

    Pursuant to the discussion at
https://www.postgresql.org/message-id/20171029112420.8920B5FB05@mx.zeyos.com
here's a patch to fix the planner so that eval costs and selectivity of
HAVING quals are factored into the appropriate plan node numbers.
Perhaps unsurprisingly, this doesn't change the results of any
existing regression tests.

It's slightly annoying that this approach will result in calculating
the eval costs and selectivity several times, once for each aggregation
plan type we consider.  I thought about inserting RestrictInfo nodes
into the havingQual so that those numbers could be cached, but that turned
out to break various code that doesn't expect to see such nodes there.
I'm not sure it's worth going to the trouble of fixing that; in the big
scheme of things, the redundant calculations likely don't cost much, since
we aren't going to have relevant statistics.

Comments?  If anyone wants to do a real review of this, I'm happy to stick
it into the upcoming CF; but without an expression of interest, I'll just
push it.  I don't think there's anything terribly controversial here.

			regards, tom lane

  2. Re: Account for cost and selectivity of HAVING quals

    Tels <nospam-pg-abuse@bloodgate.com> — 2017-10-31T23:31:29Z

    Moin,

On Tue, October 31, 2017 5:45 pm, Tom Lane wrote:
> Pursuant to the discussion at
> https://www.postgresql.org/message-id/20171029112420.8920B5FB05@mx.zeyos.com
> here's a patch to fix the planner so that eval costs and selectivity of
> HAVING quals are factored into the appropriate plan node numbers.
> Perhaps unsurprisingly, this doesn't change the results of any
> existing regression tests.
>
> It's slightly annoying that this approach will result in calculating
> the eval costs and selectivity several times, once for each aggregation
> plan type we consider.  I thought about inserting RestrictInfo nodes
> into the havingQual so that those numbers could be cached, but that turned
> out to break various code that doesn't expect to see such nodes there.
> I'm not sure it's worth going to the trouble of fixing that; in the big
> scheme of things, the redundant calculations likely don't cost much, since
> we aren't going to have relevant statistics.
>
> Comments?  If anyone wants to do a real review of this, I'm happy to stick
> it into the upcoming CF; but without an expression of interest, I'll just
> push it.  I don't think there's anything terribly controversial here.

Not a review, but the patch has this:


+                 total_cost += qual_cost.startup + output_tuples *
qual_cost.per_tuple;
+
+                 output_tuples = clamp_row_est(output_tuples *

...

That looks odd to me, it first uses output_tuples in a formula, then
overwrites the value with a new value. Should these lines be swapped?

Best regards,

Tels

  3. Re: Account for cost and selectivity of HAVING quals

    David G. Johnston <david.g.johnston@gmail.com> — 2017-10-31T23:54:08Z

    On Tue, Oct 31, 2017 at 4:31 PM, Tels <nospam-pg-abuse@bloodgate.com> wrote:

>
> ​​
> That looks odd to me, it first uses output_tuples in a formula, then
> overwrites the value with a new value. Should these lines be swapped?
>

​IIUC it is correct: the additional total_cost comes from processing every
output group to check whether it is qualified - since every group is
checked the incoming output_tuples from the prior grouping is used.  The
side-effect of the effort is that the number of output_tuples has now been
reduced to only those matching the qual - and so it now must take on a new
value to represent this.

David J.​

  4. Re: Account for cost and selectivity of HAVING quals

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-31T23:59:33Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Tue, Oct 31, 2017 at 4:31 PM, Tels <nospam-pg-abuse@bloodgate.com> wrote:
>> That looks odd to me, it first uses output_tuples in a formula, then
>> overwrites the value with a new value. Should these lines be swapped?

> ​IIUC it is correct: the additional total_cost comes from processing every
> output group to check whether it is qualified - since every group is
> checked the incoming output_tuples from the prior grouping is used.

Right --- we'll expend the effort to compute the HAVING expression once
per group row, whether the row passes the qual or not.

			regards, tom lane

  5. Re: Account for cost and selectivity of HAVING quals

    Tels <nospam-pg-abuse@bloodgate.com> — 2017-11-01T07:51:38Z

    Hello David,

On Tue, October 31, 2017 7:54 pm, David G. Johnston wrote:
> On Tue, Oct 31, 2017 at 4:31 PM, Tels <nospam-pg-abuse@bloodgate.com>
> wrote:
>
>>
>> ​​
>> That looks odd to me, it first uses output_tuples in a formula, then
>> overwrites the value with a new value. Should these lines be swapped?
>>
>
> ​IIUC it is correct: the additional total_cost comes from processing every
> output group to check whether it is qualified - since every group is
> checked the incoming output_tuples from the prior grouping is used.  The
> side-effect of the effort is that the number of output_tuples has now been
> reduced to only those matching the qual - and so it now must take on a new
> value to represent this.

Ah, makes sense. Learned something new today.

Maybe it's worth to add a comment, or would everybody else beside me
understand it easily by looking at the code? :)

Thank you,

Tels

  6. Re: Account for cost and selectivity of HAVING quals

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-11-01T12:36:55Z

    On Wed, Nov 1, 2017 at 3:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pursuant to the discussion at
> https://www.postgresql.org/message-id/20171029112420.8920B5FB05@mx.zeyos.com
> here's a patch to fix the planner so that eval costs and selectivity of
> HAVING quals are factored into the appropriate plan node numbers.
> Perhaps unsurprisingly, this doesn't change the results of any
> existing regression tests.
>
> It's slightly annoying that this approach will result in calculating
> the eval costs and selectivity several times, once for each aggregation
> plan type we consider.  I thought about inserting RestrictInfo nodes
> into the havingQual so that those numbers could be cached, but that turned
> out to break various code that doesn't expect to see such nodes there.
> I'm not sure it's worth going to the trouble of fixing that; in the big
> scheme of things, the redundant calculations likely don't cost much, since
> we aren't going to have relevant statistics.
>
> Comments?  If anyone wants to do a real review of this, I'm happy to stick
> it into the upcoming CF; but without an expression of interest, I'll just
> push it.  I don't think there's anything terribly controversial here.
>

I am not able to see how is the following hunk related to $subject
*************** create_result_path(PlannerInfo *root, Re
*** 1374,1379 ****
--- 1374,1380 ----
      pathnode->path.startup_cost = target->cost.startup;
      pathnode->path.total_cost = target->cost.startup +
          cpu_tuple_cost + target->cost.per_tuple;
+     /* Add cost of qual, if any --- but we ignore its selectivity */
      if (resconstantqual)
      {
          QualCost    qual_cost;

And may be we should try to explain why can we ignore selectivity.
Similarly for the changes in create_minmaxagg_path().

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  7. Re: Account for cost and selectivity of HAVING quals

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-11-01T15:11:54Z

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> On Wed, Nov 1, 2017 at 3:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> here's a patch to fix the planner so that eval costs and selectivity of
>> HAVING quals are factored into the appropriate plan node numbers.
>> ...
>> +     /* Add cost of qual, if any --- but we ignore its selectivity */

> And may be we should try to explain why can we ignore selectivity.
> Similarly for the changes in create_minmaxagg_path().

I'm sure you realize that's because the estimate is already just one
row ... but sure, we can spell that out.

			regards, tom lane

  8. Re: Account for cost and selectivity of HAVING quals

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-11-02T04:37:13Z

    On Wed, Nov 1, 2017 at 8:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>> On Wed, Nov 1, 2017 at 3:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> here's a patch to fix the planner so that eval costs and selectivity of
>>> HAVING quals are factored into the appropriate plan node numbers.
>>> ...
>>> +     /* Add cost of qual, if any --- but we ignore its selectivity */
>
>> And may be we should try to explain why can we ignore selectivity.
>> Similarly for the changes in create_minmaxagg_path().
>
> I'm sure you realize that's because the estimate is already just one
> row ... but sure, we can spell that out.
>

+1. That would be helpful.



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company