Re: use of SPI by postgresImportForeignStatistics

On Mon, Jun 29, 2026 at 5:14 PM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:

>> Requiring Datums simplifies things greatly inside the existing functions, but pushes that complexity to the caller. My first proposal was to keep complexity to an absolute minimum on the caller side, but your comments make me think there's considerable tolerance for more complexity on the caller side.
>
>
> I've had some time to look this over, and it's pretty clear that we don't actually need the whole positional FunctionCallInfo, we just need the fcinfo->args of it. We also need the ability to pass statistical values along with the values that comprise the key of the relation/attribute/object to be modified.
>
> There are important differences in the parameters needed by the different types of functions. The pg_restore_*() SQL function calls need to identify the schema+relname of the relation being modified, and already have all of the values as typed Datums, whereas the internal API calls already have an identified and locked open Relation, but all of the statistical parameters are just cstrings.
>
> The solution I chose was to create common functions that take a relation object and an array of just the statistical parameters. This requires the pg_restore_* functions to resolve and lock the relation themselves, and then carry forward just the subset of parameters that are statistics (right types, but wrong number of arguments). Conversely, the internal API functions need to map their array of cstring values to the correctly typed Datums (wrong types, but right number of arguments in the right order).
>
> Attached is v2. The patches are broken down into very small bites to aid digestion and to confirm that tests pass after each comparatively minor change.

Thanks for doing that work!

I like this refactoring, but while it's rather mechanical, it's pretty
large, so I think it's too late to do the refactoring at this time
just before the beta 2 release. So I'd vote for going with your
v1-0001 and v1-0002 and doing the refactoring in v20. As mentioned by
Robert, I don't think it's good to call LOCAL_FCINFO() in
import_relation_statistics() and import_attribute_statistics() to call
the guts of those functions either, but that is *consistent* with the
existing way pg_restore_relation_stats() and
pg_restore_attribute_stats() do that, so that is actually not that
bad. Also, as you mentioned above, it's inefficient for the new API
functions to lock an already-locked relation, and validate an
already-validated attname/attnum, but I think it would be negligible.

Best regards,
Etsuro Fujita