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. > 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. > 2. Did you put import_relation_statistics and import_attribute_statistics in > statistics.h because you see them as long-term publicly visible functions, > and what's in stats_utils.h to be more subject to change? If so, I could get > behind that. If not, then I fall back to my position that they seem like they > belong in new relation_stats.h and attribute_stats.h, or stats_utils.h. The answer is the former. I'll remove the changes made to stats_utils.h, though. > 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. Thanks for the comments! Best regards, Etsuro Fujita
