From: | David Rowley <dgrowleyml(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Richard Guo <guofenglinux(at)gmail(dot)com>, Ronan Dunklau <ronan(dot)dunklau(at)aiven(dot)io>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> |
Subject: | Re: Add proper planner support for ORDER BY / DISTINCT aggregates |
Date: | 2022-08-02 11:21:04 |
Message-ID: | CAApHDvpQ+LmokunMogxc1Ba+AOptP=sHqruCWeC8Xgv2XDbiDg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, 1 Aug 2022 at 03:49, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Are you going to push the other patch (adjusting
> select_outer_pathkeys_for_merge) first, so that we can see the residual
> plan changes that this patch creates? I'm not entirely comfortable
> with the regression test changes as posted.
Yes, I pushed that earlier.
> Likewise, it might be
> better to fix DEFAULT_FDW_TUPLE_COST beforehand, to detangle what
> the effects of that are.
I chatted to Andres and Thomas about this last week and their view
made me think it might not be quite as clear-cut as "just bump it up a
bunch because it's ridiculously low" that I had in mind. They
mentioned about file_fdw and another one that appears to work on
mmapped segments, which I don't recall if any names were mentioned.
Certainly that's not a reason not to change it, but it's not quite as
clear-cut as I thought. I'll open a thread with some reasonable
evidence to get a topic going and see where we end up. In the
meantime I've just coded it to do a temporary adjustment to the
fdw_tuple_cost foreign server setting just before the test in
question.
> Also, I think it's bad style to rely on aggpresorted defaulting to false.
> You should explicitly initialize it anywhere that an Aggref node is
> constructed. It looks like there are just two places to fix
> (parse_expr.c and parse_func.c).
Ooops. I'm normally good at remembering that. Not this time!
> Nothing else jumped out at me in a quick scan.
Thanks for the quick scan. I did another few myself and adjusted a
small number of things. Mostly comments and using things like
lfirst_node and list_nth_node instead of lfirst and list_nth with a
cast.
I've now pushed the patch.
Thank you to everyone who looked at this.
David
From | Date | Subject | |
---|---|---|---|
Next Message | David Steele | 2022-08-02 11:23:35 | Re: remove more archiving overhead |
Previous Message | David Rowley | 2022-08-02 11:14:37 | Re: POC: GROUP BY optimization |