Re: Remove fcinfo from statistics update internal functions

Hi Michael-san,

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

On Tue, Sep 1, 2026 at 5:17 PM Michael Paquier <michael(at)paquier(dot)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