Foreign Join pushdowns not working properly for outer joins

Started by David Rowleyabout 9 years ago25 messageshackers
Jump to latest
#1David Rowley
dgrowleyml@gmail.com

I've been asked to investigate a case of a foreign join not occurring
on the foreign server as would have been expected.

I've narrowed this down and the problem seems to only occur with outer
type joins.

The problem can be reproduced by the attached test_case.sql

Upon investigation I've discovered that the problem relates to the
citext extension not being in the shippable_extensions List for the
joinrel. Since the extension is not white listed, the qual on the
citext column is disallowed from being pushed down into the foreign
server by is_shippable().

This happens to work fine for INNER JOINs since the qual makes it into
baserestrictinfo an is properly classified by the following fragment
in postgresGetForeignRelSize()

/*
* Identify which baserestrictinfo clauses can be sent to the remote
* server and which can't.
*/
classifyConditions(root, baserel, baserel->baserestrictinfo,
&fpinfo->remote_conds, &fpinfo->local_conds);

The attached patch, based on 9.6, fixes the problem by properly
processing the foreign server options in
postgresGetForeignJoinPaths().

I ended up shifting the code which does this into functions to allow
it to be reused. I also ended up shifting out the code which processes
the table options so that it is consistent.

Reviews from people a bit closer to the foreign join pushdown code are welcome.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

test_caee.sqlapplication/octet-stream; name=test_caee.sqlDownload
foreign_outerjoin_pushdown_fix.patchapplication/octet-stream; name=foreign_outerjoin_pushdown_fix.patchDownload+59-24
#2Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: David Rowley (#1)
Re: Foreign Join pushdowns not working properly for outer joins

On 2017/03/06 11:05, David Rowley wrote:

I've been asked to investigate a case of a foreign join not occurring
on the foreign server as would have been expected.

The attached patch, based on 9.6, fixes the problem by properly
processing the foreign server options in
postgresGetForeignJoinPaths().

I ended up shifting the code which does this into functions to allow
it to be reused. I also ended up shifting out the code which processes
the table options so that it is consistent.

Reviews from people a bit closer to the foreign join pushdown code are welcome.

Thanks for working on this!

I think the fix would work well, but another way I think is much simpler
and more consistent with the existing code is to (1) move code for
getting the server info from the outer's fpinfo before calling
is_foreign_expr() in foreign_join_ok() and (2) add code for getting the
shippable extensions info from the outer's fpinfo before calling that
function, like the attached.

Best regards,
Etsuro Fujita

Attachments:

foreign_outerjoin_pushdown_fix_efujita.patchbinary/octet-stream; name=foreign_outerjoin_pushdown_fix_efujita.patchDownload+4-3
#3David Rowley
dgrowleyml@gmail.com
In reply to: Etsuro Fujita (#2)
Re: Foreign Join pushdowns not working properly for outer joins

On 6 March 2017 at 18:51, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:

On 2017/03/06 11:05, David Rowley wrote:

The attached patch, based on 9.6, fixes the problem by properly
processing the foreign server options in
postgresGetForeignJoinPaths().

I think the fix would work well, but another way I think is much simpler and
more consistent with the existing code is to (1) move code for getting the
server info from the outer's fpinfo before calling is_foreign_expr() in
foreign_join_ok() and (2) add code for getting the shippable extensions info
from the outer's fpinfo before calling that function, like the attached.

Thanks for looking over my patch.

I looked over yours and was surprised to see:

+ /* is_foreign_expr might need server and shippable-extensions info. */
+ fpinfo->server = fpinfo_o->server;
+ fpinfo->shippable_extensions = fpinfo_o->shippable_extensions;

That appears to do nothing for the other server options. For example
use_remote_estimate, which is used in estimate_path_cost_size(). As of
now, that's also broken. My patch fixes that problem too, yours
appears not to.

Even if you expanded the above code to process all server options, if
someone comes along later and adds a new option, my code will pick it
up, yours could very easily be forgotten about and be the cause of yet
more bugs.

It seems like a much better idea to keep the server option processing
in one location, which is what I did.

Do you think differently?

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: David Rowley (#3)
Re: Foreign Join pushdowns not working properly for outer joins

On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 6 March 2017 at 18:51, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:

On 2017/03/06 11:05, David Rowley wrote:

The attached patch, based on 9.6, fixes the problem by properly
processing the foreign server options in
postgresGetForeignJoinPaths().

I think the fix would work well, but another way I think is much simpler and
more consistent with the existing code is to (1) move code for getting the
server info from the outer's fpinfo before calling is_foreign_expr() in
foreign_join_ok() and (2) add code for getting the shippable extensions info
from the outer's fpinfo before calling that function, like the attached.

Thanks for looking over my patch.

I looked over yours and was surprised to see:

+ /* is_foreign_expr might need server and shippable-extensions info. */
+ fpinfo->server = fpinfo_o->server;
+ fpinfo->shippable_extensions = fpinfo_o->shippable_extensions;

That appears to do nothing for the other server options. For example
use_remote_estimate, which is used in estimate_path_cost_size(). As of
now, that's also broken. My patch fixes that problem too, yours
appears not to.

Even if you expanded the above code to process all server options, if
someone comes along later and adds a new option, my code will pick it
up, yours could very easily be forgotten about and be the cause of yet
more bugs.

It seems like a much better idea to keep the server option processing
in one location, which is what I did.

I agree with this. However
1. apply_server_options() is parsing the options strings again and
again, which seems wastage of CPU cycles. It should probably pick up
the options from one of the joining relations. Also, the patch calls
GetForeignServer(), which is not needed; in foreign_join_ok(), it will
copy it from the outer relation anyway.
2. Some server options like use_remote_estimate and fetch_size are
overridden by corresponding table level options. For a join relation
the values of these options are derived by some combination of
table-level options.

I think we should write a separate function
apply_fdw_options_for_joinrel() and pass the fpinfos of joinrel, outer
and inner relation. The function will copy the values of server level
options and derive values for table level options. We would add a note
there to keep this function in sync with apply_*_options(). I don't
think there's any better way to keep the options in sync for base
relations and join relations.

Here's the patch attached.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

foreign_join_upperrel_pushdown_fix.patchapplication/octet-stream; name=foreign_join_upperrel_pushdown_fix.patchDownload+131-75
#5Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Ashutosh Bapat (#4)
Re: Foreign Join pushdowns not working properly for outer joins

On 2017/03/06 21:22, Ashutosh Bapat wrote:

On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 6 March 2017 at 18:51, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:

On 2017/03/06 11:05, David Rowley wrote:

I looked over yours and was surprised to see:

+ /* is_foreign_expr might need server and shippable-extensions info. */
+ fpinfo->server = fpinfo_o->server;
+ fpinfo->shippable_extensions = fpinfo_o->shippable_extensions;

That appears to do nothing for the other server options. For example
use_remote_estimate, which is used in estimate_path_cost_size(). As of
now, that's also broken. My patch fixes that problem too, yours
appears not to.

Thanks for the comments! Actually, those options including
use_remote_estimate are set later in foreign_join_ok, so
estimate_path_cost_size would work well, for example.

Even if you expanded the above code to process all server options, if
someone comes along later and adds a new option, my code will pick it
up, yours could very easily be forgotten about and be the cause of yet
more bugs.

I agree with you on that point.

It seems like a much better idea to keep the server option processing
in one location, which is what I did.

Seems like a better idea.

I agree with this. However
1. apply_server_options() is parsing the options strings again and
again, which seems wastage of CPU cycles. It should probably pick up
the options from one of the joining relations. Also, the patch calls
GetForeignServer(), which is not needed; in foreign_join_ok(), it will
copy it from the outer relation anyway.
2. Some server options like use_remote_estimate and fetch_size are
overridden by corresponding table level options. For a join relation
the values of these options are derived by some combination of
table-level options.

I think we should write a separate function
apply_fdw_options_for_joinrel() and pass the fpinfos of joinrel, outer
and inner relation. The function will copy the values of server level
options and derive values for table level options. We would add a note
there to keep this function in sync with apply_*_options(). I don't
think there's any better way to keep the options in sync for base
relations and join relations.

Here's the patch attached.

I looked over the patch quickly. I think it's a better idea that to
gather various option processing in foreign_join_ok (or
foreign_grouping_ok) in one place. However, I'm wondering we really
need to introduce apply_table_options and apply_server_options. ISTM
that those option processing in postgresGetForeignRelSize is gathered in
one place well already.

Best regards,
Etsuro Fujita

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Etsuro Fujita (#5)
Re: Foreign Join pushdowns not working properly for outer joins

On Tue, Mar 7, 2017 at 9:33 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

On 2017/03/06 21:22, Ashutosh Bapat wrote:

On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 6 March 2017 at 18:51, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:

On 2017/03/06 11:05, David Rowley wrote:

I looked over yours and was surprised to see:

+ /* is_foreign_expr might need server and shippable-extensions info. */
+ fpinfo->server = fpinfo_o->server;
+ fpinfo->shippable_extensions = fpinfo_o->shippable_extensions;

That appears to do nothing for the other server options. For example
use_remote_estimate, which is used in estimate_path_cost_size(). As of
now, that's also broken. My patch fixes that problem too, yours
appears not to.

Thanks for the comments! Actually, those options including
use_remote_estimate are set later in foreign_join_ok, so
estimate_path_cost_size would work well, for example.

Even if you expanded the above code to process all server options, if
someone comes along later and adds a new option, my code will pick it
up, yours could very easily be forgotten about and be the cause of yet
more bugs.

I agree with you on that point.

It seems like a much better idea to keep the server option processing
in one location, which is what I did.

Seems like a better idea.

I agree with this. However
1. apply_server_options() is parsing the options strings again and
again, which seems wastage of CPU cycles. It should probably pick up
the options from one of the joining relations. Also, the patch calls
GetForeignServer(), which is not needed; in foreign_join_ok(), it will
copy it from the outer relation anyway.
2. Some server options like use_remote_estimate and fetch_size are
overridden by corresponding table level options. For a join relation
the values of these options are derived by some combination of
table-level options.

I think we should write a separate function
apply_fdw_options_for_joinrel() and pass the fpinfos of joinrel, outer
and inner relation. The function will copy the values of server level
options and derive values for table level options. We would add a note
there to keep this function in sync with apply_*_options(). I don't
think there's any better way to keep the options in sync for base
relations and join relations.

Here's the patch attached.

I looked over the patch quickly. I think it's a better idea that to gather
various option processing in foreign_join_ok (or foreign_grouping_ok) in one
place. However, I'm wondering we really need to introduce
apply_table_options and apply_server_options. ISTM that those option
processing in postgresGetForeignRelSize is gathered in one place well
already.

I kept that as is since I liked the way it's modularized now.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7David Rowley
dgrowleyml@gmail.com
In reply to: Ashutosh Bapat (#4)
Re: Foreign Join pushdowns not working properly for outer joins

On 7 March 2017 at 01:22, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 6 March 2017 at 18:51, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:

On 2017/03/06 11:05, David Rowley wrote:

It seems like a much better idea to keep the server option processing
in one location, which is what I did.

I agree with this. However
1. apply_server_options() is parsing the options strings again and
again, which seems wastage of CPU cycles. It should probably pick up
the options from one of the joining relations. Also, the patch calls
GetForeignServer(), which is not needed; in foreign_join_ok(), it will
copy it from the outer relation anyway.
2. Some server options like use_remote_estimate and fetch_size are
overridden by corresponding table level options. For a join relation
the values of these options are derived by some combination of
table-level options.

This seems much more sane. I'd failed to find the code which takes the
largest fetch_size.

I think we should write a separate function
apply_fdw_options_for_joinrel() and pass the fpinfos of joinrel, outer
and inner relation. The function will copy the values of server level
options and derive values for table level options. We would add a note
there to keep this function in sync with apply_*_options(). I don't
think there's any better way to keep the options in sync for base
relations and join relations.

Here's the patch attached.

Agreed. It seems like a good idea to keep that logic in a single location

I've beaten your patch around a bit and come up with the attached.

The changes from yours are mostly cosmetic, but I've also added a
regression test too.

What do you think?

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

foreign_outerjoin_pushdown_fix_v2.patchapplication/octet-stream; name=foreign_outerjoin_pushdown_fix_v2.patchDownload+161-75
#8Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: David Rowley (#7)
Re: Foreign Join pushdowns not working properly for outer joins

Here's the patch attached.

Agreed. It seems like a good idea to keep that logic in a single location

Ok.

I've beaten your patch around a bit and come up with the attached.

The new name merge_fdw_options() is shorter than the one I chose, but
we are not exactly merging options for an upper relation since there
isn't the other fpinfo to merge from. But I think we can live with
merge_fdw_options().

Looks like you forgot to compile the patch. It gave error

postgres_fdw.c: In function ‘merge_fdw_options’:
postgres_fdw.c:4337:2: error: ‘RelOptInfo’ has no member named ‘server’
postgres_fdw.c:4337:2: error: ‘RelOptInfo’ has no member named ‘server’

Once I fixed that, the testcases started showing an assertion failure,
since fpinfo of a base relation can not have an outerrel. Fixed the
assertion as well. If we are passing fpinfo's of joining relations,
there's no need to have outerrel and innerrel in fpinfo of join.

Also, you had removed comment
/*
+     * Set the foreign server to which this join will be shipped if found safe
+     * to push-down. We need server specific options like extensions to decide
+     * push-down safety, so set FDW specific options.
+     */
But I think it's important to have it there, so that reader knows why
merge_fdw_options() needs to be called before assessing quals. I have
updated it a bit to clarify this further. Also, merge_fdw_options()
shouldn't set fpinfo->server since it's not an FDW option per say.
It's better to keep that outside of this function. With your patch
fpinfo->server was being set twice for an upper relation.

The changes from yours are mostly cosmetic, but I've also added a
regression test too.

Thanks a lot for the test.

PFA updated patch.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

foreign_outerjoin_pushdown_fix_v3.patchapplication/octet-stream; name=foreign_outerjoin_pushdown_fix_v3.patchDownload+158-71
#9David Rowley
dgrowleyml@gmail.com
In reply to: Ashutosh Bapat (#8)
Re: Foreign Join pushdowns not working properly for outer joins

On 9 March 2017 at 18:06, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Here's the patch attached.

Agreed. It seems like a good idea to keep that logic in a single location

Ok.

I've beaten your patch around a bit and come up with the attached.

The new name merge_fdw_options() is shorter than the one I chose, but
we are not exactly merging options for an upper relation since there
isn't the other fpinfo to merge from. But I think we can live with
merge_fdw_options().

Perhaps "combine" is a better word? I didn't really see a problem with
that. After I posted this I wished I'd made the function even more
generic to accept either side as NULL and make up the new fpinfo from
the non-NULL one, or Merge if both were non-NULL. I liked that way
much better than giving the function too much knowledge of what its
purpose actually is. It's more likely to get used for something else
in the future, which means there's less chance that someone else will
make the same mistake.

Looks like you forgot to compile the patch. It gave error

postgres_fdw.c: In function ‘merge_fdw_options’:
postgres_fdw.c:4337:2: error: ‘RelOptInfo’ has no member named ‘server’
postgres_fdw.c:4337:2: error: ‘RelOptInfo’ has no member named ‘server’

Seems I forgot to test with asserts enabled. Thanks for finding/fixing that

Once I fixed that, the testcases started showing an assertion failure,
since fpinfo of a base relation can not have an outerrel. Fixed the
assertion as well. If we are passing fpinfo's of joining relations,
there's no need to have outerrel and innerrel in fpinfo of join.

Looks like you forgot to update the comment on the Assert()

Also, you had removed comment
/*
+     * Set the foreign server to which this join will be shipped if found safe
+     * to push-down. We need server specific options like extensions to decide
+     * push-down safety, so set FDW specific options.
+     */
But I think it's important to have it there, so that reader knows why
merge_fdw_options() needs to be called before assessing quals.

No objections.

I have
updated it a bit to clarify this further. Also, merge_fdw_options()
shouldn't set fpinfo->server since it's not an FDW option per say.
It's better to keep that outside of this function. With your patch
fpinfo->server was being set twice for an upper relation.

Thanks for noticing. I intended that not to be there, but seems I
forgot to hit delete.

The changes from yours are mostly cosmetic, but I've also added a
regression test too.

Thanks a lot for the test.

PFA updated patch.

Thanks for making the chances.

I'd say it's ready to go, pending updating the out of date comment:

+ /* We must always have fpinfo_o and it must always have an outerrel */
+ Assert(fpinfo_o);

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: David Rowley (#9)
Re: Foreign Join pushdowns not working properly for outer joins

The new name merge_fdw_options() is shorter than the one I chose, but
we are not exactly merging options for an upper relation since there
isn't the other fpinfo to merge from. But I think we can live with
merge_fdw_options().

Perhaps "combine" is a better word? I didn't really see a problem with
that. After I posted this I wished I'd made the function even more
generic to accept either side as NULL and make up the new fpinfo from
the non-NULL one, or Merge if both were non-NULL. I liked that way
much better than giving the function too much knowledge of what its
purpose actually is. It's more likely to get used for something else
in the future, which means there's less chance that someone else will
make the same mistake.

It's more like copy for an upper rel and merge for a join rel. Your
point about making it side-agnostic is good but I doubt if we want to
spend more effort there. If somebody writes a code with the input plan
stuffed in the innerrel instead of the outerrel, s/he will notice it
immediately when testing as assertion would fail or there will be a
straight segfault. We will decide what to do then.

Once I fixed that, the testcases started showing an assertion failure,
since fpinfo of a base relation can not have an outerrel. Fixed the
assertion as well. If we are passing fpinfo's of joining relations,
there's no need to have outerrel and innerrel in fpinfo of join.

Looks like you forgot to update the comment on the Assert()

Yes and I also forgot to update the function prologue to refer to the
fpinfo_o/i instead of inner and outer relations. Attached patch
corrects it.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

foreign_outerjoin_pushdown_fix_v4.patchapplication/octet-stream; name=foreign_outerjoin_pushdown_fix_v4.patchDownload+160-71
#11Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Ashutosh Bapat (#10)
Re: Foreign Join pushdowns not working properly for outer joins

Added this to 2017/07 commitfest.

On Fri, Mar 10, 2017 at 10:03 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

The new name merge_fdw_options() is shorter than the one I chose, but
we are not exactly merging options for an upper relation since there
isn't the other fpinfo to merge from. But I think we can live with
merge_fdw_options().

Perhaps "combine" is a better word? I didn't really see a problem with
that. After I posted this I wished I'd made the function even more
generic to accept either side as NULL and make up the new fpinfo from
the non-NULL one, or Merge if both were non-NULL. I liked that way
much better than giving the function too much knowledge of what its
purpose actually is. It's more likely to get used for something else
in the future, which means there's less chance that someone else will
make the same mistake.

It's more like copy for an upper rel and merge for a join rel. Your
point about making it side-agnostic is good but I doubt if we want to
spend more effort there. If somebody writes a code with the input plan
stuffed in the innerrel instead of the outerrel, s/he will notice it
immediately when testing as assertion would fail or there will be a
straight segfault. We will decide what to do then.

Once I fixed that, the testcases started showing an assertion failure,
since fpinfo of a base relation can not have an outerrel. Fixed the
assertion as well. If we are passing fpinfo's of joining relations,
there's no need to have outerrel and innerrel in fpinfo of join.

Looks like you forgot to update the comment on the Assert()

Yes and I also forgot to update the function prologue to refer to the
fpinfo_o/i instead of inner and outer relations. Attached patch
corrects it.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12David Rowley
dgrowleyml@gmail.com
In reply to: Ashutosh Bapat (#10)
Re: Foreign Join pushdowns not working properly for outer joins

On 10 March 2017 at 17:33, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Yes and I also forgot to update the function prologue to refer to the
fpinfo_o/i instead of inner and outer relations. Attached patch
corrects it.

Hi Ashutosh,

This seems to have conflicted with 28b04787. Do you want to rebase, or should I?

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: David Rowley (#12)
Re: Foreign Join pushdowns not working properly for outer joins

On Wed, Apr 12, 2017 at 12:18 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 10 March 2017 at 17:33, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Yes and I also forgot to update the function prologue to refer to the
fpinfo_o/i instead of inner and outer relations. Attached patch
corrects it.

Hi Ashutosh,

This seems to have conflicted with 28b04787. Do you want to rebase, or should I?

Here you go.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

0001-Fix-reporting-of-violation-in-ExecConstraints-again.patchapplication/octet-stream; name=0001-Fix-reporting-of-violation-in-ExecConstraints-again.patchDownload+90-22
#14David Rowley
dgrowleyml@gmail.com
In reply to: Ashutosh Bapat (#13)
Re: Foreign Join pushdowns not working properly for outer joins

On 12 April 2017 at 21:45, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

On Wed, Apr 12, 2017 at 12:18 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 10 March 2017 at 17:33, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Yes and I also forgot to update the function prologue to refer to the
fpinfo_o/i instead of inner and outer relations. Attached patch
corrects it.

Hi Ashutosh,

This seems to have conflicted with 28b04787. Do you want to rebase, or should I?

Here you go.

Looks like the wrong patch.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#15Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: David Rowley (#14)
Re: Foreign Join pushdowns not working properly for outer joins

Sorry, here's the right one.

On Wed, Apr 12, 2017 at 6:27 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 12 April 2017 at 21:45, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

On Wed, Apr 12, 2017 at 12:18 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 10 March 2017 at 17:33, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Yes and I also forgot to update the function prologue to refer to the
fpinfo_o/i instead of inner and outer relations. Attached patch
corrects it.

Hi Ashutosh,

This seems to have conflicted with 28b04787. Do you want to rebase, or should I?

Here you go.

Looks like the wrong patch.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

foreign_outerjoin_pushdown_fix_v5.patchapplication/octet-stream; name=foreign_outerjoin_pushdown_fix_v5.patchDownload+160-72
#16Peter Eisentraut
peter_e@gmx.net
In reply to: Ashutosh Bapat (#15)
Re: Foreign Join pushdowns not working properly for outer joins

Is this patch considered ready for review as a backpatch candidate?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#17David Rowley
dgrowleyml@gmail.com
In reply to: Peter Eisentraut (#16)
Re: Foreign Join pushdowns not working properly for outer joins

On 13 April 2017 at 11:22, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Is this patch considered ready for review as a backpatch candidate?

Yes, however, the v5 patch is based on master. The v4 patch should
apply to 9.6. Diffing the two patches I see another tiny change to a
comment, of which I think needs re-worded anyway.

+ * This function should usually set FDW options in fpinfo after the join is
+ * deemed safe to push down to save some CPU cycles. But We need server
+ * specific options like extensions to decide push-down safety. For
+ * checking extension shippability, we need foreign server as well.
+ */

This might be better written as:

Ordinarily, we might be tempted into delaying the merging of the FDW
options until we've deemed the foreign join to be ok. However, we must
do this before performing this test so that we know which quals can be
evaluated on the foreign server. This may depend on the
shippable_extensions.

Apart from that, it all looks fine to me.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#18Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: David Rowley (#17)
Re: Foreign Join pushdowns not working properly for outer joins

On Thu, Apr 13, 2017 at 7:35 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 13 April 2017 at 11:22, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Is this patch considered ready for review as a backpatch candidate?

Yes, however, the v5 patch is based on master. The v4 patch should
apply to 9.6. Diffing the two patches I see another tiny change to a
comment, of which I think needs re-worded anyway.

+ * This function should usually set FDW options in fpinfo after the join is
+ * deemed safe to push down to save some CPU cycles. But We need server
+ * specific options like extensions to decide push-down safety. For
+ * checking extension shippability, we need foreign server as well.
+ */

This might be better written as:

Ordinarily, we might be tempted into delaying the merging of the FDW
options until we've deemed the foreign join to be ok. However, we must
do this before performing this test so that we know which quals can be
evaluated on the foreign server. This may depend on the
shippable_extensions.

This looks better. Here are patches for master and 9.6.
Since join pushdown was supported in 9.6 the patch should be
backported to 9.6 as well. Attached is the patch (_96) for 9.6,
created by rebasing on 9.6 branch and removing conflict. _v6 is
applicable on master.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

foreign_outerjoin_pushdown_fix_96.patchapplication/octet-stream; name=foreign_outerjoin_pushdown_fix_96.patchDownload+158-54
foreign_outerjoin_pushdown_fix_v6.patchapplication/octet-stream; name=foreign_outerjoin_pushdown_fix_v6.patchDownload+161-72
#19Peter Eisentraut
peter_e@gmx.net
In reply to: Ashutosh Bapat (#18)
Re: Foreign Join pushdowns not working properly for outer joins

On 4/14/17 00:24, Ashutosh Bapat wrote:

This looks better. Here are patches for master and 9.6.
Since join pushdown was supported in 9.6 the patch should be
backported to 9.6 as well. Attached is the patch (_96) for 9.6,
created by rebasing on 9.6 branch and removing conflict. _v6 is
applicable on master.

Committed to PG10. I'll work on backpatching next.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#20Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Peter Eisentraut (#19)
Re: Foreign Join pushdowns not working properly for outer joins

On Tue, Apr 25, 2017 at 8:20 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 4/14/17 00:24, Ashutosh Bapat wrote:

This looks better. Here are patches for master and 9.6.
Since join pushdown was supported in 9.6 the patch should be
backported to 9.6 as well. Attached is the patch (_96) for 9.6,
created by rebasing on 9.6 branch and removing conflict. _v6 is
applicable on master.

Committed to PG10. I'll work on backpatching next.

Thanks.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#21Peter Eisentraut
peter_e@gmx.net
In reply to: Peter Eisentraut (#19)
#22Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Peter Eisentraut (#21)
#23David Rowley
dgrowleyml@gmail.com
In reply to: Peter Eisentraut (#21)
#24Peter Eisentraut
peter_e@gmx.net
In reply to: David Rowley (#23)
#25David Rowley
dgrowleyml@gmail.com
In reply to: Peter Eisentraut (#24)