Re: pgsql_fdw in contrib

On Tue, Jun 26, 2012 at 10:50 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> * Regarding to deparseSimpleSql(), it pulls attributes being referenced
> from baserestrictinfo and reltargetlist using pull_var_clause().
> Is it unavailable to use local_conds instead of baserestrictinfo?
> We can optimize references to the variable being consumed at the
> remote side only. All we need to transfer is variables referenced
> in targetlist and local-clauses.

Indeed, fixed.

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

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

> In addition, it checks whether relkind is RELKIND_FOREIGN_TABLE,
> or not. It seems to me this check should be Assert(), if routines of
> pgsql_fdw is called towards regular relations.
>
> * Regarding to deparseVar(), is it unnecessary to check relkind of
> the relation being referenced by Var node, isn't it?

IIRC, this is remain of past trial for general deparser... Removed
unnecessary relkind checks. Fixed.

> * Regarding to deparseBoolExpr(), compiler raised a warning
> because op can be used without initialized.

Fixed.

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

Regards,
--
Shigeru Hanada