Re: ORDER BY pushdowns seem broken in postgres_fdw

On Thu, 22 Jul 2021 at 00:28, Ronan Dunklau <ronan(dot)dunklau(at)aiven(dot)io> wrote:
> The attached patch does the following:
> - verify the opfamily is shippable to keep pathkeys
> - generate a correct order by clause using the actual operator.

Thanks for writing the patch.

This is just a very superficial review. I've not spent a great deal
of time looking at postgres_fdw code, so would rather some eyes that
were more familiar with the code looked too.

1. This comment needs to be updated. It still mentions
is_foreign_expr, which you're no longer calling.

* is_foreign_expr would detect volatile expressions as well, but
* checking ec_has_volatile here saves some cycles.
*/
- if (pathkey_ec->ec_has_volatile ||
- !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
- !is_foreign_expr(root, rel, em_expr))
+ if (!is_foreign_pathkey(root, rel, pathkey))

2. This is not a very easy return condition to read:

+ return (!pathkey_ec->ec_has_volatile &&
+ (em = find_em_for_rel(pathkey_ec, baserel)) &&
+ is_foreign_expr(root, baserel, em->em_expr) &&
+ is_shippable(pathkey->pk_opfamily, OperatorFamilyRelationId, fpinfo));

I think it would be nicer to break that down into something easier on
the eyes that could be commented a little more.

3. This comment is no longer true:

* Find an equivalence class member expression, all of whose Vars, come from
* the indicated relation.
*/
-Expr *
-find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
+EquivalenceMember*
+find_em_for_rel(EquivalenceClass *ec, RelOptInfo *rel)

Also, missing space after EquivalenceMember.

The comment can just be moved down to:

+Expr *
+find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
+{
+ EquivalenceMember *em = find_em_for_rel(ec, rel);
+ return em ? em->em_expr : NULL;
+}

and you can rewrite the one for find_em_for_rel.

David