Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Andres" == Andres Freund <andres@anarazel.de> writes:
>> I think it'd probably good to add accessors for value/nullness in
>> arguments that hide the difference between <v12 and v12, for the
>> sake of extension authors. Would probably mostly make sense if we
>> backpatched those for compatibility.
> Speaking as an affected extension author: don't backpatch them.
Yeah, I agree.
Looking at the patch, it seems like a real pain that substituting
"STACK_FCINFO_FOR_ARGS(fcinfo, ...)" for "FunctionCallInfoData fcinfo"
has the effect of converting "fcinfo" into a pointer. Is there a way
to define the macro so that that doesn't happen, and the ensuing
minor-but-invasive code changes aren't needed? (It'd be easy in C++,
but not quite seeing how to do it in C, at least not without defining
an additional macro for "fcinfo" that we'd then need to #undef at the
end of the function.)
With or without that, I'm pretty sure you wanted the pad member to be
char fcinfo_data[SizeForFunctionCallInfoData(nargs)]; \
not
char *fcinfo_data[SizeForFunctionCallInfoData(nargs)]; \
I also wonder if we should rename the type FunctionCallInfoData,
perhaps to FunctionCallInfo_Data, so as to intentionally break
code that hasn't been converted. On the other hand, that might
introduce too much useless code churn --- not sure how many live
references to that struct type will remain in place.
One more naming thought: would "LOCAL_FCINFO(...)" be a better
name for that macro? I don't think FOR_ARGS is adding much in
any case.
Why does struct agg_strict_input_check now have *both*
NullableDatum and "bool *nulls"? If that's not a typo,
it needs to be documented what the fields are for.
Please try to avoid random changes of vertical whitespace, eg in
the first hunk in pltcl.c. pgindent isn't very good about
cleaning that up.
I do not think the 0002 patch is a good idea. It's going to add
cycles to function calls, and it's not buying anything I'd call
important.
regards, tom lane