Re: ORDER BY pushdowns seem broken in postgres_fdw

Em qui., 22 de jul. de 2021 às 04:00, Ronan Dunklau <ronan.dunklau@aiven.io>
escreveu:

> Le jeudi 22 juillet 2021, 02:16:52 CEST Ranier Vilela a écrit :
> > Unfortunately your patch does not apply clear into the head.
> > So I have a few suggestions on v2, attached with the .txt extension to
> > avoid cf bot.
> > Please, if ok, make the v3.
>
> Hum weird, it applied cleanly for me, and was formatted using git show
> which I
> admit is not ideal. Please find it reattached.
>
ranier@notebook2:/usr/src/postgres$ git apply <
v2_fix_postgresfdw_orderby_handling.patch
error: falha no patch: contrib/postgres_fdw/deparse.c:37
error: contrib/postgres_fdw/deparse.c: patch does not apply
error: falha no patch: contrib/postgres_fdw/expected/postgres_fdw.out:3168
error: contrib/postgres_fdw/expected/postgres_fdw.out: patch does not apply
error: falha no patch: contrib/postgres_fdw/postgres_fdw.c:916
error: contrib/postgres_fdw/postgres_fdw.c: patch does not apply
error: falha no patch: contrib/postgres_fdw/postgres_fdw.h:165
error: contrib/postgres_fdw/postgres_fdw.h: patch does not apply
error: falha no patch: contrib/postgres_fdw/sql/postgres_fdw.sql:873
error: contrib/postgres_fdw/sql/postgres_fdw.sql: patch does not apply
error: falha no patch: src/backend/optimizer/path/equivclass.c:932
error: src/backend/optimizer/path/equivclass.c: patch does not apply
error: falha no patch: src/include/optimizer/paths.h:144
error: src/include/optimizer/paths.h: patch does not apply


>
>
> >
> > 2. appendOrderbyUsingClause function
> > Put the buffer actions together?
> >
> Not sure what you mean here ?
>
+ appendStringInfoString(buf, " USING ");
+ deparseOperatorName(buf, operform);


>
> > 3. Apply style Postgres?
> > + if (!HeapTupleIsValid(tuple))
> > + {
> > + elog(ERROR, "cache lookup failed for operator family %u",
> > pathkey->pk_opfamily);
> > + }
> >
>
> Good catch !
>
>
> > 4. Assertion not ok here?
> > + em = find_em_for_rel(pathkey->pk_eclass, baserel);
> > + em_expr = em->em_expr;
> >   Assert(em_expr != NULL);
> >
>
> If we are here there should never be a case where the em can't be found. I
> moved the assertion where it makes sense though.
>
> Your version of function is_foreign_pathkey (v4),
not reduce scope the variable PgFdwRelationInfo *fpinfo.
I still prefer the v3 version.

The C ternary operator ? : ;
It's nothing more than a disguised if else

regards,
Ranier Vilela