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