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-17 05:55:21 |
Message-ID: | 0f94310b-0d9a-6cec-bfb7-65a4d0171f49@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2016/11/16 18:14, Ashutosh Bapat wrote:
>> (1) You added the
>> following comments to deparseSelectSql:
>>
>> + /*
>> + * For a non-base relation, use the input tlist. If a base relation
>> is
>> + * being deparsed as a subquery, use input tlist, if the caller has
>> passed
>> + * one. The column aliases of the subquery are crafted based on the
>> input
>> + * tlist. If tlist is NIL, there will not be any expression
>> referring to
>> + * any of the columns of the base relation being deparsed. Hence it
>> doesn't
>> + * 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 least.)
> Hmm, I agree. I think the comment should just say, "Use tlist to
> create the SELECT clause if one has been provided. For a base relation
> with tlist = NIL, check attrs_needed information.". Does that sound
> good?
I don't think that is 100% correct, because (1) tlist can be NIL for a
join relation, you pointed out upthread, but we need to use
deparseExplicitTargetList, so the first sentence is not completely
correct, and (2) we need to check attrs_needed information not only for
a baserel but for an otherrel, so the second sentence is not completely
correct, either. How about this, instead?:
/*
* For a join relation or an upper relation, use
deparseExplicitTargetList.
* Likewise, for a base relation that is being deparsed as a
subquery, in
* which case the caller would have passed tlist that is non-NIL,
use that
* function. Otherwise, use deparseTargetList.
*/
>> (3) I don't think we need this in isSubqueryExpr, so I removed it from the
>> patch:
>>
>> + /* Keep compiler happy. */
>> + return false;
> Doesn't that cause compiler warning, saying "non-void function
> returning nothing" or something like that. Actually, the "if
> (bms_is_member(node->varno, outerrel->relids))" ends with a "return"
> always. Hence we don't need to encapsulate the code in "else" block in
> else { }. It could be taken outside.
Yeah, I think so too, but I like the "if () { } else { }" coding. That
coding can be found in other places in core, eg,
operator_same_subexprs_lookup.
>> 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.
> is_subquery_expr(Var *node -- that looks odd. Either it should
> is_subquery_var(Var * ... OR it should be is_subquery_expr(Expr * ...
> . I would prefer the first one, since that's what current patch is
> doing. When we introduce PHVs, we may change it, if required.
OK, I used is_subquery_var().
>> 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.
> Instead of adding a new member, you might want to reuse grouped_tlist
> by renaming it.
Done.
>> 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.
> I think that will happen automatically, while deparsing, whether for
> EXPLAIN or for actual execution.
Really? Anyway, I'd like to leave that as-is.
Please find attached a new version of the patch.
Best regards,
Etsuro Fujita
Attachment | Content-Type | Size |
---|---|---|
postgres-fdw-subquery-support-v7.patch | application/x-patch | 31.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tsunakawa, Takayuki | 2016-11-17 06:10:20 | Re: [bug fix] Cascading standby cannot catch up and get stuck emitting the same message repeatedly |
Previous Message | Tsunakawa, Takayuki | 2016-11-17 05:30:14 | Re: Remove the comment on the countereffectiveness of large shared_buffers on Windows |