Re: Remove fcinfo from statistics update internal functions

On Tue, Aug 25, 2026 at 03:18:25PM -0400, Corey Huinker wrote:
> The big changes from the previous effort are:
>
> 1. Instead of having separate per-stat parameters, or a shorted stat-only
> array of NullableDatum (thus requiring a separate index of datums, or some
> offset), instead we're just going to accept that the _internal function
> will ignore the first few elements of the NullableDatum array, specifically
> the ones concerning the object identification parameters that have already
> been checked/used/resolved. This removes the need for a second enum to
> index the shorter array, which in turn removes the need to rename the enum
> values.

Glop.

> 2. The wrapper functions around the recovery check and locking were not
> implemented, and so the *_update() functions were not removed, and so the
> the renaming of the *_update_internal() functions to the name of the
> recently vacated *_update() function is not done either. I think this
> leaves the _update() functions rather "thin" in what they do, but that
> gives us a chance to review how much further we want to go. The function
> attribute_statistics_update() is called in only by
> pg_restore_attribute_stats, and therefore could be entirely folded into
> that function. However, removing relation_statistics_update() would result
> in that code being duplicated in pg_restore_relation_stats() and
> pg_clear_relation_stats(), so that's less of a win without the stat_util
> wrapper function.

Less duplication sounds better to me for attribute_statistics_update().

> 0001-0003 phase out the use of FunctionCallInfo in any place where a simple
> NullableDatum array would suffice. It's still in 3 parts to make each
> change easier to see.

So, you have split the change so as one could see the changes across
the various API layers, with the most internal parts touched first:
first the stats_check_*() functions, second the stats update
functions, and third the places where we used the fake fcinfos
previously. For review, that's fine. I'd rather merge all three
changes together in the final result, but it's not the end of the
story for me... See below.

The incorrect comment in attribute_statistics_update_internal() was a
nice catch, incorrect since ce207d2a7901. Fixed that separately.

Traces related to LOCAL_FCINFO and InitFunctionCallInfoData() are gone
now with v2, which is nice.

Now, the real deal:

+ NullableDatum unused = {.isnull = true, .value = (Datum) 0};
[...]
+ args[ATTRELSCHEMA_ARG] = unused;
+ args[ATTRELNAME_ARG] = unused;
+ args[ATTNAME_ARG] = unused;
+ args[ATTNUM_ARG] = unused;
+ args[INHERITED_ARG] = unused;

I'm finding this part of the patch not acceptable, because it is
dictated by the fact that import_attribute_statistics() does not care
about these five unused parameters for postgres_fdw, these arguments
being required for the restore functions of the relation and attribute
stats.

To me, this points to a design defect of the postgres_fdw code,
because we pass to the import function pointers for each value from a
fcinfo then rebuild one. That's wasteful, and it complicates the
interfaces. Instead of a positional array, I think that we should use
two dedicated structures with named fields instead (one for
pg_class/rels, one for atts/pg_statistic), for relations and
attributes to avoid the guesses with the elements that may or may not
be used (aka the hardcoded unused pieces are not welcome here). That
would give for the attributes something among the lines of:
typedef struct AttStatsValues
{
NullableDatum null_frac;
NullableDatum avg_width;
/* And the rest, should be around a dozen in total */
} AttStatsValues;

With that, we should be able to bypass the positional issues, as well
as the fact that some of the parameters are not used, while cleaning
up the FDW-side import functions and all their arguments. The point
is where to make the cut due to the pairing of the arguments from the
fcinfos in the restore functions, but that's doable.

> 0004-0005 are the removal of "version" as a special parameter. They are
> strictly speaking outside the scope of $SUBJECT, so its fine if they don't
> get addressed in this thread.

Let's focus on the core proposal of the thread. I am not sure that
these are strongly necessary, TBH, this is just moving the check
of the version parameter from one place to another place.

With all that said, I have put my hands on my own idea of the problem,
using two structures shared by the fdw code and the stats restore code
to fill in the values, removing the need for a positional logic, and
finish with the attached, also leading to a negative in terms of code
lines:
8 files changed, 298 insertions(+), 333 deletions(-)

First I was wondering about the stats_check_*() functions being a
barrier, but it's easy enough to go through them knowing that we want
the argument names in the reports. The gain comes from
import_attribute_statistics() and import_relation_statistics(), that
do not need a zillion number of arguments. The fcinfos are of course
gone. In terms of the stats restore, the cut comes in
relation_statistics_update_internal() and
attribute_statistics_update_internal() which are the places where the
values in the structures are filled. We still need the array of
NullableDatums due to the pairing in stats_fill_args_from_arg_pairs()
during the stats restore that feeds from the original fcinfos. This
cut feels OK done this way, after watching the result this leads to on
the FDW side, which is much more palatable.

0001 is a merge of your original proposal, kept separated to show the
amount of changes I have done on top of it. 0002 is my refactoring
piece with the two structures for relation and attribute stats. Both
ought to be merged in a single commit, because they touch the same
places. HEAD-only cleanup; there is no way I would touch v19 at this
stage of the release cycle for a change that invasive.

So, what do you think of this v2?
--
Michael