On Fri, Jul 3, 2026 at 6:47 PM Etsuro Fujita <[email protected]> wrote: > On Thu, Jul 2, 2026 at 5:35 AM Corey Huinker <[email protected]> wrote: > > On Wed, Jul 1, 2026 at 8:10 AM Etsuro Fujita <[email protected]> > > wrote: > >> On Wed, Jul 1, 2026 at 12:55 AM Corey Huinker <[email protected]> > >> wrote: > > >> >> In relation to this change, I moved helper functions set_XXX_arg() > >> >> that you added to relation_stats.c and attribute_stats.c to > >> >> postgres_fdw.c. The helper functions had soft error handling, but in > >> >> the postgres_fdw use, there is no need for that, so I removed that > >> >> handling as well. > >> > > >> > My inclination would be to move the set_*_arg functions could be moved > >> > to some common utility file in v20, as other FDWs will find themselves > >> > with the same need. > >> > >> Seems like a good idea. I moved the set_*_arg functions to > >> stat_utils.c, and renamed them to stats_set_*_arg. Attached is a new > >> version of the patch. > > > > I'm concerned that by making these non-error-safe function calls available, > > we create a barrier to making those same functions error-safe in the future. > > > >> > As for removing the error-handling, it might still be handy because it > >> > would allow us to give a more context-aware error message, rather than > >> > the very narrow "this_string is an invalid this_type", for string that > >> > the user most certainly never saw. Having said that, it's something we > >> > could easily change back to error-safe if we wanted to. > >> > >> Ok, I think we could do so later if needed. > > > > I agree we could do it later if they were private functions, no problem. > > But if they're available outside of postgres_fdw then changing them to be > > error-safe potentially disrupts other callers. Someone please let me know > > if I'm being unnecessarily cautious here. > > Ah, I misunderstood your comments. I agree with you here, so I'll > move back the functions to postgres_fdw.c.
Done. > > Other Thoughts: > > > > 1. The internal functions should accept a server_version_number parameter, > > and we should document that setting that parameter to 0 mean that we can > > assume the current version. I know that we presently have no translation > > issues going forward with statistics, but someday that won't be the case, > > and if we don't have this parameter in place then it'll be too late to fix > > it. > > Good idea! Will do. I used the name "version" for the consistency with pg_restore_relation_stats/pg_restore_attribute_stats. Also, as I couldn't find a note about the arrangement in the source code, I added this comment to match a comment in stats_fill_fcinfo_from_arg_pairs: * For now, the 'version' argument is ignored. In the future it can be used * to interpret older statistics properly. > > 3. The import_stats_functions in their current "bridging" form should do > > null checks on all of the NullableDatum pointers, or at least Assert()s on > > them. > > Will do. Done. As the functions are provided for developers, I just added the assertions. Also, I updated comments/docs a little bit. Attached is a new version of the patch. I'm planning to push and back-patch it, if no objections. Best regards, Etsuro Fujita
v3-Remove-SPI-from-stat-import-in-postgres-fdw-efujita.patch
Description: Binary data
