Laurenz Albe wrote:
> Here is the code review for patch number 2:
> +static void
> +CloseGOutput(FILE *gfile_fout, bool is_pipe)
>
> It makes sense to factor out this code.
> But shouldn't these functions have a prototype at the beginning of the file?
Looking at the other static functions in psql/common.c, there
are 22 of them but only 3 have prototypes at the top of the file.
These 3 functions are called before being defined, so these prototypes
are mandatory.
The other static functions that are defined before being called happen
not to have forward declarations, so SetupGOutput() and CloseGOutput()
follow that model.
> Here is a suggestion for a consolidated comment:
>
> Fetch the result in chunks if FETCH_COUNT is set. We don't enable chunking
> if SHOW_ALL_RESULTS is false, since that requires us to accumulate all rows
> before we can tell what should be displayed, which would counter the idea
> of FETCH_COUNT. Chunk fetching is also disabled if \gset, \crosstab,
> \gexec and \watch are used.
OK, done like that.
> > + if (fetch_count > 0 && result_status == PGRES_TUPLES_CHUNK)
>
> Could it be that result_status == PGRES_TUPLES_CHUNK, but fetch_count is 0?
> if not, perhaps there should be an Assert that verifies that, and the "if"
> statement should only check for the latter condition.
Good point. In fact it can be simplified to
if (result_status == PGRES_TUPLES_CHUNK),
and fetch_count as a variable can be removed from the function.
Done that way.
> > --- a/src/bin/psql/t/001_basic.pl
> > +++ b/src/bin/psql/t/001_basic.pl
> > @@ -184,10 +184,10 @@ like(
> > "\\set FETCH_COUNT 1\nSELECT error;\n\\errverbose",
> > on_error_stop => 0))[2],
> > qr/\A^psql:<stdin>:2: ERROR: .*$
> > -^LINE 2: SELECT error;$
> > +^LINE 1: SELECT error;$
> > ^ *^.*$
> > ^psql:<stdin>:3: error: ERROR: [0-9A-Z]{5}: .*$
> > -^LINE 2: SELECT error;$
> > +^LINE 1: SELECT error;$
>
> Why does the output change? Perhaps there is a good and harmless
> explanation, but the naïve expectation would be that it doesn't.
Unpatched, psql builds this query:
DECLARE _psql_cursor NO SCROLL CURSOR FOR \n
<user-query>
therefore the user query starts at line 2.
With the patch, the user query is sent as-is, starting at line1,
hence the different error location.
> After fixing the problem manually, it builds without warning.
> The regression tests pass, and the feature works as expected.
Thanks for testing.
Updated patches are attached.
Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite
