On Fri, 2024-03-29 at 14:07 +0100, Laurenz Albe wrote:
> I had a look at patch 0001 (0002 will follow).
Here is the code review for patch number 2:
> diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
[...]
+static bool
+SetupGOutput(PGresult *result, FILE **gfile_fout, bool *is_pipe)
[...]
+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?
> + /*
> + * If FETCH_COUNT is set and the context allows it, use the single row
> + * mode to fetch results and have no more than FETCH_COUNT rows in
> + * memory.
> + */
That comment talks about single-row mode, whey you are using chunked mode.
You probably forgot to modify the comment from a previous version of the patch.
> + if (fetch_count > 0 && !pset.crosstab_flag && !pset.gexec_flag &&
> !is_watch
> + && !pset.gset_prefix && pset.show_all_results)
> + {
> + /*
> + * The row-by-chunks fetch is not enabled when SHOW_ALL_RESULTS is
> false,
> + * since we would need to accumulate all rows before knowing
> + * whether they need to be discarded or displayed, which contradicts
> + * FETCH_COUNT.
> + */
> + if (!PQsetChunkedRowsMode(pset.db, fetch_count))
> + {
I think that comment should be before the "if" statement, not inside it.
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.
> + 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.
> --- 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.
The patch does not apply any more because of a conflict with the
non-blocking PQcancel patch.
After fixing the problem manually, it builds without warning.
The regression tests pass, and the feature works as expected.
Yours,
Laurenz Albe
