Le lundi 6 septembre 2021, 11:25:39 CEST Zhihong Yu a écrit : > On Mon, Sep 6, 2021 at 1:17 AM Ronan Dunklau <ronan@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.