Paul Jungwirth <p...@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
