On Thu, Dec 31, 2015 at 1:10 AM, Corey Huinker <corey.huin...@gmail.com> wrote: > Here's a re-based patch. Notable changes since the last patch are: > > - some changes had to move to postgres_fdw.h > - the regression tests are in their own script fetch_size.sql / > fetch_size.out. that should make things easier for the reviewer(s), even if > we later merge it into postgres_fdw.sql/.out. > - I put braces around a multi-line error ereport(). That might be a style > violation, but the statement seemed confusing without it. > - The fetch sql is reported as a DEBUG1 level ereport(), and confirms that > server level options are respected, and table level options are used in > favor of server-level options. > > I left the DEBUG1-level message in this patch so that others can see the > output, but I assume we would omit that for production code, with > corresponding deletions from fetch_size.sql and fetch_size.out.
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 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. - We could consider folding fetch_size into "Remote Execution Options", but maybe that's too clever. I would not worry about trying to get this into the regression tests. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
