Re: Remove fcinfo from statistics update internal functions

On Wed, Sep 02, 2026 at 01:21:25PM -0400, Corey Huinker wrote:
>> Also, we have exported import_attribute_statistics() in v19, so we
>> should avoid changing its signature. So I think the second patch
>> should be reverted at least. Sorry, but this isn't that trivial a
>> change, so I think you should have taken more time (at least a few
>> days) for others to look at it.
>>
>
> I remembered this hours after sending my message, and I agree with the
> concern.

Sorry about that. I was just focusing on that yesterday and the
cleanup was feeling worth doing, and well... It still does.

>>> 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.
>>
>> My way of thinking is the exact opposite of yours (and Corey's): I
>> think it's better to handle both restore and FDW cases in a unified
>> way, rather than complicating the code for the latter case, which also
>> makes the version diff large and thus makes back-patching hard.
>>
>
> I'm actually going to carve out a third way of thinking here. I think we
> should settle on the FDW API now, and we can fix the internals in v20.

I still think that using a set of two structures to pass all the
information is the cleanest way forward, for the following reasons:
- Previous patch 0001 tried to reuse the same structures as the
restore APIs, with some of the parameters getting unused. This
creates a weird design layer, because the import code begins to rely
on portions it does not care about.
- The dependencies of the StatsInfo structures used by the restore
functions become more integrated with the import code, but I doubt we
want that: the import code cares only about the values, knowing
already about OIDs of the objects to manipulate in the catalogs.
- We finish with APIs that are weaker to argument reordering. Values
in structures are self-documented.
- A suspicion: long-term maintainability cost.

> The existing v19 FDW has an unattractive number of parameters, but that
> becomes a strength in situations where new stat types are added: a missing
> parameter is always a compile error, whereas uninitialized struct values
> are not, and undersized arrays passed into a structure are similarly
> dangerous. This strength is more important here because the programmer
> likely using that API is an extension writer who will likely compiling that
> extension across multiple pg-versions, and wouldn't likely notice that an
> array got one element longer from v22 to v23, or this struct had a member
> added from v21 to v22. So I would rank them as mass-of-ugly-parameters >
> pointer to big struct > array. `They will notice a compile error, though,
> and that avoids a lot of POLA-violations for our consumers.

We are never going to add a new stats value in a import APIs on a
stable branch, as it touches the shape of the catalogs, but this kind
of code style itches because it is weak to the ordering of the data
given in input, and it still forces anybody who uses this code to set
every field:
/* Try to import the statistics. */
if (!import_attribute_statistics(relation, attnum, false,
&args[0], &args[1], &args[2],
&args[3], &args[4], &args[5],
&args[6], &args[7], &args[8],
&args[9], &args[10], &args[11],
&args[12], &args[13]))

Honestly, that's not a pattern I have seen a lot in the code base; we
tend to use structure-based approaches to pass down data rather than a
lot of args, because C makes this kind of code written easier to
parse and less opaque by assigning values to each member of the
structure. One example, the [auto]vacuum code for its state data and
its options passed across various stacks.

> Internally, the calculus is different, and the pointers to a struct feel
> cleaner and more self documenting, and also serves to alleviate my
> discomfort over using regular strings over constants or defines, as the
> string being used is always the struct member name in quotes.

The discomfort I still feel on HEAD regarding the fact that isnull
would be set to false for each NullableArray, meaning that some could
would attempt to set a value even if nothing is safe, is something
that is IMO a defect of the original v18 stats code: we have no
protection against that at all. Using the fake FunctionCallInfos or
even the 0001 patch does not offer this kind of protection either in
the restore code, which is a problem. I kind of agree that this is
kind of impossible to miss but that still makes me very uneasy. In
the import code, we rely on a ABI breakage to let extension developers
know, why not but it does not handle the isnull=true case cleanly
either.

So, isnull=true is not something that we can enforce using a C
initializer of the type {0}. However, it is something that we can
enforce with an initializer macro, as in the lines of (same for atts
and rels) in statistics.h:
#define RELATION_STATS_VALUES_NULL \
{ \
.version = {(Datum) 0, true}, \
.relpages = {(Datum) 0, true}, \
[...]
}

And then use this initializer for the defined structures, for both
the restore *and* the import code.

At this point, it would be impossible to miss that the initialization
macros need to be updated when adding a new field, and extensions can
use that to deal with their own choices. That also takes care of
enforcing a cleaner default for the restore code, something we do not
do now, even if we don't add stats fields very often (the last
instance was around v11, isn't it?). It is true that it does not make
extension developers consider about the new fields, but I would prime
on the arguments that a safer default for the catalogs is saner
argument, and put the trust on the developers if they begin to use the
import API to push data to the catalogs.

> The internal bit can wait for v20, as was always the plan. If I had thought
> it wasn't the plan, I would have kept up the work on this thru July. The
> FDW API, however, I think we need to decide now. I'd be ok with going to
> the struct pointers in the FDW API, but we're creating a hassle for
> ourselves if we decide to do that down the road.

What matters the most to me is the long-term maintenance of both code
structures, and the more I think about it, the more the structures
lead to a so-much-cleaner result. That would be even better with two
initializer macros for each structure that extensions can just reuse
to force isnull=true for each element, though. This enforces a safer
catalog insert policy for the import *and* the restore code.
--
Michael