ORDER BY pushdowns seem broken in postgres_fdw
I'm working on a patch [1]/messages/by-id/1882015.KPgzjnsp5C@aivenronan 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
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
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
Attachments:
v1_fix_postgresfdw_orderby_handlingtext/plain; charset=UTF-8; name=v1_fix_postgresfdw_orderby_handlingDownload+143-46
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
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
Attachments:
v2_fix_postgresfdw_orderby_handling.patchtext/x-patch; charset=UTF-8; name=v2_fix_postgresfdw_orderby_handling.patchDownload+158-50
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
Attachments:
v3_fix_postgresfdw_orderby_handling.txttext/plain; charset=US-ASCII; name=v3_fix_postgresfdw_orderby_handling.txtDownload+164-50
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
Attachments:
v4_fix_postgresfdw_orderby_handling.txttext/plain; charset=UTF-8; name=v4_fix_postgresfdw_orderby_handling.txtDownload+154-52
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
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
Attachments:
v5_fix_postgresfdw_orderby_handling.patchtext/x-patch; charset=UTF-8; name=v5_fix_postgresfdw_orderby_handling.patchDownload+154-52
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
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
Attachments:
v6_fix_postgresfdw_orderby_handling.patchapplication/octet-stream; name=v6_fix_postgresfdw_orderby_handling.patchDownload+187-67
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
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
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."
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 testedApplied 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
Attachments:
v7-0001-Fix-orderby-handling-in-postgres_fdw.patchtext/x-patch; charset=UTF-8; name=v7-0001-Fix-orderby-handling-in-postgres_fdw.patchDownload+187-68
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 testedApplied 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
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 testedApplied 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 DunklauHi,
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
Attachments:
v8-0001-Fix-orderby-handling-in-postgres_fdw.patchtext/x-patch; charset=UTF-8; name=v8-0001-Fix-orderby-handling-in-postgres_fdw.patchDownload+187-68
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 testedApplied 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 DunklauHi,
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
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 testedApplied 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
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