Horton

Thread

Commits

  1. Estimate cost of elided SubqueryScan, Append, MergeAppend nodes better.

  2. Wrap overly long lines

  1. Costing elided SubqueryScans more nearly correctly

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-04T22:32:00Z

    In [1] I complained about how SubqueryScans that get deleted from
a plan tree by setrefs.c nonetheless contribute cost increments
that might cause the planner to make odd choices.  That turned
out not to be the proximate cause of that particular issue, but
it still seems like it might be a good idea to do something about
it.  Here's a little patch to improve matters.

It turns out to be hard to predict perfectly whether setrefs.c will
remove a SubqueryScan, because createplan.c plays some games with
moving tlist evaluations around and/or inserting "physical"
(i.e. trivial) tlists, which can falsify any earlier estimate of
whether a SubqueryScan is trivial.  I'm not especially interested in
redesigning those mechanisms, so the best we can hope for is an
approximate determination.  (Those same behaviors also make a lot of
other path cost estimates a bit incorrect, so it doesn't seem like
we need to feel too awful about not getting SubqueryScan perfect.)

Given that ground rule, however, it's not very hard to determine
whether a SubqueryScanPath looks like it will be trivial and change
its costing accordingly.  The attached draft patch does that.

I instrumented the code in setrefs.c, and found that during the
core regression tests this patch estimates correctly in 2103
places while guessing wrongly in 54, so that seems like a pretty
good step forward.

Perhaps I overcomplicated matters by making the callers responsible
for determining triviality of the paths' targets.  We could just
make cost_subqueryscan check that for itself (using logic similar
to what I wrote in set_subquery_pathlist), but that'd result in
duplicative calculations anytime we make more than one Path for a
subquery.  On the other hand, said calculations wouldn't be that
expensive, so perhaps a more localized patch would be better.

I also notice that setrefs.c can elide Append and MergeAppend nodes
too in some cases, but AFAICS costing of those node types doesn't
take that into account.

Anyway, I'll stick this in the queue for v16.

			regards, tom lane

[1] https://www.postgresql.org/message-id/328872.1651247595%40sss.pgh.pa.us

  2. Re: Costing elided SubqueryScans more nearly correctly

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-04T23:02:57Z

    I wrote:
> I instrumented the code in setrefs.c, and found that during the
> core regression tests this patch estimates correctly in 2103
> places while guessing wrongly in 54, so that seems like a pretty
> good step forward.

On second thought, that's not a terribly helpful summary.  Breaking
things down to the next level, there were

   1088 places where we correctly guessed a subquery isn't trivial
	(so no change from current behavior, which is correct)

   1015 places where we correctly guessed a subquery is trivial
	(hence, improving the cost estimate from before)

     40 places where we incorrectly guessed a subquery isn't trivial
        (so no change from current behavior, although that's wrong)

     14 places where we incorrectly guessed a subquery is trivial
	(hence, incorrectly charging zero for the SubqueryScan)

1015 improvements to 14 disimprovements isn't a bad score.  I'm
a bit surprised there are that many removable SubqueryScans TBH;
maybe that's an artifact of all the "SELECT *" queries.

			regards, tom lane

  3. Re: Costing elided SubqueryScans more nearly correctly

    Richard Guo <guofenglinux@gmail.com> — 2022-05-05T07:30:33Z

    On Thu, May 5, 2022 at 7:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > I instrumented the code in setrefs.c, and found that during the
> > core regression tests this patch estimates correctly in 2103
> > places while guessing wrongly in 54, so that seems like a pretty
> > good step forward.
>
> On second thought, that's not a terribly helpful summary.  Breaking
> things down to the next level, there were
>
>    1088 places where we correctly guessed a subquery isn't trivial
>         (so no change from current behavior, which is correct)
>
>    1015 places where we correctly guessed a subquery is trivial
>         (hence, improving the cost estimate from before)
>
>      40 places where we incorrectly guessed a subquery isn't trivial
>         (so no change from current behavior, although that's wrong)
>
>      14 places where we incorrectly guessed a subquery is trivial
>         (hence, incorrectly charging zero for the SubqueryScan)
>
> 1015 improvements to 14 disimprovements isn't a bad score.  I'm
> a bit surprised there are that many removable SubqueryScans TBH;
> maybe that's an artifact of all the "SELECT *" queries.
>

The patch looks sane to me. 1015 vs 14 is a good win.

Thanks
Richard

  4. Re: Costing elided SubqueryScans more nearly correctly

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2022-06-02T11:40:35Z

    On Thu, May 5, 2022 at 4:30 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Thu, May 5, 2022 at 7:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 1015 improvements to 14 disimprovements isn't a bad score.  I'm
>> a bit surprised there are that many removable SubqueryScans TBH;
>> maybe that's an artifact of all the "SELECT *" queries.

> The patch looks sane to me. 1015 vs 14 is a good win.

+1

Best regards,
Etsuro Fujita

  5. Re: Costing elided SubqueryScans more nearly correctly

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-17T19:16:05Z

    I wrote:
> I also notice that setrefs.c can elide Append and MergeAppend nodes
> too in some cases, but AFAICS costing of those node types doesn't
> take that into account.

I took a closer look at this point and realized that in fact,
create_append_path and create_merge_append_path do attempt to account
for this.  But they get it wrong!  Somebody changed the rules in
setrefs.c to account for parallelism, and did not update the costing
side of things.

The attached v2 is the same as v1 plus adding a fix for this point.
No regression test results change from that AFAICT.

			regards, tom lane

  6. Re: Costing elided SubqueryScans more nearly correctly

    Richard Guo <guofenglinux@gmail.com> — 2022-07-18T05:48:51Z

    On Mon, Jul 18, 2022 at 3:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > I also notice that setrefs.c can elide Append and MergeAppend nodes
> > too in some cases, but AFAICS costing of those node types doesn't
> > take that into account.
>
> I took a closer look at this point and realized that in fact,
> create_append_path and create_merge_append_path do attempt to account
> for this.  But they get it wrong!  Somebody changed the rules in
> setrefs.c to account for parallelism, and did not update the costing
> side of things.
>
> The attached v2 is the same as v1 plus adding a fix for this point.
> No regression test results change from that AFAICT.


The new fix looks good to me. Seems setrefs.c added a new logic to check
parallel_aware when removing single-child Appends/MergeAppends in
f9a74c14, but it neglected to update the related costing logic. And I
can see this patch fixes the costing for that.

BTW, not related to this patch, the new lines for parallel_aware check
in setrefs.c are very wide. How about wrap them to keep consistent with
arounding codes?

Thanks
Richard

  7. Re: Costing elided SubqueryScans more nearly correctly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-07-18T16:36:19Z

    On 2022-Jul-18, Richard Guo wrote:

> BTW, not related to this patch, the new lines for parallel_aware check
> in setrefs.c are very wide. How about wrap them to keep consistent with
> arounding codes?

Not untrue!  Something like this, you mean?  Fixed the nearby typo while
at it.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  8. Re: Costing elided SubqueryScans more nearly correctly

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-18T17:30:27Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2022-Jul-18, Richard Guo wrote:
>> BTW, not related to this patch, the new lines for parallel_aware check
>> in setrefs.c are very wide. How about wrap them to keep consistent with
>> arounding codes?

> Not untrue!  Something like this, you mean?  Fixed the nearby typo while
> at it.

WFM.  (I'd fixed the comment typo in my patch, but I don't mind if
you get there first.)

			regards, tom lane

  9. Re: Costing elided SubqueryScans more nearly correctly

    Richard Guo <guofenglinux@gmail.com> — 2022-07-19T01:03:58Z

    On Tue, Jul 19, 2022 at 1:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > On 2022-Jul-18, Richard Guo wrote:
> >> BTW, not related to this patch, the new lines for parallel_aware check
> >> in setrefs.c are very wide. How about wrap them to keep consistent with
> >> arounding codes?
>
> > Not untrue!  Something like this, you mean?  Fixed the nearby typo while
> > at it.
>
> WFM.  (I'd fixed the comment typo in my patch, but I don't mind if
> you get there first.)


+1 The fix looks good to me.

Thanks
Richard

  10. Re: Costing elided SubqueryScans more nearly correctly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-07-19T08:17:18Z

    On 2022-Jul-19, Richard Guo wrote:

> On Tue, Jul 19, 2022 at 1:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> > WFM.  (I'd fixed the comment typo in my patch, but I don't mind if
> > you get there first.)

Ah, I see now you had other grammatical fixes and even more content
there.

> +1 The fix looks good to me.

Thanks, pushed.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  11. Re: Costing elided SubqueryScans more nearly correctly

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-19T15:19:39Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Thanks, pushed.

Pushed the original patch now too.

			regards, tom lane