> > Also, we have exported import_attribute_statistics() in v19, so we > should avoid changing its signature. So I think the second patch > should be reverted at least. Sorry, but this isn't that trivial a > change, so I think you should have taken more time (at least a few > days) for others to look at it. >
I remembered this hours after sending my message, and I agree with the concern. > > With that, we should be able to bypass the positional issues, as well > > as the fact that some of the parameters are not used, while cleaning > > up the FDW-side import functions and all their arguments. The point > > is where to make the cut due to the pairing of the arguments from the > > fcinfos in the restore functions, but that's doable. > > My way of thinking is the exact opposite of yours (and Corey's): I > think it's better to handle both restore and FDW cases in a unified > way, rather than complicating the code for the latter case, which also > makes the version diff large and thus makes back-patching hard. > I'm actually going to carve out a third way of thinking here. I think we should settle on the FDW API now, and we can fix the internals in v20. The existing v19 FDW has an unattractive number of parameters, but that becomes a strength in situations where new stat types are added: a missing parameter is always a compile error, whereas uninitialized struct values are not, and undersized arrays passed into a structure are similarly dangerous. This strength is more important here because the programmer likely using that API is an extension writer who will likely compiling that extension across multiple pg-versions, and wouldn't likely notice that an array got one element longer from v22 to v23, or this struct had a member added from v21 to v22. So I would rank them as mass-of-ugly-parameters > pointer to big struct > array. `They will notice a compile error, though, and that avoids a lot of POLA-violations for our consumers. Internally, the calculus is different, and the pointers to a struct feel cleaner and more self documenting, and also serves to alleviate my discomfort over using regular strings over constants or defines, as the string being used is always the struct member name in quotes. 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.
