Message-ID: <5C6251A7.40204@lab.ntt.co.jp>
Date: Tue, 12 Feb 2019 13:55:03 +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>
In-Reply-To: <5C5D778F.3080501@lab.ntt.co.jp>
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/08 21:35), Etsuro Fujita wrote:
> (2019/02/08 2:04), Antonin Houska wrote:
>> * add_foreign_ordered_paths()
>>
>> Exprssion root->upper_targets[UPPERREL_FINAL] is used to access the
>> target.
>>
>> I think create_ordered_paths() should actually set the target so that
>> postgresGetForeignUpperPaths() can simply find it in
>> output_rel->reltarget. This is how postgres_fdw already retrieves the
>> target
>> for grouped paths. Small patch is attached that makes this possible.
>
> Seems like a good idea. Thanks for the patch! Will review.

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;

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().  Having said that, my question here is: 
do we really need this (and the same for UPPERREL_FINAL you added)?  For 
the purpose of the FDW convenience, the upper_targets[] change seems to 
me to be enough.

Best regards,
Etsuro Fujita