Re: Remove fcinfo from statistics update internal functions

On Tue, Sep 1, 2026 at 4:17 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

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

Sorry to disappoint you (at least that's what I think that means). I did it
that way based on feedback [1]:

>> The other alternative would be to require the import_* functions to fill
out the
>> same array create by the corresponding pg_restore_*_stats function, which
>> means it's adding in nulls for schemaname, relname, and other values
that will
>> not be used by the internal update function. That's clearly wasteful,
but would
>> reduce the need for moving all the stats args into a shorter array.

> +1, because 1) the cost for that would be negligible, and 2) in future
> we might need the full array, with the values for such parameters set,
> in the internal update function.
It wasn't the ideal solution, but if we were collectively tolerate the
unused fields, then the enumeration renaming would no longer be necessary,
and more extensive changes had already been rejected.

> > 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().
>

So you'd prefer we (eventually) move the code out of
attribute_statsistics_update(), but leave relation_statistics_update in
place? If so, maybe it makes sense to keep the stat-utils functions I
proposed, but leave them static-local to relation stats.

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

Yes, that was my assumption of what would be done. Showing the clear
intention of every change seemed more important, knowing it could be
squashed together later.

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

That was the trade-off that was required to make minimal changes to the
stat_utils functions, but not redefine new arrays and thus need new their
own index enums.

> 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;
>

+1. That's a return to an older design that was later refactored to
leverage the statsarginfo structures.

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

I like this change, and had this very change in [2].

I'm mildly curious why you used string literals that match the argname in
the statsarginfo structure rather than referencing the argname directly, or
having both using a common string via either a static constant or a
#define, but that takes nothing away from my liking the change.

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

+1

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

+1 to the use of [2]
+1 to HEAD-only, but that accepts that we're accepting the v19 code as-is,
including the redundant lock checks, etc.

> So, what do you think of this v2?

LGTM

[1]
https://www.postgresql.org/message-id/CAPmGK15AvxbzhA6MWJzDDcvSWRvNG1EP9kJLC0-HcRer-zBf0A%40mail.gmail.com

[2]
https://www.postgresql.org/message-id/attachment/201082/v3-0010-Decouple-stats_check_-functions-from-StatsArgInfo.patch