Re: Remove fcinfo from statistics update internal functions

On Tue, Sep 01, 2026 at 03:13:25PM -0400, Corey Huinker wrote:
> 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:
> Sorry to disappoint you (at least that's what I think that means). I did it
> that way based on feedback [1]:

That's fine.

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

Not sure about this part, perhaps. It depends on how it would build
on top of what I am proposing.

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

Noted. Was that discussed at the early stages of the stats restore
thread? I see less arguments in favor of this approach as we only had
the stats restore back then, nothing about the import code in FDWs.
With both things in place, the structure-based approach feels cleaner
overall.

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

Self-documentation, a reduction of the trace of attarginfo in the code
was feeling pretty nice overall, still one bigger reason is because
this completely cuts attarginfo from the stats import path. I don't
disagree that this leads to duplicates with the strings, but I'm
feeling rather OK about that as the arg names are only there for the
error reports. That also relates to the unused arguments, a little,
decoupling this knowledge between the import and restore code feels
pretty nice.

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

Ah, yes, noted. At least I am not going crazy yet. That alone feels
slightly shy because it does not consider the full stack of arguments.

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

Yeah, FWIW I don't feel that anything discussed here is worth touching
in v19. Refactoring pieces done late in the release cycle of what's
already a stable branch does not pay off and fires back easily, in my
experience. The diffs across branches is annoying, but it would not
be the first time I deal with refactoring-only pieces after forking a
stable branch.

Okay, so I am planning to cleanup all that after a second lookup.
Whether we should do this in v19 may be a different discussion, but at
this point I think that it is just too risky to consider, so I'll leave
the stable beasts alone.
--
Michael