Re: Remove fcinfo from statistics update internal functions

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

Right, we're both waiting until the smoke clears.

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

Further back then that, I think mid-2024 before the stats_utils.c came to
be.

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

Thanks for explaining what otherwise seemed like a coin-flip decision. My
own sqwick factor with the strings is that it's one more chance to mistype
the name, and the string being used only in diagnostic user output was what
alleviated that concern for you.

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

This thread was only about v20, but it attracted the attention of the
release team.

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

This morning I remembered that you were talking about not doing any of this
for v19, and that means that the FDW API changes from 19 to 20. I see that
Etsuro Fujita has clearly pointed that out as well. I'll address that in
response to his message.