Thread

use of SPI by postgresImportForeignStatistics

Robert Haas <robertmhaas@gmail.com> — 2026-06-15T17:50:46Z

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

Re: use of SPI by postgresImportForeignStatistics

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-16T12:04:37Z

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

Re: use of SPI by postgresImportForeignStatistics

Robert Haas <robertmhaas@gmail.com> — 2026-06-16T14:34:37Z

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

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-16T18:08:51Z

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.

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-16T19:11:45Z

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.

Re: use of SPI by postgresImportForeignStatistics

Robert Haas <robertmhaas@gmail.com> — 2026-06-16T21:53:55Z

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

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-16T22:50:20Z

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

Re: use of SPI by postgresImportForeignStatistics

Robert Haas <robertmhaas@gmail.com> — 2026-06-16T22:57:04Z

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

Re: use of SPI by postgresImportForeignStatistics

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-17T10:23:07Z

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

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-17T16:00:02Z

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

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-17T16:03:33Z

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.

Re: use of SPI by postgresImportForeignStatistics

Robert Haas <robertmhaas@gmail.com> — 2026-06-17T23:19:05Z

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

Re: use of SPI by postgresImportForeignStatistics

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-18T10:42:56Z

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

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-18T21:23:40Z

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.

Re: use of SPI by postgresImportForeignStatistics

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-19T12:30:02Z

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

Re: use of SPI by postgresImportForeignStatistics

Robert Haas <robertmhaas@gmail.com> — 2026-06-19T13:45:18Z

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

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-20T04:11:17Z

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.

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-22T19:08:33Z

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.

Re: use of SPI by postgresImportForeignStatistics

jian he <jian.universality@gmail.com> — 2026-06-23T11:43:14Z

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?



--
jian
https://www.enterprisedb.com/

Re: use of SPI by postgresImportForeignStatistics

Robert Haas <robertmhaas@gmail.com> — 2026-06-23T12:10:52Z

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

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-23T15:32:48Z

>
>
> Just a quick thought: the above can be replaced by InputFunctionCallSafe?
>

Potentially, yes. The functions were originally returning the actual data
types rather than the Datum serializations. Which way those functions go
depends a lot on the data structure they end up populating, which is
currently in flux.

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-23T15:53:22Z

On Tue, Jun 23, 2026 at 8:11 AM Robert Haas <robertmhaas@gmail.com> wrote:

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

It seems dangerous to me to provide an externally visible function that
assumes the caller has already self-verified that they have permission to
modify stats for an object.

It's possible that the existing analyze() code already has verified these
permissions, but at this moment I'm unsure.


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

I mean that I don't know if that check is required in situations where the
caller already has an open relation with the right lock, and if we can
avoid the call by having the API take a Relation rather than an Oid, then
that would be preferable.


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


That's basically what I was trying, though I wasn't brave enough to use
bitflags in a v1 patch.

The #defines already exist now in the form of enums that act as indexes
into the FunctionCallInfo array. But if my data structure is an array of
Datums with an array of is-not-nulls, then I'm just not that far away from
the FunctionCallInfo structure we already have.


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

Requiring Datums simplifies things greatly inside the existing functions,
but pushes that complexity to the caller. My first proposal was to keep
complexity to an absolute minimum on the caller side, but your comments
make me think there's considerable tolerance for more complexity on the
caller side.


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

The issue becomes moot if we require the caller to construct their own
isnull and values arrays as was suggested above.

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-29T08:14:11Z

>
>
> Requiring Datums simplifies things greatly inside the existing functions,
> but pushes that complexity to the caller. My first proposal was to keep
> complexity to an absolute minimum on the caller side, but your comments
> make me think there's considerable tolerance for more complexity on the
> caller side.
>

I've had some time to look this over, and it's pretty clear that we don't
actually need the whole positional FunctionCallInfo, we just need the
fcinfo->args of it. We also need the ability to pass statistical values
along with the values that comprise the key of the
relation/attribute/object to be modified.

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.

The solution I chose was to create common functions that take a relation
object and an array of just the statistical parameters. This requires the
pg_restore_* functions to resolve and lock the relation themselves, and
then carry forward just the subset of parameters that are statistics (right
types, but wrong number of arguments). Conversely, the internal API
functions need to map their array of cstring values to the correctly typed
Datums (wrong types, but right number of arguments in the right order).

Attached is v2. The patches are broken down into very small bites to aid
digestion and to confirm that tests pass after each comparatively minor
change.

0001-0003: standardize enum values to further indicate which enum the
values belong to. This will be valuable later when values from an array of
one length have to be mapped to values in an array of a different
length. One patch each for relation stats, attribute stats, extended stats.

0004: Refactors the stats_check_* functions that previously took an fcinfo,
but now just take a NullableDatum array.

0005-0007: Change the function signature of
(relation|attribute|extended)_statistics_update() to no longer take
FunctionCallInfo as a parameter and instead take a NullableDatum[].

0008: Changes stats_fill_fcinfo_from_args() which maps a keyword args
fcinfo to a positional fcinfo to map directly to a NullableDatum[], and
changes all callers to use the new function and signature.

0009: rename relation_statistics_update -> update_relstats and
relation_stats_argnum -> relation_args_argnum to make room for
public-facing objects in the following patch.
0010: Introduce new public facing relation_statistics_update, which takes
isnull/string arrays that will be filled out by the caller, currently
postgres_fdw, and calls update_relstats same as pg_restore_relation_stats().

0011: same as 0009, but for attribute stats
0012: same as 0010, but for attribute stats

0013: remove SPI calls from postgres_fdw, and use the newly public
relation_statistics_update() and attribute_statistics_update() functions
instead.

Re: use of SPI by postgresImportForeignStatistics

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-29T11:50:45Z

On Mon, Jun 29, 2026 at 5:14 PM Corey Huinker <corey.huinker@gmail.com> wrote:

>> Requiring Datums simplifies things greatly inside the existing functions, but pushes that complexity to the caller. My first proposal was to keep complexity to an absolute minimum on the caller side, but your comments make me think there's considerable tolerance for more complexity on the caller side.
>
>
> I've had some time to look this over, and it's pretty clear that we don't actually need the whole positional FunctionCallInfo, we just need the fcinfo->args of it. We also need the ability to pass statistical values along with the values that comprise the key of the relation/attribute/object to be modified.
>
> 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.
>
> The solution I chose was to create common functions that take a relation object and an array of just the statistical parameters. This requires the pg_restore_* functions to resolve and lock the relation themselves, and then carry forward just the subset of parameters that are statistics (right types, but wrong number of arguments). Conversely, the internal API functions need to map their array of cstring values to the correctly typed Datums (wrong types, but right number of arguments in the right order).
>
> Attached is v2. The patches are broken down into very small bites to aid digestion and to confirm that tests pass after each comparatively minor change.

Thanks for doing that work!

I like this refactoring, but while it's rather mechanical, it's pretty
large, so I think it's too late to do the refactoring at this time
just before the beta 2 release.  So I'd vote for going with your
v1-0001 and v1-0002 and doing the refactoring in v20.  As mentioned by
Robert, I don't think it's good to call LOCAL_FCINFO() in
import_relation_statistics() and import_attribute_statistics() to call
the guts of those functions either, but that is *consistent* with the
existing way pg_restore_relation_stats() and
pg_restore_attribute_stats() do that, so that is actually not that
bad.  Also, as you mentioned above, it's inefficient for the new API
functions to lock an already-locked relation, and validate an
already-validated attname/attnum, but I think it would be negligible.

Best regards,
Etsuro Fujita

Re: use of SPI by postgresImportForeignStatistics

Robert Haas <robertmhaas@gmail.com> — 2026-06-29T18:17:02Z

On Mon, Jun 29, 2026 at 7:50 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> I like this refactoring, but while it's rather mechanical, it's pretty
> large, so I think it's too late to do the refactoring at this time
> just before the beta 2 release.  So I'd vote for going with your
> v1-0001 and v1-0002 and doing the refactoring in v20.  As mentioned by
> Robert, I don't think it's good to call LOCAL_FCINFO() in
> import_relation_statistics() and import_attribute_statistics() to call
> the guts of those functions either, but that is *consistent* with the
> existing way pg_restore_relation_stats() and
> pg_restore_attribute_stats() do that, so that is actually not that
> bad.  Also, as you mentioned above, it's inefficient for the new API
> functions to lock an already-locked relation, and validate an
> already-validated attname/attnum, but I think it would be negligible.

Fujita-san, is it then your plan to get those two patches committed?

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

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-30T01:10:41Z

On Mon, Jun 29, 2026 at 2:17 PM Robert Haas <robertmhaas@gmail.com> wrote:

> On Mon, Jun 29, 2026 at 7:50 AM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> > I like this refactoring, but while it's rather mechanical, it's pretty
> > large, so I think it's too late to do the refactoring at this time
> > just before the beta 2 release.  So I'd vote for going with your
> > v1-0001 and v1-0002 and doing the refactoring in v20.  As mentioned by
> > Robert, I don't think it's good to call LOCAL_FCINFO() in
> > import_relation_statistics() and import_attribute_statistics() to call
> > the guts of those functions either, but that is *consistent* with the
> > existing way pg_restore_relation_stats() and
> > pg_restore_attribute_stats() do that, so that is actually not that
> > bad.  Also, as you mentioned above, it's inefficient for the new API
> > functions to lock an already-locked relation, and validate an
> > already-validated attname/attnum, but I think it would be negligible.

> Fujita-san, is it then your plan to get those two patches committed?

Are we ok with changing the functions that postgres_fdw makes from v19 to
v20? Because what I put in the v2 patch does not match the v1 patch. If we
are NOT ok with that, then we'd need to:

1. rename the two relation_stats_argnum -> relation_args_argnum and
attribute_stats_argnum -> attribute_args_argnum out of the way of the ones
created in relation_stats.h and attribute_stats.h. The actual enumeration
values can stay the same, as there are no conflicts there, just ambiguity
about which set of values they belong to.
2. rename the existing static functions relation_statistics_update ->
update_relstats and attribute_statistics_update -> update_relstats to make
way for the public functions of the same names.
3. Create new relation_statistics_update and attribute_statistics_update,
with the isnull/values, and have those fcinfo-invoke the respective
pg_restore functions just like v1 does.

That would at least make the user API consistent in v19 vs v20.

I'm on vacation this week, but time is short for beta2, so I'll make an
exception to get the above into beta2. Let me know if you want me to write
that up.

Re: use of SPI by postgresImportForeignStatistics

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-30T12:29:58Z

On Tue, Jun 30, 2026 at 10:10 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> On Mon, Jun 29, 2026 at 2:17 PM Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Jun 29, 2026 at 7:50 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> > I like this refactoring, but while it's rather mechanical, it's pretty
>> > large, so I think it's too late to do the refactoring at this time
>> > just before the beta 2 release.  So I'd vote for going with your
>> > v1-0001 and v1-0002 and doing the refactoring in v20.  As mentioned by
>> > Robert, I don't think it's good to call LOCAL_FCINFO() in
>> > import_relation_statistics() and import_attribute_statistics() to call
>> > the guts of those functions either, but that is *consistent* with the
>> > existing way pg_restore_relation_stats() and
>> > pg_restore_attribute_stats() do that, so that is actually not that
>> > bad.  Also, as you mentioned above, it's inefficient for the new API
>> > functions to lock an already-locked relation, and validate an
>> > already-validated attname/attnum, but I think it would be negligible.
>>
>> Fujita-san, is it then your plan to get those two patches committed?

Yes, it is.

> Are we ok with changing the functions that postgres_fdw makes from v19 to v20? Because what I put in the v2 patch does not match the v1 patch. If we are NOT ok with that, then we'd need to:
>
> 1. rename the two relation_stats_argnum -> relation_args_argnum and attribute_stats_argnum -> attribute_args_argnum out of the way of the ones created in relation_stats.h and attribute_stats.h. The actual enumeration values can stay the same, as there are no conflicts there, just ambiguity about which set of values they belong to.
> 2. rename the existing static functions relation_statistics_update -> update_relstats and attribute_statistics_update -> update_relstats to make way for the public functions of the same names.
> 3. Create new relation_statistics_update and attribute_statistics_update, with the isnull/values, and have those fcinfo-invoke the respective pg_restore functions just like v1 does.
>
> That would at least make the user API consistent in v19 vs v20.
>
> I'm on vacation this week, but time is short for beta2, so I'll make an exception to get the above into beta2. Let me know if you want me to write that up.

Thanks!

Here is an updated patch, which merges your v1-0001 and v1-0002.  One
notable change I made to your version is the signatures of
import_relation_statistics() and import_attribute_statistics().  As
mentioned upthread, the proposed signatures of these functions would
be too tailored for postgres_fdw, and might not be useful for other
FDWs, so I changed them to have NullableDatum arguments for stats-data
inputs like this:

bool
import_relation_statistics(Relation rel,
                           NullableDatum *relpages,
                           NullableDatum *reltuples,
                           NullableDatum *relallvisible,
                           NullableDatum *relallfrozen)

bool
import_attribute_statistics(Relation rel, AttrNumber attnum, bool inherited,
                            NullableDatum *null_frac,
                            NullableDatum *avg_width,
                            NullableDatum *n_distinct,
                            NullableDatum *most_common_vals,
                            NullableDatum *most_common_freqs,
                            NullableDatum *histogram_bounds,
                            NullableDatum *correlation,
                            NullableDatum *most_common_elems,
                            NullableDatum *most_common_elem_freqs,
                            NullableDatum *elem_count_histogram,
                            NullableDatum *range_length_histogram,
                            NullableDatum *range_empty_frac,
                            NullableDatum *range_bounds_histogram)

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.  I think we could instead
use the values/isnull arrays as proposed in your v2, but the problem
about doing so is error messages in functions in stat_utils.c like
this:

        ereport(WARNING,
                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                 errmsg("argument \"%s\" must not be a multidimensional array",
                        arginfo[argnum].argname)));

Note that the message uses the word "argument", so I think it's better
for the new functions as well to have individual arguments for
stats-data inputs.

Also, I removed the attname argument from
import_attribute_statistics(), to 1) make the signature (and the
internal processing) simple and 2) match it to
delete_attribute_statistics().

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.

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.

Best regards,
Etsuro Fujita

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-30T15:55:03Z

On Tue, Jun 30, 2026 at 7:30 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

> On Tue, Jun 30, 2026 at 10:10 AM Corey Huinker <corey.huinker@gmail.com>
> wrote:
> > On Mon, Jun 29, 2026 at 2:17 PM Robert Haas <robertmhaas@gmail.com>
> wrote:
> >> On Mon, Jun 29, 2026 at 7:50 AM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> >> > I like this refactoring, but while it's rather mechanical, it's pretty
> >> > large, so I think it's too late to do the refactoring at this time
> >> > just before the beta 2 release.  So I'd vote for going with your
> >> > v1-0001 and v1-0002 and doing the refactoring in v20.  As mentioned by
> >> > Robert, I don't think it's good to call LOCAL_FCINFO() in
> >> > import_relation_statistics() and import_attribute_statistics() to call
> >> > the guts of those functions either, but that is *consistent* with the
> >> > existing way pg_restore_relation_stats() and
> >> > pg_restore_attribute_stats() do that, so that is actually not that
> >> > bad.  Also, as you mentioned above, it's inefficient for the new API
> >> > functions to lock an already-locked relation, and validate an
> >> > already-validated attname/attnum, but I think it would be negligible.
> >>
> >> Fujita-san, is it then your plan to get those two patches committed?
>
> Yes, it is.
>
> > Are we ok with changing the functions that postgres_fdw makes from v19
> to v20? Because what I put in the v2 patch does not match the v1 patch. If
> we are NOT ok with that, then we'd need to:
> >
> > 1. rename the two relation_stats_argnum -> relation_args_argnum and
> attribute_stats_argnum -> attribute_args_argnum out of the way of the ones
> created in relation_stats.h and attribute_stats.h. The actual enumeration
> values can stay the same, as there are no conflicts there, just ambiguity
> about which set of values they belong to.
> > 2. rename the existing static functions relation_statistics_update ->
> update_relstats and attribute_statistics_update -> update_relstats to make
> way for the public functions of the same names.
> > 3. Create new relation_statistics_update and
> attribute_statistics_update, with the isnull/values, and have those
> fcinfo-invoke the respective pg_restore functions just like v1 does.
> >
> > That would at least make the user API consistent in v19 vs v20.
> >
> > I'm on vacation this week, but time is short for beta2, so I'll make an
> exception to get the above into beta2. Let me know if you want me to write
> that up.
>
> Thanks!
>
> Here is an updated patch, which merges your v1-0001 and v1-0002.  One
> notable change I made to your version is the signatures of
> import_relation_statistics() and import_attribute_statistics().  As
> mentioned upthread, the proposed signatures of these functions would
> be too tailored for postgres_fdw, and might not be useful for other
> FDWs, so I changed them to have NullableDatum arguments for stats-data
> inputs like this:
>
> bool
> import_relation_statistics(Relation rel,
>                            NullableDatum *relpages,
>                            NullableDatum *reltuples,
>                            NullableDatum *relallvisible,
>                            NullableDatum *relallfrozen)
>
> bool
> import_attribute_statistics(Relation rel, AttrNumber attnum, bool
> inherited,
>                             NullableDatum *null_frac,
>                             NullableDatum *avg_width,
>                             NullableDatum *n_distinct,
>                             NullableDatum *most_common_vals,
>                             NullableDatum *most_common_freqs,
>                             NullableDatum *histogram_bounds,
>                             NullableDatum *correlation,
>                             NullableDatum *most_common_elems,
>                             NullableDatum *most_common_elem_freqs,
>                             NullableDatum *elem_count_histogram,
>                             NullableDatum *range_length_histogram,
>                             NullableDatum *range_empty_frac,
>                             NullableDatum *range_bounds_histogram)
>
> 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.




>   I think we could instead
> use the values/isnull arrays as proposed in your v2, but the problem
> about doing so is error messages in functions in stat_utils.c like
> this:
>
>         ereport(WARNING,
>                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>                  errmsg("argument \"%s\" must not be a multidimensional
> array",
>                         arginfo[argnum].argname)));
>
> Note that the message uses the word "argument", so I think it's better
> for the new functions as well to have individual arguments for
> stats-data inputs.


That's a good example of how our error messages get a bit clumsy with so
many ways of calling things, so I'm fine with your solution. It also
eliminates the possibility that a caller fails to notice a newly added stat
parameter and instead calls the input function with the older short array
length.

My initial concern was that this would bloat up postgres_fdw with type
conversion code, but the patch shows that it isn't that bad.

One thing we do lose in this is the assurance that the Datums passed are of
the correct type. Presently that is checked via
stats_fill_fcinfo_from_arg_pairs, which allows the *_statistics_update()
functions to trust the type correctness of the Datums that they're passed,
but I can be convinced that it isn't a big problem.


> Also, I removed the attname argument from
> import_attribute_statistics(), to 1) make the signature (and the
> internal processing) simple and 2) match it to
> delete_attribute_statistics().
>

I think that's reasonable. Internal callers have already resolved which
attribute they want to modify. The attname parameter was there so that
pg_restore_* calls could survive dump/restores where there were dropped
columns so the attnum has changed.

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.

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.

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.

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-06-30T16:21:08Z

>
>
> My initial concern was that this would bloat up postgres_fdw with type
> conversion code, but the patch shows that it isn't that bad.
>

One additional thought - in an offline conversation I had with Robert Haas,
he had taken the position that any FDW was likely pulling values across a
wire protocol and then synthesizing postgres-looking statistics from that,
so going with cstring inputs was fine, possibly preferable. I'm
highlighting that we're now passing in NullableDatums in case he wanted to
object to that change.

Re: use of SPI by postgresImportForeignStatistics

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-07-01T12:09:58Z

On Wed, Jul 1, 2026 at 12:55 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> On Tue, Jun 30, 2026 at 7:30 AM Etsuro Fujita <etsuro.fujita@gmail.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

Re: use of SPI by postgresImportForeignStatistics

Corey Huinker <corey.huinker@gmail.com> — 2026-07-01T20:34:53Z

On Wed, Jul 1, 2026 at 8:10 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

> On Wed, Jul 1, 2026 at 12:55 AM Corey Huinker <corey.huinker@gmail.com>
> wrote:
> > On Tue, Jun 30, 2026 at 7:30 AM Etsuro Fujita <etsuro.fujita@gmail.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.
>

I'm concerned that by making these non-error-safe function calls available,
we create a barrier to making those same functions error-safe in the future.

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

I agree we could do it later if they were private functions, no problem.
But if they're available outside of postgres_fdw then changing them to be
error-safe potentially disrupts other callers. Someone please let me know
if I'm being unnecessarily cautious here.

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
>

Other Thoughts:

1. The internal functions should accept a server_version_number parameter,
and we should document that setting that parameter to 0 mean that we can
assume the current version. I know that we presently have no translation
issues going forward with statistics, but someday that won't be the case,
and if we don't have this parameter in place then it'll be too late to fix
it.

2. Did you put import_relation_statistics and import_attribute_statistics
in statistics.h because you see them as long-term publicly visible
functions, and what's in stats_utils.h to be more subject to change? If so,
I could get behind that. If not, then I fall back to my position that they
seem like they belong in new relation_stats.h and attribute_stats.h, or
stats_utils.h.

3. The import_stats_functions in their current "bridging" form should do
null checks on all of the NullableDatum pointers, or at least Assert()s on
them.

Re: use of SPI by postgresImportForeignStatistics

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-07-03T09:47:54Z

On Thu, Jul 2, 2026 at 5:35 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> On Wed, Jul 1, 2026 at 8:10 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> On Wed, Jul 1, 2026 at 12:55 AM Corey Huinker <corey.huinker@gmail.com> wrote:

>> >> 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.
>
> I'm concerned that by making these non-error-safe function calls available, we create a barrier to making those same functions error-safe in the future.
>
>> > 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.
>
> I agree we could do it later if they were private functions, no problem. But if they're available outside of postgres_fdw then changing them to be error-safe potentially disrupts other callers. Someone please let me know if I'm being unnecessarily cautious here.

Ah, I misunderstood your comments.  I agree with you here, so I'll
move back the functions to postgres_fdw.c.

> Other Thoughts:
>
> 1. The internal functions should accept a server_version_number parameter, and we should document that setting that parameter to 0 mean that we can assume the current version. I know that we presently have no translation issues going forward with statistics, but someday that won't be the case, and if we don't have this parameter in place then it'll be too late to fix it.

Good idea!  Will do.

> 2. Did you put import_relation_statistics and import_attribute_statistics in statistics.h because you see them as long-term publicly visible functions, and what's in stats_utils.h to be more subject to change? If so, I could get behind that. If not, then I fall back to my position that they seem like they belong in new relation_stats.h and attribute_stats.h, or stats_utils.h.

The answer is the former.  I'll remove the changes made to
stats_utils.h, though.

> 3. The import_stats_functions in their current "bridging" form should do null checks on all of the NullableDatum pointers, or at least Assert()s on them.

Will do.

Thanks for the comments!

Best regards,
Etsuro Fujita

Re: use of SPI by postgresImportForeignStatistics

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-07-08T12:04:51Z

On Fri, Jul 3, 2026 at 6:47 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Thu, Jul 2, 2026 at 5:35 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> > On Wed, Jul 1, 2026 at 8:10 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> >> On Wed, Jul 1, 2026 at 12:55 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>
> >> >> 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.
> >
> > I'm concerned that by making these non-error-safe function calls available, we create a barrier to making those same functions error-safe in the future.
> >
> >> > 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.
> >
> > I agree we could do it later if they were private functions, no problem. But if they're available outside of postgres_fdw then changing them to be error-safe potentially disrupts other callers. Someone please let me know if I'm being unnecessarily cautious here.
>
> Ah, I misunderstood your comments.  I agree with you here, so I'll
> move back the functions to postgres_fdw.c.

Done.

> > Other Thoughts:
> >
> > 1. The internal functions should accept a server_version_number parameter, and we should document that setting that parameter to 0 mean that we can assume the current version. I know that we presently have no translation issues going forward with statistics, but someday that won't be the case, and if we don't have this parameter in place then it'll be too late to fix it.
>
> Good idea!  Will do.

I used the name "version" for the consistency with
pg_restore_relation_stats/pg_restore_attribute_stats.  Also, as I
couldn't find a note about the arrangement in the source code, I added
this comment to match a comment in stats_fill_fcinfo_from_arg_pairs:

 * For now, the 'version' argument is ignored. In the future it can be used
 * to interpret older statistics properly.

> > 3. The import_stats_functions in their current "bridging" form should do null checks on all of the NullableDatum pointers, or at least Assert()s on them.
>
> Will do.

Done.  As the functions are provided for developers, I just added the
assertions.

Also, I updated comments/docs a little bit.  Attached is a new version
of the patch.  I'm planning to push and back-patch it, if no
objections.

Best regards,
Etsuro Fujita