Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 27 April 2017 at 01:31, Peter Eisentraut
 wrote:
> committed

Great. Thanks!

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Peter Eisentraut]

On 4/26/17 04:32, David Rowley wrote:
>> For backpatching to 9.6, I came up with the attached reduced version.
>> Since we don't have add_foreign_grouping_paths() in 9.6, we can omit the
>> refactoring and keep the changes much simpler.  Does that make sense?
> 
> That makes sense to me. It fixes the reported issue and is less
> invasive than the pg10 patch.

committed

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 26 April 2017 at 02:12, Peter Eisentraut
 wrote:
> On 4/24/17 22:50, Peter Eisentraut 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.
>
> For backpatching to 9.6, I came up with the attached reduced version.
> Since we don't have add_foreign_grouping_paths() in 9.6, we can omit the
> refactoring and keep the changes much simpler.  Does that make sense?

That makes sense to me. It fixes the reported issue and is less
invasive than the pg10 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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

On Tue, Apr 25, 2017 at 7:42 PM, Peter Eisentraut
 wrote:
> On 4/24/17 22:50, Peter Eisentraut 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.
>
> For backpatching to 9.6, I came up with the attached reduced version.
> Since we don't have add_foreign_grouping_paths() in 9.6, we can omit the
> refactoring and keep the changes much simpler.  Does that make sense?

Looks good to me.

There's minor complaint. In case we change the option related
functions in master because of a bug, backpatching those changes to
9.6 may not be straightforward. There's very less chance that we will
require to change those functions, so may be we can take that
theoretical risk.
-- 
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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Peter Eisentraut]

On 4/24/17 22:50, Peter Eisentraut 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.

For backpatching to 9.6, I came up with the attached reduced version.
Since we don't have add_foreign_grouping_paths() in 9.6, we can omit the
refactoring and keep the changes much simpler.  Does that make sense?

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


0001-postgres_fdw-Fix-join-push-down-with-extensions.patch
Description: invalid/octet-stream

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

On Tue, Apr 25, 2017 at 8:20 AM, Peter Eisentraut
 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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Peter Eisentraut]

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

On Thu, Apr 13, 2017 at 7:35 AM, David Rowley
 wrote:
> On 13 April 2017 at 11:22, Peter Eisentraut
>  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


foreign_outerjoin_pushdown_fix_96.patch
Description: Binary data


foreign_outerjoin_pushdown_fix_v6.patch
Description: Binary data

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 13 April 2017 at 11:22, Peter Eisentraut
 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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Peter Eisentraut]

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

Sorry, here's the right one.

On Wed, Apr 12, 2017 at 6:27 PM, David Rowley
 wrote:
> On 12 April 2017 at 21:45, Ashutosh Bapat
>  wrote:
>> On Wed, Apr 12, 2017 at 12:18 PM, David Rowley
>>  wrote:
>>> On 10 March 2017 at 17:33, Ashutosh Bapat
>>>  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


foreign_outerjoin_pushdown_fix_v5.patch
Description: Binary data

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 12 April 2017 at 21:45, Ashutosh Bapat
 wrote:
> On Wed, Apr 12, 2017 at 12:18 PM, David Rowley
>  wrote:
>> On 10 March 2017 at 17:33, Ashutosh Bapat
>>  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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

On Wed, Apr 12, 2017 at 12:18 PM, David Rowley
 wrote:
> On 10 March 2017 at 17:33, Ashutosh Bapat
>  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


0001-Fix-reporting-of-violation-in-ExecConstraints-again.patch
Description: Binary data

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 10 March 2017 at 17:33, Ashutosh Bapat
 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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

Added this to 2017/07 commitfest.

On Fri, Mar 10, 2017 at 10:03 AM, Ashutosh Bapat
 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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

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


foreign_outerjoin_pushdown_fix_v4.patch
Description: Binary data

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 9 March 2017 at 18:06, Ashutosh Bapat
 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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

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


foreign_outerjoin_pushdown_fix_v3.patch
Description: Binary data

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 7 March 2017 at 01:22, Ashutosh Bapat
 wrote:
> On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
>  wrote:
>> On 6 March 2017 at 18:51, Etsuro Fujita  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


foreign_outerjoin_pushdown_fix_v2.patch
Description: Binary data

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

On Tue, Mar 7, 2017 at 9:33 AM, Etsuro Fujita
 wrote:
> On 2017/03/06 21:22, Ashutosh Bapat wrote:
>>
>> On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
>>  wrote:
>>>
>>> On 6 March 2017 at 18:51, Etsuro Fujita 
>>> 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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Etsuro Fujita]

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

On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
 wrote:

On 6 March 2017 at 18:51, Etsuro Fujita  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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
 wrote:
> On 6 March 2017 at 18:51, Etsuro Fujita  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


foreign_join_upperrel_pushdown_fix.patch
Description: Binary data

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 6 March 2017 at 18:51, Etsuro Fujita  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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Etsuro Fujita]

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


foreign_outerjoin_pushdown_fix_efujita.patch
Description: binary/octet-stream

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

[HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

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,
  >remote_conds, >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


test_caee.sql
Description: Binary data


foreign_outerjoin_pushdown_fix.patch
Description: Binary data

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