Re: Remove fcinfo from statistics update internal functions

On Thu, Sep 3, 2026 at 8:22 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Wed, Sep 02, 2026 at 01:21:25PM -0400, Corey Huinker wrote:

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

How about setting those parameters as well, as we did before, because
we might use them in *_statistics_update_internal() in the future, as
I said upthread? IOW: I still don't think it's a good idea to
complicate the core code just for those parameters in the FDW case. I
think that that would just make the version diff large and
back-patching hard.

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

I don't think so; the issue would be just hidden inside the two structures.

> - A suspicion: long-term maintainability cost.

I don't follow this part. Could you elaborate on it a bit more?

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

+1

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

I think that that is just an appearance problem; we could address it
by defining NullableDatums for each stats in import_fetched_function()
like:

NullableDatum version;
NullableDatum null_frac;
NullableDatum avg_width;
NullableDatum n_distinct;
...

and providing these to the function like:

if (!import_attribute_statistics(relation, attnum, false,
version,
null_frac,
avg_width,
n_distinct,
...

Also, I think it's important for import_*_statistics() to have
arguments like this that are consistent with the error messages in
stats-checking functions I mentioned upthread. Using the structures
doesn't match the messages, so that would degrade developer
experience.

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

That's true. However, having such a tendency is no reason to prohibit this.

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

I don't feel the need for such macros at least for the use of
import_*_statistics(), because those functions are only used by FDW
authors, and they reference postgres_fdw when developing their FDWs,
so I think it's enough to provide a safe use of those functions in
postgres_fdw (and the documentation for it) even without those macros.

Best regards,
Etsuro Fujita