Re: Push down more full joins in postgres_fdw

On Mon, Nov 28, 2016 at 3:52 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2016/11/24 21:45, Etsuro Fujita wrote:
>>
>> On 2016/11/22 18:28, Ashutosh Bapat wrote:
>>>
>>> The comments should explain why is the assertion true.
>>> + /* Shouldn't be NIL */
>>> + Assert(tlist != NIL);
>
>
>> I noticed that I was wrong; in the Assertion the tlist can be empty. An
>> example for such a case is:
>>
>> SELECT 1 FROM (SELECT c1 FROM ft4 WHERE c1 between 50 and 60) t1 FULL
>> JOIN (SELECT c1 FROM ft5 WHERE c1 between 50 and 60) t2 ON (TRUE);
>>
>> In this example any columns of the relations ft4 and ft5 wouldn't be
>> needed for the join or the final output, so both the tlists for the
>> relations created in deparseRangeTblRef would be empty. Attached is an
>> updated version fixing this bug. Changes are:
>>
>> * I removed make_outerrel_subquery/make_innerrel_subquery from fpinfo
>> and added a new member "is_subquery_rel" to fpinfo, as a flag on whether
>> to deparse the relation as a subquery.

Replacing make_outerrel_subquery/make_innerrel_subquery with is_subquery_rel
seems to be a bad idea. Whether a relation needs to be deparsed as a subquery
or not depends upon the relation which joins it with another relation. Take for
example a case when a join ABC can pull up the conditions on AB, but ABD can
not. In this case we will mark that AB in ABD needs to be deparsed as a
subquery but will not mark so in ABC. So, if we choose to join ABCD as (ABC)D
we don't need AB to be deparsed as a subquery. If we choose to join ABCD as
(ABD)C, we will need AB to deparsed as a subquery. Regression doesn't have a
testcase like that hence the code seems to be working.

Some more comments
1. The output of the following query
+SELECT 1 FROM (SELECT c1 FROM ft4 WHERE c1 between 50 and 60) t1 FULL
JOIN (SELECT c1 FROM ft5 WHERE c1 between 50 and 60) t2 ON (TRUE);
produces 21 rows all containing a "1". That's not quite good use of expected
output space, given that all that the test tests is the empty
targetlists are deparsed
correctly. You might want to either just test EXPLAIN output or make "between 50
and 60" condition more stringent to reduce the number of rows output.

2. Got lost here. I guess, all you need to say is "test deparsing FOR UPDATE
clause with subqueries.". Anyway, the sentence is very hard to read and needs
simplification.
+-- d. test deparsing the remote queries for simple foreign table scans in
+-- an EPQ subplan of a foreign join in which the foreign tables are full
+-- joined and in the remote join query the foreign tables are deparsed as
+-- subqueries

3. Why is foreignrel variable changed to rel?
-extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root,
- RelOptInfo *foreignrel, List *tlist,
- List *remote_conds, List *pathkeys,
- List **retrieved_attrs, List **params_list);
+extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo
*root, RelOptInfo *rel,
+ List *tlist, List *remote_conds, List *pathkeys,
+ bool is_subquery, List **retrieved_attrs,
+ List **params_list);

4. I am still not happy with this change
+ /*
+ * Since (1) the expressions in foreignrel's reltarget doesn't contain
+ * any PHVs and (2) foreignrel's local_conds is empty, the tlist
+ * created by build_tlist_to_deparse must be one-to-one with the
+ * expressions.
+ */
+ Assert(list_length(tlist) ==
list_length(foreignrel->reltarget->exprs));
the assertion only checks that the number of elements in both the lists are
same but does not check whether those lists are same i.e. they contain the same
elements in the same order. This equality is crucial to deparsing logic. If
somehow build_tlist_to_deparse() breaks that assumption in future, we have no
way to detect it, unless a regression test fails.

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