Re: Push down more full joins in postgres_fdw

On 2016/11/11 20:50, Etsuro Fujita wrote:
> On 2016/11/11 19:22, Ashutosh Bapat wrote:
>> The patch looks in good shape now. Here are some comments. I have also
>> made several changes to comments correcting grammar, typos, style and
>> at few places logic. Let me know if the patch looks good.

> OK, will look into that.

Done.  +1 for the changes you made, except for few things; (1) You added
the following comments to deparseSelectSql:

+       /*
+        * For a non-base relation, use the input tlist. If a base
relation is
+        * being deparsed as a subquery, use input tlist, if the caller
has passed
+        * one. The column aliases of the subquery are crafted based on
the input
+        * tlist. If tlist is NIL, there will not be any expression
referring to
+        * any of the columns of the base relation being deparsed. Hence
it doesn't
+        * matter whether the base relation is being deparsed as
subquery or not.
+        */

The last sentence seems confusing to me.  My understanding is: if a base
relation has tlist=NIL, then the base relation *must* be deparsed as
ordinary, not as a subquery.  Maybe I'm missing something, though.  (I
left that as-is, but I think we need to reword that to be more clear, at
least.)

(2) You added the following comments to deparseRangeTblRef:

 > +  * If make_subquery is true, deparse the relation as a subquery.
Otherwise,
 > +  * deparse it as relation name with alias.

The second sentence seems confusing to me, too, because it looks like
the relation being deparsed is assumed to be a base relation, but the
relation can be a join relation, which might join base relations, lower
join relations, and/or lower subqueries.  So, I modified the sentence a bit.

(3) I don't think we need this in isSubqueryExpr, so I removed it from
the patch:

+    /* Keep compiler happy. */
+    return false;

Also, I revised comments you added a little bit.

>> I guess, below code
>> +   if (!fpinfo->subquery_rels)
>> +       return false;
>> can be changed to
>>     if (!bms_is_member(node->varno, fpinfo->subquery_rels))
>>         return false;
>> Also the return values from the recursive calls to isSubqueryExpr()
>> can be
>> returned as is. I have included this change in the patch.

> Will look into that too.

Done.  That's a good idea!

>> deparse.c seems to be using capitalized names for function which
>> actually deparse something and an non-capitalized form for helper
>> functions.

>> From that perspective attached patch renames isSubqueryExpr
>> as is_subquery_var() and getSubselectAliasInfo() as
>> get_alias_id_for_var(). Actually both these functions accept a Var
>> node but somehow their names refer to expr.

OK, I changed isSubqueryExpr to is_subquery_expr; I kept to refer to
expr because I think we would soon extend that function so that it can
handle PHVs, as I said upthread.  For getSubselectAliasInfo, I changed
the name to get_subselect_alias_id, because (1) the word "alias" seems
general and (2) the "for_var" part would make the name a bit long.

>> This patch is using make_tlist_from_pathtarget() to create tlist to be
>> deparsed but search in RelOptInfo::reltarget::exprs for a given Var.
>> As long as the relations deparsed do not carry expressions, this might
>> work, but it will certainly break once we start deparsing relations
>> with expressions since the upper most query's tlist contains only
>> Vars. Instead, we should probably, create tlist and save it in fpinfo
>> and use it later for searching (tlist_member()?). Possibly use using
>> build_tlist_to_deparse(), to create the tlist similar so that
>> targetlist list creation logic is same for all the relations being
>> deparsed. I haven't included this change in the patch.

> Seems like a good idea.  Will revise.

Done.  I modified the patch as proposed; create the tlist by
build_tlist_to_deparse in foreign_join_ok, if needed, and search the
tlist by tlist_member.  I also added a new member "tlist" to
PgFdwRelationInfo to save the tlist created in foreign_join_ok.

Another idea on the "tlist" member would be to save a tlist created for
EXPLAIN into that member in estimate_patch_cost_size, so that we don't
need to generate the tlist again in postgresGetForeignPlan, when
use_remote_estimate=true.  But I'd like to leave that for another patch.

Please find attached an updated version of the patch.

Best regards,
Etsuro Fujita