statatt_build_stavalues->LOCAL_FCINFO wrong number
Hi.
https://git.postgresql.org/cgit/postgresql.git/commit/?id=213a1b89527049cb27bbcd6871fdb0c0916b43e1
+Datum
+statatt_build_stavalues(const char *staname, FmgrInfo *array_in,
Datum d, Oid typid,
+ int32 typmod, bool *ok)
+{
+ LOCAL_FCINFO(fcinfo, 8);
+ char *s;
+ Datum result;
+ ErrorSaveContext escontext = {T_ErrorSaveContext};
+
+ escontext.details_wanted = true;
+
+ s = TextDatumGetCString(d);
+
+ InitFunctionCallInfoData(*fcinfo, array_in, 3, InvalidOid,
+ (Node *) &escontext, NULL);
+
+ fcinfo->args[0].value = CStringGetDatum(s);
+ fcinfo->args[0].isnull = false;
+ fcinfo->args[1].value = ObjectIdGetDatum(typid);
+ fcinfo->args[1].isnull = false;
+ fcinfo->args[2].value = Int32GetDatum(typmod);
+ fcinfo->args[2].isnull = false;
+
+ result = FunctionCallInvoke(fcinfo);
+
+ pfree(s);
+
+ if (escontext.error_occurred)
+ {
+ escontext.error_data->elevel = WARNING;
+ ThrowErrorData(escontext.error_data);
+ *ok = false;
+ return (Datum) 0;
+ }The above LOCAL_FCINFO(fcinfo, 8);
should be
LOCAL_FCINFO(fcinfo, 3);
That mistake is harmless now, but if somebody use
LOCAL_FCINFO(fcinfo, 1), then it may have potential problems.
So let's try to use InputFunctionCallSafe instead, see attached.
I found this while trying to use InputFunctionCallSafe inside array_in_safe.
Should we consider fully refactoring array_in_safe to use it too?
Attachments:
v1-0001-use-InputFunctionCallSafe-when-possible.patchtext/x-patch; charset=US-ASCII; name=v1-0001-use-InputFunctionCallSafe-when-possible.patchDownload+2-17
On Sat, Jun 27, 2026 at 01:25:39PM +0800, jian he wrote:
That mistake is harmless now, but if somebody use
LOCAL_FCINFO(fcinfo, 1), then it may have potential problems.
So let's try to use InputFunctionCallSafe instead, see attached.
It's an overallocation, harmless still incorrect. This also exists on
REL_18_STABLE; this code has been moved on HEAD to its current
location from atttribute_stats.c.
I found this while trying to use InputFunctionCallSafe inside array_in_safe.
Should we consider fully refactoring array_in_safe to use it too?
Yep, let's do this cleanup in array_in_safe() as well. I was
wondering about doing that only once v20 opens up, but array_in_safe()
is new in v19. So let's just make the code right now rather than
create an inconsistency before v19 gets released.
Thanks for the report. Will do something for both spots.
--
Michael
On Mon, Jun 29, 2026 at 4:23 PM Michael Paquier <michael@paquier.xyz> wrote:
Thanks for the report. Will do something for both spots.
checking attribute_stats.c, extended_stats_funcs.c
/*
* Build an array datum with element type elemtypid from a text datum, used as
* value of an attribute in a pg_statistic tuple.
*
* If an error is encountered, capture it, and reduce the elevel to WARNING.
*
* This is an adaptation of statatt_build_stavalues().
*/
static Datum
array_in_safe(FmgrInfo *array_in, const char *s, Oid typid, int32 typmod,
AttrNumber exprnum, const char *element_name, bool *ok)
/*
* Build an array with element type elemtypid from a text datum, used as
* value of an attribute in a tuple to-be-inserted into pg_statistic.
*
statatt_build_stavalues
"elemtypid" does not appear within the function (mentioned above), so
"elemtypid" comment is wrong?
The general idea is to add a trailing comma to the last field of each enum.
We need to add a comma after the last field in enum attribute_stats_argnum,
clear_attribute_stats_argnum, and extended_stats_exprs_element.
On Mon, Jun 29, 2026 at 07:13:07PM +0800, jian he wrote:
"elemtypid" does not appear within the function (mentioned above), so
"elemtypid" comment is wrong?
Nice catch. These comments missed a refresh when this code has been
reworked. I have bundled that in the patch, while on it.
The general idea is to add a trailing comma to the last field of each enum.
We need to add a comma after the last field in enum attribute_stats_argnum,
clear_attribute_stats_argnum, and extended_stats_exprs_element.
I didn't see a strong need in this one, though..
--
Michael
On Mon, Jun 29, 2026 at 7:32 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Jun 29, 2026 at 07:13:07PM +0800, jian he wrote:
"elemtypid" does not appear within the function (mentioned above), so
"elemtypid" comment is wrong?Nice catch. These comments missed a refresh when this code has been
reworked. I have bundled that in the patch, while on it.The general idea is to add a trailing comma to the last field of each
enum.
We need to add a comma after the last field in enum
attribute_stats_argnum,
clear_attribute_stats_argnum, and extended_stats_exprs_element.
I didn't see a strong need in this one, though..
In all of those cases, the last value of the enum is essentially "how many
distinct values are in this enum", so that last value must always be the
last value, hence not having a trailing ','.
Hi.
I have one more tiny question...
pg_restore_attribute_stats->attribute_statistics_update->statatt_build_stavalues->FunctionCallInvoke
which may fail, causing pg_restore_attribute_stats to return false.
/ * Parameters corresponding to ANYARRAY columns are instead passed in as text
* values, which is a valid input string for an array of the type or element
* type of the attribute. Any error generated by the array_in() function will
* in turn fail the function.
*/
To me, the phrase "fail the function" suggests that it raises
ereport(ERROR), rather than simply returning false.
However, here, array_in() does not appear to trigger a hard
ereport(ERROR). Instead, it causes attribute_statistics_update() to return
false.
Because of that, I'm not sure what the last sentence is trying to convey. Does
"fail the function" mean "cause the function to return false" rather than "raise
an error"?
On Mon, Jun 29, 2026, 11:21 PM jian he <jian.universality@gmail.com> wrote:
Hi.
I have one more tiny question...
pg_restore_attribute_stats->attribute_statistics_update->statatt_build_stavalues->FunctionCallInvoke
which may fail, causing pg_restore_attribute_stats to return false./ * Parameters corresponding to ANYARRAY columns are instead passed in as
text
* values, which is a valid input string for an array of the type or
element
* type of the attribute. Any error generated by the array_in() function
will
* in turn fail the function.
*/To me, the phrase "fail the function" suggests that it raises
ereport(ERROR), rather than simply returning false.
However, here, array_in() does not appear to trigger a hard
ereport(ERROR). Instead, it causes attribute_statistics_update() to return
false.
Because of that, I'm not sure what the last sentence is trying to convey.
Does
"fail the function" mean "cause the function to return false" rather than
"raise
an error"?
We want to return false. We can reword the comment.