Re: Push down more full joins in postgres_fdw

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Push down more full joins in postgres_fdw
Date: 2016-11-11 11:50:43
Message-ID: 8942907d-2d78-bb96-7e4d-9ff796f6c477@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2016-11-11 12:06:18 Re: [RFC] Should we fix postmaster to avoid slow shutdown?
Previous Message Ashutosh Bapat 2016-11-11 11:49:22 Re: Declarative partitioning - another take