> > I still think that using a set of two structures to pass all the > information is the cleanest way forward, for the following reasons: > - Previous patch 0001 tried to reuse the same structures as the > restore APIs, with some of the parameters getting unused. This > creates a weird design layer, because the import code begins to rely > on portions it does not care about. > - The dependencies of the StatsInfo structures used by the restore > functions become more integrated with the import code, but I doubt we > want that: the import code cares only about the values, knowing > already about OIDs of the objects to manipulate in the catalogs. > - We finish with APIs that are weaker to argument reordering. Values > in structures are self-documented. > - A suspicion: long-term maintainability cost. >
The argument reordering argument is a good one. > > We are never going to add a new stats value in a import APIs on a > stable branch, as it touches the shape of the catalogs, but this kind > That wasn't my concern. My concern was that somebody implements these functions for their own FDW extension, and then when Postgres N+1 comes out we've added a new stats type, but they don't notice because their code written for version N still compiles. The initializer macro below does alleviate my concern a bit. > And then use this initializer for the defined structures, for both > the restore *and* the import code. > Sounds good so far. > > At this point, it would be impossible to miss that the initialization > macros need to be updated when adding a new field, and extensions can > use that to deal with their own choices. That also takes care of > enforcing a cleaner default for the restore code, something we do not > do now, even if we don't add stats fields very often (the last > instance was around v11, isn't it?). There's adding fields to pg_statistic, which we haven't done in a while, but the stats import is leveraged through pg_stats, and that changed as recently as bc3c8db8ae2f (2023) when we exposed range_length_histogram and range_bounds_histogram. The fact that it doesn't happen all that often makes the change all the more likely to be overlooked. > > The internal bit can wait for v20, as was always the plan. If I had > thought > > it wasn't the plan, I would have kept up the work on this thru July. The > > FDW API, however, I think we need to decide now. I'd be ok with going to > > the struct pointers in the FDW API, but we're creating a hassle for > > ourselves if we decide to do that down the road. > > What matters the most to me is the long-term maintenance of both code > structures, and the more I think about it, the more the structures > lead to a so-much-cleaner result. That would be even better with two > initializer macros for each structure that extensions can just reuse > to force isnull=true for each element, though. This enforces a safer > catalog insert policy for the import *and* the restore code. > Does that mean you're in favor of pushing the structs and postgres_fdw API change to v19 then?
