Re: Problems with plan estimates in postgres_fdw

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Antonin Houska <ah(at)cybertec(dot)at>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Problems with plan estimates in postgres_fdw
Date: 2019-02-15 12:19:30
Message-ID: 5C66AE52.4020605@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

(2019/02/14 18:44), Etsuro Fujita wrote:
> (2019/02/12 20:43), Antonin Houska wrote:
>> Etsuro Fujita<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> Here are my review comments:
>>>
>>> root->upper_targets[UPPERREL_FINAL] = final_target;
>>> + root->upper_targets[UPPERREL_ORDERED] = final_target;
>>> root->upper_targets[UPPERREL_WINDOW] = sort_input_target;
>>> root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target;
>>>
>>> I think that's a good idea. I think we'll soon need the PathTarget for
>>> UPPERREL_DISTINCT, so how about saving that as well like this?
>>>
>>> root->upper_targets[UPPERREL_FINAL] = final_target;
>>> + root->upper_targets[UPPERREL_ORDERED] = final_target;
>>> + root->upper_targets[UPPERREL_DISTINCT] = sort_input_target;
>>> root->upper_targets[UPPERREL_WINDOW] = sort_input_target;
>>> root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target;
>>
>> I see nothing wrong about this.
>
> Cool!
>
>>> Another is about this:
>>>
>>> /*
>>> + * Set reltarget so that it's consistent with the paths. Also it's more
>>> + * convenient for FDW to find the target here.
>>> + */
>>> + ordered_rel->reltarget = target;
>>>
>>> I think this is correct if there are no SRFs in the targetlist, but
>>> otherwise
>>> I'm not sure this is really correct because the relation's reltarget
>>> would not
>>> match the target of paths added to the relation after
>>> adjust_paths_for_srfs().
>>
>> I wouldn't expect problem here. create_grouping_paths() also sets the
>> reltarget although adjust_paths_for_srfs() can be alled on grouped_rel
>> afterwards.
>
> Yeah, but as I said in a previous email, I think the root->upper_targets
> change would be enough at least currently for FDWs to consider
> performing post-UPPERREL_GROUP_AGG steps remotely, as shown in my
> patches, so I'm inclined to leave the retarget change for future work.

So, here is an updated patch. If there are no objections from you or
anyone else, I'll commit the patch as a preliminary one for what's
proposed in this thread.

Best regards,
Etsuro Fujita

Attachment Content-Type Size
0001-Save-PathTargets-for-distinct-ordered-relations-in-r.patch text/x-patch 1.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ideriha, Takeshi 2019-02-15 12:31:52 RE: Protect syscache from bloating with negative cache entries
Previous Message Michael Meskes 2019-02-15 12:11:45 Re: [PROPOSAL]a new data type 'bytea' for ECPG