public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Andy Fan <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Zhang Mingli <[email protected]>
Subject: Re: make add_paths_to_append_rel aware of startup cost
Date: Wed, 27 Sep 2023 21:03:43 +1300
Message-ID: <CAApHDvqf+YNFr_GbU4aJ5G7ZGJoZJ-R6CcFNaVi3QRpcgnNyAA@mail.gmail.com> (raw)
In-Reply-To: <CAKU4AWqaTNPrYcb_cMEDDYWZVGfFxuUjr75F5LBZwnUQ0JvVPw@mail.gmail.com>
References: <CAKU4AWrXSkUV=Pt-gRxQT7EbfUeNssprGyNsB=5mJibFZ6S3ww@mail.gmail.com>
<CAApHDvry0nSV62kAOH3iccvfPhGPLN0Q97+=b1RsDPXDz3=CiQ@mail.gmail.com>
<CAKU4AWoksFSs_6K5ZCzz68Oq+3uZb5g6sb8-dXJ6cChgYQm-yw@mail.gmail.com>
<CAApHDvoMGyc+1eb8g5rEMUUMeGWhe2c_f8yvJjUO1kUHZj0h7w@mail.gmail.com>
<CAKU4AWp9aTafjWwCqOauC7jmbuX+tGEWkRrBwXYhW-CFQXmRig@mail.gmail.com>
<CAKU4AWqaTNPrYcb_cMEDDYWZVGfFxuUjr75F5LBZwnUQ0JvVPw@mail.gmail.com>
On Mon, 18 Sept 2023 at 22:55, Andy Fan <[email protected]> wrote:
> Here is an updated version to show what's in my mind.
My current thoughts on this are that the fractional cost part adds
quite a bit more complexity than the minimalistic approach of just
also considering the cheapest startup path.
There's also quite a bit I don't like about the extra code you've added:
1. RelOptInfo.tuple_fraction is not given a default value in locations
where we do makeNode(RelOptInfo);
2. This is very poorly documented and badly named. Also seems to have
a typo "stopper"
+ /* Like order by, group by, distinct and so. */
+ bool has_stoper_op;
With that, nobody has a hope of knowing if some new operation should
set that value to true or false.
I think it needs to define the meaning, which I think (very roughly)
is "does the query require any additional upper-planner operations
which could require having to read more tuples from the final join
relation than the number of tuples which are read from the final upper
rel."
3. get_fractional_path_cost() goes to the trouble of setting
total_rows then does not use it.
4. I don't see why it's ok to take the total_rows from the first Path
in the list in get_cheapest_fractional_path_ext(). What if another
Path has some other value?
But overall, I'm more inclined to just go with the more simple "add a
cheap unordered startup append path if considering cheap startup
plans" version. I see your latest patch does both. So, I'd suggest two
patches as I do see the merit in keeping this simple and cheap. If we
can get the first part in and you still find cases where you're not
getting the most appropriate startup plan based on the tuple fraction,
then we can reconsider what extra complexity we should endure in the
code based on the example query where we've demonstrated the planner
is not choosing the best startup path appropriate to the given tuple
fraction.
David
view thread (18+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: make add_paths_to_append_rel aware of startup cost
In-Reply-To: <CAApHDvqf+YNFr_GbU4aJ5G7ZGJoZJ-R6CcFNaVi3QRpcgnNyAA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox