Re: Push down more full joins in postgres_fdw

On 2016/11/16 18:14, Ashutosh Bapat wrote:
>> (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.)

> Hmm, I agree. I think the comment should just say, "Use tlist to
> create the SELECT clause if one has been provided. For a base relation
> with tlist = NIL, check attrs_needed information.". Does that sound
> good?

I don't think that is 100% correct, because (1) tlist can be NIL for a
join relation, you pointed out upthread, but we need to use
deparseExplicitTargetList, so the first sentence is not completely
correct, and (2) we need to check attrs_needed information not only for
a baserel but for an otherrel, so the second sentence is not completely
correct, either. How about this, instead?:

/*
* For a join relation or an upper relation, use
deparseExplicitTargetList.
* Likewise, for a base relation that is being deparsed as a
subquery, in
* which case the caller would have passed tlist that is non-NIL,
use that
* function. Otherwise, use deparseTargetList.
*/

>> (3) I don't think we need this in isSubqueryExpr, so I removed it from the
>> patch:
>>
>> + /* Keep compiler happy. */
>> + return false;

> Doesn't that cause compiler warning, saying "non-void function
> returning nothing" or something like that. Actually, the "if
> (bms_is_member(node->varno, outerrel->relids))" ends with a "return"
> always. Hence we don't need to encapsulate the code in "else" block in
> else { }. It could be taken outside.

Yeah, I think so too, but I like the "if () { } else { }" coding. That
coding can be found in other places in core, eg,
operator_same_subexprs_lookup.

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

> is_subquery_expr(Var *node -- that looks odd. Either it should
> is_subquery_var(Var * ... OR it should be is_subquery_expr(Expr * ...
> . I would prefer the first one, since that's what current patch is
> doing. When we introduce PHVs, we may change it, if required.

OK, I used is_subquery_var().

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

> Instead of adding a new member, you might want to reuse grouped_tlist
> by renaming it.

Done.

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

> I think that will happen automatically, while deparsing, whether for
> EXPLAIN or for actual execution.

Really? Anyway, I'd like to leave that as-is.

Please find attached a new version of the patch.

Best regards,
Etsuro Fujita