Re: make add_paths_to_append_rel aware of startup cost

On Mon, 18 Sept 2023 at 01:42, Andy Fan <zhihui.fan1213@gmail.com> wrote:
> On Fri, Sep 15, 2023 at 3:15 PM David Rowley <dgrowleyml@gmail.com> wrote:
>> Instead of doing that, why don't you just create a completely new
>> AppendPath containing all the cheapest_startup_paths and add that to
>> the append rel. You can optimise this and only do it when
>> rel->consider_startup is true.
>>
>> Does the attached do anything less than what your patch does?
>
>
> We can work like this,  but  there is one difference from what
> my current patch does. It is cheapest_startup_path vs cheapest
> fraction path.  For example if we have the following 3 paths with
> all of the estimated rows is 100 and the tuple_fraction is 10.

Yeah, it's true that the version I wrote didn't consider the
fractional part, but I didn't really see it as worse than what you
did.  It looks like you're assuming that every append child will have
the same number of tuples read from it, but it seems to me that it
would only be valid to use the fractional part for the first child.
The path chosen for subsequent child paths would, if done correctly,
need to account for the estimated rows from the previous child paths.
It's not valid here to copy the code in generate_orderedappend_paths()
as MergeAppend won't necessarily exhaust the first child subpath first
like Append will.

> Path 1:  startup_cost = 60,  total_cost = 80  -- cheapest total cost.
> Path 2:  startup_cost = 10,  total_cost = 1000  -- cheapest startup cost
> Path 3:  startup_cost = 20,  total_cost = 90  -- cheapest fractional cost
>
> So with the patch you propose,  Path 1 & Path 3 are chosen to build
> append path.  but with my current patch,  Only path 3 is kept.  IIUC,
> path 3 should be the best one in this case.

I assume you mean mine would build AppendPaths for 1+2, not 1+3.

You mentioned:

> I just find it is hard to build the case for Identifier 2/3/4.

I wonder if this is because generate_orderedappend_paths() handles
startup paths and most cases will that need a cheap startup plan will
require some sort of pathkeys.

The example you mentioned of:

(select * from tenk1 order by hundred)
 UNION ALL
 (select * from tenk1 order by hundred)
 limit 3;

I don't find this to be a compellingly real-world case.  The planner
is under no obligation to output rows from the 1st branch of the UNION
ALL before the 2nd one. If the user cared about that then they'd have
instead added a top-level ORDER BY, in which case the planner seems
happy to use the index scan:

regression=# explain (costs off) (select * from tenk1) UNION ALL
(select * from tenk1) order by hundred limit 3;
                         QUERY PLAN
-------------------------------------------------------------
 Limit
   ->  Merge Append
         Sort Key: tenk1.hundred
         ->  Index Scan using tenk1_hundred on tenk1
         ->  Index Scan using tenk1_hundred on tenk1 tenk1_1

It would be good if you could provide a bit more detail on the cases
you want to improve here.  For example, if your #4 case, you have
"WHERE xxx". I don't know if "xxx" is just a base qual or if there's a
correlation to the outer query in there.

Another concern I have with your patch is that it seems to be under
the impression that there being further joins to evaluate at this
query level is the only reason that we would have to pull more than
the tuple fraction number of rows from the query. What gives you the
confidence that's the only reason we may want to pull more than the
tuple fraction of tuples from the append child?

David