Re: use of SPI by postgresImportForeignStatistics

On Fri, Jul 3, 2026 at 6:47 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> On Thu, Jul 2, 2026 at 5:35 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
> > On Wed, Jul 1, 2026 at 8:10 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> >> On Wed, Jul 1, 2026 at 12:55 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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