BUG #16040: PL/PGSQL RETURN QUERY statement never uses a parallel plan

The following bug has been logged on the website:

Bug reference: 16040
Logged by: Jeremy Smith
Email address: jeremy(at)musicsmith(dot)net
PostgreSQL version: 12.0
Operating system: Official Docker Image, CentOS7
Description:

I have also tried this with 11.3, 11.4, and 11.5, so this is not new in
12.0. Here's a really basic way to reproduce this:

postgres=# BEGIN;
BEGIN
postgres=#
postgres=# -- Create a test table and some data
postgres=# CREATE TABLE test (a int);
CREATE TABLE
postgres=# INSERT INTO test SELECT generate_series(1,10);
INSERT 0 10
postgres=# alter table test set (parallel_workers = 4);
ALTER TABLE
postgres=# -- Use auto_explain to show plan of query in the function
postgres=# LOAD 'auto_explain';
LOAD
postgres=# SET auto_explain.log_analyze = on;
SET
postgres=# SET client_min_messages = log;
SET
postgres=# SET auto_explain.log_nested_statements = on;
SET
postgres=# SET auto_explain.log_min_duration = 0;
SET
postgres=# -- Set parallel costs artificially low, for demonstration
purposes
postgres=# set parallel_tuple_cost = 0;
SET
postgres=# set parallel_setup_cost = 0;
SET
postgres=# set max_parallel_workers_per_gather = 4;
SET
postgres=# -- Normal query will use 4 workers
postgres=# SELECT test.a, count(*) FROM test GROUP BY test.a;
LOG: duration: 19.280 ms plan:
Query Text: SELECT test.a, count(*) FROM test GROUP BY test.a;
Finalize HashAggregate (cost=25.56..27.56 rows=200 width=12) (actual
time=16.649..16.795 rows=10 loops=1)
Group Key: a
-> Gather (cost=19.56..21.56 rows=800 width=12) (actual
time=2.853..18.744 rows=10 loops=1)
Workers Planned: 4
Workers Launched: 4
-> Partial HashAggregate (cost=19.56..21.56 rows=200 width=12)
(actual time=0.493..0.519 rows=2 loops=5)
Group Key: a
-> Parallel Seq Scan on test (cost=0.00..16.38 rows=638
width=4) (actual time=0.009..0.083 rows=2 loops=5)
a | count
----+-------
9 | 1
3 | 1
5 | 1
4 | 1
10 | 1
6 | 1
2 | 1
7 | 1
1 | 1
8 | 1
(10 rows)

postgres=#
postgres=# CREATE OR REPLACE FUNCTION test_count()
postgres-# RETURNS TABLE (a int, n bigint) AS
postgres-# $$
postgres$# BEGIN
postgres$# RETURN QUERY SELECT test.a, count(*) FROM test GROUP BY
test.a;
postgres$# END;
postgres$# $$
postgres-# LANGUAGE PLPGSQL;
CREATE FUNCTION
postgres=#
postgres=# -- This query will not use parallel workers
postgres=# SELECT * FROM test_count();
LOG: duration: 0.437 ms plan:
Query Text: SELECT test.a, count(*) FROM test GROUP BY test.a
HashAggregate (cost=48.25..50.25 rows=200 width=12) (actual
time=0.193..0.276 rows=10 loops=1)
Group Key: a
-> Seq Scan on test (cost=0.00..35.50 rows=2550 width=4) (actual
time=0.010..0.096 rows=10 loops=1)
LOG: duration: 1.069 ms plan:
Query Text: SELECT * FROM test_count();
Function Scan on test_count (cost=0.25..10.25 rows=1000 width=12) (actual
time=0.895..0.968 rows=10 loops=1)
a | n
----+---
9 | 1
3 | 1
5 | 1
4 | 1
10 | 1
6 | 1
2 | 1
7 | 1
1 | 1
8 | 1
(10 rows)

postgres=# -- A workaround for long-running queries, using CREATE TABLE,
which will run in parallel
postgres=# CREATE OR REPLACE FUNCTION test_count2()
postgres-# RETURNS TABLE (a int, n bigint) AS
postgres-# $$
postgres$# BEGIN
postgres$# CREATE TEMPORARY TABLE test_count2_temp_table AS
postgres$# SELECT test.a, count(*) FROM test GROUP BY test.a;
postgres$# RETURN QUERY select * from test_count2_temp_table;
postgres$# END;
postgres$# $$
postgres-# LANGUAGE PLPGSQL;
CREATE FUNCTION
postgres=#
postgres=# -- The CREATE TABLE AS query will use parallel workers, but the
postgres=# -- RETURN QUERY statement will not
postgres=# SELECT * FROM test_count2();
LOG: duration: 24.139 ms plan:
Query Text: CREATE TEMPORARY TABLE test_count2_temp_table AS
SELECT test.a, count(*) FROM test GROUP BY test.a
Finalize HashAggregate (cost=25.56..27.56 rows=200 width=12) (actual
time=21.819..21.896 rows=10 loops=1)
Group Key: a
-> Gather (cost=19.56..21.56 rows=800 width=12) (actual
time=0.755..22.966 rows=10 loops=1)
Workers Planned: 4
Workers Launched: 4
-> Partial HashAggregate (cost=19.56..21.56 rows=200 width=12)
(actual time=0.105..0.148 rows=2 loops=5)
Group Key: a
-> Parallel Seq Scan on test (cost=0.00..16.38 rows=638
width=4) (actual time=0.009..0.056 rows=2 loops=5)
LOG: duration: 0.420 ms plan:
Query Text: select * from test_count2_temp_table
Seq Scan on test_count2_temp_table (cost=0.00..30.40 rows=2040 width=12)
(actual time=0.014..0.305 rows=10 loops=1)
LOG: duration: 26.118 ms plan:
Query Text: SELECT * FROM test_count2();
Function Scan on test_count2 (cost=0.25..10.25 rows=1000 width=12) (actual
time=25.845..25.994 rows=10 loops=1)
a | n
----+---
9 | 1
3 | 1
5 | 1
4 | 1
10 | 1
6 | 1
2 | 1
7 | 1
1 | 1
8 | 1
(10 rows)

It's not obvious from the documentation
(https://www.postgresql.org/docs/12/when-can-parallel-query-be-used.html)
that this should be the case. RETURN QUERY is not interruptible, like a
cursor or for loop.

PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
> [ $SUBJECT ]

I got around to looking at this today, and what I find is that the
problem is that exec_stmt_return_query() uses a portal (i.e. a cursor)
to read the results of the query. That seemed like a good idea, back
in the late bronze age, because it allowed plpgsql to fetch the query
results a few rows at a time and not risk blowing out memory with a huge
SPI result. However, the parallel-query infrastructure refuses to
parallelize when the query is being read via a cursor.

I think that the latter restriction is probably sane, because we don't
want to suspend execution of a parallel query while we've got worker
processes waiting. And there might be some implementation restrictions
lurking under it too --- that's not a part of the code I know in any
detail.

However, there's no fundamental reason why exec_stmt_return_query has
to use a cursor. It's going to run the query to completion immediately
anyway, and shove all the result rows into a tuplestore. What we lack
is a way to get the SPI query to pass its results directly to a
tuplestore, without the SPITupleTable intermediary. (Note that the
tuplestore can spill a large result to disk, whereas SPITupleTable
can't do that.)

So, attached is a draft patch to enable that. By getting rid of the
intermediate SPITupleTable, this should improve the performance of
RETURN QUERY somewhat even without considering the possibility of
parallelizing the source query. I've not tried to measure that though.
I've also not looked for other places that could use this new
infrastructure, but there may well be some.

One thing I'm not totally pleased about with this is adding another
SPI interface routine using the old parameter-values API (that is,
null flags as char ' '/'n'). That was the path of least resistance
given the other moving parts in pl_exec.c and spi.c, but maybe we
should try to modernize that before we set it in stone.

Another thing standing between this patch and committability is suitable
additions to the SPI documentation. But I saw no value in writing that
before the previous point is settled.

I will go add this to the next commitfest (for v14), but I wonder
if we should try to squeeze it into v13? This isn't the only
complaint we've gotten about non-parallelizability of RETURN QUERY.

regards, tom lane

ne 22. 3. 2020 v 4:23 odesílatel Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> napsal:

> PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
> > [ $SUBJECT ]
>
> I got around to looking at this today, and what I find is that the
> problem is that exec_stmt_return_query() uses a portal (i.e. a cursor)
> to read the results of the query. That seemed like a good idea, back
> in the late bronze age, because it allowed plpgsql to fetch the query
> results a few rows at a time and not risk blowing out memory with a huge
> SPI result. However, the parallel-query infrastructure refuses to
> parallelize when the query is being read via a cursor.
>
> I think that the latter restriction is probably sane, because we don't
> want to suspend execution of a parallel query while we've got worker
> processes waiting. And there might be some implementation restrictions
> lurking under it too --- that's not a part of the code I know in any
> detail.
>
> However, there's no fundamental reason why exec_stmt_return_query has
> to use a cursor. It's going to run the query to completion immediately
> anyway, and shove all the result rows into a tuplestore. What we lack
> is a way to get the SPI query to pass its results directly to a
> tuplestore, without the SPITupleTable intermediary. (Note that the
> tuplestore can spill a large result to disk, whereas SPITupleTable
> can't do that.)
>
> So, attached is a draft patch to enable that. By getting rid of the
> intermediate SPITupleTable, this should improve the performance of
> RETURN QUERY somewhat even without considering the possibility of
> parallelizing the source query. I've not tried to measure that though.
> I've also not looked for other places that could use this new
> infrastructure, but there may well be some.
>
> One thing I'm not totally pleased about with this is adding another
> SPI interface routine using the old parameter-values API (that is,
> null flags as char ' '/'n'). That was the path of least resistance
> given the other moving parts in pl_exec.c and spi.c, but maybe we
> should try to modernize that before we set it in stone.
>
> Another thing standing between this patch and committability is suitable
> additions to the SPI documentation. But I saw no value in writing that
> before the previous point is settled.
>
> I will go add this to the next commitfest (for v14), but I wonder
> if we should try to squeeze it into v13? This isn't the only
> complaint we've gotten about non-parallelizability of RETURN QUERY.
>

+1

Pavel

> regards, tom lane
>
>

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

All good with this patch.

--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950  EMAIL: mailto:hamid(dot)akhtar(at)highgo(dot)ca
SKYPE: engineeredvirus

The new status of this patch is: Ready for Committer

Hamid Akhtar <hamid(dot)akhtar(at)gmail(dot)com> writes:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: tested, passed
> Documentation: tested, passed

> All good with this patch.

Thanks for testing!

Anybody have an objection to cramming this into v13? It is a bit late,
but it seems like a performance bug fix ...

regards, tom lane

I wrote:
> ... attached is a draft patch to enable that. By getting rid of the
> intermediate SPITupleTable, this should improve the performance of
> RETURN QUERY somewhat even without considering the possibility of
> parallelizing the source query. I've not tried to measure that though.
> I've also not looked for other places that could use this new
> infrastructure, but there may well be some.

> One thing I'm not totally pleased about with this is adding another
> SPI interface routine using the old parameter-values API (that is,
> null flags as char ' '/'n'). That was the path of least resistance
> given the other moving parts in pl_exec.c and spi.c, but maybe we
> should try to modernize that before we set it in stone.

Here's a revised patch that does the additional legwork needed to
use ParamListInfo throughout the newly-added code. I was able to
get pl_exec.c out of the business of using old-style null flags
entirely, which seems like a nice improvement.

> Another thing standing between this patch and committability is suitable
> additions to the SPI documentation. But I saw no value in writing that
> before the previous point is settled.

Took care of that too.

I looked around for other places that could use this infrastructure.
It turns out that most places that are fetching via SPITupleTables
don't really have much of an issue, because they are only expecting
to get one or so tuples anyway. There are a few where it might be
worth changing, but it's hard to get really excited because they all
have other constraints on the max amount of data. As an example,
the various table-to-xml thingies in utils/adt/xml.c could be converted,
but they're still funneling their output into an XML string. As long
as that has a hard limit at 1GB, it's not very realistic to expect that
you can shove huge tables into it.

A different sort of cleanup we could undertake is to deprecate and
eventually remove some of the SPI API functions. As of this patch,
for example, SPI_cursor_open_with_args and SPI_execute_with_args are
unused anywhere in our code. But since we document them, it's hard
to guess whether any external code is relying on them. I suppose
deprecation would be a multi-year project in any case.

I think this is committable now. Any objections?

regards, tom lane

On Sat, Mar 21, 2020 at 11:23 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I think that the latter restriction is probably sane, because we don't
> want to suspend execution of a parallel query while we've got worker
> processes waiting.

Right.

> And there might be some implementation restrictions
> lurking under it too --- that's not a part of the code I know in any
> detail.

There are. When you EnterParallelMode(), various normally-permissible
options are restricted and will error out (e.g. updating your snapshot
or command ID). Parallel query's not safe unless you remain in
parallel mode from start to finish, but that means you can't let
control escape into code that might do arbitrary things. That in a
nutshell is why the cursor restriction is there.

This is a heck of a nice improvement. Thanks for working on it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

First congrats to the postgres 14 release 👏

I’ve just started testing with it and I found some unexpected behavior with some plpgsql function.
A function that inserts data and tries to return with a table now results in the error `query is not a SELECT`.
In previous versions that query succeeded.

While the message got updated in https://www.postgresql.org/message-id/flat/1914708.1629474624%40sss.pgh.pa.us, the changes here might cause the actual issue.
Here’s a quite simplified version to reproduce the issue.
Is this some new expected behavior that’s not documented or mentioned in the change log?

CREATE TABLE t (value text);
CREATE FUNCTION t_insert(v text)
RETURNS SETOF t
AS '
BEGIN
RETURN QUERY
INSERT INTO t ("value")
VALUES (v)
RETURNING *;
END
' LANGUAGE plpgsql;

SELECT * FROM t_insert('foo’);

ERROR: query is not a SELECT

While a CTE query is working:

CREATE OR REPLACE FUNCTION t_insert(v text) RETURNS SETOF t
AS '
BEGIN
RETURN QUERY
WITH q AS (INSERT INTO t ("value") VALUES (v) RETURNING *)
SELECT * FROM q;
END
' LANGUAGE plpgsql;

SELECT * FROM t_insert('foo’);

value
--------
foo

> On 12 Jun 2020, at 20:13, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Sat, Mar 21, 2020 at 11:23 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think that the latter restriction is probably sane, because we don't
>> want to suspend execution of a parallel query while we've got worker
>> processes waiting.
>
> Right.
>
>> And there might be some implementation restrictions
>> lurking under it too --- that's not a part of the code I know in any
>> detail.
>
> There are. When you EnterParallelMode(), various normally-permissible
> options are restricted and will error out (e.g. updating your snapshot
> or command ID). Parallel query's not safe unless you remain in
> parallel mode from start to finish, but that means you can't let
> control escape into code that might do arbitrary things. That in a
> nutshell is why the cursor restriction is there.
>
> This is a heck of a nice improvement. Thanks for working on it.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
>
>

Marc Bachmann <marc(dot)brookman(at)gmail(dot)com> writes:
> A function that inserts data and tries to return with a table now results in the error `query is not a SELECT`.
> In previous versions that query succeeded.

Hmm ... I'm a bit surprised that that worked before, but since it did,
we shouldn't break it. It looks like this was an accidental side-effect
of refactoring rather than something intentional. Will look closer
tomorrow or so.

regards, tom lane

I wrote:
> Marc Bachmann <marc(dot)brookman(at)gmail(dot)com> writes:
>> A function that inserts data and tries to return with a table now results in the error `query is not a SELECT`.
>> In previous versions that query succeeded.

> Hmm ... I'm a bit surprised that that worked before, but since it did,
> we shouldn't break it.

Fix pushed, thanks for the report!

regards, tom lane

I’m happy to help.
And sorry that you had to go through that many changes right after the release.

Kind regards
Marc

> On 3 Oct 2021, at 19:22, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> I wrote:
>> Marc Bachmann <marc(dot)brookman(at)gmail(dot)com> writes:
>>> A function that inserts data and tries to return with a table now results in the error `query is not a SELECT`.
>>> In previous versions that query succeeded.
>
>> Hmm ... I'm a bit surprised that that worked before, but since it did,
>> we shouldn't break it.
>
> Fix pushed, thanks for the report!
>
> regards, tom lane