On Mon, 1 Aug 2022 at 03:49, Tom Lane <tgl@sss.pgh.pa.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
