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