public inbox for [email protected]
help / color / mirror / Atom feedFrom: Laurenz Albe <[email protected]>
To: Daniel Verite <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Jakub Wartak <[email protected]>
Subject: Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs
Date: Fri, 29 Mar 2024 18:25:29 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
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
view thread (10+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox