Re: use of SPI by postgresImportForeignStatistics

On Tue, Aug 11, 2026 at 4:58 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
> On Mon, Aug 10, 2026 at 6:45 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
>> 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.

IIUC we aren't allowed to do back-patching for such a reason, so if we
rename these, we should do so for master only. The renaming would be
an improvement, but I'm not sure that the advantage overweighs the
disadvantage of making back-patching hard.

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

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

I think we should have the named parameters for import_* functions,
for error messages in stats-check functions in stat_utils.c like this,
as mentioned before:

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

BUT: I don't think we need to do so for internal functions; for them,
I think we should just use arrays of NullableDatums, for the reason
you mentioned above.

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

IIUC I think what you are proposing here is:

* Removing the construction of FCINFO from pg_restore_*_stats/import_* functions
* Removing the special handling of the version parameter
* Renaming the argnum enum values

Am I right? If so, I think these are in the order of necessity, so I
recommend re-splitting the patchset into the three parts in the order.
I lowered the priority of the renaming, because 1) I don't make it a
blocker for others, and 2) I want to see how it improves things on its
own.

Also, as this is an improvement for v20, I recommend creating a new
thread (and a new CF entry) for it.

Thanks for working on this!

Best regards,
Etsuro Fujita