On Mon, Aug 10, 2026 at 6:45 AM Etsuro Fujita <[email protected]> wrote:
> On Tue, Aug 4, 2026 at 7:48 AM Corey Huinker <[email protected]> > 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.
