Re: ORDER BY pushdowns seem broken in postgres_fdw

On Thu, 22 Jul 2021 at 20:49, Ronan Dunklau <ronan(dot)dunklau(at)aiven(dot)io> wrote:
>
> Le jeudi 22 juillet 2021, 09:44:54 CEST David Rowley a écrit :
> > Can you also use explain (verbose, costs off) the same as the other
> > tests in that area. Having the costs there would never survive a run
> > of the buildfarm. Different hardware will produce different costs, e.g
> > 32-bit hardware might cost cheaper due to narrower widths.
> >
>
> Sorry about that. Here it is.

I had a look over the v5 patch and noticed a few issues and a few
things that could be improved.

This is not ok:

+ tuple = SearchSysCache4(AMOPSTRATEGY,
+ ObjectIdGetDatum(pathkey->pk_opfamily),
+ em->em_datatype,
+ em->em_datatype,
+ pathkey->pk_strategy);

SearchSysCache* expects Datum inputs, so you must use the *GetDatum()
macro for each input that isn't already a Datum.

I also:
1. Changed the error message for when that lookup fails so that it's
the same as the others that perform a lookup with AMOPSTRATEGY.
2. Put back the comment in equivclass.c for find_em_expr_for_rel. I
saw no reason that comment should be changed when the function does
exactly what it did before.
3. Renamed appendOrderbyUsingClause to appendOrderBySuffix. I wasn't
happy that the name indicated it was only handling USING clauses when
it also handled ASC/DESC. I also moved in the NULLS FIRST/LAST stuff
in there
4. Adjusted is_foreign_pathkey() to make it easier to read and do
is_shippable() before calling find_em_expr_for_rel(). I didn't see
the need to call find_em_expr_for_rel() when is_shippable() returned
false.
5. Adjusted find_em_expr_for_rel() to remove the ternary operator.

I've attached what I ended up with.

It seems that it was the following commit that introduced the ability
for sorts to be pushed down to the foreign server, so it would be good
if the authors of that patch could look over this.

commit f18c944b6137329ac4a6b2dce5745c5dc21a8578
Author: Robert Haas <rhaas(at)postgresql(dot)org>
Date: Tue Nov 3 12:46:06 2015 -0500

postgres_fdw: Add ORDER BY to some remote SQL queries.

David