From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
To: Antonin Houska <ah@cybertec.at>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andrew Gierth <andrew@tao11.riddles.org.uk>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Problems with plan estimates in postgres_fdw
Date: Tue, 12 Feb 2019 13:55:03 +0900
Message-ID: <5C6251A7.40204@lab.ntt.co.jp> (raw)
In-Reply-To: <5C5D778F.3080501@lab.ntt.co.jp>
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>
(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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: fujita.etsuro@lab.ntt.co.jp, ah@cybertec.at, tgl@sss.pgh.pa.us, andrew@tao11.riddles.org.uk
Subject: Re: Problems with plan estimates in postgres_fdw
In-Reply-To: <5C6251A7.40204@lab.ntt.co.jp>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox