Hi, I'm concerned about the way that postgresImportForeignStatistics is doing what it does. First, it's using SPI to execute two constant SQL queries, attimport_sql and attclear_sql. However, it doesn't seem to set the search_path, and these queries aren't fully robust against a possibly-unsafe search_path (e.g. the call to pg_restore_attribute_stats is schema-qualified, but the type names are not!). Furthermore, the function we're calling takes a schema name and a relation name as two separate arguments, rather than a single OID argument. I'm not sure it's completely guaranteed that we'll end up affecting the same relation that we locked in postgresImportForeignStatistics, because e.g. what if the containing schema is being concurrently renamed?
But even if it is absolutely guaranteed that we latch onto the correct relation, this seems like the fundamentally wrong way to do this kind of thing. It seems crazy to me that instead of exposing an interface that would be well-suited to direct use by an FDW, the statistics-import stuff exposes an interface that can only be reasonably called via the FunctionCallInfo interface, which then results in postgres_fdw needing to jump through hoops to construct and execute SQL statements. This doesn't look entirely easy to straighten out. A function like attribute_statistics_update() is just not a good idea -- it mixes together considerations that only exist when you want to call something from SQL with general validity checking that's needed regardless. But I don't think we should ship this like this. Best case scenario, it's overly complicated. Medium case scenario, it's also unreliable. Worst case scenario, there are security vulnerabilities in here. -- Robert Haas EDB: http://www.enterprisedb.com
