Re: [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

On Tue, Jun 14, 2016 at 4:06 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2016/06/14 6:51, Robert Haas wrote:
>> On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> Although I have done a bit of review of this patch, it needs more
>>> thought than I have so far had time to give it.  I will update again
>>> by Tuesday.
>>
>> I've reviewed this a bit further and have discovered an infelicity.
>> The following is all with the patch applied.
>>
>> By itself, this join can be pushed down:
>>
>> contrib_regression=# EXPLAIN SELECT 13 FROM ft1 RIGHT JOIN ft2 ON
>> ft1.c1 = ft2.c1;
>>                       QUERY PLAN
>> ------------------------------------------------------
>>  Foreign Scan  (cost=100.00..137.66 rows=822 width=4)
>>    Relations: (public.ft2) LEFT JOIN (public.ft1)
>> (2 rows)
>>
>> That's great.  However, when that query is used as the outer rel of a
>> left join, it can't:
>>
>> contrib_regression=# explain verbose select * from ft4 LEFT JOIN
>> (SELECT 13 FROM ft1 RIGHT JOIN ft2 ON ft1.c1 = ft2.c1) q on true;
>>                                          QUERY PLAN
>> ---------------------------------------------------------------------------------------------
>>  Nested Loop Left Join  (cost=353.50..920.77 rows=41100 width=19)
>>    Output: ft4.c1, ft4.c2, ft4.c3, (13)
>>    ->  Foreign Scan on public.ft4  (cost=100.00..102.50 rows=50 width=15)
>>          Output: ft4.c1, ft4.c2, ft4.c3
>>          Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
>>    ->  Materialize  (cost=253.50..306.57 rows=822 width=4)
>>          Output: (13)
>>          ->  Hash Left Join  (cost=253.50..302.46 rows=822 width=4)
>>                Output: 13
>>                Hash Cond: (ft2.c1 = ft1.c1)
>>                ->  Foreign Scan on public.ft2  (cost=100.00..137.66
>> rows=822 width=4)
>>                      Output: ft2.c1
>>                      Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
>>                ->  Hash  (cost=141.00..141.00 rows=1000 width=4)
>>                      Output: ft1.c1
>>                      ->  Foreign Scan on public.ft1
>> (cost=100.00..141.00 rows=1000 width=4)
>>                            Output: ft1.c1
>>                            Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
>> (18 rows)
>>
>> Of course, because of the PlaceHolderVar, there's no way to push down
>> the ft1-ft2-ft4 join to the remote side.  But we could still push down
>> the ft1-ft2 join and then locally perform the join between the result
>> and ft4.  However, the proposed fix doesn't allow that, because
>> ph_eval_at is (b 4 5) and relids for the ft1-ft2 join is also (b 4 5),
>> and so the bms_is_subset(phinfo->ph_eval_at, relids) test returns
>> true.
>
> You're right.  It indeed should be possible to push down ft1-ft2 join.
> However it could not be done without also modifying
> build_tlist_to_deparse() a little (as Ashutosh proposed [1] to do
> upthread).  Attached new version of the patch with following changes:
>
> 1) Modified the check in foreign_join_ok() so that it is not overly
> restrictive.  Previously, it used ph_needed as the highest level at which
> the PHV is needed (by definition) and checked using it whether it is above
> the join under consideration, which ended up being an overkill.  ISTM, we
> can just decide from joinrel->relids of the join under consideration
> whether we are above the lowest level where the PHV could be evaluated
> (ph_eval_at) and stop if so.  So in the example you provided, it won't now
> reject {ft1, ft2}, but will {ft4, ft1, ft2}.
>
> 2) Modified build_tlist_to_deparse() to get rid of the PlaceHolderVars in
> the targetlist to deparse by using PVC_RECURSE_PLACEHOLDER flag of
> pull_var_clause().  That is because we do not yet support anything other
> than Vars in deparseExplicitTargetList() (+1 to your patch to change the
> Assert to elog).

OK, I committed this version with some cosmetic changes.  I ripped out
the header comment to foreign_join_ok(), which is just a nuisance to
maintain.  It unnecessarily recapitulates the tests contained within
the function, requiring us to update the comments in two places
instead of one every time we make a change for no real gain.  And I
rewrote the comment you added.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company