Remove fcinfo from statistics update internal functions

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.

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

0001-0003 phase out the use of FunctionCallInfo in any place where a simple
NullableDatum array would suffice. It's still in 3 parts to make each
change easier to see.

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.

The incorrect comment in attribute_statistics_update_internal() was a
nice catch, incorrect since ce207d2a7901. Fixed that separately.

Traces related to LOCAL_FCINFO and InitFunctionCallInfoData() are gone
now with v2, which is nice.

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.

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;

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.

0004-0005 are the removal of "version" as a special parameter. They are
strictly speaking outside the scope of $SUBJECT, so its fine if they don't
get addressed in this thread.

Let's focus on the core proposal of the thread. I am not sure that
these are strongly necessary, TBH, this is just moving the check
of the version parameter from one place to another place.

With all that said, I have put my hands on my own idea of the problem,
using two structures shared by the fdw code and the stats restore code
to fill in the values, removing the need for a positional logic, and
finish with the attached, also leading to a negative in terms of code
lines:
8 files changed, 298 insertions(+), 333 deletions(-)

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

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.

So, what do you think of this v2?
--
Michael

On Tue, Sep 1, 2026 at 4:17 AM Michael Paquier <michael@paquier.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]/messages/by-id/CAPmGK15AvxbzhA6MWJzDDcvSWRvNG1EP9kJLC0-HcRer-zBf0A@mail.gmail.com:

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]/messages/by-id/attachment/201082/v3-0010-Decouple-stats_check_-functions-from-StatsArgInfo.patch.

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]: /messages/by-id/CAPmGK15AvxbzhA6MWJzDDcvSWRvNG1EP9kJLC0-HcRer-zBf0A@mail.gmail.com
/messages/by-id/CAPmGK15AvxbzhA6MWJzDDcvSWRvNG1EP9kJLC0-HcRer-zBf0A@mail.gmail.com

[2]: /messages/by-id/attachment/201082/v3-0010-Decouple-stats_check_-functions-from-StatsArgInfo.patch
/messages/by-id/attachment/201082/v3-0010-Decouple-stats_check_-functions-from-StatsArgInfo.patch

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

Hi Michael-san,

I know you committed the patches already, but...

On Tue, Sep 1, 2026 at 5:17 PM Michael Paquier <michael@paquier.xyz> wrote:

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.

The reason why I set up those parameters was that I thought we might
use them in attribute_statistics_update_internal() in the future even
when called from import_attribute_statistics(). Consider eg, adding
to the former function a feature for stats-logging or anything that
takes as an input a NullableDatum array that is assumed to store those
parameters as well.

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 from me. As I said in the original thread,
import_attribute_statistics() should have individual arguments for
each stats value, to fit error messages in stats-checking functions in
stat_utils.c like this:

ereport(WARNING,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("argument \"%s\" must be specified when
argument \"%s\" is specified",
arginfo[nullarg].argname,
arginfo[otherarg].argname)));

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.

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.

Thanks for working on this!

Best regards,
Etsuro Fujita

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.

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.

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.

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.

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

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

On Thu, Sep 03, 2026 at 08:21:49AM +0900, Michael Paquier wrote:

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.

In short just the attached, minus the fields that do not need to be
set anymore.
--
Michael

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 argument reordering argument is a good one.

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

That wasn't my concern. My concern was that somebody implements these
functions for their own FDW extension, and then when Postgres N+1 comes out
we've added a new stats type, but they don't notice because their code
written for version N still compiles. The initializer macro below does
alleviate my concern a bit.

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

Sounds good so far.

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

There's adding fields to pg_statistic, which we haven't done in a while,
but the stats import is leveraged through pg_stats, and that changed as
recently as bc3c8db8ae2f (2023) when we exposed range_length_histogram and
range_bounds_histogram. The fact that it doesn't happen all that often
makes the change all the more likely to be overlooked.

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.

Does that mean you're in favor of pushing the structs and postgres_fdw API
change to v19 then?

In short just the attached, minus the fields that do not need to be
set anymore.

+1

On Wed, Sep 02, 2026 at 10:57:15PM -0400, Corey Huinker wrote:

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

That wasn't my concern. My concern was that somebody implements these
functions for their own FDW extension, and then when Postgres N+1 comes out
we've added a new stats type, but they don't notice because their code
written for version N still compiles. The initializer macro below does
alleviate my concern a bit.

I'm perhaps less concerned than you are on this point due to the
addition argument that happens not that many times. So, I choose
prefer the safety net of a clean default. My suspicion is that folks
doing out-of-core business with this code are knowledgeable enough
about the internals that they would detect the new fields, or they
would not work on this amound of stats details at all.

Does that mean you're in favor of pushing the structs and postgres_fdw API
change to v19 then?

Hmm. Adding Nathan in CC on this one for comment from the RMT. It's
rather low priority compared to all the open items I can see listed on
the wiki, but it's an option I find worth considering by the day. I
don't want to put more stress than necessary on the RMT regarding this
thread, though..

On a risk scale, the risk is close to zero here, still not completely
zero due to b345a554edf3 in postgres_fdw.c, where we should use do a
set_int32_arg()->set_uint32_arg() before import_relation_statistics()
to be completely consistent. There is no difference in
src/backend/statistics/ between HEAD and REL_19_STABLE, the new bound
checks for the range values are outside the code paths touched by
cc9a8eb112fa. So I am not really concerned about a cherry-pick of
cc9a8eb112fa to REL_19_STABLE.
--
Michael

On Thu, Sep 3, 2026 at 8:22 AM Michael Paquier <michael@paquier.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

On Thu, Sep 03, 2026 at 02:35:53PM +0900, Etsuro Fujita wrote:

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.

Hmm. I'm not sure that this is correct with a v19 in mind.

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

Not sure that I that much. It does not address my concerns regarding
the import APIs.

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.

Ditto.

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.

Disagreed. Consistency is usually good in code style. In terms of
extension experience, the structures feel much easier to work with.
That's less index positioning.

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.

Even with my disagreements, it is pretty clear that you do not like
these improvements, so I'll revert the refactoring piece. I am not
planning to argue much for the FDW part (honestly I don't like the way
this has been coded, and I regret not having spent a couple of cycles
looking at what was happening, but I did not commit that). For the
stats part, though, I am going to be on standby and more careful for
any changes suggested.

Another part that I do not find acceptable is the "unused" arguments
based on the StatsInfo. That's overlapping the import and restore
bits, IMO. That part alone makes the 0001 proposed upthread a no-go
here.
--
Michael

On Thu, Sep 3, 2026 at 4:09 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Sep 03, 2026 at 02:35:53PM +0900, Etsuro Fujita wrote:

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.

Hmm. I'm not sure that this is correct with a v19 in mind.

Even with my disagreements, it is pretty clear that you do not like
these improvements, so I'll revert the refactoring piece. I am not
planning to argue much for the FDW part (honestly I don't like the way
this has been coded, and I regret not having spent a couple of cycles
looking at what was happening, but I did not commit that). For the
stats part, though, I am going to be on standby and more careful for
any changes suggested.

Thanks for the consideration! I'm open to comments/suggestions, but
I'm not 100% sure that the changes in the FDW part are really
improvements.

Another part that I do not find acceptable is the "unused" arguments
based on the StatsInfo. That's overlapping the import and restore
bits, IMO. That part alone makes the 0001 proposed upthread a no-go
here.

I like the 0001 patch (except the handling of the "unused" arguments).
Code changes made by it is really minimized, so it won't make
back-patching hard, which is good. Thanks for working on the patch!

As for the handling, I think we should just set the arguments as well,
as we did before, for the possible use in
*_statistics_update_internal() in the future, as I said above. I
think that that is more future-proof.

Best regards,
Etsuro Fujita

On Thu, Sep 03, 2026 at 12:35:26PM +0900, Michael Paquier wrote:

On Wed, Sep 02, 2026 at 10:57:15PM -0400, Corey Huinker wrote:

Does that mean you're in favor of pushing the structs and postgres_fdw API
change to v19 then?

Hmm. Adding Nathan in CC on this one for comment from the RMT. It's
rather low priority compared to all the open items I can see listed on
the wiki, but it's an option I find worth considering by the day. I
don't want to put more stress than necessary on the RMT regarding this
thread, though..

From later the discussion, it looks like there's nothing that needs RMT
review at the moment. Please let me know if I've got that wrong.

--
nathan

On Thu, Sep 03, 2026 at 01:42:09PM -0500, Nathan Bossart wrote:

On Thu, Sep 03, 2026 at 12:35:26PM +0900, Michael Paquier wrote:

Hmm. Adding Nathan in CC on this one for comment from the RMT. It's
rather low priority compared to all the open items I can see listed on
the wiki, but it's an option I find worth considering by the day. I
don't want to put more stress than necessary on the RMT regarding this
thread, though..

From later the discussion, it looks like there's nothing that needs RMT
review at the moment. Please let me know if I've got that wrong.

FWIW, the best conclusion that I can offer regarding this thread on my
side is that I am pretty unhappy with the decisions taken on
REL_19_STABLE on the core backend side regarding the way the stats
import code has been plugged, reflecting in the way FDWs need to work
with that. It is not entirely the fault of the stats import code,
because the stats import code has been trying to work around decisions
that have been taken in v18 for the relation and attribute stats.
What feels wrong to me is that we are trying to work around these
decisions after-the-fact. A more logical flow that I try to follow is
first to fix the original underlying issues, even if it means
reverting any new code, provide a clean basis, then work on top of the
clean basis, and not work *around* them.

That pretty much reflects my feeling regarding this thread: 0001 is a
reflection of what looks like a workaround (the "unused" bits are one
part of that, but not everything I feel rather weird about), and the
early choices from v18 reflect into the rest in the code tree in what
spreads as unelegant ways. That has a long-term cost I'd like to not
underestimate.

Using structures and splitting the dependencies between the stats
restore and stats import logic as I was proposing was providing a
cleaner split, IMO. Even if I agree that it still is a sort of
workaround, there is nothing better I can propose as long as we don't
have a cleaner basis. I don't think that I would not have done things
this way and did not originally commit any of that, so I'm by
definition not its owner and we all have our style quirks. In terms
of my own qwirks I tend to pick up community-style consistency, not of
all us do which is fine by me. So, shrug? ¯\(ツ)/¯
--
Michael