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

On Tue, Jun 14, 2016 at 4:06 AM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2016/06/14 6:51, Robert Haas wrote:
>> On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas <robertmhaas(at)gmail(dot)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