On Thu, Oct 13, 2022 at 12:48:20PM -0700, Andres Freund wrote:
> I unfortunately just noticed this now, just after we released...
Thanks for the feedback.
> Even leaving the confusion with ExprSingleResult aside, calling it "Single"
> still seems very non-descriptive. I assume it's named to contrast with
> init_MultiFuncCall() etc. While those are also not named greatly, they're not
> typically used directly but wraped in SRF_FIRSTCALL_INIT etc.
This is mentioned on the original thread here, as of the point that we
go through the function one single time:
https://www.postgresql.org/message-id/Yh8SBTuzKhq7Jwda@paquier.xyz
> I also quite intensely dislike SRF_SINGLE_USE_EXPECTED. It sounds like it's
> saying that a single use of the SRF is expected, but that's not at all what it
> means: "use expectedDesc as tupdesc".
Okay. Something like the USE_EXPECTED_DESC you are suggesting or
USE_EXPECTED_TUPLE_DESC would be fine by me.
> I'm also confused by SRF_SINGLE_BLESS - the comment says "validate tuple for
> SRF". BlessTupleDesc can't really be described as validation, or am I missing
> something?
I'd rather keep the flag name to match the history behind this API.
How about updating the comment as of "complete tuple descriptor, for a
transient RECORD datatype", or something like that?
> Maybe something like InitMaterializedSRF() w/
> MAT_SRF_(USE_EXPECTED_DESC|BLESS)
Or just SetMaterializedFuncCall()? Do we always have to mention the
SRF part of it once we tell about the materialization part? The
latter sort implies the former once a function returns multiple
tuples.
I don't mind doing some renaming of all that even post-release, though
comes the question of keeping some compabitility macros for
compilation in case one uses these routines? Any existing extension
code works out-of-the-box without these new routines, so the chance of
somebody using the new stuff outside core sounds rather limited but it
does not seem worth taking a risk, either.. And this has been in the
tree for a bit more than half a year now.
--
Michael
