use of SPI by postgresImportForeignStatistics
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
Hi Robert,
On Tue, Jun 16, 2026 at 2:50 AM Robert Haas <robertmhaas@gmail.com> wrote:
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!).
Good catch!
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?
Ugh. As pg_restore_attribute_stats/pg_restore_relation_stats are
volatile functions, SPI executes the queries in read-write mode,
causing an error, like "schema "foo" does not exist", or other
unexpected results in such a case. Not sure what to do about this
issue right now. Will think about it some more.
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.
I thought it would be a good idea to use
pg_restore_attribute_stats/pg_restore_relation_stats, because future
changes in attribute/relation stats would be absorbed by these
functions, which would lower the maintenance cost of this feature.
Thanks for the comments!
Best regards,
Etsuro Fujita
On Tue, Jun 16, 2026 at 8:04 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
I thought it would be a good idea to use
pg_restore_attribute_stats/pg_restore_relation_stats, because future
changes in attribute/relation stats would be absorbed by these
functions, which would lower the maintenance cost of this feature.
I agree that we want to reuse code, but this isn't the right way to do
it. For example, when we want to look up the OID of a relation from
SQL, we can say 'whatever'::regclass::oid. But when we want to do the
same thing from C, we don't construct a SELECT statement and execute
it via SPI. Instead, we have functions like RangeVarGetRelidExtended()
which provide access to the same underlying functionality more
directly.
Another example is converting strings to integers. The user calls
int4in(), which then hands off the call to pg_strtoint32_safe(), which
can also be called via pg_strtoint32(). Hence, C code should prefer to
use pg_strtoint32(), while SQL will go through int4in(). Both
ultimately reach the underlying pg_strtoint32_safe() function, but the
interfaces are different, so that we can have it be suitable both for
SQL access and for C access.
This needs to work more like that. The underlying code that ingests
and updates the stats should be shared, but the stuff that is specific
to a FunctionCallInfo interface needs to be separated out so that we
don't need to go through that when calling from C.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, Jun 16, 2026 at 10:34 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jun 16, 2026 at 8:04 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:I thought it would be a good idea to use
pg_restore_attribute_stats/pg_restore_relation_stats, because future
changes in attribute/relation stats would be absorbed by these
functions, which would lower the maintenance cost of this feature.I agree that we want to reuse code, but this isn't the right way to do
it. For example, when we want to look up the OID of a relation from
SQL, we can say 'whatever'::regclass::oid. But when we want to do the
same thing from C, we don't construct a SELECT statement and execute
it via SPI. Instead, we have functions like RangeVarGetRelidExtended()
which provide access to the same underlying functionality more
directly.Another example is converting strings to integers. The user calls
int4in(), which then hands off the call to pg_strtoint32_safe(), which
can also be called via pg_strtoint32(). Hence, C code should prefer to
use pg_strtoint32(), while SQL will go through int4in(). Both
ultimately reach the underlying pg_strtoint32_safe() function, but the
interfaces are different, so that we can have it be suitable both for
SQL access and for C access.This needs to work more like that. The underlying code that ingests
and updates the stats should be shared, but the stuff that is specific
to a FunctionCallInfo interface needs to be separated out so that we
don't need to go through that when calling from C.
Back when pg_restore_attribute_stats and pg_restore_relation_stats were
being converted from positional to variadic functions, there was a patchset
or two (possibly un-posted, because I couldn't find it just now) where the
variadic functions re-marshalled the arguments into a call to a
fixed-parameter function. If that model were revived, we could have a more
conventional DirectFunctonCall() interface.
Is it possible that we never built a direct function call interface to a
variadic because we never needed one? Perhaps that time is now.
On Tue, Jun 16, 2026 at 2:08 PM Corey Huinker <corey.huinker@gmail.com>
wrote:
On Tue, Jun 16, 2026 at 10:34 AM Robert Haas <robertmhaas@gmail.com>
wrote:On Tue, Jun 16, 2026 at 8:04 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:I thought it would be a good idea to use
pg_restore_attribute_stats/pg_restore_relation_stats, because future
changes in attribute/relation stats would be absorbed by these
functions, which would lower the maintenance cost of this feature.I agree that we want to reuse code, but this isn't the right way to do
it. For example, when we want to look up the OID of a relation from
SQL, we can say 'whatever'::regclass::oid. But when we want to do the
same thing from C, we don't construct a SELECT statement and execute
it via SPI. Instead, we have functions like RangeVarGetRelidExtended()
which provide access to the same underlying functionality more
directly.Another example is converting strings to integers. The user calls
int4in(), which then hands off the call to pg_strtoint32_safe(), which
can also be called via pg_strtoint32(). Hence, C code should prefer to
use pg_strtoint32(), while SQL will go through int4in(). Both
ultimately reach the underlying pg_strtoint32_safe() function, but the
interfaces are different, so that we can have it be suitable both for
SQL access and for C access.This needs to work more like that. The underlying code that ingests
and updates the stats should be shared, but the stuff that is specific
to a FunctionCallInfo interface needs to be separated out so that we
don't need to go through that when calling from C.Back when pg_restore_attribute_stats and pg_restore_relation_stats were
being converted from positional to variadic functions, there was a patchset
or two (possibly un-posted, because I couldn't find it just now) where the
variadic functions re-marshalled the arguments into a call to a
fixed-parameter function. If that model were revived, we could have a more
conventional DirectFunctonCall() interface.Is it possible that we never built a direct function call interface to a
variadic because we never needed one? Perhaps that time is now.
Obviously, even that wouldn't get us to the no-FunctionCallInfo-at-all
goal, but it would get us out of the SPI situation. If we did have a
non-fcinfo function API, that API would either have to pass char *params
almost exclusively, or else each caller would have to do the translation or
float-array strings to double[] and such, which would be a lot of work for
the caller.
Would you be ok with a function like attribute_statistics_update, but
purely with cstring args? Obviously the callers would have to modify their
calls each time a new stat param is added, but that is seemingly preferable
for you than the current situation.
On Tue, Jun 16, 2026 at 3:11 PM Corey Huinker <corey.huinker@gmail.com> wrote:
Obviously, even that wouldn't get us to the no-FunctionCallInfo-at-all goal, but it would get us out of the SPI situation. If we did have a non-fcinfo function API, that API would either have to pass char *params almost exclusively, or else each caller would have to do the translation or float-array strings to double[] and such, which would be a lot of work for the caller.
Would you be ok with a function like attribute_statistics_update, but purely with cstring args? Obviously the callers would have to modify their calls each time a new stat param is added, but that is seemingly preferable for you than the current situation.
I feel pretty strongly that we want a way to identify the relation by
OID rather than by schema and name. I'm not sure whether attributes
should be identified by name or by number in this context. Apart from
those things, if passing strings and leaving it up to the called
function to interpret them makes most sense, that's OK with me.
I agree with you that getting rid of SPI is a much higher priority
than getting rid of the FunctionCallInfo stuff altogether. I could
live with DirectFunctionCall if we don't have a better option. I'm a
bit suspicious that this will lead to code bloat in postgres_fdw or
elsewhere, but maybe not.
--
Robert Haas
EDB: http://www.enterprisedb.com
Would you be ok with a function like attribute_statistics_update, but
purely with cstring args? Obviously the callers would have to modify their
calls each time a new stat param is added, but that is seemingly preferable
for you than the current situation.I feel pretty strongly that we want a way to identify the relation by
OID rather than by schema and name.
Previous versions of pg_set_attribute_stats() were like that, but it went
away as the use-cases folded into pg_restore_attribute_stats(). So it's
do-able, but will make sharing code with the existing function harder in
the near term. We may have to settle for something that takes the oid, but
then plugs into the existing functions until such time as we can refactor
the existing callers.
I'm not sure whether attributes
should be identified by name or by number in this context. Apart from
those things, if passing strings and leaving it up to the called
function to interpret them makes most sense, that's OK with me.
Identifying attributes by attnum is mostly for index expressions, so
attname would be the preference here.
I agree with you that getting rid of SPI is a much higher priority
than getting rid of the FunctionCallInfo stuff altogether. I could
live with DirectFunctionCall if we don't have a better option.
Ok, good to know that I have that as a fallback position.
I think the right first step is to create the functions
import_relation_stats() and import_attribute_stats() which take the oid of
the destination relation and the rest of the parameters are const char
pointers, which is the ideal case for a bunch of values being fetched from
PQgetValue(). Those functions will for now just make the call to
attribute_statistics_update() / relation_statistics_update(), but at least
it will be localized inside relation_stats.c and attribute_stats.c where we
can make internal refactors without disturbing anyone else.
Now, we *could* make these 2 new functions take the oid/oid+attname
followed by a variadic array of keyname+value string pairs like the
existing pg_restore_*_stats() functions, so the functions call signature
would be somewhat future-proofed. Let me know if that violates your vision
of what this should be.
I'm a
bit suspicious that this will lead to code bloat in postgres_fdw or
elsewhere, but maybe not.
postgres_fdw will get slimmer for not having SPI in it. relation_stats.c
and attribute_stats.c will have some temporary bloat until things can be
refactored.
On Tue, Jun 16, 2026 at 6:50 PM Corey Huinker <corey.huinker@gmail.com> wrote:
postgres_fdw will get slimmer for not having SPI in it. relation_stats.c and attribute_stats.c will have some temporary bloat until things can be refactored.
Whatever ends up in core can (indeed must) be reused by every FDW. So
making things simpler for the FDW and more complex for core seems
likely to be the right tradeoff in general.
--
Robert Haas
EDB: http://www.enterprisedb.com
Hi Corey,
On Wed, Jun 17, 2026 at 7:50 AM Corey Huinker <corey.huinker@gmail.com> wrote:
I think the right first step is to create the functions import_relation_stats() and import_attribute_stats() which take the oid of the destination relation and the rest of the parameters are const char pointers, which is the ideal case for a bunch of values being fetched from PQgetValue(). Those functions will for now just make the call to attribute_statistics_update() / relation_statistics_update(), but at least it will be localized inside relation_stats.c and attribute_stats.c where we can make internal refactors without disturbing anyone else.
+1
Now, we *could* make these 2 new functions take the oid/oid+attname followed by a variadic array of keyname+value string pairs like the existing pg_restore_*_stats() functions, so the functions call signature would be somewhat future-proofed. Let me know if that violates your vision of what this should be.
IMO I don't think we need to go that far, because 1) the new functions
are provided for FDW authors only, and 2) we don't make changes to the
stats that often.
Thanks!
Best regards,
Etsuro Fujita
IMO I don't think we need to go that far, because 1) the new functions
are provided for FDW authors only, and 2) we don't make changes to the
stats that often.
That would be simpler, to be sure. However, in the past I've received
pushback on functions that had a large number of parameters, and this would
definitely be a large number of parameters, approximately 17, so I thought
I should at least offer the variadic way.
I'm proceeding with the many-parameters model.
On Tue, Jun 16, 2026 at 6:57 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jun 16, 2026 at 6:50 PM Corey Huinker <corey.huinker@gmail.com>
wrote:postgres_fdw will get slimmer for not having SPI in it. relation_stats.c
and attribute_stats.c will have some temporary bloat until things can be
refactored.Whatever ends up in core can (indeed must) be reused by every FDW. So
making things simpler for the FDW and more complex for core seems
likely to be the right tradeoff in general.
I had assumed we wanted a generic C API to these two functions, but if we
want something that is specific to FDWs, that might change where the
functions land in the header files. It's an easy change to make if we
change our minds, but it would be good to know if we do or do not want
something specific to FDWs. Personally, I think FDWs will be 90-95% of the
usages outside of the existing SQL-defined functions, but 95% is not 100%,
so I'd be inclined to leave them in a statistics/stats utils header.
On Wed, Jun 17, 2026 at 12:03 PM Corey Huinker <corey.huinker@gmail.com> wrote:
I had assumed we wanted a generic C API to these two functions, but if we want something that is specific to FDWs, that might change where the functions land in the header files. It's an easy change to make if we change our minds, but it would be good to know if we do or do not want something specific to FDWs. Personally, I think FDWs will be 90-95% of the usages outside of the existing SQL-defined functions, but 95% is not 100%, so I'd be inclined to leave them in a statistics/stats utils header.
I don't want it to be specific to FDWs, just convenient for FDWs.
Agree they should stay in a statistics header.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, Jun 18, 2026 at 1:00 AM Corey Huinker <corey.huinker@gmail.com> wrote:
IMO I don't think we need to go that far, because 1) the new functions
are provided for FDW authors only, and 2) we don't make changes to the
stats that often.That would be simpler, to be sure. However, in the past I've received pushback on functions that had a large number of parameters, and this would definitely be a large number of parameters, approximately 17, so I thought I should at least offer the variadic way.
I'm proceeding with the many-parameters model.
I think that that's acceptable, considering that
heap_create_with_catalog() has 21 parameters.
Thanks!
Best regards,
Etsuro Fujita
On Thu, Jun 18, 2026 at 6:43 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:
On Thu, Jun 18, 2026 at 1:00 AM Corey Huinker <corey.huinker@gmail.com>
wrote:IMO I don't think we need to go that far, because 1) the new functions
are provided for FDW authors only, and 2) we don't make changes to the
stats that often.That would be simpler, to be sure. However, in the past I've received
pushback on functions that had a large number of parameters, and this would
definitely be a large number of parameters, approximately 17, so I thought
I should at least offer the variadic way.I'm proceeding with the many-parameters model.
I think that that's acceptable, considering that
heap_create_with_catalog() has 21 parameters.Thanks!
Best regards,
Etsuro Fujita
Attached are two patches to remove SPI from the postgres_fdw.c, replaced by
local C functions. The division into two patches is entirely for
readability. Separately, each one represents a half-measure that would
benefit no one. The first does the minimal changes needed to get the
relation-stats to stop using SPI, and the second does the same for
attribute stats, removes all vestiges of SPI from postgres_fdw.c, and adds
some tests to make sure that a foreign table has the exact stats of the
source loopback table.
The function import_attribute_stats() is a bit parameter-happy, but that's
mostly necessary. I left the non-key parameters as all type char* because
that avoids even more "bool foo_isnull" parameters, and that allows the
caller to not have to rework the various stat types into their
corresponding C types (float, int4, float[], and the anyarrays that
basically aren't cast-able by any client program anyway). I'm open to
requiring the caller to use the right datatypes for some of the parameters,
but as I said there is no way to "win" with the anyarrays, and even the
pg_restore_attribute_stats() function falls back to type text for those.
To be clear, this solves the SPI problem, but questions about the design of
attribute_statistics_update() and relation_statistics_update() remain, but
those concerns are now isolated within their respective files
attribute_stats.c and relation_stats.c. The inefficiencies therein aren't
really in a critical path, so if we wanted to leave them be until v20 they
could, but if time allows I'd at least like try unwinding some of that. But
first, let's get SPI out of postgres_fdw.c.
Attachments:
v1-0002-Remove-SPI-in-favor-of-import_attribute_statistic.patchtext/x-patch; charset=US-ASCII; name=v1-0002-Remove-SPI-in-favor-of-import_attribute_statistic.patchDownload+339-221
v1-0001-Remove-SPI-in-favor-of-import_relation_statistics.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Remove-SPI-in-favor-of-import_relation_statistics.patchDownload+133-48
On Fri, Jun 19, 2026 at 6:23 AM Corey Huinker <corey.huinker@gmail.com> wrote:
Attached are two patches to remove SPI from the postgres_fdw.c, replaced by local C functions. The division into two patches is entirely for readability. Separately, each one represents a half-measure that would benefit no one. The first does the minimal changes needed to get the relation-stats to stop using SPI, and the second does the same for attribute stats, removes all vestiges of SPI from postgres_fdw.c, and adds some tests to make sure that a foreign table has the exact stats of the source loopback table.
The function import_attribute_stats() is a bit parameter-happy, but that's mostly necessary. I left the non-key parameters as all type char* because that avoids even more "bool foo_isnull" parameters, and that allows the caller to not have to rework the various stat types into their corresponding C types (float, int4, float[], and the anyarrays that basically aren't cast-able by any client program anyway). I'm open to requiring the caller to use the right datatypes for some of the parameters, but as I said there is no way to "win" with the anyarrays, and even the pg_restore_attribute_stats() function falls back to type text for those.
To be clear, this solves the SPI problem, but questions about the design of attribute_statistics_update() and relation_statistics_update() remain, but those concerns are now isolated within their respective files attribute_stats.c and relation_stats.c. The inefficiencies therein aren't really in a critical path, so if we wanted to leave them be until v20 they could, but if time allows I'd at least like try unwinding some of that. But first, let's get SPI out of postgres_fdw.c.
Ok, I'll review the patches.
Thanks!
Best regards,
Etsuro Fujita
On Thu, Jun 18, 2026 at 5:23 PM Corey Huinker <corey.huinker@gmail.com> wrote:
To be clear, this solves the SPI problem, but questions about the design of attribute_statistics_update() and relation_statistics_update() remain, but those concerns are now isolated within their respective files attribute_stats.c and relation_stats.c. The inefficiencies therein aren't really in a critical path, so if we wanted to leave them be until v20 they could, but if time allows I'd at least like try unwinding some of that. But first, let's get SPI out of postgres_fdw.c.
I think that's the right order of priority, but I don't think that
having import_attribute_statistics construct an fcinfo is great.
Ideally, attribute_statistics_update would call
import_attribute_statistics rather than the other way around.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Fri, Jun 19, 2026 at 9:45 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jun 18, 2026 at 5:23 PM Corey Huinker <corey.huinker@gmail.com>
wrote:To be clear, this solves the SPI problem, but questions about the design
of attribute_statistics_update() and relation_statistics_update() remain,
but those concerns are now isolated within their respective files
attribute_stats.c and relation_stats.c. The inefficiencies therein aren't
really in a critical path, so if we wanted to leave them be until v20 they
could, but if time allows I'd at least like try unwinding some of that. But
first, let's get SPI out of postgres_fdw.c.I think that's the right order of priority, but I don't think that
having import_attribute_statistics construct an fcinfo is great.
Me either, I'm looking at "phase 2" already where
relation/attribute_statistics_update becomes a conventional function, and
pg_clear_attribute_stats and pg_restore_attribute_stats (and their relstats
equivalents) marshal their parameters to call that instead.
Ideally, attribute_statistics_update would call
import_attribute_statistics rather than the other way around.
I think we're mostly on the same page. If we require the caller to
understand the stats data structures to a greater level of detail, and
require it to do the transformations to the proper input types
(BlockNumbers, floats, float arrays, cstrings for anyarray, etc), then the
import_attribute_statistics functon and the new attribute_statistics_update
would be one-in-the-same. The v1 patch leaned heavily (perhaps too far)
towards letting the caller pass along string values fetched via
PQgetvalue() from a pg_stats without modification.
On Sat, Jun 20, 2026 at 12:11 AM Corey Huinker <corey.huinker@gmail.com>
wrote:
On Fri, Jun 19, 2026 at 9:45 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jun 18, 2026 at 5:23 PM Corey Huinker <corey.huinker@gmail.com>
wrote:To be clear, this solves the SPI problem, but questions about the
design of attribute_statistics_update() and relation_statistics_update()
remain, but those concerns are now isolated within their respective files
attribute_stats.c and relation_stats.c. The inefficiencies therein aren't
really in a critical path, so if we wanted to leave them be until v20 they
could, but if time allows I'd at least like try unwinding some of that. But
first, let's get SPI out of postgres_fdw.c.I think that's the right order of priority, but I don't think that
having import_attribute_statistics construct an fcinfo is great.Me either, I'm looking at "phase 2" already where
relation/attribute_statistics_update becomes a conventional function, and
pg_clear_attribute_stats and pg_restore_attribute_stats (and their relstats
equivalents) marshal their parameters to call that instead.Ideally, attribute_statistics_update would call
import_attribute_statistics rather than the other way around.I think we're mostly on the same page. If we require the caller to
understand the stats data structures to a greater level of detail, and
require it to do the transformations to the proper input types
(BlockNumbers, floats, float arrays, cstrings for anyarray, etc), then the
import_attribute_statistics functon and the new attribute_statistics_update
would be one-in-the-same. The v1 patch leaned heavily (perhaps too far)
towards letting the caller pass along string values fetched via
PQgetvalue() from a pg_stats without modification.
An update on my progress on Phase 2:
I was able to convert relation_statistics_update() with relatively little
fuss, but ran into trouble in doing so for attribute_statistics_update().
Initially the plan was to have a data structure
RelationStatsData/AttributeStatsData which contained a series of boolean
has_FOO flags alongside FOO of the actual stat type (int32, float, float[],
or cstring for anyarray value) and have the respective functions convert
their parameters to this common structure before calling the common
function.
The first bit of dissonance comes from the SQL-level functions having
schemaname+relname parameters, and the reloid is resolved via
RangeVarGetRelidExtended() which has a callback to check for correct
permissions and setting the proper lock level. However, the C-caller would
either have the reloid already, or an already open Relation, but no
assurance that the caller has the correct permissions for that table or the
correct lock level on the table. So either we make an equivalent to
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 the RangeVar and putting an Assert on the before/after reloids,
but perhaps the smarter play is to make a function that checks for
ShareUpdateExclusiveLock on the Relation, and then does the equivalent of
RangeVarCallbackForStats().
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
call takes a relation, thus ensuring that some level of locking worked,
thus we aren't in recovery.
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/attstatsinfo structure, and this makes a lot of type checking
and value checking compact and uniform. If we want to keep this sort of
uniformity, the resulting StatsData structure will end up looking a lot
like the FunctionCallInfo that we already had.
Next is the fact that the end-destination for every value passed in is a
Datum for a pg_statistic heaptuple. Most Datum values are checked only for
their null-ness and if they're the correct type, so the value itself is
usually just passed directly from fcinfo into the heap tuple values[]
array. The float[] values are checked for number of elements and whether
any elements are NULL, but that is done via array functions that take a
Datum input. Only in a few cases do we actually look at the actual internal
value of the Datum (reltuples, attname, attnum, the anyarrays), so there's
little to gain there.
There's some additional hassle in the fact that
pg_restore_attribute_stats() can take an attnum parameter OR an attname
parameter, but not both. I was able to resolve that in a semi-elegant
fashion, but the other issues have convinced me that we're probably better
off continuing to use the FunctionCallInfo version of
attribute_stats_update(), though perhaps with a different name, allowing us
to use that name for the new API call instead of
import_attribute_statistics().
One thing we *do* need to change from my v1 patch is moving the recovery
check and the RangeVar check out of relation_statistics_update() and
attribute_statsistics_update(), and having the respective callers do those
checks themselves first, passing in the now-authoritative reloid from
stats_acquire_relation_lock().
To that end, here's a new and rebased patch set:
0001 - exactly the same as before
0002 - exactly the same as before
0003 - New stat_utils.c function stats_acquire_relation_lock() which covers
the RangeVar and Recovery checks common to all the functions that modify
relation or attribute stats.
0004 - Add a small regression test to relation stats
0005 - Use stats_acquire_relation_lock() in relstats functions, and rename
the publicly-facing relstats C function.
0006 - Rename the publicly-facing attstats functions.
0007 - Use stats_acquire_relation_lock() in attstats functions.
Attachments:
v2-0001-Remove-SPI-in-favor-of-import_relation_statistics.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Remove-SPI-in-favor-of-import_relation_statistics.patchDownload+133-48
v2-0002-Remove-SPI-in-favor-of-import_attribute_statistic.patchtext/x-patch; charset=US-ASCII; name=v2-0002-Remove-SPI-in-favor-of-import_attribute_statistic.patchDownload+339-221
v2-0003-New-function-stats_acquire_relation_lock.patchtext/x-patch; charset=US-ASCII; name=v2-0003-New-function-stats_acquire_relation_lock.patchDownload+28-1
v2-0005-Rename-import_relation_statistics-to-relation_sta.patchtext/x-patch; charset=US-ASCII; name=v2-0005-Rename-import_relation_statistics-to-relation_sta.patchDownload+105-94
v2-0004-postgres_fdw-additional-regression-reverse-set-di.patchtext/x-patch; charset=US-ASCII; name=v2-0004-postgres_fdw-additional-regression-reverse-set-di.patchDownload+19-1
v2-0006-Rename-attribute_statistics-functions.patchtext/x-patch; charset=US-ASCII; name=v2-0006-Rename-attribute_statistics-functions.patchDownload+11-12
v2-0007-Make-callers-of-update_attstats-use-stats_acquire.patchtext/x-patch; charset=US-ASCII; name=v2-0007-Make-callers-of-update_attstats-use-stats_acquire.patchDownload+51-59
On Tue, Jun 23, 2026 at 3:08 AM Corey Huinker <corey.huinker@gmail.com> wrote:
To that end, here's a new and rebased patch set:
0001 - exactly the same as before
+/*
+ * Convenience routine to parse float values, and emit a warning on parse
+ * errors.
+ *
+ * Returns -1.0 if the value is NULL or invalid.
+ */
+static float
+str_to_float(const char *s)
+{
+ const float default_value = -1.0;
+
+ float result;
+
+ ErrorSaveContext escontext = {T_ErrorSaveContext};
+
+ if (!s)
+ return default_value;
+
+ result = float4in_internal((char *) s, NULL, "float", s, (Node *) &escontext);
+
+ if (escontext.error_occurred)
+ {
+ escontext.error_data->elevel = WARNING;
+ ThrowErrorData(escontext.error_data);
+ FreeErrorData(escontext.error_data);
+ return default_value;
+ }
+
+ return result;
+}
+Just a quick thought: the above can be replaced by InputFunctionCallSafe?
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 is resolved via RangeVarGetRelidExtended() which has a callback to check for correct permissions and setting the proper lock level. However, the C-caller would either have the reloid already, or an already open Relation, but no assurance that the caller has the correct permissions for that table or the correct lock level on the table. So either we make an equivalent to 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 the RangeVar and putting an Assert on the before/after reloids, but perhaps the smarter play is to make a function that checks for 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 call takes 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/attstatsinfo structure, and this makes a lot of type checking and value checking compact and uniform. If we want to keep this sort of uniformity, the resulting StatsData structure will end up looking a lot like the FunctionCallInfo that 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 Datum values are checked only for their null-ness and if they're the correct type, so the value itself is usually just passed directly from fcinfo into the heap tuple values[] array. The float[] values are checked for number of elements and whether any elements are NULL, but that is done via array functions that take a Datum input. Only in a few cases do we actually look at the actual internal value of the Datum (reltuples, attname, attnum, the anyarrays), so there's little to 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 attname parameter, but not both. I was able to resolve that in a semi-elegant fashion, but the other issues have convinced me that we're probably better off continuing to use the FunctionCallInfo version of attribute_stats_update(), though perhaps 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
