Re: pgsql_fdw in contrib

2012/7/5 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
>> In addition, is pull_var_clause() reasonable to list up all the attribute
>> referenced at the both expression tree? It seems to be pull_varattnos()
>> is more useful API in this situation.
>
> Only for searching, yes. However, sooner or later we need Var objects
> to deparse remote SELECT clause, so pull_var_clause seems better here.
> ISTM one of advantage to use bitmap is matching with another bitmap,
> like index only scan code checks whether all used attributes is
> contained by indexed attributes.
>
It seems to me a reasonable decision.

>> * Regarding to deparseRelation(), it scans simple_rte_array to fetch
>> RangeTblEntry with relation-id of the target foreign table.
>> It does not match correct entry if same foreign table is appeared in
>> this list twice or more, like a case of self-join. I'd like to recommend
>> to use simple_rte_array[baserel->relid] instead.
>
> Reasonable idea. After some thoughts, I think that deparseRelation
> should receive RangeTblEntry instead of relation oid (and then
> PlannerInfo is not necessary).
>
I also like this design.

>> * Even though it is harmless, sortConditions() is a misleading function
>> name. How about categolizeConditions() or screeningConditions()?
>
> How about classifyConditions?
> # I hope native English speaker's help for wording issue... :-)
>
Even though I'm not a native English speaker, it seems to me
"classify" is less confusing term than "sort" in this context.
If it would be a matter, you can get pointed out on committer's
review. So, please go ahead.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>