Also another point
I guess, this note doesn't add much value in the given context.
Probably we should remove it.
+ * Note: the tlist would have one-to-one correspondence with the
+ * joining relation's reltarget->exprs because (1) the above test
+ * on PHVs guarantees that the reltarget->exprs doesn't contain
+ * any PHVs and (2) the joining relation's local_conds is NIL.
+ * This allows us to search the targetlist entry matching a given
+ * Var node from the tlist in get_subselect_alias_id.
On Fri, Nov 18, 2016 at 5:10 PM, Ashutosh Bapat
<ashutosh.ba...@enterprisedb.com> wrote:
>>
>> /*
>> * 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.
>> */
>
> This looks correct. I have modified it to make it simple in the given
> patch. But, I think we shouldn't refer to a function e.g.
> deparseExplicitTargetlist() in the comment. Instead we should describe
> the intent e.g. "deparse SELECT clause from the given targetlist" or
> "deparse SELECT clause from attr_needed".
>
>>
>>>> (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.
>
>
>>
>>>> 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.
>
> Right now, we are calculating tlist whether or not the ForeignPath
> emerges as the cheapest path. Instead we should calculate tlist, the
> first time we need it and then add it to the fpinfo.
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
