Thread: New "single-call SRF" APIs are very confusingly named
Hi,
I unfortunately just noticed this now, just after we released...
In
commit 9e98583898c347e007958c8a09911be2ea4acfb9
Author: Michael Paquier <michael@paquier.xyz>
Date: 2022-03-07 10:26:29 +0900
Create routine able to set single-call SRFs for Materialize mode
a new helper was added:
#define SRF_SINGLE_USE_EXPECTED 0x01 /* use expectedDesc as tupdesc */
#define SRF_SINGLE_BLESS 0x02 /* validate tuple for SRF */
extern void SetSingleFuncCall(FunctionCallInfo fcinfo, bits32 flags);
I think the naming here is very poor. For one, "Single" here conflicts with
ExprSingleResult which indicates "expression does not return a set",
i.e. precisely the opposite what SetSingleFuncCall() is used for. For another
the "Set" in SetSingleFuncCall makes it sound like it's function setting a
property.
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.
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".
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?
This IMO needs to be cleaned up.
Maybe something like InitMaterializedSRF() w/
MAT_SRF_(USE_EXPECTED_DESC|BLESS)
Greetings,
Andres Freund
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
Attachment
Hi, On 2022-10-14 10:28:34 +0900, Michael Paquier wrote: > On Thu, Oct 13, 2022 at 12:48:20PM -0700, Andres Freund wrote: > > Maybe something like InitMaterializedSRF() w/ > > MAT_SRF_(USE_EXPECTED_DESC|BLESS) > > Or just SetMaterializedFuncCall()? I think starting any function that's not a setter with Set* is very likely to be misunderstood (SetReturning* is clearer, but long). This just reads like you're setting the materialized function call on something. > Do we always have to mention the SRF part of it once we tell about the > materialization part? Yes. The SRF is the important part. > The latter sort implies the former once a function returns multiple tuples. There's lot of other other things that can be materialized. > 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? Agreed that we'd need compat. I think it'd need to be compatibility function, not just renaming via macro, so we keep ABI compatibility. Greetings, Andres Freund
On Thu, Oct 13, 2022 at 3:48 PM Andres Freund <andres@anarazel.de> wrote:
> I unfortunately just noticed this now, just after we released...
>
> In
>
> commit 9e98583898c347e007958c8a09911be2ea4acfb9
> Author: Michael Paquier <michael@paquier.xyz>
> Date: 2022-03-07 10:26:29 +0900
>
> Create routine able to set single-call SRFs for Materialize mode
>
>
> a new helper was added:
>
> #define SRF_SINGLE_USE_EXPECTED 0x01 /* use expectedDesc as tupdesc */
> #define SRF_SINGLE_BLESS 0x02 /* validate tuple for SRF */
> extern void SetSingleFuncCall(FunctionCallInfo fcinfo, bits32 flags);
>
>
> I think the naming here is very poor. For one, "Single" here conflicts with
> ExprSingleResult which indicates "expression does not return a set",
> i.e. precisely the opposite what SetSingleFuncCall() is used for. For another
> the "Set" in SetSingleFuncCall makes it sound like it's function setting a
> property.
>
> 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.
So, while I agree that the "Single" in SetSingleFuncCall() could be
confusing given the name of ExprSingleResult, I feel like actually all
of the names are somewhat wrong.
ExprSingleResult, ExprMultipleResult, and ExprEndResult are used as
values of ReturnSetInfo->isDone, used in value-per-call mode to indicate
whether or not a given value is the last or not. The comment on
ExprSingleResult says it means "expression does not return a set",
however, in Materialize mode (which is for functions returning a set),
isDone is supposed to be set to ExprSingleResult.
Take this code in ExecMakeFunctionResultSet()
else if (rsinfo.returnMode == SFRM_Materialize)
{
/* check we're on the same page as the function author */
if (rsinfo.isDone != ExprSingleResult)
So, isDone is used for a different purpose in value-per-call and
materialize modes (and with pretty contradictory definitions) which is
pretty confusing.
Besides that, it is not clear to me that ExprMultipleResult conveys that
the result is a member of or an element of a set. Perhaps it should be
ExprSetMemberResult and instead of using ExprSingleResult for
Materialize mode there should be another enum value that indicates "not
used" or "materialize mode". It could even be ExprSetResult -- since the
whole result is a set. Though that may be confusing since isDone is not
used for Materialize mode except to ensure "we're on the same page as
the function author".
Expr[Single|Multiple]Result aside, I do see how SINGLE/Single when used
for a helper function that does set up for SFRM_Materialize mode
functions is confusing.
The routines for SFRM_ValuePerCall all use multi, so I don't think it
was unreasonable to use single. However, I agree it would be better to
use something else (probably materialize).
The different dimensions requiring distinction are:
- returns a set (Y/N)
- called multiple times to produce a single result (Y/N)
- builds a tuplestore for result set (Y/N)
SFRM_Materialize comment says "result set instantiated in Tuplestore" --
So, I feel like the question is, does a function which returns its
entire result set in a single invocation have to do so using a
tuplestore and does one that returns part of its result set on each
invocation have to do so without a tuplestore (does each invocation have
to return a scalar or tuple)?
The current implementation may not support it, but it doesn't seem like
using a tuplestore and returning all elements of the result set vs some
of them in one invocation are alternatives.
It might be better if the SetFunctionReturnMode stuck to distinguishing
between functions returning their entire result in one invocation or
part of their result in one invocation.
That being said, the current SetSingleFuncCall() makes the tuplestore
and ensures the TupleDesc required by Materialize mode is set or
created. Since it seems only to apply to Materialize mode, I am in favor
of using "materialize" instead of "single".
> Maybe something like InitMaterializedSRF() w/
> MAT_SRF_(USE_EXPECTED_DESC|BLESS)
I also agree that starting the function name with Set isn't the best. I
like InitMaterializedSRF() and MAT_SRF_USE_EXPECTED_TUPLE_DESC. Are there
other kinds of descs?
Also, "single call" and "multi call" are confusing because they kind of
seem like they are describing a behavior of the function limiting the
number of times it can be called. Perhaps the multi* function names
could eventually be renamed something to convey how much of a function's
result can be expected to be produced on an invocation.
To summarize, I am in support of renaming SetSingleFuncCall() ->
InitMaterializedSRF() and SRF_SINGLE_USE_EXPECTED ->
MAT_SRF_USE_EXPECTED_TUPLE_DESC (or just DESC) as suggested elsewhere in
this thread. And I think we should eventually consider renaming the
multi* function names and consider if ExprSingleResult is a good name.
- Melanie
Melanie Plageman <melanieplageman@gmail.com> writes:
> So, while I agree that the "Single" in SetSingleFuncCall() could be
> confusing given the name of ExprSingleResult, I feel like actually all
> of the names are somewhat wrong.
Maybe, but ExprSingleResult et al. have been there for decades and
are certainly embedded in a ton of third-party code. It's a bit
late to rename them, whether you think they're confusing or not.
Maybe we can get away with changing names introduced in v15, but
even that I'm afraid will get some pushback.
Having said that, I'd probably have used names based on "materialize"
not "single" for what this code is doing.
regards, tom lane
On Fri, Oct 14, 2022 at 05:09:46PM -0400, Melanie Plageman wrote: > To summarize, I am in support of renaming SetSingleFuncCall() -> > InitMaterializedSRF() and SRF_SINGLE_USE_EXPECTED -> > MAT_SRF_USE_EXPECTED_TUPLE_DESC (or just DESC) as suggested elsewhere in > this thread. And I think we should eventually consider renaming the > multi* function names and consider if ExprSingleResult is a good name. As already mentioned, these have been around for years, so the impact would be bigger. Attached is a patch for HEAD and REL_15_STABLE to switch this stuff with new names, with what's needed for ABI compatibility. My plan would be to keep the compatibility parts only in 15, and drop them from HEAD. -- Michael
Attachment
On Fri, Oct 14, 2022 at 7:41 PM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Oct 14, 2022 at 05:09:46PM -0400, Melanie Plageman wrote:
> To summarize, I am in support of renaming SetSingleFuncCall() ->
> InitMaterializedSRF() and SRF_SINGLE_USE_EXPECTED ->
> MAT_SRF_USE_EXPECTED_TUPLE_DESC (or just DESC) as suggested elsewhere in
> this thread. And I think we should eventually consider renaming the
> multi* function names and consider if ExprSingleResult is a good name.
As already mentioned, these have been around for years, so the impact
would be bigger.
That makes sense.
Attached is a patch for HEAD and REL_15_STABLE to
switch this stuff with new names, with what's needed for ABI
compatibility. My plan would be to keep the compatibility parts only
in 15, and drop them from HEAD.
- * SetSingleFuncCall
+ * Compatibility function for v15.
+ */
+void
+SetSingleFuncCall(FunctionCallInfo fcinfo, bits32 flags)
+{
+ InitMaterializedSRF(fcinfo, flags);
+}
+
+ * Compatibility function for v15.
+ */
+void
+SetSingleFuncCall(FunctionCallInfo fcinfo, bits32 flags)
+{
+ InitMaterializedSRF(fcinfo, flags);
+}
+
Any reason not to use a macro?
- Melanie
Hi,
On 2022-10-16 13:22:41 -0700, Melanie Plageman wrote:
> On Fri, Oct 14, 2022 at 7:41 PM Michael Paquier <michael@paquier.xyz> wrote:
> - * SetSingleFuncCall
> + * Compatibility function for v15.
> + */
> +void
> +SetSingleFuncCall(FunctionCallInfo fcinfo, bits32 flags)
> +{
> + InitMaterializedSRF(fcinfo, flags);
> +}
> +
>
> Any reason not to use a macro?
Yes - it'd introduce an ABI break, i.e. an already compiled extension
referencing SetSingleFuncCall() wouldn't fail to load into an upgraded sever,
due to the reference to the SetSingleFuncCall, which wouldn't exist anymore.
Greetings,
Andres Freund
Hi, On 2022-10-15 11:41:08 +0900, Michael Paquier wrote: > Attached is a patch for HEAD and REL_15_STABLE to switch this stuff with new > names, with what's needed for ABI compatibility. My plan would be to keep > the compatibility parts only in 15, and drop them from HEAD. -- Michael Looks reasonable to me. Thanks for working on this. > -/* flag bits for SetSingleFuncCall() */ > -#define SRF_SINGLE_USE_EXPECTED 0x01 /* use expectedDesc as tupdesc */ > -#define SRF_SINGLE_BLESS 0x02 /* validate tuple for SRF */ > +/* flag bits for InitMaterializedSRF() */ > +#define MAT_SRF_USE_EXPECTED_DESC 0x01 /* use expectedDesc as tupdesc */ > +#define MAT_SRF_BLESS 0x02 /* complete tuple descriptor, for > + * a transient RECORD datatype */ I don't really know what "complete tuple descriptor" means. BlessTupleDesc() does say "make a completed tuple descriptor useful for SRFs" - but I don't think that means that Bless* makes them complete, but that they *have* to be complete to be blessed. > @@ -2164,8 +2164,8 @@ elements_worker_jsonb(FunctionCallInfo fcinfo, const char *funcname, > > rsi = (ReturnSetInfo *) fcinfo->resultinfo; > > - SetSingleFuncCall(fcinfo, > - SRF_SINGLE_USE_EXPECTED | SRF_SINGLE_BLESS); > + InitMaterializedSRF(fcinfo, > + MAT_SRF_USE_EXPECTED_DESC | MAT_SRF_BLESS); Already was the case, so maybe not worth mucking with: Why the newline here, but not in other cases? Greetings, Andres Freund
On Sun, Oct 16, 2022 at 03:04:43PM -0700, Andres Freund wrote: > Yes - it'd introduce an ABI break, i.e. an already compiled extension > referencing SetSingleFuncCall() wouldn't fail to load into an upgraded sever, > due to the reference to the SetSingleFuncCall, which wouldn't exist anymore. Note that this layer should just be removed on HEAD. Once an extension catches up with the new name, they would not even need to play with PG_VERSION_NUM even for a new version compiled with REL_15_STABLE. -- Michael
Attachment
On Sun, Oct 16, 2022 at 03:09:14PM -0700, Andres Freund wrote: > On 2022-10-15 11:41:08 +0900, Michael Paquier wrote: >> -/* flag bits for SetSingleFuncCall() */ >> -#define SRF_SINGLE_USE_EXPECTED 0x01 /* use expectedDesc as tupdesc */ >> -#define SRF_SINGLE_BLESS 0x02 /* validate tuple for SRF */ >> +/* flag bits for InitMaterializedSRF() */ >> +#define MAT_SRF_USE_EXPECTED_DESC 0x01 /* use expectedDesc as tupdesc */ >> +#define MAT_SRF_BLESS 0x02 /* complete tuple descriptor, for >> + * a transient RECORD datatype */ > > I don't really know what "complete tuple descriptor" means. BlessTupleDesc() > does say "make a completed tuple descriptor useful for SRFs" - but I don't > think that means that Bless* makes them complete, but that they *have* to be > complete to be blessed. That's just assign_record_type_typmod(), which would make sure to fill the cache for a RECORD tupdesc. How about "fill the cache with the information of the tuple descriptor type, for a transient RECORD datatype"? If you have a better, somewhat less confusing, idea, I am open to suggestions. >> @@ -2164,8 +2164,8 @@ elements_worker_jsonb(FunctionCallInfo fcinfo, const char *funcname, >> >> rsi = (ReturnSetInfo *) fcinfo->resultinfo; >> >> - SetSingleFuncCall(fcinfo, >> - SRF_SINGLE_USE_EXPECTED | SRF_SINGLE_BLESS); >> + InitMaterializedSRF(fcinfo, >> + MAT_SRF_USE_EXPECTED_DESC | MAT_SRF_BLESS); > > Already was the case, so maybe not worth mucking with: Why the newline here, > but not in other cases? Yeah, that's fine as well. -- Michael
Attachment
On Mon, Oct 17, 2022 at 10:13:33AM +0900, Michael Paquier wrote: > That's just assign_record_type_typmod(), which would make sure to fill > the cache for a RECORD tupdesc. How about "fill the cache with the > information of the tuple descriptor type, for a transient RECORD > datatype"? If you have a better, somewhat less confusing, idea, I am > open to suggestions. At the end, I was unhappy with this formulation, so I have just mentioned what the top of BlessTupleDesc() tells, with the name of the function used on the tuple descriptor when the flag is activated by the init call. The compatibility functions have been removed from HEAD, by the way. -- Michael
Attachment
Hi, Thanks for "fixing" this so quickly. Greetings, Andres Freund