On Mon, Sep 6, 2021 at 2:41 AM Ronan Dunklau <ronan.dunk...@aiven.io> wrote:
> Le lundi 6 septembre 2021, 11:25:39 CEST Zhihong Yu a écrit : > > On Mon, Sep 6, 2021 at 1:17 AM Ronan Dunklau <ro...@dunklau.fr> wrote: > > > Le vendredi 3 septembre 2021, 22:54:25 CEST David Zhang a écrit : > > > > The following review has been posted through the commitfest > application: > > > > make installcheck-world: tested, failed > > > > Implements feature: tested, passed > > > > Spec compliant: not tested > > > > Documentation: not tested > > > > > > > > Applied the v6 patch to master branch and ran regression test for > > > > > > contrib, > > > > > > > the result was "All tests successful." > > > > > > What kind of error did you get running make installcheck-world ? If it > > > passed > > > the make check for contrib, I can't see why it would fail running make > > > installcheck-world. > > > > > > In any case, I just checked and running make installcheck-world doesn't > > > produce any error. > > > > > > Since HEAD had moved a bit since the last version, I rebased the patch, > > > resulting in the attached v7. > > > > > > Best regards, > > > > > > -- > > > Ronan Dunklau > > > > Hi, > > bq. a pushed-down order by could return wrong results. > > > > Can you briefly summarize the approach for fixing the bug in the > > description ? > > Done, let me know what you think about it. > > > > > + * Returns true if it's safe to push down a sort as described by > 'pathkey' > > to > > + * the foreign server > > + */ > > +bool > > +is_foreign_pathkey(PlannerInfo *root, > > > > It would be better to name the method which reflects whether pushdown is > > safe. e.g. is_pathkey_safe_for_pushdown. > > The convention used here is the same one as in is_foreign_expr and > is_foreign_param, which are also related to pushdown-safety. > > -- > Ronan Dunklau Hi, w.r.t. description: bq. original operator associated to the pathkey associated to -> associated with w.r.t. method name, it is fine to use the current name, considering the functions it calls don't have pushdown in their names. Cheers
