On Mon, Nov 7, 2016 at 5:50 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2016/11/07 11:24, Etsuro Fujita wrote:
>>
>> On 2016/11/04 19:55, Etsuro Fujita wrote:
>>>
>>> Attached is an updated version of the patch.
>
>
>> I noticed that I have included an unrelated regression test in the
>> patch. Attached is a patch with the test removed.
>
>
> I noticed that I inadvertently removed some changes from that patch, so I
> fixed that. Please find attached an updated version of the patch. I'm also
> attaching an updated version of another patch for evaluating PHVs remotely,
> which has been created on top of that patch. Changes to the latter: I
> revised comments and added a bit more regression tests.
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.
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.
deparse.c seems to be using capitalized names for function which
actually deparse something and an non-capitalized form for helper
functions. 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.
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.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company