Re: ORDER BY pushdowns seem broken in postgres_fdw

Em qua., 21 de jul. de 2021 às 11:33, Ronan Dunklau <ronan(dot)dunklau(at)aiven(dot)io>
escreveu:

> Le mercredi 21 juillet 2021 15:45:15 CEST, vous avez écrit :
> > 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.
>
> 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.
>
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.

1. new version is_foreign_pathke?
+bool
+is_foreign_pathkey(PlannerInfo *root,
+ RelOptInfo *baserel,
+ PathKey *pathkey)
+{
+ EquivalenceClass *pathkey_ec = pathkey->pk_eclass;
+ EquivalenceMember *em;
+
+ /*
+ * is_foreign_expr would detect volatile expressions as well, but
+ * checking ec_has_volatile here saves some cycles.
+ */
+ if (pathkey_ec->ec_has_volatile)
+ return false;
+
+ /*
+ * Found member's expression is foreign?
+ */
+ em = find_em_for_rel(pathkey_ec, baserel);
+ if (em != NULL && is_foreign_expr(root, baserel, em->em_expr))
+ {
+ PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) baserel->fdw_private;
+
+ /*
+ * Operator family is shippable?
+ */
+ return is_shippable(pathkey->pk_opfamily, OperatorFamilyRelationId,
fpinfo);
+ }
+
+ return false;
+}

2. appendOrderbyUsingClause function
Put the buffer actions together?

3. Apply style Postgres?
+ if (!HeapTupleIsValid(tuple))
+ {
+ elog(ERROR, "cache lookup failed for operator family %u",
pathkey->pk_opfamily);
+ }

4. Assertion not ok here?
+ em = find_em_for_rel(pathkey->pk_eclass, baserel);
+ em_expr = em->em_expr;
Assert(em_expr != NULL);

find_em_for_rel function can returns NULL.
I think that is need deal with em_expr == NULL at runtime.

5. More readable version?
+find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
+{
+ EquivalenceMember *em = find_em_for_rel(ec, rel);
+
+ if (em != NULL)
+ return em->em_expr;
+
+ return NULL;
+}

regards,
Ranier Vilela