|From:||Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>|
|Subject:||Re: Push down more full joins in postgres_fdw|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 2016/11/11 20:50, Etsuro Fujita wrote:
> On 2016/11/11 19:22, Ashutosh Bapat wrote:
>> 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.
Done. +1 for the changes you made, except for few things; (1) You added
the following comments to deparseSelectSql:
+ * For a non-base relation, use the input tlist. If a base
+ * being deparsed as a subquery, use input tlist, if the caller
+ * one. The column aliases of the subquery are crafted based on
+ * tlist. If tlist is NIL, there will not be any expression
+ * any of the columns of the base relation being deparsed. Hence
+ * matter whether the base relation is being deparsed as
subquery or not.
The last sentence seems confusing to me. My understanding is: if a base
relation has tlist=NIL, then the base relation *must* be deparsed as
ordinary, not as a subquery. Maybe I'm missing something, though. (I
left that as-is, but I think we need to reword that to be more clear, at
(2) You added the following comments to deparseRangeTblRef:
> + * If make_subquery is true, deparse the relation as a subquery.
> + * deparse it as relation name with alias.
The second sentence seems confusing to me, too, because it looks like
the relation being deparsed is assumed to be a base relation, but the
relation can be a join relation, which might join base relations, lower
join relations, and/or lower subqueries. So, I modified the sentence a bit.
(3) I don't think we need this in isSubqueryExpr, so I removed it from
+ /* Keep compiler happy. */
+ return false;
Also, I revised comments you added a little bit.
>> 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.
Done. That's a good idea!
>> deparse.c seems to be using capitalized names for function which
>> actually deparse something and an non-capitalized form for helper
>> 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.
OK, I changed isSubqueryExpr to is_subquery_expr; I kept to refer to
expr because I think we would soon extend that function so that it can
handle PHVs, as I said upthread. For getSubselectAliasInfo, I changed
the name to get_subselect_alias_id, because (1) the word "alias" seems
general and (2) the "for_var" part would make the name a bit long.
>> 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.
Done. I modified the patch as proposed; create the tlist by
build_tlist_to_deparse in foreign_join_ok, if needed, and search the
tlist by tlist_member. I also added a new member "tlist" to
PgFdwRelationInfo to save the tlist created in foreign_join_ok.
Another idea on the "tlist" member would be to save a tlist created for
EXPLAIN into that member in estimate_patch_cost_size, so that we don't
need to generate the tlist again in postgresGetForeignPlan, when
use_remote_estimate=true. But I'd like to leave that for another patch.
Please find attached an updated version of the patch.
|Next Message||Amit Kapila||2016-11-15 12:59:04||Re: Fix checkpoint skip logic on idle systems by tracking LSN progress|
|Previous Message||Daniel Verite||2016-11-15 12:09:45||Re: Improvements in psql hooks for variables|