Re: use of SPI by postgresImportForeignStatistics

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.

> 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.

>> What do you think? If there is no problem with this change, we
>> wouldn't need to worry about the stability of postgres_fdw (and other
>> FDWs) in v20.
>
> If you're fine with the names import_relation_statistics and import_attribute_statistics, then yes. Those names made sense to me in the narrow case of being called from an FDW handler and handing the function a bunch of strings, but in a more general case with properly typed datums, the name feels less appropriate, but it's just a name and I wouldn't let it get in the way of the overall patch.

I like the names, but I'm open to suggestions.

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