Re: Push down more full joins in postgres_fdw

On 2016/11/11 19:22, Ashutosh Bapat wrote:
> The patch looks in good shape now.

Thanks for the review!

> The patch looks in good shape now. Here are some comments. I have also
> made several changes to comments correcting grammar, typos, style and
> at few places logic. Let me know if the patch looks good.

OK, will look into that.

> I guess, below code
> + if (!fpinfo->subquery_rels)
> + return false;
> can be changed to
> if (!bms_is_member(node->varno, fpinfo->subquery_rels))
> return false;
> Also the return values from the recursive calls to isSubqueryExpr() can be
> returned as is. I have included this change in the patch.

Will look into that too.

> deparse.c seems to be using capitalized names for function which
> actually deparse something and an non-capitalized form for helper
> functions.

That's not true. There is a function named classifyConditions(). The
function naming in deparse.c is a bit arbitrary.

> From that perspective attached patch renames isSubqueryExpr
> as is_subquery_var() and getSubselectAliasInfo() as
> get_alias_id_for_var(). Actually both these functions accept a Var
> node but somehow their names refer to expr.

The reason why I named that function isSubqueryExpr is that I think that
function would be soon extended so as to handle PHVs. See another patch
for evaluating PHVs remotely.

> This patch is using make_tlist_from_pathtarget() to create tlist to be
> deparsed but search in RelOptInfo::reltarget::exprs for a given Var.
> As long as the relations deparsed do not carry expressions, this might
> work, but it will certainly break once we start deparsing relations
> with expressions since the upper most query's tlist contains only
> Vars. Instead, we should probably, create tlist and save it in fpinfo
> and use it later for searching (tlist_member()?). Possibly use using
> build_tlist_to_deparse(), to create the tlist similar so that
> targetlist list creation logic is same for all the relations being
> deparsed. I haven't included this change in the patch.

Seems like a good idea. Will revise.

Best regards,
Etsuro Fujita