Re: inherit support for foreign tables

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