Add tests for PL/pgSQL SRFs
Hello Hackers,
While working on inlining non-SQL SRFs [1]https://commitfest.postgresql.org/49/5083/ I noticed we don't have tests for when a PL/pgSQL
function requires materialize mode but doesn't have a result TupleDesc. Here is a patch adding tests
for that, as well as some other conditions around SRF calls with `SETOF RECORD` vs `TABLE (...)`.
There aren't any code changes, just some new tests.
But IMO it might be better to change the code. This error message is a bit confusing:
+-- materialize mode requires a result TupleDesc:
+select array_to_set2(array['one', 'two']); -- fail
+ERROR: materialize mode required, but it is not allowed in this context
+CONTEXT: PL/pgSQL function array_to_set2(anyarray) line 3 at RETURN QUERYPerhaps it would be better to give the same error as here?:
+select * from array_to_set2(array['one', 'two']); -- fail
+ERROR: a column definition list is required for functions returning "record"
+LINE 1: select * from array_to_set2(array['one', 'two']);If folks agree, I can work on a patch for that. Otherwise, at least this patch documents the current
behavior and increases coverage.
[1]: https://commitfest.postgresql.org/49/5083/
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Attachments:
v1-0001-Add-tests-for-PL-pgSQL-Set-Returning-Functions.patchtext/x-patch; charset=UTF-8; name=v1-0001-Add-tests-for-PL-pgSQL-Set-Returning-Functions.patchDownload
From 763db868d2dc121f8745ba9f90e4f737c135bcaa Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com>
Date: Wed, 28 Aug 2024 19:44:02 -0700
Subject: [PATCH v1] Add tests for PL/pgSQL Set-Returning Functions
These tests exercize SRFs with and without a result TupleDesc (in other
words RETURNS TABLE (...) vs RETURNS SETOF RECORD). We can only support
materialize mode in the former case. On the other hand, that case
rejects a column definition list as redundant. The position of these
tests shows the contrast with SQL functions (tested above), which
support SETOF RECORD in more places.
---
src/test/regress/expected/rangefuncs.out | 90 ++++++++++++++++++++++++
src/test/regress/sql/rangefuncs.sql | 42 +++++++++++
2 files changed, 132 insertions(+)
diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out
index 397a8b35d6d..3d3ebd17780 100644
--- a/src/test/regress/expected/rangefuncs.out
+++ b/src/test/regress/expected/rangefuncs.out
@@ -2169,6 +2169,96 @@ LINE 1: select * from sin(3) as t(f1 int8,f2 int8);
drop type rngfunc_type cascade;
NOTICE: drop cascades to function testrngfunc()
--
+-- test use of PL/pgSQL functions returning setof record
+--
+create or replace function array_to_set2(anyarray) returns setof record as $$
+ begin
+ return query execute 'select i AS "index", $1[i] AS "value" from generate_subscripts($1, 1) i'
+ using $1;
+ end;
+$$ language plpgsql immutable;
+-- materialize mode requires a result TupleDesc:
+select array_to_set2(array['one', 'two']); -- fail
+ERROR: materialize mode required, but it is not allowed in this context
+CONTEXT: PL/pgSQL function array_to_set2(anyarray) line 3 at RETURN QUERY
+select * from array_to_set2(array['one', 'two']) as t(f1 int,f2 text);
+ f1 | f2
+----+-----
+ 1 | one
+ 2 | two
+(2 rows)
+
+select * from array_to_set2(array['one', 'two']); -- fail
+ERROR: a column definition list is required for functions returning "record"
+LINE 1: select * from array_to_set2(array['one', 'two']);
+ ^
+select * from array_to_set2(array['one', 'two']) as t(f1 numeric(4,2),f2 text); -- fail
+ERROR: structure of query does not match function result type
+DETAIL: Returned type integer does not match expected type numeric(4,2) in column 1.
+CONTEXT: SQL statement "select i AS "index", $1[i] AS "value" from generate_subscripts($1, 1) i"
+PL/pgSQL function array_to_set2(anyarray) line 3 at RETURN QUERY
+select * from array_to_set2(array['one', 'two']) as t(f1 point,f2 text); -- fail
+ERROR: structure of query does not match function result type
+DETAIL: Returned type integer does not match expected type point in column 1.
+CONTEXT: SQL statement "select i AS "index", $1[i] AS "value" from generate_subscripts($1, 1) i"
+PL/pgSQL function array_to_set2(anyarray) line 3 at RETURN QUERY
+explain (verbose, costs off)
+ select * from array_to_set2(array['one', 'two']) as t(f1 int, f2 text);
+ QUERY PLAN
+-----------------------------------------------------
+ Function Scan on public.array_to_set2 t
+ Output: f1, f2
+ Function Call: array_to_set2('{one,two}'::text[])
+(3 rows)
+
+drop function array_to_set2;
+--
+-- test use of PL/pgSQL functions returning table
+--
+create or replace function array_to_set2(anyarray) returns table(index int, value anyelement) as $$
+ begin
+ return query execute 'select i AS "index", $1[i] AS "value" from generate_subscripts($1, 1) i'
+ using $1;
+ end;
+$$ language plpgsql immutable;
+-- materialize mode requires a result TupleDesc:
+select array_to_set2(array['one', 'two']);
+ array_to_set2
+---------------
+ (1,one)
+ (2,two)
+(2 rows)
+
+select * from array_to_set2(array['one', 'two']) as t(f1 int,f2 text); -- fail
+ERROR: a column definition list is redundant for a function with OUT parameters
+LINE 1: ...ct * from array_to_set2(array['one', 'two']) as t(f1 int,f2 ...
+ ^
+select * from array_to_set2(array['one', 'two']);
+ index | value
+-------+-------
+ 1 | one
+ 2 | two
+(2 rows)
+
+select * from array_to_set2(array['one', 'two']) as t(f1 numeric(4,2),f2 text); -- fail
+ERROR: a column definition list is redundant for a function with OUT parameters
+LINE 1: ...ct * from array_to_set2(array['one', 'two']) as t(f1 numeric...
+ ^
+select * from array_to_set2(array['one', 'two']) as t(f1 point,f2 text); -- fail
+ERROR: a column definition list is redundant for a function with OUT parameters
+LINE 1: ...ct * from array_to_set2(array['one', 'two']) as t(f1 point,f...
+ ^
+explain (verbose, costs off)
+ select * from array_to_set2(array['one', 'two']);
+ QUERY PLAN
+-----------------------------------------------------
+ Function Scan on public.array_to_set2
+ Output: index, value
+ Function Call: array_to_set2('{one,two}'::text[])
+(3 rows)
+
+drop function array_to_set2;
+--
-- Check some cases involving added/dropped columns in a rowtype result
--
create temp table users (userid text, seq int, email text, todrop bool, moredrop int, enabled bool);
diff --git a/src/test/regress/sql/rangefuncs.sql b/src/test/regress/sql/rangefuncs.sql
index 3c47c98e113..a898ac116de 100644
--- a/src/test/regress/sql/rangefuncs.sql
+++ b/src/test/regress/sql/rangefuncs.sql
@@ -647,6 +647,48 @@ select * from sin(3) as t(f1 int8,f2 int8); -- fail, scalar result type
drop type rngfunc_type cascade;
+--
+-- test use of PL/pgSQL functions returning setof record
+--
+
+create or replace function array_to_set2(anyarray) returns setof record as $$
+ begin
+ return query execute 'select i AS "index", $1[i] AS "value" from generate_subscripts($1, 1) i'
+ using $1;
+ end;
+$$ language plpgsql immutable;
+
+-- materialize mode requires a result TupleDesc:
+select array_to_set2(array['one', 'two']); -- fail
+select * from array_to_set2(array['one', 'two']) as t(f1 int,f2 text);
+select * from array_to_set2(array['one', 'two']); -- fail
+select * from array_to_set2(array['one', 'two']) as t(f1 numeric(4,2),f2 text); -- fail
+select * from array_to_set2(array['one', 'two']) as t(f1 point,f2 text); -- fail
+explain (verbose, costs off)
+ select * from array_to_set2(array['one', 'two']) as t(f1 int, f2 text);
+drop function array_to_set2;
+
+--
+-- test use of PL/pgSQL functions returning table
+--
+
+create or replace function array_to_set2(anyarray) returns table(index int, value anyelement) as $$
+ begin
+ return query execute 'select i AS "index", $1[i] AS "value" from generate_subscripts($1, 1) i'
+ using $1;
+ end;
+$$ language plpgsql immutable;
+
+-- materialize mode requires a result TupleDesc:
+select array_to_set2(array['one', 'two']);
+select * from array_to_set2(array['one', 'two']) as t(f1 int,f2 text); -- fail
+select * from array_to_set2(array['one', 'two']);
+select * from array_to_set2(array['one', 'two']) as t(f1 numeric(4,2),f2 text); -- fail
+select * from array_to_set2(array['one', 'two']) as t(f1 point,f2 text); -- fail
+explain (verbose, costs off)
+ select * from array_to_set2(array['one', 'two']);
+drop function array_to_set2;
+
--
-- Check some cases involving added/dropped columns in a rowtype result
--
--
2.45.0
Paul Jungwirth <pj@illuminatedcomputing.com> writes:
While working on inlining non-SQL SRFs [1] I noticed we don't have tests for when a PL/pgSQL
function requires materialize mode but doesn't have a result TupleDesc. Here is a patch adding tests
for that, as well as some other conditions around SRF calls with `SETOF RECORD` vs `TABLE (...)`.
There aren't any code changes, just some new tests.
AFAICT this test case adds coverage of exactly one line of code,
and that line is an unremarkable ereport(ERROR). I can't get
excited about spending test cycles on this forevermore, especially
not core-regression-test cycles.
But IMO it might be better to change the code. This error message is a bit confusing:
+-- materialize mode requires a result TupleDesc: +select array_to_set2(array['one', 'two']); -- fail +ERROR: materialize mode required, but it is not allowed in this context +CONTEXT: PL/pgSQL function array_to_set2(anyarray) line 3 at RETURN QUERY
A quick grep shows that there are ten other places throwing exactly
this same error message. About half of them are throwing it strictly
for
if (!(rsinfo->allowedModes & SFRM_Materialize))
and I think that that's a reasonable way to report that condition.
But the other half are throwing in other conditions such as
if (!(rsinfo->allowedModes & SFRM_Materialize) ||
rsinfo->expectedDesc == NULL)
and I agree with you that maybe we're being too lazy there.
I could get behind separate error messages for these conditions,
like
if (!(rsinfo->allowedModes & SFRM_Materialize))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("materialize mode required, but it is not allowed in this context")));
if (rsinfo->expectedDesc == NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("a column definition list is required for functions returning \"record\"")));
It's not quite clear to me if that's the same thing you're suggesting?
I'm also a bit uncomfortable with using that phrasing of the second
error, because it seems to be making assumptions that are well beyond
what this code knows to be true. Is it possible to get here in a
function that *doesn't* return record? Maybe we should just say
"a column definition list is required for this function", or words
to that effect (throwing in the function name might be good).
In any case, if we do something about it I'd want to do so in all
of the places that are taking similar shortcuts, not only plpgsql.
A different line of thought is to try to remove this implementation
restriction, but I've not looked at what that would entail.
regards, tom lane