Horton

Thread

Commits

  1. Fix postgres_fdw to check shippability of sort clauses properly.

  2. postgres_fdw: Add ORDER BY to some remote SQL queries.

  1. ORDER BY pushdowns seem broken in postgres_fdw

    David Rowley <dgrowleyml@gmail.com> — 2021-07-21T02:25:00Z

    I'm working on a patch [1] to get the planner to consider adding
PathKeys to satisfy ORDER BY / DISTINCT aggregates.  I think this has
led me to discover some problems with postgres_fdw's handling of
pushing down ORDER BY clauses into the foreign server.

The following test exists in the postgres_fdw module:

create operator class my_op_class for type int using btree family
my_op_family as
 operator 1 public.<^,
 operator 3 public.=^,
 operator 5 public.>^,
 function 1 my_op_cmp(int, int);
-- This will not be pushed as user defined sort operator is not part of the
-- extension yet.
explain (verbose, costs off)
select array_agg(c1 order by c1 using operator(public.<^)) from ft2
where c2 = 6 and c1 < 100 group by c2;
                                         QUERY PLAN
--------------------------------------------------------------------------------------------
 GroupAggregate
   Output: array_agg(c1 ORDER BY c1 USING <^ NULLS LAST), c2
   Group Key: ft2.c2
   ->  Foreign Scan on public.ft2
         Output: c1, c2
         Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE (("C 1" <
100)) AND ((c2 = 6))
(6 rows)

Here the test claims that it wants to ensure that the order by using
operator(public.<^) is not pushed down into the foreign scan.
However, unless I'm mistaken, it seems there's a completely wrong
assumption there that the planner would even attempt that.  In current
master we don't add PathKeys for ORDER BY aggregates, why would that
sort get pushed down in the first place?

If I adjust that query to something that would have the planner set
pathkeys for, it does push the ORDER BY to the foreign server without
any consideration that the sort operator is not shippable to the
foreign server.

postgres=# explain verbose select * from ft2 order by c1 using
operator(public.<^);
                                              QUERY PLAN
-------------------------------------------------------------------------------------------------------
 Foreign Scan on public.ft2  (cost=100.28..169.27 rows=1000 width=88)
   Output: c1, c2, c3, c4, c5, c6, c7, c8
   Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T
1" ORDER BY "C 1" ASC NULLS LAST
(3 rows)

Am I missing something here, or is postgres_fdw.c's
get_useful_pathkeys_for_relation() just broken?

David

[1] https://www.postgresql.org/message-id/flat/1882015.KPgzjnsp5C%40aivenronan#159e89188e172ca38cb28ef7c5be9b2c

  2. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2021-07-21T09:05:14Z

    Le mercredi 21 juillet 2021, 04:25:00 CEST David Rowley a écrit :
> Here the test claims that it wants to ensure that the order by using
> operator(public.<^) is not pushed down into the foreign scan.
> However, unless I'm mistaken, it seems there's a completely wrong
> assumption there that the planner would even attempt that.  In current
> master we don't add PathKeys for ORDER BY aggregates, why would that
> sort get pushed down in the first place?

The whole aggregate, including it's order by clause, can be pushed down so 
there is nothing related to pathkeys here.

> 
> If I adjust that query to something that would have the planner set
> pathkeys for, it does push the ORDER BY to the foreign server without
> any consideration that the sort operator is not shippable to the
> foreign server.
> 
> Am I missing something here, or is postgres_fdw.c's
> get_useful_pathkeys_for_relation() just broken?

I think you're right, we need to add a check if the opfamily is shippable. 
I'll submit a patch for that including regression tests.

Regards,

-- 
Ronan Dunklau

  3. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2021-07-21T12:28:30Z

    Le mercredi 21 juillet 2021, 11:05:14 CEST Ronan Dunklau a écrit :
> Le mercredi 21 juillet 2021, 04:25:00 CEST David Rowley a écrit :
> > Here the test claims that it wants to ensure that the order by using
> > operator(public.<^) is not pushed down into the foreign scan.
> > However, unless I'm mistaken, it seems there's a completely wrong
> > assumption there that the planner would even attempt that.  In current
> > master we don't add PathKeys for ORDER BY aggregates, why would that
> > sort get pushed down in the first place?
> 
> The whole aggregate, including it's order by clause, can be pushed down so
> there is nothing related to pathkeys here.
> 
> > If I adjust that query to something that would have the planner set
> > pathkeys for, it does push the ORDER BY to the foreign server without
> > any consideration that the sort operator is not shippable to the
> > foreign server.
> > 
> > Am I missing something here, or is postgres_fdw.c's
> > get_useful_pathkeys_for_relation() just broken?
> 
> I think you're right, we need to add a check if the opfamily is shippable.
> I'll submit a patch for that including regression tests.
> 

In fact the support for generating the correct USING clause was inexistent 
too, so that needed a bit more work.

The attached patch does the following:
  - verify the opfamily is shippable to keep pathkeys
  - generate a correct order by clause using the actual operator.

The second part needed a bit of refactoring: the find_em_expr_for_input_target 
and find_em_expr_for_rel need to return the whole EquivalenceMember, because we 
can't know the type used by the opfamily from the expression (example: the 
expression could be of type intarray, but the type used by the opfamily could 
be anyarray).

I also moved the "USING <operator>"' string generation to a separate function 
since it's now used by appendAggOrderBy and appendOrderByClause.

The find_em_expr_for_rel is exposed in optimizer/paths.h, so I kept the 
existing function which returns the expr directly in case it is used out of 
tree. 



-- 
Ronan Dunklau

  4. Re: ORDER BY pushdowns seem broken in postgres_fdw

    David Rowley <dgrowleyml@gmail.com> — 2021-07-21T13:45:15Z

    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.

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

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.

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.

David

  5. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2021-07-21T14:33:10Z

    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

  6. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ranier Vilela <ranier.vf@gmail.com> — 2021-07-22T00:16:52Z

    Em qua., 21 de jul. de 2021 às 11:33, Ronan Dunklau <ronan.dunklau@aiven.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.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.
>
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

  7. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2021-07-22T07:00:20Z

    Le jeudi 22 juillet 2021, 02:16:52 CEST Ranier Vilela a écrit :
> 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.

Hum weird, it applied cleanly for me, and was formatted using git show which I 
admit is not ideal. Please find it reattached. 


> 
> 2. appendOrderbyUsingClause function
> Put the buffer actions together?
> 
Not sure what you mean here ?

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

Good catch ! 


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

If we are here there should never be a case where the em can't be found. I 
moved the assertion where it makes sense though.



Regards,


-- 
Ronan Dunklau

  8. Re: ORDER BY pushdowns seem broken in postgres_fdw

    David Rowley <dgrowleyml@gmail.com> — 2021-07-22T07:44:54Z

    On Thu, 22 Jul 2021 at 19:00, Ronan Dunklau <ronan.dunklau@aiven.io> wrote:
> Please find it reattached.

+-- This will not be pushed either
+explain verbose select * from ft2 order by c1 using operator(public.<^);
+                                  QUERY PLAN
+-------------------------------------------------------------------------------
+ Sort  (cost=190.83..193.33 rows=1000 width=142)


Can you also use explain (verbose, costs off) the same as the other
tests in that area.  Having the costs there would never survive a run
of the buildfarm. Different hardware will produce different costs, e.g
32-bit hardware might cost cheaper due to narrower widths.

History lesson: costs off was added so we could test plans. Before
that, I don't think that the regression tests had any coverage for
plans.  Older test files still likely lack much testing with EXPLAIN.

David

  9. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2021-07-22T08:49:14Z

    Le jeudi 22 juillet 2021, 09:44:54 CEST David Rowley a écrit :
> +-- This will not be pushed either
> +explain verbose select * from ft2 order by c1 using operator(public.<^);
> +                                  QUERY PLAN
> +---------------------------------------------------------------------------
> ---- + Sort  (cost=190.83..193.33 rows=1000 width=142)
> 
> 
> Can you also use explain (verbose, costs off) the same as the other
> tests in that area.  Having the costs there would never survive a run
> of the buildfarm. Different hardware will produce different costs, e.g
> 32-bit hardware might cost cheaper due to narrower widths.
> 

Sorry about that. Here it is. 


Regards,

-- 
Ronan Dunklau

  10. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ranier Vilela <ranier.vf@gmail.com> — 2021-07-22T09:00:15Z

    Em qui., 22 de jul. de 2021 às 04:00, Ronan Dunklau <ronan.dunklau@aiven.io>
escreveu:

> Le jeudi 22 juillet 2021, 02:16:52 CEST Ranier Vilela a écrit :
> > 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.
>
> Hum weird, it applied cleanly for me, and was formatted using git show
> which I
> admit is not ideal. Please find it reattached.
>
ranier@notebook2:/usr/src/postgres$ git apply <
v2_fix_postgresfdw_orderby_handling.patch
error: falha no patch: contrib/postgres_fdw/deparse.c:37
error: contrib/postgres_fdw/deparse.c: patch does not apply
error: falha no patch: contrib/postgres_fdw/expected/postgres_fdw.out:3168
error: contrib/postgres_fdw/expected/postgres_fdw.out: patch does not apply
error: falha no patch: contrib/postgres_fdw/postgres_fdw.c:916
error: contrib/postgres_fdw/postgres_fdw.c: patch does not apply
error: falha no patch: contrib/postgres_fdw/postgres_fdw.h:165
error: contrib/postgres_fdw/postgres_fdw.h: patch does not apply
error: falha no patch: contrib/postgres_fdw/sql/postgres_fdw.sql:873
error: contrib/postgres_fdw/sql/postgres_fdw.sql: patch does not apply
error: falha no patch: src/backend/optimizer/path/equivclass.c:932
error: src/backend/optimizer/path/equivclass.c: patch does not apply
error: falha no patch: src/include/optimizer/paths.h:144
error: src/include/optimizer/paths.h: patch does not apply


>
>
> >
> > 2. appendOrderbyUsingClause function
> > Put the buffer actions together?
> >
> Not sure what you mean here ?
>
+ appendStringInfoString(buf, " USING ");
+ deparseOperatorName(buf, operform);


>
> > 3. Apply style Postgres?
> > + if (!HeapTupleIsValid(tuple))
> > + {
> > + elog(ERROR, "cache lookup failed for operator family %u",
> > pathkey->pk_opfamily);
> > + }
> >
>
> Good catch !
>
>
> > 4. Assertion not ok here?
> > + em = find_em_for_rel(pathkey->pk_eclass, baserel);
> > + em_expr = em->em_expr;
> >   Assert(em_expr != NULL);
> >
>
> If we are here there should never be a case where the em can't be found. I
> moved the assertion where it makes sense though.
>
> Your version of function is_foreign_pathkey (v4),
not reduce scope the variable PgFdwRelationInfo *fpinfo.
I still prefer the v3 version.

The C ternary operator ? : ;
It's nothing more than a disguised if else

regards,
Ranier Vilela

  11. Re: ORDER BY pushdowns seem broken in postgres_fdw

    David Rowley <dgrowleyml@gmail.com> — 2021-07-27T01:19:18Z

    On Thu, 22 Jul 2021 at 20:49, Ronan Dunklau <ronan.dunklau@aiven.io> wrote:
>
> Le jeudi 22 juillet 2021, 09:44:54 CEST David Rowley a écrit :
> > Can you also use explain (verbose, costs off) the same as the other
> > tests in that area.  Having the costs there would never survive a run
> > of the buildfarm. Different hardware will produce different costs, e.g
> > 32-bit hardware might cost cheaper due to narrower widths.
> >
>
> Sorry about that. Here it is.

I had a look over the v5 patch and noticed a few issues and a few
things that could be improved.

This is not ok:

+        tuple = SearchSysCache4(AMOPSTRATEGY,
+                                ObjectIdGetDatum(pathkey->pk_opfamily),
+                                em->em_datatype,
+                                em->em_datatype,
+                                pathkey->pk_strategy);

SearchSysCache* expects Datum inputs, so you must use the *GetDatum()
macro for each input that isn't already a Datum.

I also:
1. Changed the error message for when that lookup fails so that it's
the same as the others that perform a lookup with AMOPSTRATEGY.
2. Put back the comment in equivclass.c for find_em_expr_for_rel. I
saw no reason that comment should be changed when the function does
exactly what it did before.
3. Renamed appendOrderbyUsingClause to appendOrderBySuffix. I wasn't
happy that the name indicated it was only handling USING clauses when
it also handled ASC/DESC. I also moved in the NULLS FIRST/LAST stuff
in there
4. Adjusted is_foreign_pathkey() to make it easier to read and do
is_shippable() before calling find_em_expr_for_rel().  I didn't see
the need to call find_em_expr_for_rel() when is_shippable() returned
false.
5. Adjusted find_em_expr_for_rel() to remove the ternary operator.

I've attached what I ended up with.

It seems that it was the following commit that introduced the ability
for sorts to be pushed down to the foreign server, so it would be good
if the authors of that patch could look over this.

commit f18c944b6137329ac4a6b2dce5745c5dc21a8578
Author: Robert Haas <rhaas@postgresql.org>
Date:   Tue Nov 3 12:46:06 2015 -0500

    postgres_fdw: Add ORDER BY to some remote SQL queries.

David

  12. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2021-07-27T05:20:17Z

    Le mardi 27 juillet 2021, 03:19:18 CEST David Rowley a écrit :
> On Thu, 22 Jul 2021 at 20:49, Ronan Dunklau <ronan.dunklau@aiven.io> wrote:
> > Le jeudi 22 juillet 2021, 09:44:54 CEST David Rowley a écrit :
> > > Can you also use explain (verbose, costs off) the same as the other
> > > tests in that area.  Having the costs there would never survive a run
> > > of the buildfarm. Different hardware will produce different costs, e.g
> > > 32-bit hardware might cost cheaper due to narrower widths.
> > 
> > Sorry about that. Here it is.
> 
> I had a look over the v5 patch and noticed a few issues and a few
> things that could be improved.

Thank you.

> 
> This is not ok:
> 
> +        tuple = SearchSysCache4(AMOPSTRATEGY,
> +                                ObjectIdGetDatum(pathkey->pk_opfamily),
> +                                em->em_datatype,
> +                                em->em_datatype,
> +                                pathkey->pk_strategy);
> 
> SearchSysCache* expects Datum inputs, so you must use the *GetDatum()
> macro for each input that isn't already a Datum.

Noted.

> 
> I also:
> 1. Changed the error message for when that lookup fails so that it's
> the same as the others that perform a lookup with AMOPSTRATEGY.
> 2. Put back the comment in equivclass.c for find_em_expr_for_rel. I
> saw no reason that comment should be changed when the function does
> exactly what it did before.
> 3. Renamed appendOrderbyUsingClause to appendOrderBySuffix. I wasn't
> happy that the name indicated it was only handling USING clauses when
> it also handled ASC/DESC. I also moved in the NULLS FIRST/LAST stuff
> in there

I agree that name is better.


> 4. Adjusted is_foreign_pathkey() to make it easier to read and do
> is_shippable() before calling find_em_expr_for_rel().  I didn't see
> the need to call find_em_expr_for_rel() when is_shippable() returned
> false.
> 5. Adjusted find_em_expr_for_rel() to remove the ternary operator.
> 
> I've attached what I ended up with.

Looks good to me.

> 
> It seems that it was the following commit that introduced the ability
> for sorts to be pushed down to the foreign server, so it would be good
> if the authors of that patch could look over this.

One thing in particular I was not sure about was how to fetch the operator 
associated with the path key ordering. I chose to go through the opfamily 
recorded on the member, but maybe locating the original SortGroupClause by its 
ref and getting the operator number here woud have worked. It seems more 
straightforward like this though.


-- 
Ronan Dunklau

  13. Re: ORDER BY pushdowns seem broken in postgres_fdw

    David Rowley <dgrowleyml@gmail.com> — 2021-07-27T06:05:19Z

    On Tue, 27 Jul 2021 at 17:20, Ronan Dunklau <ronan.dunklau@aiven.io> wrote:
> One thing in particular I was not sure about was how to fetch the operator
> associated with the path key ordering. I chose to go through the opfamily
> recorded on the member, but maybe locating the original SortGroupClause by its
> ref and getting the operator number here woud have worked. It seems more
> straightforward like this though.

I spent a bit of time trying to find a less invasive way of doing that
and didn't manage to come up with anything. I'm interested to hear if
anyone else has any better ideas.

David

  14. Re: ORDER BY pushdowns seem broken in postgres_fdw

    David Zhang <david.zhang@highgo.ca> — 2021-09-03T20:54:25Z

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

  15. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ronan Dunklau <ronan@dunklau.fr> — 2021-09-06T08:16:21Z

    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

  16. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Zhihong Yu <zyu@yugabyte.com> — 2021-09-06T09:25:39Z

    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 ?

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

Cheers

  17. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2021-09-06T09:41:26Z

    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

  18. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Zhihong Yu <zyu@yugabyte.com> — 2021-09-06T16:46:37Z

    On Mon, Sep 6, 2021 at 2:41 AM Ronan Dunklau <ronan.dunklau@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 <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.

Cheers

  19. Re: ORDER BY pushdowns seem broken in postgres_fdw

    David Zhang <david.zhang@highgo.ca> — 2021-09-10T21:23:48Z

    On 2021-09-06 1:16 a.m., Ronan Dunklau 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.
Just to clarify, the error I encountered was not related with patch v6, 
it was related with other extensions.
> 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
-- 
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca

  20. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-30T23:41:37Z

    Ronan Dunklau <ronan.dunklau@aiven.io> writes:
> [ v8-0001-Fix-orderby-handling-in-postgres_fdw.patch ]

I looked through this patch.  It's going in the right direction,
but I have a couple of nitpicks:

1. There are still some more places that aren't checking shippability
of the relevant opfamily.

2. The existing usage of find_em_expr_for_rel is fundamentally broken,
because that function will seize on the first EC member that is from the
given rel, whether it's shippable or not.  There might be another one
later that is shippable, so this is just the wrong API.  It's not like
this function gives us any useful isolation from the details of ECs,
because postgres_fdw is already looking into those elsewhere, notably
in find_em_expr_for_input_target --- which has the same order-sensitivity
bug.

I think that instead of doubling down on a wrong API, we should just
take that out and move the logic into postgres_fdw.c.  This also has
the advantage of producing a patch that's much safer to backpatch,
because it doesn't rely on the core backend getting updated before
postgres_fdw.so is.

So hacking on those two points, and doing some additional cleanup,
led me to the attached v9.  (In this patch, the removal of code
from equivclass.c is only meant to be applied to HEAD; we have to
leave the function in place in the back branches for API stability.)

If no objections, I think this is committable.

			regards, tom lane

  21. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2022-03-31T08:02:54Z

    Le jeudi 31 mars 2022, 01:41:37 CEST Tom Lane a écrit :
> I looked through this patch.  It's going in the right direction,
> but I have a couple of nitpicks:

Thank you Tom for taking a look at this.

> I think that instead of doubling down on a wrong API, we should just
> take that out and move the logic into postgres_fdw.c.  This also has
> the advantage of producing a patch that's much safer to backpatch,
> because it doesn't rely on the core backend getting updated before
> postgres_fdw.so is.

It makes total sense.

> If no objections, I think this is committable.

No objections on my end.

-- 
Ronan Dunklau

  22. Re: ORDER BY pushdowns seem broken in postgres_fdw

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-31T18:51:33Z

    Ronan Dunklau <ronan.dunklau@aiven.io> writes:
> Le jeudi 31 mars 2022, 01:41:37 CEST Tom Lane a écrit :
>> If no objections, I think this is committable.

> No objections on my end.

Pushed.

			regards, tom lane