On Mon, Jun 29, 2026 at 5:14 PM Corey Huinker <[email protected]> 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
