Re: Foreign Join pushdowns not working properly for outer joins

From: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
To: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
Cc: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Foreign Join pushdowns not working properly for outer joins
Date: 2017-03-10 04:33:08
Message-ID: CAFjFpRfMOn9jMjg90LPy8VzxB87CrLYB=fH5h3w_LzzGmxuyow@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
foreign_outerjoin_pushdown_fix_v4.patch application/octet-stream 13.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2017-03-10 04:43:22 Re: Parallel Append implementation
Previous Message Peter Geoghegan 2017-03-10 04:21:39 Re: [PATCH] Use $ parameters as replacement characters for pg_stat_statements