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

From: "Daniel Verite" <daniel(at)manitou-mail(dot)org>
To: Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Jakub Wartak <jakub(dot)wartak(at)enterprisedb(dot)com>
Subject: Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs
Date: 2024-04-01 17:52:42
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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/
> > +++ b/src/bin/psql/t/
> > @@ -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:
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é
Twitter: @DanielVerite

Attachment Content-Type Size
v7-0001-Implement-retrieval-of-results-in-chunks-with-lib.patch text/plain 18.7 KB
v7-0002-Reimplement-FETCH_COUNT-with-the-chunked-mode-in-.patch text/plain 20.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2024-04-01 17:56:13 Re: Statistics Import and Export
Previous Message Tom Lane 2024-04-01 17:52:03 Re: Fix some resources leaks (src/bin/pg_basebackup/pg_createsubscriber.c)