Re: inherit support for foreign tables

Hello,

> >> 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.

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;
=====

> > 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..

> > Finally, although I insist the necessity of the warning for child
> > foreign tables on alter table, if you belive it to be put off,
> > I'll compromise by putting a note to CF-app that last judgement
> > is left to committer.
>
> OK So, if there are no objections of other, I'll mark this patch as
> "ready for committer" and do that.

Thank you.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center