Re: Push down more full joins in postgres_fdw

Thanks.

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

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?

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

OK.

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


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


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

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

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