Message-ID: <5C653888.70705@lab.ntt.co.jp>
Date: Thu, 14 Feb 2019 18:44:40 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: Problems with plan estimates in postgres_fdw
References: <87pnz1aby9.fsf@news-spur.riddles.org.uk>
 <1175.1533220908@sss.pgh.pa.us> <5BB739CD.6040002@lab.ntt.co.jp>
 <5BBC4140.50403@lab.ntt.co.jp> <5C17A00E.5000005@lab.ntt.co.jp>
 <5C232F39.9060509@lab.ntt.co.jp> <5C25C7B5.2010802@lab.ntt.co.jp>
 <5C518631.5080401@lab.ntt.co.jp> <10994.1549559088@localhost>
 <5C5D778F.3080501@lab.ntt.co.jp> <5C6251A7.40204@lab.ntt.co.jp>
 <26618.1549971814@localhost>
In-Reply-To: <26618.1549971814@localhost>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
To: Antonin Houska <ah@cybertec.at>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Andrew Gierth <andrew@tao11.riddles.org.uk>,
        pgsql-hackers@postgresql.org
Precedence: bulk

(2019/02/12 20:43), Antonin Houska wrote:
> Etsuro Fujita<fujita.etsuro@lab.ntt.co.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.

Best regards,
Etsuro Fujita