Re: use of SPI by postgresImportForeignStatistics

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:
> > On Tue, Jun 30, 2026 at 7:30 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
> wrote:
>
> >> As postgresImportForeignStatistics() is provided for FDWs that want to
> >> calculate statistics remotely, so I'm assuming that they have enough
> >> knowledge about the stats-data structures.
>
> CORRECTION: s/postgresImportForeignStatistics()/ImportForeignStatistics()/
> Sorry for that.
>
> >> 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.

I modified the patch further. To support these differences:
>
> "There are important differences in the parameters needed by the
> different types of functions. The pg_restore_*() SQL function calls
> need to identify the schema+relname of the relation being modified,
> and already have all of the values as typed Datums, whereas the
> internal API calls already have an identified and locked open
> Relation, but all of the statistical parameters are just cstrings."
>
> I incorporated your v2-0010 and v2-0012 into the patch: 1) separate
> the guts of relation_statistics_update() and
> attribute_statistics_update() into new functions
> relation_statistics_update_internal() and
> attribute_statistics_update_internal(), and 2) modify
> import_relation_statistics() and import_attribute_statistics() to call
> these new functions instead. Also, I changed the stats-data
> argument-type in import_relation_statistics() and
> import_attribute_statistics() from NullableDatum * to const
> NullableDatum *.
>
> Best regards,
> Etsuro Fujita
>

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.

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.

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.