On Wed, Jul 1, 2026 at 12:55 AM Corey Huinker <[email protected]> wrote: > On Tue, Jun 30, 2026 at 7:30 AM Etsuro Fujita <[email protected]> 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
v2-Remove-SPI-from-stat-import-in-postgres-fdw-efujita.patch
Description: Binary data
