> Review:
> - 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()
> calls.

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.

Reply via email to