Re: ORDER BY pushdowns seem broken in postgres_fdw
От | Ronan Dunklau |
---|---|
Тема | Re: ORDER BY pushdowns seem broken in postgres_fdw |
Дата | |
Msg-id | 2476587.xQJ17RxkC0@aivenronan обсуждение исходный текст |
Ответ на | Re: ORDER BY pushdowns seem broken in postgres_fdw (David Rowley <dgrowleyml@gmail.com>) |
Ответы |
Re: ORDER BY pushdowns seem broken in postgres_fdw
|
Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления: