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