Re: Add tests for PL/pgSQL SRFs

Paul Jungwirth <pj(at)illuminatedcomputing(dot)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