Re: ORDER BY pushdowns seem broken in postgres_fdw

Le mardi 27 juillet 2021, 03:19:18 CEST David Rowley a écrit :
> On Thu, 22 Jul 2021 at 20:49, Ronan Dunklau <ronan.dunklau@aiven.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.

Thank you.

> 
> 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.

Noted.

> 
> 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

I agree that name is better.


> 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.

Looks good to me.

> 
> 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.

One thing in particular I was not sure about was how to fetch the operator 
associated with the path key ordering. I chose to go through the opfamily 
recorded on the member, but maybe locating the original SortGroupClause by its 
ref and getting the operator number here woud have worked. It seems more 
straightforward like this though.


-- 
Ronan Dunklau