Re: ORDER BY pushdowns seem broken in postgres_fdw

Le mercredi 21 juillet 2021 15:45:15 CEST, vous avez écrit :
> On Thu, 22 Jul 2021 at 00:28, Ronan Dunklau <ronan.dunklau@aiven.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.

Thank you for the review.

> 
> 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))
> 
Done. By the way, the comment just above mentions we don't have a way to use a 
prefix pathkey, but I suppose we should revisit that now that we have 
IncrementalSort. I'll mark it in my todo list for another patch.

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

Done, let me know what you think about it.

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

I have done it the other way around. I'm not sure we really need to keep the 
find_em_expr_for_rel function on HEAD. If we decide to backpatch, it would need 
to be kept though. 

-- 
Ronan Dunklau