Re: [POC] FETCH limited by bytes.

On Thu, Dec 31, 2015 at 1:10 AM, Corey Huinker <corey(dot)huinker(at)gmail(dot)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