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

Jakub Wartak wrote:

It might be a not so well known fact (?) that CTEs are not executed
with cursor when asked to do so, but instead silently executed with
potential huge memory allocation going on. Patch is attached. My one
doubt is that not every statement starting with "WITH" is WITH(..)
SELECT of course.

Yes, that's why WITH queries are currently filtered out by the
FETCH_COUNT feature.

Case in point:

test=> begin;
BEGIN

test=> create table tbl(i int);
CREATE TABLE

test=> declare psql_cursor cursor for
with r(i) as (values (1))
insert into tbl(i) select i from r;
ERROR: syntax error at or near "insert"
LINE 3: insert into tbl(i) select i from r;

So the fix you're proposing would fail on that kind of queries.

A solution would be for psql to use PQsetSingleRowMode() to retrieve
results row-by-row, as opposed to using a cursor, and then allocate
memory for only FETCH_COUNT rows at a time. Incidentally it solves
other problems like queries containing multiple statements, that also
fail to work properly with cursors, or UPDATE/INSERT... RETURNING.. on
large number of rows that could also benefit from pagination in
memory.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

On Wed, Jan 4, 2023 at 10:22 AM Daniel Verite <daniel@manitou-mail.org> wrote:

A solution would be for psql to use PQsetSingleRowMode() to retrieve
results row-by-row, as opposed to using a cursor, and then allocate
memory for only FETCH_COUNT rows at a time. Incidentally it solves
other problems like queries containing multiple statements, that also
fail to work properly with cursors, or UPDATE/INSERT... RETURNING.. on
large number of rows that could also benefit from pagination in
memory.

Is there any reason that someone hasn't, like, already done this?

Because if there isn't, we should really do this. And if there is,
like say that it would hurt performance or something, then we should
come up with a fix for that problem and then do something like this.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Jan 4, 2023 at 10:22 AM Daniel Verite <daniel@manitou-mail.org> wrote:

A solution would be for psql to use PQsetSingleRowMode() to retrieve
results row-by-row, as opposed to using a cursor, and then allocate
memory for only FETCH_COUNT rows at a time.

Is there any reason that someone hasn't, like, already done this?

As you well know, psql's FETCH_COUNT mechanism is far older than
single-row mode. I don't think anyone's tried to transpose it
onto that. I agree that it seems like a good idea to try.
There will be more per-row overhead, but the increase in flexibility
is likely to justify that.

regards, tom lane

On Wed, Jan 4, 2023 at 11:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

As you well know, psql's FETCH_COUNT mechanism is far older than
single-row mode. I don't think anyone's tried to transpose it
onto that. I agree that it seems like a good idea to try.
There will be more per-row overhead, but the increase in flexibility
is likely to justify that.

Yeah, I was vaguely worried that there might be more per-row overhead,
not that I know a lot about this topic. I wonder if there's a way to
mitigate that. I'm a bit suspicious that what we want here is really
more of an incremental mode than a single-row mode i.e. yeah, you want
to fetch rows without materializing the whole result, but maybe not in
batches of exactly size one.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Jan 4, 2023 at 6:38 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Jan 4, 2023 at 11:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

As you well know, psql's FETCH_COUNT mechanism is far older than
single-row mode. I don't think anyone's tried to transpose it
onto that. I agree that it seems like a good idea to try.
There will be more per-row overhead, but the increase in flexibility
is likely to justify that.

Yeah, I was vaguely worried that there might be more per-row overhead,
not that I know a lot about this topic. I wonder if there's a way to
mitigate that. I'm a bit suspicious that what we want here is really
more of an incremental mode than a single-row mode i.e. yeah, you want
to fetch rows without materializing the whole result, but maybe not in
batches of exactly size one.

Given the low importance and very low priority of this, how about
adding it as a TODO wiki item then and maybe adding just some warning
instead? I've intentionally avoided parsing grammar and regexp so it's
not perfect (not that I do care about this too much either, as web
crawlers already have indexed this $thread). BTW I've found two
threads if know what are you looking for [1]/messages/by-id/a0a854b6-563c-4a11-bf1c-d6c6f924004d@manitou-mail.org[2]/messages/by-id/1274761885.4261.233.camel@minidragon

-Jakub Wartak.

[1]: /messages/by-id/a0a854b6-563c-4a11-bf1c-d6c6f924004d@manitou-mail.org
[2]: /messages/by-id/1274761885.4261.233.camel@minidragon

Tom Lane wrote:

I agree that it seems like a good idea to try.
There will be more per-row overhead, but the increase in flexibility
is likely to justify that.

Here's a POC patch implementing row-by-row fetching.

If it wasn't for the per-row overhead, we could probably get rid of
ExecQueryUsingCursor() and use row-by-row fetches whenever
FETCH_COUNT is set, independently of the form of the query.

However the difference in processing time seems to be substantial: on
some quick tests with FETCH_COUNT=10000, I'm seeing almost a 1.5x
increase on large datasets. I assume it's the cost of more allocations.
I would have hoped that avoiding the FETCH queries and associated
round-trips with the cursor method would compensate for that, but it
doesn't appear to be the case, at least with a fast local connection.

So in this patch, psql still uses the cursor method if the
query starts with "select", and falls back to the row-by-row in
the main code (ExecQueryAndProcessResults) otherwise.
Anyway it solves the main issue of the over-consumption of memory
for CTE and update/insert queries returning large resultsets.

Best regards,

--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

I wrote:

Here's a POC patch implementing row-by-row fetching.

PFA an updated patch.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

"Daniel Verite" <daniel@manitou-mail.org> writes:

PFA an updated patch.

This gives me several "-Wincompatible-pointer-types" warnings
(as are also reported by the cfbot):

common.c: In function 'ExecQueryAndProcessResults':
common.c:1686:24: warning: passing argument 1 of 'PrintQueryTuples' from incompatible pointer type [-Wincompatible-pointer-types]
PrintQueryTuples(result_array, ntuples, &my_popt, tuples_fout);
^~~~~~~~~~~~
common.c:679:35: note: expected 'const PGresult **' {aka 'const struct pg_result **'} but argument is of type 'PGresult **' {aka 'struct pg_result **'}
PrintQueryTuples(const PGresult **result, int nresults, const printQueryOpt *opt,
~~~~~~~~~~~~~~~~~^~~~~~
common.c:1720:24: warning: passing argument 1 of 'PrintQueryTuples' from incompatible pointer type [-Wincompatible-pointer-types]
PrintQueryTuples(result_array, ntuples, &my_popt, tuples_fout);
^~~~~~~~~~~~
common.c:679:35: note: expected 'const PGresult **' {aka 'const struct pg_result **'} but argument is of type 'PGresult **' {aka 'struct pg_result **'}
PrintQueryTuples(const PGresult **result, int nresults, const printQueryOpt *opt,
~~~~~~~~~~~~~~~~~^~~~~~

I think the cause is the inconsistency about whether PGresult pointers
are pointer-to-const or not. Even without compiler warnings, I find
code like this very ugly:

-				success = PrintQueryTuples(result, opt, printQueryFout);
+				success = PrintQueryTuples((const PGresult**)&result, 1, opt, printQueryFout);

I think what you probably ought to do to avoid all that is to change
the arguments of PrintQueryResult and nearby routines to be "const
PGresult *result" not just "PGresult *result".

I find it sad that we can't get rid of ExecQueryUsingCursor().
Maybe a little effort towards reducing overhead in the single-row
mode would help?

regards, tom lane

Tom Lane wrote:

This gives me several "-Wincompatible-pointer-types" warnings
[...]
I think what you probably ought to do to avoid all that is to change
the arguments of PrintQueryResult and nearby routines to be "const
PGresult *result" not just "PGresult *result".

The const-ness issue that I ignored in the previous patch is that
while C is fine with passing T* to a function expecting const T*, it's
not okay with passing T** to a function expecting const T**,
or more generally converting T** to const T**.

When callers need to pass arrays of PGresult* instead of const
PGresult*, I've opted to remove the const qualifiers for the functions
that are concerned by this change.

PFA an updated patch.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Hi,

Here's a new version to improve the performance of FETCH_COUNT
and extend the cases when it can be used.

Patch 0001 adds a new mode in libpq to allow the app to retrieve
larger chunks of results than the single row of the row-by-row mode.
The maximum number of rows per PGresult is set by the user.

Patch 0002 uses that mode in psql and gets rid of the cursor
implementation as suggested upthread.

The performance numbers look good.
For a query retrieving 50M rows of about 200 bytes:
select repeat('abc', 200) from generate_series(1,5000000)
/usr/bin/time -v psql -At -c $query reports these metrics
(medians of 5 runs):

version | fetch_count | clock_time | user_time | sys_time | max_rss_size
(kB)
-----------+-------------+------------+-----------+----------+-------------------
16-stable | 0 | 6.58 | 3.98 | 2.09 |
3446276
16-stable | 100 | 9.25 | 4.10 | 1.90 |
8768
16-stable | 1000 | 11.13 | 5.17 | 1.66 |
8904
17-patch | 0 | 6.5 | 3.94 | 2.09 |
3442696
17-patch | 100 | 5 | 3.56 | 0.93 |
4096
17-patch | 1000 | 6.48 | 4.00 | 1.55 |
4344

Interestingly, retrieving by chunks of 100 rows appears to be a bit faster
than the default one big chunk. It means that independently
of using less memory, FETCH_COUNT implemented that way
would be a performance enhancement compared to both
not using it and using it in v16 with the cursor implementation.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Hi,

PFA a rebased version.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

On Tue, 2 Jan 2024 at 20:28, Daniel Verite <daniel@manitou-mail.org> wrote:

Hi,

PFA a rebased version.

CFBot shows that the patch does not apply anymore as in [1]http://cfbot.cputube.org/patch_46_4233.log:
=== Applying patches on top of PostgreSQL commit ID
a3a836fb5e51183eae624d43225279306c2285b8 ===
=== applying patch
./v5-0001-Implement-retrieval-of-results-in-chunks-with-lib.patch
patching file doc/src/sgml/libpq.sgml
...
patching file src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
...
patching file src/interfaces/libpq/exports.txt
Hunk #1 FAILED at 191.
1 out of 1 hunk FAILED -- saving rejects to file
src/interfaces/libpq/exports.txt.rej

Please post an updated version for the same.

[1]: http://cfbot.cputube.org/patch_46_4233.log

Regards,
Vignesh

vignesh C wrote:

patching file src/interfaces/libpq/exports.txt
Hunk #1 FAILED at 191.
1 out of 1 hunk FAILED -- saving rejects to file
src/interfaces/libpq/exports.txt.rej

Please post an updated version for the same.

PFA a rebased version.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Hi Daniel,

On Tue, Jan 30, 2024 at 3:29 PM Daniel Verite <daniel@manitou-mail.org> wrote:

PFA a rebased version.

Thanks for the patch! I've tested it using my original reproducer and
it works great now against the original problem description. I've
taken a quick look at the patch, it looks good for me. I've tested
using -Werror for both gcc 10.2 and clang 11.0 and it was clean. I
have one slight doubt:

when I run with default pager (more or less):
\set FETCH_COUNT 1000
WITH data AS (SELECT generate_series(1, 20000000) as Total) select
repeat('a',100) || data.Total || repeat('b', 800) as total_pat from
data;
-- it enters pager, a skip couple of pages and then "q"

.. then - both backend and psql - go into 100% CPU as it were still
receiving (that doesn't happen e.g. with export PAGER=cat). So I'm
not sure, maybe ExecQueryAndProcessResults() should somewhat faster
abort when the $PAGER is exiting normally(?).

And oh , btw, in v6-0001 (so if you would be sending v7 for any other
reason -- other reviewers -- maybe worth realigning it as detail):

+  int PQsetChunkedRowsMode(PGconn *conn,
+                           int maxRows);

but the code has (so "maxRows" != "chunkSize"):

+PQsetChunkedRowsMode(PGconn *conn, int chunkSize)

-J.

Jakub Wartak wrote:

when I run with default pager (more or less):
\set FETCH_COUNT 1000
WITH data AS (SELECT generate_series(1, 20000000) as Total) select
repeat('a',100) || data.Total || repeat('b', 800) as total_pat from
data;
-- it enters pager, a skip couple of pages and then "q"

.. then - both backend and psql - go into 100% CPU as it were still
receiving

Thanks for looking into this patch!

What's happening after the pager has quit is that psql continues
to pump results from the server until there are no more results.

If the user wants to interrupt that, they should hit Ctrl+C to
cancel the query. I think psql should not cancel it implicitly
on their behalf, as it also cancels the transaction.

The behavior differs from the cursor implementation, because in
the cursor case, when the pager is displaying results, no query is
running. The previous FETCH results have been entirely
read, and the next FETCH has not been sent to the server yet.
This is why quitting the pager in the middle of this can
be dealt with instantly.

(that doesn't happen e.g. with export PAGER=cat). So I'm
not sure, maybe ExecQueryAndProcessResults() should somewhat
faster abort when the $PAGER is exiting normally(?).

I assume that when using PAGER=cat, you cancel the display
with Ctrl+C, which propagates to psql and have the effect
to also cancel the query. In that case it displays
"Cancel request sent",
and then shortly after it gets back from the server:
"ERROR: canceling statement due to user request".
That case corresponds to the generic query canceling flow.

OTOH if killing the "cat" process with kill -TERM I see the same
behavior than with "more" or "less", that is postgres running
the query to completion and psql pumping the results.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

On Tue, 2024-01-30 at 15:29 +0100, Daniel Verite wrote:

PFA a rebased version.

I had a look at patch 0001 (0002 will follow).

- <sect1 id="libpq-single-row-mode">
-  <title>Retrieving Query Results Row-by-Row</title>
+ <sect1 id="libpq-chunked-results-modes">
+  <title>Retrieving Query Results by chunks</title>

That should be "in chunks".

+    <para>
+   <variablelist>
+    <varlistentry id="libpq-PQsetChunkedRowsMode">
+      <term><function>PQsetChunkedRowsMode</function>
+        <indexterm><primary>PQsetChunkedRowsMode</primary></indexterm></term>
+     <listitem>
+      <para>
+       Select the mode retrieving results in chunks for the currently-executing query.

That is questionable English. How about

Select to receive the results for the currently-executing query in chunks.

+       This function is similar to <xref linkend="libpq-PQsetSingleRowMode"/>,
+       except that it can retrieve a user-specified number of rows
+       per call to <xref linkend="libpq-PQgetResult"/>, instead of a single row.

The "user-specified number" is "maxRows". So a better wording would be:

... except that it can retrieve <replaceable>maxRows</replaceable> rows
per call to <xref linkend="libpq-PQgetResult"/> instead of a single row.

-    error.  But in single-row mode, those rows will have already been
+    error.  But in single-row or chunked modes, those rows will have already been

I'd say it should be "in *the* single-row or chunk modes".

--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -41,7 +41,8 @@ char     *const pgresStatus[] = {
"PGRES_COPY_BOTH",
"PGRES_SINGLE_TUPLE",
"PGRES_PIPELINE_SYNC",
-   "PGRES_PIPELINE_ABORTED"
+   "PGRES_PIPELINE_ABORTED",
+   "PGRES_TUPLES_CHUNK"
};

I think that PGRES_SINGLE_TUPLE and PGRES_TUPLES_CHUNK should be next to each
other, but that's no big thing.
The same applies to the change in src/interfaces/libpq/libpq-fe.h

I understand that we need to keep the single-row mode for compatibility
reasons. But I think that under the hood, "single-row mode" should be the
same as "chunk mode with chunk size one".
That should save some code repetition.

Yours,
Laurenz Albe

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

Laurenz Albe wrote:

I had a look at patch 0001 (0002 will follow).

Thanks for reviewing this!

I've implemented the suggested doc changes. A patch update
will follow with the next part of the review.

--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -41,7 +41,8 @@ char     *const pgresStatus[] = {
"PGRES_COPY_BOTH",
"PGRES_SINGLE_TUPLE",
"PGRES_PIPELINE_SYNC",
-   "PGRES_PIPELINE_ABORTED"
+   "PGRES_PIPELINE_ABORTED",
+   "PGRES_TUPLES_CHUNK"
};

I think that PGRES_SINGLE_TUPLE and PGRES_TUPLES_CHUNK should be next to
each other, but that's no big thing.
The same applies to the change in src/interfaces/libpq/libpq-fe.h

I assume we can't renumber/reorder existing values, otherwise it would be
an ABI break. We can only add new values.

I understand that we need to keep the single-row mode for compatibility
reasons. But I think that under the hood, "single-row mode" should be the
same as "chunk mode with chunk size one".

I've implemented it like that at first, and wasn't thrilled with the result.
libpq still has to return PGRES_SINGLE_TUPLE in single-row
mode and PGRES_TUPLES_CHUNK with chunks of size 1, so
the mutualization did not work that well in practice.
I also contemplated not creating PGRES_TUPLES_CHUNK
and instead using PGRES_SINGLE_TUPLE for N rows, but I found
it too ugly.

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

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