Horton

Thread

Commits

  1. Simplify some stats restore code with InputFunctionCallSafe()

  2. Move attribute statistics functions to stat_utils.c

  1. statatt_build_stavalues->LOCAL_FCINFO wrong number

    jian he <jian.universality@gmail.com> — 2026-06-27T05:25:39Z

    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?

  2. Re: statatt_build_stavalues->LOCAL_FCINFO wrong number

    Michael Paquier <michael@paquier.xyz> — 2026-06-29T08:23:41Z

    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

  3. Re: statatt_build_stavalues->LOCAL_FCINFO wrong number

    jian he <jian.universality@gmail.com> — 2026-06-29T11:13:07Z

    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.



--
jian
https://www.enterprisedb.com/

  4. Re: statatt_build_stavalues->LOCAL_FCINFO wrong number

    Michael Paquier <michael@paquier.xyz> — 2026-06-29T23:32:30Z

    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

  5. Re: statatt_build_stavalues->LOCAL_FCINFO wrong number

    Corey Huinker <corey.huinker@gmail.com> — 2026-06-30T00:51:34Z

    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 ','.

  6. Re: statatt_build_stavalues->LOCAL_FCINFO wrong number

    jian he <jian.universality@gmail.com> — 2026-06-30T04:20:57Z

    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"?

  7. Re: statatt_build_stavalues->LOCAL_FCINFO wrong number

    Corey Huinker <corey.huinker@gmail.com> — 2026-06-30T15:18:30Z

    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.

>