> - There is an established idiom in postgres_fdw for how to extract
> data from lists of DefElems, and this patch does something else
> instead. Please make it look like postgresIsForeignRelUpdatable,
> postgresGetForeignRelSize, and postgresImportForeignSchema instead of
> inventing something new. (Note that your approach would require
> multiple passes over the list if you needed information on multiple
> options, whereas the existing approach does not.)
I will look into that. The original patch pre-dates import foreign schema,
so I'm not surprised that I missed the established pattern.
> - I think the comment in InitPgFdwOptions() could be reduced to a
> one-line comment similar to those already present.
> - I would reduce the debug level on the fetch command to something
> lower than DEBUG1, or drop it entirely. If you keep it, you need to
> fix the formatting so that the line breaks look like other ereport()
As I mentioned, I plan to drop it entirely. It was only there to show a
reviewer that it works as advertised. There's not much to see without it.
> - We could consider folding fetch_size into "Remote Execution
> Options", but maybe that's too clever.
If you care to explain, I'm listening. Otherwise I'm going forward with the
other suggestions you've made.
> I would not worry about trying to get this into the regression tests.
Happy to hear that.