Re: use of SPI by postgresImportForeignStatistics - Mailing list pgsql-hackers

On Mon, Jun 22, 2026 at 3:08 PM Corey Huinker <corey.huinker@gmail.com> wrote:
> The first bit of dissonance comes from the SQL-level functions having schemaname+relname parameters, and the reloid
isresolved via RangeVarGetRelidExtended() which has a callback to check for correct permissions and setting the proper
locklevel. However, the C-caller would either have the reloid already, or an already open Relation, but no assurance
thatthe caller has the correct permissions for that table or the correct lock level on the table. So either we make an
equivalentto RangeVarGetRelidExtended() that takes an oid, or the C-caller has to derive a RangeVar, call the existing
RangeVarGetRelidExtended()function, and verify the result reloid against the supplied parameter. I went with deriving
theRangeVar and putting an Assert on the before/after reloids, but perhaps the smarter play is to make a function that
checksfor ShareUpdateExclusiveLock on the Relation, and then does the equivalent of RangeVarCallbackForStats(). 

I don't think this is really a problem. If the caller is specifying
the OID, they should have called RangeVarGetRelidExtended themselves.
Permissions-checking, locking, and opening the relation should all
happen simultaneously, and the logic shouldn't be duplicated later.

> A smaller bit of dissonance was with RecoveryInProgress(), which if I recall we're checking before
RangeVarGetRelidExtended()to avoid trying to take a lock that will fail. That check might not be meaningful if the C
calltakes a relation, thus ensuring that some level of locking worked, thus we aren't in recovery. 

I don't quite follow this part.

> Next is the existing validation functions stats_check_required_arg(), stats_check_arg_array(), and
stats_check_arg_pairs()all work on values indexed by the positional functioncallinfo and the corresponding
relstatsinfo/attstatsinfostructure, and this makes a lot of type checking and value checking compact and uniform.  If
wewant to keep this sort of uniformity, the resulting StatsData structure will end up looking a lot like the
FunctionCallInfothat we already had. 

Yeah, this is worth thinking about. You could consider putting an
array inside the struct and use #defines for the indexes. And instead
of having a separate Boolean for each index, you could use the same
index to reference the N'th bit of a single integer flag variable.

> Next is the fact that the end-destination for every value passed in is a Datum for a pg_statistic heaptuple. Most
Datumvalues are checked only for their null-ness and if they're the correct type, so the value itself is usually just
passeddirectly from fcinfo into the heap tuple values[] array. The float[] values are checked for number of elements
andwhether any elements are NULL, but that is done via array functions that take a Datum input. Only in a few cases do
weactually look at the actual internal value of the Datum (reltuples, attname, attnum, the anyarrays), so there's
littleto gain there. 

Right, so the question is whether it makes more sense to pass down C
strings or Datums.

> There's some additional hassle in the fact that pg_restore_attribute_stats() can take an attnum parameter OR an
attnameparameter, but not both. I was able to resolve that in a semi-elegant fashion, but the other issues have
convincedme that we're probably better off continuing to use the FunctionCallInfo version of attribute_stats_update(),
thoughperhaps with a different name, allowing us to use that name for the new API call instead of
import_attribute_statistics().

On that particular point, I think I'm still unconvinced, but I also
haven't looked deeply into this just yet, so maybe I'm wrong.

--
Robert Haas
EDB: http://www.enterprisedb.com