Re: use of SPI by postgresImportForeignStatistics

On Mon, Aug 10, 2026 at 6:45 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
wrote:

> On Tue, Aug 4, 2026 at 7:48 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> wrote:
> > There has been some time for v19 dust to settle, so I've taken the work
> I had done in the v2 patch set above, re-based that, and modified the end
> result to use named parameters over arg arrays of differing lengths
> depending on the purpose per Fujita-san's work, and tried to keep those
> changes in the same very small layered steps for easier reviewing.
>
> Thanks for working on this!
>
> As I said before, this is a pretty large change (and is actually an
> issue since v18 in part), so it's too late to do it at this stage.
> IIUC we are nearing the end of the stabilization phase for v19.
>

Sorry if I wasn't clear, this is all for v20. If there was a chance that we
wanted any of this for v19 I would have worked on it earlier, but at the
time I got the sense that we wanted the most minimal viable change for 19,
so I intentionally stopped there.

> I took a quick look at some patches:
>
> > Patches 0001-0003: Rename the argnum enum values to have a common prefix
> (RELARG_, ATTARG_, EXTARG_)
>
> Do we really need this change? If not, I think that that would result
> in just making back-patching hard. This applies to all the changes,
> not just this one, but to make it easy, we need to consider the
> consistency across versions as much as possible.
>

Need? No, but it does help highlight the patterns shared by the 3 stats
types, and I think it's worth it. If backpatcching is the issue, I'd
suggest backpatching these all the way back to 18. If there comes a time
when these data structures ever have to mix in the same code, having names
that are unambiguous about which group they belong to will be key.

Also, if we stick with arrays of NullableDatums, then the renaming becomes
absolutely essential because we will then have two enumerations per stat
type (relation/attribute/extended), one for the positional order of the
arguments that could have come from the pg_restore_*_stats() function call
and the enum that is the shorter subset of the first set, minus the
schemaname/relname/statschema/statname which have already been resolved and
locked, which would index the array of NullableDatums passed to the
internal update() call. This results in a lot of shuffling between the new
arrays where we get lines like

stats_args[EXTSTAT_NULL_FRAC] = positional_args[EXTARG_NULL_FRAC];

In that example, stats_arg must be index by EXTSTAT_* enum values, whereas
positional_args must be indexed by the EXTARG_* family of enum values. And
that's after we standardize and align the naming structure - it would be
even more confusing without the standardization. Now clearly we can save
ourselves a lot of copying boilerplate by keeping the stat values in the
same order in the two arrays, and then doing an offset calculation with a
loop, but it's a consistency that has to be maintained.

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.

> Patch 0009: Stop treating "version" as a special-case parameter and add
> it to the StatsArgInfo arrays.
>
> +1, but I'd separate this from the patch series, as it's a different
> improvement than removing the LOCAL_FCINFO call from SQL functions for
> stats import like pg_restore_relation_stats.
>

Can do. It could go in before or after the rest of the patches, neither way
seems particularly difficult.

>
> > Patch 0011: Change the "internal" update functions to stop using the arg
> arrays (of which some values are now empty because we've already resolved
> the relation oid, etc) and instead use named NullableDatum parameters, like
> the import_*_statistics() functions.
>
> I'm not sure this is really a good idea, as it's easier to use the arg
> arrays than the NullableDatum parameters, which also minimizes the
> differences between versions (including future versions), making
> back-patching easy.
>

That's how it was originally. It's interesting that you take that position
given that's what the functions that are now the import_* functions
originally had. I see the maintenance advantages of going with arrays of
NullableDatums: fewer Assert() checks for null-ness, less code churn when
new stats are added. Even if we went with a struct that named all the stat
types, we would get those same advantages. The benefit of the named
parameters is that by adding a new parameter for the stat function, we
ensure that all callers must also add that parameter in order to recompile,
whereas adding a new value to the end of an array could be missed resulting
in an index out of bounds, and passing a pointer to a structure could
result in the new values remaining uninitialized. Granted, these are
internal functions so we don't have to worry about enforcing those changes
with outside callers, I'm just pointing out that each method has its own
maintenance/safey advantages. I went with this method because it 1)
mirrored the design choice made with the import_* functions and 2)
eliminated the array shuffling I cited in the example earlier in this
message.

My only strong opinion in all of these changes is that if we go with the
NullableDatum array for internal callers, then we must do the enum-renaming
as well.