From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: shigeru.hanada@gmail.com
Cc: pgsql-hackers@postgresql.org
Subject: Re: inherit support for foreign tables
Date: Thu, 27 Mar 2014 17:55:41 +0900
Message-ID: <5333E78D.2070909@lab.ntt.co.jp> (raw)
In-Reply-To: <20140327.170917.91664276.horiguchi.kyotaro@lab.ntt.co.jp>
References: <532F8EC1.6020401@lab.ntt.co.jp>
<20140326.171454.142890940.horiguchi.kyotaro@lab.ntt.co.jp>
<533383BF.1050004@lab.ntt.co.jp>
<20140327.170917.91664276.horiguchi.kyotaro@lab.ntt.co.jp>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Hi Horiguchi-san,
(2014/03/27 17:09), Kyotaro HORIGUCHI wrote:
>>>> analyze.c: In function ‘acquire_inherited_sample_rows’:
>>>> analyze.c:1461: warning: unused variable ‘saved_rel’
>>
>> I've fixed this in the latest version (v8) of the patch.
>
> Mmm. sorry. I missed v8 patch. Then I had a look on that and have
> a question.
Thank you for the review!
> You've added a check for relkind of baserel of the foreign path
> to be reparameterized. If this should be an assertion - not a
> conditional branch -, it would be better to put the assertion in
> create_foreignscan_path instead of there.
>
> =====
> --- a/src/backend/optimizer/util/pathnode.c
> +++ b/src/backend/optimizer/util/pathnode.c
> @@ -1723,6 +1723,7 @@ create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel,
> List *fdw_private)
> {
> ForeignPath *pathnode = makeNode(ForeignPath);
> + Assert(rel->rtekind == RTE_RELATION);
>
> pathnode->path.pathtype = T_ForeignScan;
> pathnode->path.parent = rel;
> =====
Maybe I'm missing the point, but I don't think it'd be better to put the
assertion in create_foreignscan_path(). And I think it'd be the caller'
responsiblity to ensure that equality, as any other pathnode creation
routine for a baserel in pathnode.c assumes that equality.
>>> And for file-fdw, you made a change to re-create foreignscan node
>>> instead of the previous copy-and-modify. Is the reason you did it
>>> that you considered the cost of 're-checking whether to
>>> selectively perform binary conversion' is low enough? Or other
>>> reasons?
>>
>> The reason is that we get the result of the recheck from
>> path->fdw_private. Sorry, I'd forgotten it. So, I modified the code to
>> simply call create_foreignscan_path().
>
> Anyway you new code seems closer to the basics and the gain by
> the previous optimization don't seem to be significant..
Yeah, that's true. I have to admit that the previous optimization is
nonsense.
Thanks,
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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, horiguchi.kyotaro@lab.ntt.co.jp, shigeru.hanada@gmail.com
Subject: Re: inherit support for foreign tables
In-Reply-To: <5333E78D.2070909@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