Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs

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