Message-ID: <5333E78D.2070909@lab.ntt.co.jp>
Date: Thu, 27 Mar 2014 17:55:41 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.0;
 rv:24.0) Gecko/20100101 Thunderbird/24.4.0
MIME-Version: 1.0
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
CC: shigeru.hanada@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: inherit support for foreign tables
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>
In-Reply-To: <20140327.170917.91664276.horiguchi.kyotaro@lab.ntt.co.jp>
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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