Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

postgres_fdw: Replace buffers in RemoteAttributeMapping with pointers.
- aa1f93a3387a 19 (unreleased) landed
Add support for importing statistics from remote servers.
- 28972b6fc3dc 19 (unreleased) landed

Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-08-12T17:02:39Z

Attached is my current work on adding remote fetching of statistics to
postgres_fdw, and opening the possibility of doing so to other foreign data
wrappers.

This involves adding two new options to postgres_fdw at the server and
table level.

The first option, fetch_stats, defaults to true at both levels. If enabled,
it will cause an ANALYZE of a postgres_fdw foreign table to first attempt
to fetch relation and attribute statistics from the remote table. If this
succeeds, then those statistics are imported into the local foreign table,
allowing us to skip a potentially expensive sampling operation.

The second option, remote_analyze, defaults to false at both levels, and
only comes into play if the first fetch succeeds but no attribute
statistics (i.e. the stats from pg_stats) are found. If enabled then the
function will attempt to ANALYZE the remote table, and if that is
successful then a second attempt at fetching remote statistics will be made.

If no statistics were fetched, then the operation will fall back to the
normal sampling operation per settings.

Note patches 0001 and 0002 are already a part of a separate thread
https://www.postgresql.org/message-id/flat/CADkLM%3DcpUiJ3QF7aUthTvaVMmgQcm7QqZBRMDLhBRTR%2BgJX-Og%40mail.gmail.com
regarding a bug (0001) and a nitpick (0002) that came about as a
side-effect to this effort, and but I expect those to be resolved one way
or another soon. Any feedback on those two can be handled there.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-08-14T07:37:58Z

On Tue, Aug 12, 2025 at 10:33 PM Corey Huinker <corey.huinker@gmail.com> wrote:
>
>
> Attached is my current work on adding remote fetching of statistics to postgres_fdw, and opening the possibility of doing so to other foreign data wrappers.
>
> This involves adding two new options to postgres_fdw at the server and table level.
>
> The first option, fetch_stats, defaults to true at both levels. If enabled, it will cause an ANALYZE of a postgres_fdw foreign table to first attempt to fetch relation and attribute statistics from the remote table. If this succeeds, then those statistics are imported into the local foreign table, allowing us to skip a potentially expensive sampling operation.
>
> The second option, remote_analyze, defaults to false at both levels, and only comes into play if the first fetch succeeds but no attribute statistics (i.e. the stats from pg_stats) are found. If enabled then the function will attempt to ANALYZE the remote table, and if that is successful then a second attempt at fetching remote statistics will be made.
>
> If no statistics were fetched, then the operation will fall back to the normal sampling operation per settings.
>
> Note patches 0001 and 0002 are already a part of a separate thread https://www.postgresql.org/message-id/flat/CADkLM%3DcpUiJ3QF7aUthTvaVMmgQcm7QqZBRMDLhBRTR%2BgJX-Og%40mail.gmail.com regarding a bug (0001) and a nitpick (0002) that came about as a side-effect to this effort, and but I expect those to be resolved one way or another soon. Any feedback on those two can be handled there.

I think this is very useful to avoid fetching rows from foreign server
and analyzing them locally.

This isn't a full review. I looked at the patches mainly to find out
how does it fit into the current method of analysing a foreign table.
Right now, do_analyze_rel() is called with FDW specific acquireFunc,
which collects a sample of rows. The sample is passed to attribute
specific compute_stats which fills VacAttrStats for that attribute.
VacAttrStats for all the attributes is passed to update_attstats(),
which updates pg_statistics. The patch changes that to fetch the
statistics from the foreign server and call pg_restore_attribute_stats
for each attribute. Instead I was expecting that after fetching the
stats from the foreign server, it would construct VacAttrStats and
call update_attstats(). That might be marginally faster since it
avoids SPI call and updates stats for all the attributes. Did you
consider this alternate approach and why it was discarded?

If a foreign table points to an inheritance parent on the foreign
server, we will receive two rows for that table - one with inherited =
false and other with true in that order. I think the stats with
inherited=true are relevant to the local server since querying the
parent will fetch rows from children. Since that stats is applied
last, the pg_statistics will retain the intended statistics. But why
to fetch two rows in the first place and waste computing cycles?

-- 
Best Wishes,
Ashutosh Bapat

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-08-14T18:34:43Z

>
>
> This isn't a full review. I looked at the patches mainly to find out
> how does it fit into the current method of analysing a foreign table.
>

Any degree of review is welcome. We're chasing views, reviews, etc.


> Right now, do_analyze_rel() is called with FDW specific acquireFunc,
> which collects a sample of rows. The sample is passed to attribute
> specific compute_stats which fills VacAttrStats for that attribute.
> VacAttrStats for all the attributes is passed to update_attstats(),
> which updates pg_statistics. The patch changes that to fetch the
> statistics from the foreign server and call pg_restore_attribute_stats
> for each attribute.


That recap is accurate.


> Instead I was expecting that after fetching the
> stats from the foreign server, it would construct VacAttrStats and
> call update_attstats(). That might be marginally faster since it
> avoids SPI call and updates stats for all the attributes. Did you
> consider this alternate approach and why it was discarded?
>

It might be marginally faster, but it would duplicate a lot of the
pair-checking (must have a most-common-freqs with a most-common-vals, etc)
and type-checking logic (the vals in a most-common vals must all input
coerce to the correct datatype for the _destination_ column, etc), and
we've already got that in pg_restore_attribute_stats. There used to be a
non-fcinfo function that took a long list of Datums and isnull boolean
pairs, but that wasn't pretty to look at and was replaced with the
positional fcinfo version we have today. This use case might be a reason to
bring that back, or expose the existing positional fcinfo function
(presently static) if we want to avoid SPI badly enough. As it is, the SPI
code is fairly future proof in that it isn't required to add new stat types
as they are created. My first attempt at this patch attempted to make a
FunctionCallInvoke() on the variadic pg_restore_attribute_stats, but that
would require a filled out fn_expr, and to get that we'd have to duplicate
a lot of logic from the parser, so the infrastructure isn't available to
easily call a variadic function.


>
> If a foreign table points to an inheritance parent on the foreign
> server, we will receive two rows for that table - one with inherited =
> false and other with true in that order. I think the stats with
> inherited=true are relevant to the local server since querying the
> parent will fetch rows from children. Since that stats is applied
> last, the pg_statistics will retain the intended statistics. But why
> to fetch two rows in the first place and waste computing cycles?
>

Glad you agree that we're fetching the right statistics.

That was the only way I could think of to do one client fetch and still get
exactly one row back.

Anything else involved fetching the inh=true first, and if that failed
fetching the inh=false statistics. That adds extra round-trips especially
given that inherited statistics are more rare than non-inherited
statistics. Moreoever, we're making decisions (analyze yes/no, fallback to
sampling yes/no) based on whether or not we got statistics back from the
foreign server at all, and having to consider the result of two fetches
instead of one makes that logic more complicated.

If, however, you were referring to the work we're handing the remote
server, I'm open to queries that you think would be more lightweight.
However, the pg_stats view is a security barrier view, so we reduce
overhead by passing through that barrier as few times as possible.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-09-12T05:17:01Z
```
Rebased.
```
Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-10-19T00:32:24Z
```
On Fri, Sep 12, 2025 at 1:17 AM Corey Huinker <corey.huinker@gmail.com>
wrote:

> Rebased.
>

Rebased again.
```

Re: Import Statistics in postgres_fdw before resorting to sampling.

Michael Paquier <michael@paquier.xyz> — 2025-10-20T06:45:14Z

On Sat, Oct 18, 2025 at 08:32:24PM -0400, Corey Huinker wrote:
> Rebased again.

Hearing an opinion from Fujita-san would be interesting here, so I am
adding him in CC.  I have been looking a little bit at this patch.

+	ImportStatistics_function ImportStatistics;

All FDW callbacks are documented in fdwhandler.sgml.  This new one is
not.

I am a bit uncomfortable regarding the design you are using here,
where the ImportStatistics callback, if defined, takes priority over
the existing AnalyzeForeignTable, especially regarding the fact that
both callbacks attempt to retrieve the same data, except that the
existing callback has a different idea of the timing to use to
retrieve reltuples and relpages.  The original callback
AnalyzeForeignTable retrieves immediately the total number of pages
via SQL, to feed ANALYZE.  reltuples is then fed to ANALYZE from a
callback function that that's defined in AnalyzeForeignTable().

My point is: rather than trying to implement a second solution with a
new callback, shouldn't we try to rework the existing callback so as
it would fit better with what you are trying to do here: feed data
that ANALYZE would then be in charge of inserting?  Relying on
pg_restore_relation_stats() for the job feels incorrect to me knowing 
that ANALYZE is the code path in charge of updating the stats of a
relation.  The new callback is a shortcut that bypasses what ANALYZE
does, so the problem, at least it seems to me, is that we want to
retrieve all the data in a single step, like your new callback, not in
two steps, something that only the existing callback allows.  Hence,
wouldn't it be more natural to retrieve the total number of pages and
reltuples from one callback, meaning that for local relations we
should delay RelationGetNumberOfBlocks() inside the existing
"acquire_sample_rows" callback (renaming it would make sense)?  This
requires some redesign of AnalyzeForeignTable and the internals of
analyze.c, but it would let FDW extensions know about the potential
efficiency gain.

There is also a performance concern to be made with the patch, but as
it's an ANALYZE path that may be acceptable: if we fail the first
callback, then we may call the second callback.

Fujita-san, what do you think?
--
Michael

Re: Import Statistics in postgres_fdw before resorting to sampling.

Michael Paquier <michael@paquier.xyz> — 2025-10-20T08:07:40Z

On Mon, Oct 20, 2025 at 03:45:14PM +0900, Michael Paquier wrote:
> On Sat, Oct 18, 2025 at 08:32:24PM -0400, Corey Huinker wrote:
> > Rebased again.
> 
> Hearing an opinion from Fujita-san would be interesting here, so I am
> adding him in CC.  I have been looking a little bit at this patch.

By the way, as far as I can see this patch is still in the past commit
fest:
https://commitfest.postgresql.org/patch/5959/

You may want to move it if you are planning to continue working on
that.
--
Michael

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-11-03T05:26:42Z

>
> I am a bit uncomfortable regarding the design you are using here,
> where the ImportStatistics callback, if defined, takes priority over
> the existing AnalyzeForeignTable, especially regarding the fact that
> both callbacks attempt to retrieve the same data, except that the
> existing callback has a different idea of the timing to use to
> retrieve reltuples and relpages.  The original callback
> AnalyzeForeignTable retrieves immediately the total number of pages
> via SQL, to feed ANALYZE.  reltuples is then fed to ANALYZE from a
> callback function that that's defined in AnalyzeForeignTable().
>

They don't try to retrieve the same data. AnalyzeForeignTable tries to
retrieve a table sample which it feeds into the normal ANALYZE process. If
that sample is going to be any good, it has to be a lot of rows, that
that's a lot of network traffic.

ImportStatistics just grabs the results that ANALYZE computed for the
remote table, using a far better sample than we'd want to pull across the
wire.



> My point is: rather than trying to implement a second solution with a
> new callback, shouldn't we try to rework the existing callback so as
> it would fit better with what you are trying to do here: feed data
> that ANALYZE would then be in charge of inserting?


To do that, we would have to somehow generate fake data locally from the
pg_stats data that we did pull over the wire, just to have ANALYZE compute
it back down to the data we already had.


>   Relying on
> pg_restore_relation_stats() for the job feels incorrect to me knowing
> that ANALYZE is the code path in charge of updating the stats of a
> relation.


That sounds like an argument for expanding ANALYZE to have syntax that will
digest pre-computed rows, essentially taking over what
pg_restore_relation_stats and pg_restore_attribute_stats already do, and
that idea was dismissed fairly early on in development, though I wasn't
against it at the time.

As it is, those two functions were developed to match what ANALYZE does.
pg_restore_relation_stats even briefly had inplace updates because that's
what ANALYZE did.



> The new callback is a shortcut that bypasses what ANALYZE
> does, so the problem, at least it seems to me, is that we want to
> retrieve all the data in a single step, like your new callback, not in
> two steps, something that only the existing callback allows.  Hence,
> wouldn't it be more natural to retrieve the total number of pages and
> reltuples from one callback, meaning that for local relations we
> should delay RelationGetNumberOfBlocks() inside the existing
> "acquire_sample_rows" callback (renaming it would make sense)?  This
> requires some redesign of AnalyzeForeignTable and the internals of
> analyze.c, but it would let FDW extensions know about the potential
> efficiency gain.
>

I wanted to make minimal disruption to the existing callbacks, but that may
have been misguided.

One problem I do see, however, is that the queries for fetching relation
(pg_class) stats should never fail and always return one row, even if the
values returned are meaningless. It's the query against pg_stats that lets
us know if 1) the database on the other side is a real-enough postgres and
2) the remote table is itself analyzed. Only once we're happy that we have
good attribute stats should we bother with the relation stats. All of this
logic is specific to postgres, so it was confined to postgres_fdw. I don't
know that other FDWs would be that much different, but minimizing the
generic impact was a goal.

I'll look at this again, but I'm not sure I'm going to come up with much
different.


> There is also a performance concern to be made with the patch, but as
> it's an ANALYZE path that may be acceptable: if we fail the first
> callback, then we may call the second callback.
>

I think the big win is the network traffic savings.


>
> Fujita-san, what do you think?


Very interested to know as well.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2025-11-21T18:35:17Z

Hi Corey,

This is an important feature.  Thanks for working on it.

On Mon, Nov 3, 2025 at 2:26 PM Corey Huinker <corey.huinker@gmail.com> wrote:

>> My point is: rather than trying to implement a second solution with a
>> new callback, shouldn't we try to rework the existing callback so as
>> it would fit better with what you are trying to do here: feed data
>> that ANALYZE would then be in charge of inserting?

> To do that, we would have to somehow generate fake data locally from the pg_stats data that we did pull over the wire, just to have ANALYZE compute it back down to the data we already had.

>>   Relying on
>> pg_restore_relation_stats() for the job feels incorrect to me knowing
>> that ANALYZE is the code path in charge of updating the stats of a
>> relation.

> That sounds like an argument for expanding ANALYZE to have syntax that will digest pre-computed rows, essentially taking over what pg_restore_relation_stats and pg_restore_attribute_stats already do, and that idea was dismissed fairly early on in development, though I wasn't against it at the time.
>
> As it is, those two functions were developed to match what ANALYZE does. pg_restore_relation_stats even briefly had inplace updates because that's what ANALYZE did.

To me it seems like a good idea to rely on pg_restore_relation_stats
and pg_restore_attribute_stats, rather than doing some rework on
analyze.c; IMO I don't think it's a good idea to do such a thing for
something rather special like this.

>> The new callback is a shortcut that bypasses what ANALYZE
>> does, so the problem, at least it seems to me, is that we want to
>> retrieve all the data in a single step, like your new callback, not in
>> two steps, something that only the existing callback allows.  Hence,
>> wouldn't it be more natural to retrieve the total number of pages and
>> reltuples from one callback, meaning that for local relations we
>> should delay RelationGetNumberOfBlocks() inside the existing
>> "acquire_sample_rows" callback (renaming it would make sense)?  This
>> requires some redesign of AnalyzeForeignTable and the internals of
>> analyze.c, but it would let FDW extensions know about the potential
>> efficiency gain.

> I wanted to make minimal disruption to the existing callbacks, but that may have been misguided.

+1 for the minimal disruption.

Other initial comments:

The commit message says:

    This is managed via two new options, fetch_stats and remote_analyze,
    both are available at the server level and table level. If fetch_stats
    is true, then the ANALYZE command will first attempt to fetch statistics
    from the remote table and import those statistics locally.

    If remote_analyze is true, and if the first attempt to fetch remote
    statistics found no attribute statistics, then an attempt will be made
    to ANALYZE the remote table before a second and final attempt to fetch
    remote statistics.

    If no statistics are found, then ANALYZE will fall back to the normal
    behavior of sampling and local analysis.

I think the first step assumes that the remote stats are up-to-date;
if they aren't, it would cause a regression.  (If the remote relation
is a plain table, they are likely to be up-to-date, but for example,
if it is a foreign table, it's possible that they are stale.)  So how
about making it the user's responsibility to make them up-to-date?  If
doing so, we wouldn't need to do the second and third steps anymore,
making the patch simple.

On the other hand:

    This operation will only work on remote relations that can have stored
    statistics: tables, partitioned tables, and materialized views. If the
    remote relation is a view then remote fetching/analyzing is just wasted
    effort and the user is better of setting fetch_stats to false for that
    table.

I'm not sure the waste effort is acceptable; IMO, if the remote table
is a view, I think that the system should detect that in some way, and
then just do the normal ANALYZE processing.

That's it for now.

My apologies for the delayed response.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-11-21T21:31:23Z

>
> Other initial comments:
>
> The commit message says:
>
>     This is managed via two new options, fetch_stats and remote_analyze,
>     both are available at the server level and table level. If fetch_stats
>     is true, then the ANALYZE command will first attempt to fetch
> statistics
>     from the remote table and import those statistics locally.
>
>     If remote_analyze is true, and if the first attempt to fetch remote
>     statistics found no attribute statistics, then an attempt will be made
>     to ANALYZE the remote table before a second and final attempt to fetch
>     remote statistics.
>
>     If no statistics are found, then ANALYZE will fall back to the normal
>     behavior of sampling and local analysis.
>
> I think the first step assumes that the remote stats are up-to-date;
> if they aren't, it would cause a regression.  (If the remote relation
> is a plain table, they are likely to be up-to-date, but for example,
> if it is a foreign table, it's possible that they are stale.)  So how
> about making it the user's responsibility to make them up-to-date?  If
> doing so, we wouldn't need to do the second and third steps anymore,
> making the patch simple.
>

Obviously there is no way to know the quality/freshness of remote stats if
they are found.

The analyze option was borne of feedback from other postgres hackers while
brainstorming on what this option might look like. I don't think we *need*
this extra option for the feature to be a success, but it's relative
simplicity did make me want to put it out there to see who else liked it.


>
> On the other hand:
>
>     This operation will only work on remote relations that can have stored
>     statistics: tables, partitioned tables, and materialized views. If the
>     remote relation is a view then remote fetching/analyzing is just wasted
>     effort and the user is better of setting fetch_stats to false for that
>     table.
>
> I'm not sure the waste effort is acceptable; IMO, if the remote table
> is a view, I think that the system should detect that in some way, and
> then just do the normal ANALYZE processing.
>

The stats fetch query is pretty light, but I can see fetching the relkind
along with the relstats, and making decisions on whether to continue from
there, only applying the relstats after attrstats have been successfully
applied.



> That's it for now.
>

I'll see what I can do to make that work.


> My apologies for the delayed response.
>

Valuable responses are worth waiting for.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-11-23T07:27:49Z

>
>
>
>> My apologies for the delayed response.
>>
>
> Valuable responses are worth waiting for.
>

I've reorganized some things a bit, mostly to make resource cleanup
simpler.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Chao Li <li.evan.chao@gmail.com> — 2025-11-24T09:25:13Z


> On Nov 23, 2025, at 15:27, Corey Huinker <corey.huinker@gmail.com> wrote:
> 
>   My apologies for the delayed response.
> 
> Valuable responses are worth waiting for.
> 
> I've reorganized some things a bit, mostly to make resource cleanup simpler. 
> <v4-0001-Add-remote-statistics-fetching-to-postgres_fdw.patch>


Few comments:

1 - commit message
```
effort and the user is better of setting fetch_stats to false for that
```

I guess “of” should be “off”

2 - postgres-fdw.sgml
```
+       server, determines wheter an <command>ANALYZE</command> on a foreign
```

Typo: wheter => whether 

3 - postgres-fdw.sgml
```
+       data sampling if no statistics were availble. This option is only
```

Typo: availble => available

4 - option.c
```
+		/* fetch_size is available on both server and table */
+		{"fetch_stats", ForeignServerRelationId, false},
+		{"fetch_stats", ForeignTableRelationId, false},
```

In the comment, I guess fetch_size should be fetch_stats.

5 - analyze.c
```
+			 * XXX: Should this be it's own fetch type? If not, then there might be
```

Typo: “it’s own” => “its own”

6 - analyze.c
```
+				case FDW_IMPORT_STATS_NOTFOUND:
+					ereport(INFO,
+							errmsg("Found no remote statistics for \"%s\"",
+								   RelationGetRelationName(onerel)));
```

`Found no remote statistics for \"%s\””` could be rephrased as `""No remote statistics found for foreign table \"%s\””`, sounds better wording in server log.

Also, I wonder if this message at INFO level is too noisy?

7 - postgres_fdw.c
```
+		default:
+			ereport(INFO,
+					errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					errmsg("Remote table %s.%s does not support statistics.",
+						   quote_identifier(remote_schemaname),
+						   quote_identifier(remote_relname)),
+					errdetail("Remote relation if of relkind \"%c\"", relkind));
```

I think “if of” should be “is of”.

8 - postgres_fdw.c
```
+						errmsg("Attribute statistics found for %s.%s but no columns matched",
+							   quote_identifier(schemaname),
+							   quote_identifier(relname))));
```

Instead of using "%s.%s” and calling quote_identifier() twice, there is a simple function to use: quote_qualified_identifier(schemaname, relname).

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2025-11-27T12:46:09Z

On Sat, Nov 22, 2025 at 6:31 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>> Other initial comments:
>>
>> The commit message says:
>>
>>     This is managed via two new options, fetch_stats and remote_analyze,
>>     both are available at the server level and table level. If fetch_stats
>>     is true, then the ANALYZE command will first attempt to fetch statistics
>>     from the remote table and import those statistics locally.
>>
>>     If remote_analyze is true, and if the first attempt to fetch remote
>>     statistics found no attribute statistics, then an attempt will be made
>>     to ANALYZE the remote table before a second and final attempt to fetch
>>     remote statistics.
>>
>>     If no statistics are found, then ANALYZE will fall back to the normal
>>     behavior of sampling and local analysis.
>>
>> I think the first step assumes that the remote stats are up-to-date;
>> if they aren't, it would cause a regression.  (If the remote relation
>> is a plain table, they are likely to be up-to-date, but for example,
>> if it is a foreign table, it's possible that they are stale.)  So how
>> about making it the user's responsibility to make them up-to-date?  If
>> doing so, we wouldn't need to do the second and third steps anymore,
>> making the patch simple.

> Obviously there is no way to know the quality/freshness of remote stats if they are found.
>
> The analyze option was borne of feedback from other postgres hackers while brainstorming on what this option might look like. I don't think we *need* this extra option for the feature to be a success, but it's relative simplicity did make me want to put it out there to see who else liked it.

Actually, I have some concerns about the ANALYZE and fall-back
options.  As for the former, if the remote user didn't have the
MAINTAIN privilege on the remote table, remote ANALYZE would be just a
waste effort.  As for the latter, imagine the situation where a user
ANALYZEs a foreign table whose remote table is significantly large.
When the previous attempts fail, the user might want to re-try to
import remote stats after ANALYZEing the remote table in the remote
side in some way, rather than postgres_fdw automatically falling back
to the normal lengthy processing.  I think just throwing an error if
the first attempt fails would make the system not only simple but
reliable while providing some flexibility to users.

>> On the other hand:
>>
>>     This operation will only work on remote relations that can have stored
>>     statistics: tables, partitioned tables, and materialized views. If the
>>     remote relation is a view then remote fetching/analyzing is just wasted
>>     effort and the user is better of setting fetch_stats to false for that
>>     table.
>>
>> I'm not sure the waste effort is acceptable; IMO, if the remote table
>> is a view, I think that the system should detect that in some way, and
>> then just do the normal ANALYZE processing.
>
>
> The stats fetch query is pretty light, but I can see fetching the relkind along with the relstats, and making decisions on whether to continue from there, only applying the relstats after attrstats have been successfully applied.

Good idea!  I would vote for throwing an error if the relkind is view,
making the user set fetch_stats to false for the foreign table.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2025-11-27T12:48:34Z

On Sun, Nov 23, 2025 at 4:28 PM Corey Huinker <corey.huinker@gmail.com> wrote:
> I've reorganized some things a bit, mostly to make resource cleanup simpler.

Thanks for updating the patch!  I will look into it.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-12-11T21:59:19Z

On Thu, Nov 27, 2025 at 7:48 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

> On Sun, Nov 23, 2025 at 4:28 PM Corey Huinker <corey.huinker@gmail.com>
> wrote:
> > I've reorganized some things a bit, mostly to make resource cleanup
> simpler.
>
> Thanks for updating the patch!  I will look into it.
>
> Best regards,
> Etsuro Fujita
>

Rebase, no changes.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Chao Li <li.evan.chao@gmail.com> — 2025-12-12T06:11:31Z


> On Dec 12, 2025, at 05:59, Corey Huinker <corey.huinker@gmail.com> wrote:
> 
> On Thu, Nov 27, 2025 at 7:48 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Sun, Nov 23, 2025 at 4:28 PM Corey Huinker <corey.huinker@gmail.com> wrote:
> > I've reorganized some things a bit, mostly to make resource cleanup simpler.
> 
> Thanks for updating the patch!  I will look into it.
> 
> Best regards,
> Etsuro Fujita
> 
> Rebase, no changes. 
> <v5-0001-Add-remote-statistics-fetching-to-postgres_fdw.patch>

A kind reminder, I don’t see my comments are addressed:

https://postgr.es/m/F9C73EF2-F977-46E4-9F61-B6CF72BF1A69@gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2025-12-12T13:45:46Z

On Thu, Nov 27, 2025 at 9:46 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Sat, Nov 22, 2025 at 6:31 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> >> Other initial comments:
> >>
> >> The commit message says:
> >>
> >>     This is managed via two new options, fetch_stats and remote_analyze,
> >>     both are available at the server level and table level. If fetch_stats
> >>     is true, then the ANALYZE command will first attempt to fetch statistics
> >>     from the remote table and import those statistics locally.
> >>
> >>     If remote_analyze is true, and if the first attempt to fetch remote
> >>     statistics found no attribute statistics, then an attempt will be made
> >>     to ANALYZE the remote table before a second and final attempt to fetch
> >>     remote statistics.
> >>
> >>     If no statistics are found, then ANALYZE will fall back to the normal
> >>     behavior of sampling and local analysis.
> >>
> >> I think the first step assumes that the remote stats are up-to-date;
> >> if they aren't, it would cause a regression.  (If the remote relation
> >> is a plain table, they are likely to be up-to-date, but for example,
> >> if it is a foreign table, it's possible that they are stale.)  So how
> >> about making it the user's responsibility to make them up-to-date?  If
> >> doing so, we wouldn't need to do the second and third steps anymore,
> >> making the patch simple.
>
> > Obviously there is no way to know the quality/freshness of remote stats if they are found.
> >
> > The analyze option was borne of feedback from other postgres hackers while brainstorming on what this option might look like. I don't think we *need* this extra option for the feature to be a success, but it's relative simplicity did make me want to put it out there to see who else liked it.
>
> Actually, I have some concerns about the ANALYZE and fall-back
> options.  As for the former, if the remote user didn't have the
> MAINTAIN privilege on the remote table, remote ANALYZE would be just a
> waste effort.  As for the latter, imagine the situation where a user
> ANALYZEs a foreign table whose remote table is significantly large.
> When the previous attempts fail, the user might want to re-try to
> import remote stats after ANALYZEing the remote table in the remote
> side in some way, rather than postgres_fdw automatically falling back
> to the normal lengthy processing.  I think just throwing an error if
> the first attempt fails would make the system not only simple but
> reliable while providing some flexibility to users.

I think I was narrow-minded here; as for the ANALYZE option, if the
remote user has the privilege, it would work well, so +1 for it, but I
don't think it's a must-have option, so I'd vote for making it a
separate patch.  As for the fall-back behavior, however, sorry, I
still think it reduces flexibility.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2025-12-12T14:04:26Z

On Fri, Dec 12, 2025 at 6:59 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> Rebase, no changes.

Thanks for rebasing!  I reviewed the changes made to the core:

@@ -196,13 +196,56 @@ analyze_rel(Oid relid, RangeVar *relation,
    {
        /*
         * For a foreign table, call the FDW's hook function to see whether it
-        * supports analysis.
+        * supports statistics import and/or analysis.
         */
        FdwRoutine *fdwroutine;
        bool        ok = false;

        fdwroutine = GetFdwRoutineForRelation(onerel, false);

+       if (fdwroutine->ImportStatistics != NULL)
+       {
+           FdwImportStatsResult    res;
+
+           /*
+            * Fetching pre-existing remote stats is not guaranteed to
be a quick
+            * operation.
+            *
+            * XXX: Should this be it's own fetch type? If not, then
there might be
+            * confusion when a long stats-fetch fails, followed by a
regular analyze,
+            * which would make it look like the table was analyzed twice.
+            */
+           pgstat_progress_start_command(PROGRESS_COMMAND_ANALYZE,
+                                         RelationGetRelid(onerel));
+
+           res = fdwroutine->ImportStatistics(onerel);
+
+           pgstat_progress_end_command();
+
+           /*
+            * If we were able to import statistics, then there is no
need to collect
+            * samples for local analysis.
+            */
+           switch(res)
+           {
+               case FDW_IMPORT_STATS_OK:
+                   relation_close(onerel, ShareUpdateExclusiveLock);
+                   return;
+               case FDW_IMPORT_STATS_DISABLED:
+                   break;
+               case FDW_IMPORT_STATS_NOTFOUND:
+                   ereport(INFO,
+                           errmsg("Found no remote statistics for \"%s\"",
+                                  RelationGetRelationName(onerel)));
+                   break;
+               case FDW_IMPORT_STATS_FAILED:
+               default:
+                   ereport(INFO,
+                           errmsg("Fetching remote statistics from
\"%s\" failed",
+                                  RelationGetRelationName(onerel)));
+           }
+       }

Returning in the FDW_IMPORT_STATS_OK case isn't 100% correct; if the
foreign table is an inheritance parent, we would fail to do inherited
stats.

IIUC, the FDW needs to do two things within the ImportStatistics
callback function: check the importability, and if ok, do the work.  I
think that would make the API complicated.  To avoid that, how about
1) splitting the callback function into the two functions shown below
and then 2) rewriting the above to something like the attached?  The
attached addresses the returning issue mentioned above as well.

bool
StatisticsAreImportable(Relation relation)

Checks the importability, and if ok, returns true, in which case the
following callback function is called.  In the postgres_fdw case, we
could implement this to check if the fetch_stats flag for the foreign
table is set to true, and if so, return true.

void
ImportStatistics(Relation relation, List *va_cols, int elevel)

Imports the stats for the foreign table from the foreign server.  As
mentioned in previous emails, I don't think it's a good idea to fall
back to the normal processing when the attempt to import the stats
fails, in which case I think we should just throw an error (or
warning) so that the user can re-try to import the stats after fixing
the foreign side in some way.  So I re-defined this as a void
function.  Note that this re-definition removes the concern mentioned
in the comment starting with "XXX:".  In the postgres_fdw case, as
mentioned in a previous email, I think it would be good to implement
this so that it checks whether the remote table is a view or not when
importing the relation stats from the remote server, and if so, just
throws an error (or warning), making the user reset the fetch_stats
flag.

I added two arguments to the callback function: va_cols, for
supporting the column list in the ANALYZE command, and elevel, for
proper logging.  I'm not sure if VaccumParams should be added as well.

That's it for now.  I'll continue to review the patch.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-12-12T19:03:55Z
```
>
>
> A kind reminder, I don’t see my comments are addressed:
>
>
My apologies. Will get into the next rev.
```

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-12-12T19:45:42Z

CCing Jonathan Katz and Nathan Bossart as they had been sounding boards for
me when I started designing this feature.


> Returning in the FDW_IMPORT_STATS_OK case isn't 100% correct; if the
> foreign table is an inheritance parent, we would fail to do inherited
> stats.
>

Perhaps I'm not understanding completely, but I believe what we're doing
now should be ok.

The local table of type 'f' can be a member of a partition, but can't be a
partition itself, so whatever stats we get for it, we're storing them as
inh = false.

On the remote side, the table could be an inheritance parent, in which case
we ONLY want the inh=true stats, but we will still store them locally as
inh = false.

The DISTINCT ON(a.attname)....ORDER BY a.attname, s.inherited DESC part of
the query ensures that we get inh=true stats if they're there in preference
to the inh=false steps.

I will grant you that in an old-style inheritance (i.e. not formal
partitions) situation we'd probably want some weighted mix of the inherited
and non-inherited stats, but 1) very few people use old-style inheritance
anymore, 2) few of those export tables via a FDW, and 3) there's no way to
do that weighting so we should fall back to sampling anyway.

None of this takes away from your suggestions down below.


> IIUC, the FDW needs to do two things within the ImportStatistics
> callback function: check the importability, and if ok, do the work.  I
> think that would make the API complicated.  To avoid that, how about
> 1) splitting the callback function into the two functions shown below
> and then 2) rewriting the above to something like the attached?  The
> attached addresses the returning issue mentioned above as well.
>
> bool
> StatisticsAreImportable(Relation relation)
>
> Checks the importability, and if ok, returns true, in which case the
> following callback function is called.  In the postgres_fdw case, we
> could implement this to check if the fetch_stats flag for the foreign
> table is set to true, and if so, return true.
>

+1


> void
> ImportStatistics(Relation relation, List *va_cols, int elevel)
>
> Imports the stats for the foreign table from the foreign server.  As
> mentioned in previous emails, I don't think it's a good idea to fall
> back to the normal processing when the attempt to import the stats
> fails, in which case I think we should just throw an error (or
> warning) so that the user can re-try to import the stats after fixing
> the foreign side in some way.  So I re-defined this as a void
> function.  Note that this re-definition removes the concern mentioned
> in the comment starting with "XXX:".  In the postgres_fdw case, as
> mentioned in a previous email, I think it would be good to implement
> this so that it checks whether the remote table is a view or not when
> importing the relation stats from the remote server, and if so, just
> throws an error (or warning), making the user reset the fetch_stats
> flag.
>

I think I'm ok with this design as the decision, as it still leaves open
the fdw-specific options of how to handle initially finding no remote stats.

I can still see a situation where a local table expects the remote table to
eventually have proper statistics on it, but until that happens it will
fall back to table samples. This design decision means that either the user
lives without any statistics for a while, or alters the foreign table
options and hopefully remembers to set them back. While I understand the
desire to first implement something very simple, I think that adding the
durability that fallback allows for will be harder to implement if we don't
build it in from the start. I'm interested to hear with Nathan and/or
Jonathan have to say about that.


> I added two arguments to the callback function: va_cols, for
> supporting the column list in the ANALYZE command, and elevel, for
> proper logging.  I'm not sure if VaccumParams should be added as well.
>

Good catch, I forgot about that one.

Going to think some more on this before I work incorporating your

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-12-13T19:12:16Z

On Fri, Dec 12, 2025 at 2:45 PM Corey Huinker <corey.huinker@gmail.com>
wrote:

> CCing Jonathan Katz and Nathan Bossart as they had been sounding boards
> for me when I started designing this feature.
>
>
>> Returning in the FDW_IMPORT_STATS_OK case isn't 100% correct; if the
>> foreign table is an inheritance parent, we would fail to do inherited
>> stats.
>>
>
> Perhaps I'm not understanding completely, but I believe what we're doing
> now should be ok.
>
> The local table of type 'f' can be a member of a partition, but can't be a
> partition itself, so whatever stats we get for it, we're storing them as
> inh = false.
>
> On the remote side, the table could be an inheritance parent, in which
> case we ONLY want the inh=true stats, but we will still store them locally
> as inh = false.
>
> The DISTINCT ON(a.attname)....ORDER BY a.attname, s.inherited DESC part of
> the query ensures that we get inh=true stats if they're there in preference
> to the inh=false steps.
>
> I will grant you that in an old-style inheritance (i.e. not formal
> partitions) situation we'd probably want some weighted mix of the inherited
> and non-inherited stats, but 1) very few people use old-style inheritance
> anymore, 2) few of those export tables via a FDW, and 3) there's no way to
> do that weighting so we should fall back to sampling anyway.
>
> None of this takes away from your suggestions down below.
>
>
>> IIUC, the FDW needs to do two things within the ImportStatistics
>> callback function: check the importability, and if ok, do the work.  I
>> think that would make the API complicated.  To avoid that, how about
>> 1) splitting the callback function into the two functions shown below
>> and then 2) rewriting the above to something like the attached?  The
>> attached addresses the returning issue mentioned above as well.
>>
>> bool
>> StatisticsAreImportable(Relation relation)
>>
>> Checks the importability, and if ok, returns true, in which case the
>> following callback function is called.  In the postgres_fdw case, we
>> could implement this to check if the fetch_stats flag for the foreign
>> table is set to true, and if so, return true.
>>
>
> +1
>
>
>> void
>> ImportStatistics(Relation relation, List *va_cols, int elevel)
>>
>> Imports the stats for the foreign table from the foreign server.  As
>> mentioned in previous emails, I don't think it's a good idea to fall
>> back to the normal processing when the attempt to import the stats
>> fails, in which case I think we should just throw an error (or
>> warning) so that the user can re-try to import the stats after fixing
>> the foreign side in some way.  So I re-defined this as a void
>> function.  Note that this re-definition removes the concern mentioned
>> in the comment starting with "XXX:".  In the postgres_fdw case, as
>> mentioned in a previous email, I think it would be good to implement
>> this so that it checks whether the remote table is a view or not when
>> importing the relation stats from the remote server, and if so, just
>> throws an error (or warning), making the user reset the fetch_stats
>> flag.
>>
>
> I think I'm ok with this design as the decision, as it still leaves open
> the fdw-specific options of how to handle initially finding no remote stats.
>
> I can still see a situation where a local table expects the remote table
> to eventually have proper statistics on it, but until that happens it will
> fall back to table samples. This design decision means that either the user
> lives without any statistics for a while, or alters the foreign table
> options and hopefully remembers to set them back. While I understand the
> desire to first implement something very simple, I think that adding the
> durability that fallback allows for will be harder to implement if we don't
> build it in from the start. I'm interested to hear with Nathan and/or
> Jonathan have to say about that.
>
>
>> I added two arguments to the callback function: va_cols, for
>> supporting the column list in the ANALYZE command, and elevel, for
>> proper logging.  I'm not sure if VaccumParams should be added as well.
>>
>
> Good catch, I forgot about that one.
>
> Going to think some more on this before I work incorporating your
>

Heh, the word "changes" got cut off.

Anyway, here's v6 incorporating both threads of feedback.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2025-12-14T18:40:49Z

On Sun, Dec 14, 2025 at 4:12 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> On Fri, Dec 12, 2025 at 2:45 PM Corey Huinker <corey.huinker@gmail.com> wrote:
>> CCing Jonathan Katz and Nathan Bossart as they had been sounding boards for me when I started designing this feature.

>>> Returning in the FDW_IMPORT_STATS_OK case isn't 100% correct; if the
>>> foreign table is an inheritance parent, we would fail to do inherited
>>> stats.

>> Perhaps I'm not understanding completely, but I believe what we're doing now should be ok.
>>
>> The local table of type 'f' can be a member of a partition, but can't be a partition itself, so whatever stats we get for it, we're storing them as inh = false.
>>
>> On the remote side, the table could be an inheritance parent, in which case we ONLY want the inh=true stats, but we will still store them locally as inh = false.
>>
>> The DISTINCT ON(a.attname)....ORDER BY a.attname, s.inherited DESC part of the query ensures that we get inh=true stats if they're there in preference to the inh=false steps.
>>
>> I will grant you that in an old-style inheritance (i.e. not formal partitions) situation we'd probably want some weighted mix of the inherited and non-inherited stats, but 1) very few people use old-style inheritance anymore, 2) few of those export tables via a FDW, and 3) there's no way to do that weighting so we should fall back to sampling anyway.

Ah, I mean the case where the foreign table is an inheritance parent
on the *local* side.  In that case, the return would cause us to skip
the recursive ANALYZE (i.e., do_analyze_rel() with inh=true), leading
to no inherited stats.  I agree that the case is minor, but I don't
think that that's acceptable.

>>> void
>>> ImportStatistics(Relation relation, List *va_cols, int elevel)
>>>
>>> Imports the stats for the foreign table from the foreign server.  As
>>> mentioned in previous emails, I don't think it's a good idea to fall
>>> back to the normal processing when the attempt to import the stats
>>> fails, in which case I think we should just throw an error (or
>>> warning) so that the user can re-try to import the stats after fixing
>>> the foreign side in some way.  So I re-defined this as a void
>>> function.  Note that this re-definition removes the concern mentioned
>>> in the comment starting with "XXX:".  In the postgres_fdw case, as
>>> mentioned in a previous email, I think it would be good to implement
>>> this so that it checks whether the remote table is a view or not when
>>> importing the relation stats from the remote server, and if so, just
>>> throws an error (or warning), making the user reset the fetch_stats
>>> flag.

>> I think I'm ok with this design as the decision, as it still leaves open the fdw-specific options of how to handle initially finding no remote stats.
>>
>> I can still see a situation where a local table expects the remote table to eventually have proper statistics on it, but until that happens it will fall back to table samples. This design decision means that either the user lives without any statistics for a while, or alters the foreign table options and hopefully remembers to set them back. While I understand the desire to first implement something very simple, I think that adding the durability that fallback allows for will be harder to implement if we don't build it in from the start. I'm interested to hear with Nathan and/or Jonathan have to say about that.

My concern about the fall-back behavior is that it reduces flexibility
in some cases, as mentioned upthread.  Maybe that could be addressed
by making the behavior an option, but my another (bigger) concern is
that considering that it's the user's responsibility to make remote
stats up-to-date when he/she uses this feature, the "no remote stats
found" result would be his/her fault; it might not be worth
complicating the code for something like that.

Anyway, I too would like to hear the opinions of them (or anyone else).

> Anyway, here's v6 incorporating both threads of feedback.

Thanks for updating the patch!  I will review the postgres_fdw changes next.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2025-12-14T20:01:35Z

>
> Ah, I mean the case where the foreign table is an inheritance parent
> on the *local* side.  In that case, the return would cause us to skip
> the recursive ANALYZE (i.e., do_analyze_rel() with inh=true), leading
> to no inherited stats.  I agree that the case is minor, but I don't
> think that that's acceptable.
>

When such a corner case occurs (stats import configured to true, but table
is an inheritance parent), should we raise an error, or raise a warning and
return false on the CanImportStats() call? I guess the answer may depend on
the feedback we get.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2025-12-15T10:01:44Z

On Mon, Dec 15, 2025 at 5:01 AM Corey Huinker <corey.huinker@gmail.com> wrote:

>> Ah, I mean the case where the foreign table is an inheritance parent
>> on the *local* side.  In that case, the return would cause us to skip
>> the recursive ANALYZE (i.e., do_analyze_rel() with inh=true), leading
>> to no inherited stats.  I agree that the case is minor, but I don't
>> think that that's acceptable.

> When such a corner case occurs (stats import configured to true, but table is an inheritance parent), should we raise an error, or raise a warning and return false on the CanImportStats() call? I guess the answer may depend on the feedback we get.

As mentioned upthread, the FDW API that I proposed addresses this
issue; even in such a case it allows the FDW to import stats, instead
of doing the normal non-recursive ANALYZE, and then do the recursive
ANALYZE, for the inherited stats.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-01-03T10:06:12Z

On Mon, Dec 15, 2025 at 3:40 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Sun, Dec 14, 2025 at 4:12 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> > Anyway, here's v6 incorporating both threads of feedback.
>
> I will review the postgres_fdw changes next.

I spent some time reviewing the changes.

+     <term><literal>fetch_stats</literal> (<type>boolean</type>)</term>
+     <listitem>
+      <para>
+       This option, which can be specified for a foreign table or a foreign
+       server, determines if <command>ANALYZE</command> on a foreign table
+       will first attempt to fetch and import the existing relation and
+       attribute statistics from the remote table, and only attempt regular
+       data sampling if no statistics were available. This option is only
+       useful if the remote relation is one that can have regular statistics
+       (tables and materialized views).
+       The default is <literal>true</literal>.

This version of the patch wouldn't fall back to the normal ANALYZE
processing anymore, so this documentation should be updated as such.
Also, as I think it's the user's responsibility to ensure the existing
statistics are up-to-date, as I said before, I think we should add a
note about that here.  Also, as some users wouldn't be able to ensure
it, I'm wondering if the default should be false.

+     <term><literal>remote_analyze</literal> (<type>boolean</type>)</term>
+     <listitem>
+      <para>
+       This option, which can be specified for a foreign table or a foreign
+       server, determines whether an <command>ANALYZE</command> on a foreign
+       table will attempt to <command>ANALYZE</command> the remote table if
+       the first attempt to fetch remote statistics fails, and will then
+       make a second and final attempt to fetch remote statistics. This option
+       is only meaningful if the foreign table has
+       <literal>fetch_stats</literal> enabled at either the server or table
+       level.

If the user wasn't able to ensure the statistics are up-to-date, I
think he/she might want to do remote ANALYZE *before* fetching the
statistics, not after trying to do so.  So I think we could instead
provide this option as such.  What do you think about that?  Anyway,
I'd vote for leaving this for another patch, as I think it's a
nice-to-have option rather than a must-have one, as I said before.

ISTM that the code is well organized overall.  Here are a few comments:

+static const char *relstats_query_18 =
+   "SELECT c.relkind, c.relpages, c.reltuples, c.relallvisible,
c.relallfrozen "
+   "FROM pg_catalog.pg_class AS c "
+   "JOIN pg_catalog.pg_namespace AS n ON n.oid = c.relnamespace "
+   "WHERE n.nspname = $1 AND c.relname = $2";

We don't use relallvisible and relallfrozen for foreign tables (note
that do_analyze_rel() calls vac_update_relstats() with relallvisible=0
and relallfrozen=0 for them).  Do we really need to retrieve (and
restore) them?

+static const char *attstats_query_17 =
+   "SELECT DISTINCT ON (s.attname) attname, s.null_frac, s.avg_width, "
+   "s.n_distinct, s.most_common_vals, s.most_common_freqs, "
+   "s.histogram_bounds, s.correlation, s.most_common_elems, "
+   "s.most_common_elem_freqs, s.elem_count_histogram, "
+   "s.range_length_histogram, s.range_empty_frac, s.range_bounds_histogram "
+   "FROM pg_catalog.pg_stats AS s "
+   "WHERE s.schemaname = $1 AND s.tablename = $2 "
+   "ORDER BY s.attname, s.inherited DESC";

I think we should retrieve the attribute statistics for only the
referenced columns of the remote table, not all the columns of it, to
reduce the data transfer and the cost of matching local/remote
attributes in import_fetched_statistics().

In fetch_remote_statistics()

+   if (server_version_num >= 180000)
+       relation_sql = relstats_query_18;
+   else if (server_version_num >= 140000)
+       relation_sql = relstats_query_14;
+   else
+       relation_sql = relstats_query_default;

I think that having the definition for each of relstats_query_18,
relstats_query_14 and relstats_query_default makes the code redundant
and the maintenance hard.  To avoid that, how about building the
relation_sql query dynamically as done for the query to fetch all
table data from the remote server in postgresImportForeignSchema().
Same for the attribute_sql query.

That's all I have for now.  I will continue to review the changes.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-04T02:56:31Z

>
>
> This version of the patch wouldn't fall back to the normal ANALYZE
> processing anymore, so this documentation should be updated as such.
> Also, as I think it's the user's responsibility to ensure the existing
> statistics are up-to-date, as I said before, I think we should add a
> note about that here.  Also, as some users wouldn't be able to ensure
> it, I'm wondering if the default should be false.
>

I agree, if there is no fallback, then the default should be false. When I
was initially brainstorming this patch, Nathan Bossart had suggested making
it the default because 1) that would be an automatic benefit to users and
2) the cost for attempting to import stats was small in comparison to a
table stample, so it was worth the attempt. I still want users to get that
automatic benefit, but if there is no fallback to sampling then the default
only makes sense as false.


> If the user wasn't able to ensure the statistics are up-to-date, I
> think he/she might want to do remote ANALYZE *before* fetching the
> statistics, not after trying to do so.  So I think we could instead
> provide this option as such.  What do you think about that?  Anyway,
> I'd vote for leaving this for another patch, as I think it's a
> nice-to-have option rather than a must-have one, as I said before.
>

I hadn't thought of that one. I think it has some merit, but I think we'd
want the try-after case as well. So the settings would be: "off" (never
remote analyze, default), "on" (always analyze before fetching), and
"retry" (analyze if no stats were found). This feature feeds into the same
thinking that the default setting did, which was to make this feature just
do what is usually the smart thing, and do it automatically. Building it in
pieces might be easier to get committed, but it takes away the dream of
seamless automatic improvement, and establishes defaults that can't be
changed in future versions. That dream was probably unrealistic, but I
wanted to try.


>
> ISTM that the code is well organized overall.  Here are a few comments:
>

Thanks!


>
> We don't use relallvisible and relallfrozen for foreign tables (note
> that do_analyze_rel() calls vac_update_relstats() with relallvisible=0
> and relallfrozen=0 for them).  Do we really need to retrieve (and
> restore) them?
>

No, and as you stated, we wouldn't want to. The query was lifted verbatim
from pg_dump, with a vague hope of moving the queries to a common library
that both pg_dump and postgres_fdw could draw upon. But that no longer
makes sense, so I'll fix.


> +static const char *attstats_query_17 =
> +   "SELECT DISTINCT ON (s.attname) attname, s.null_frac, s.avg_width, "
> +   "s.n_distinct, s.most_common_vals, s.most_common_freqs, "
> +   "s.histogram_bounds, s.correlation, s.most_common_elems, "
> +   "s.most_common_elem_freqs, s.elem_count_histogram, "
> +   "s.range_length_histogram, s.range_empty_frac,
> s.range_bounds_histogram "
> +   "FROM pg_catalog.pg_stats AS s "
> +   "WHERE s.schemaname = $1 AND s.tablename = $2 "
> +   "ORDER BY s.attname, s.inherited DESC";
>
> I think we should retrieve the attribute statistics for only the
> referenced columns of the remote table, not all the columns of it, to
> reduce the data transfer and the cost of matching local/remote
> attributes in import_fetched_statistics().
>

I thought about this, and decided that we wanted to 1) avoid per-column
round trips and 2) keep the remote queries simple. It's not such a big deal
to add "AND s.attname = ANY($3)" and construct a '{att1,att2,"ATt3"}'
string, as we already do in pg_dump in a few places.


>
> In fetch_remote_statistics()
>
> +   if (server_version_num >= 180000)
> +       relation_sql = relstats_query_18;
> +   else if (server_version_num >= 140000)
> +       relation_sql = relstats_query_14;
> +   else
> +       relation_sql = relstats_query_default;
>
> I think that having the definition for each of relstats_query_18,
> relstats_query_14 and relstats_query_default makes the code redundant
> and the maintenance hard.  To avoid that, how about building the
> relation_sql query dynamically as done for the query to fetch all
> table data from the remote server in postgresImportForeignSchema().
> Same for the attribute_sql query.
>

It may be a moot point. If we're not fetching relallfrozen, then the 14 &
18 cases are now the same, and since the pre-14 case concerns
differentiating between analyzed and unanalyzed tables, we would just map
that to 0 IF we kept those stats, but we almost never would because an
unanalyzed remote table would not have the attribute stats necessary to
qualify as a good remote fetch. So we're down to just one static query.


>
> That's all I have for now.  I will continue to review the changes.
>

Much appreciated.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-01-06T11:58:05Z

On Sun, Jan 4, 2026 at 11:56 AM Corey Huinker <corey.huinker@gmail.com> wrote:

>> This version of the patch wouldn't fall back to the normal ANALYZE
>> processing anymore, so this documentation should be updated as such.
>> Also, as I think it's the user's responsibility to ensure the existing
>> statistics are up-to-date, as I said before, I think we should add a
>> note about that here.  Also, as some users wouldn't be able to ensure
>> it, I'm wondering if the default should be false.

> I agree, if there is no fallback, then the default should be false. When I was initially brainstorming this patch, Nathan Bossart had suggested making it the default because 1) that would be an automatic benefit to users and 2) the cost for attempting to import stats was small in comparison to a table stample, so it was worth the attempt. I still want users to get that automatic benefit, but if there is no fallback to sampling then the default only makes sense as false.

I think that the FDW API that I proposed could actually allow us to
fall back to sampling, by modifying StatisticsAreImportable so that it
also checks if 1) there are statistics on the remote server and 2) the
data is fresh enough, and if so, returns true; otherwise, returns
false; in the latter case we could fall back to sampling.  And if we
modified it as such, I think we could change the default to true.
(Checking #2 is necessary to avoid importing stale data, which would
degrade plan quality.)

>> If the user wasn't able to ensure the statistics are up-to-date, I
>> think he/she might want to do remote ANALYZE *before* fetching the
>> statistics, not after trying to do so.  So I think we could instead
>> provide this option as such.  What do you think about that?  Anyway,
>> I'd vote for leaving this for another patch, as I think it's a
>> nice-to-have option rather than a must-have one, as I said before.

> I hadn't thought of that one. I think it has some merit, but I think we'd want the try-after case as well. So the settings would be: "off" (never remote analyze, default), "on" (always analyze before fetching), and "retry" (analyze if no stats were found). This feature feeds into the same thinking that the default setting did, which was to make this feature just do what is usually the smart thing, and do it automatically. Building it in pieces might be easier to get committed, but it takes away the dream of seamless automatic improvement, and establishes defaults that can't be changed in future versions. That dream was probably unrealistic, but I wanted to try.

Remote ANALYZE would be an interesting idea, but to get the automatic
improvement, I think we should first work on the issue I mentioned
above.  So I still think we should leave this for future work.

From a different perspective, even without the automatic improvement
including remote ANALYZE, I think this feature is useful for many
users.

>> +static const char *attstats_query_17 =
>> +   "SELECT DISTINCT ON (s.attname) attname, s.null_frac, s.avg_width, "
>> +   "s.n_distinct, s.most_common_vals, s.most_common_freqs, "
>> +   "s.histogram_bounds, s.correlation, s.most_common_elems, "
>> +   "s.most_common_elem_freqs, s.elem_count_histogram, "
>> +   "s.range_length_histogram, s.range_empty_frac, s.range_bounds_histogram "
>> +   "FROM pg_catalog.pg_stats AS s "
>> +   "WHERE s.schemaname = $1 AND s.tablename = $2 "
>> +   "ORDER BY s.attname, s.inherited DESC";
>>
>> I think we should retrieve the attribute statistics for only the
>> referenced columns of the remote table, not all the columns of it, to
>> reduce the data transfer and the cost of matching local/remote
>> attributes in import_fetched_statistics().

> I thought about this, and decided that we wanted to 1) avoid per-column round trips and 2) keep the remote queries simple. It's not such a big deal to add "AND s.attname = ANY($3)" and construct a '{att1,att2,"ATt3"}' string, as we already do in pg_dump in a few places.

Yeah, I was also thinking of modifying the query as you proposed.

>> In fetch_remote_statistics()
>>
>> +   if (server_version_num >= 180000)
>> +       relation_sql = relstats_query_18;
>> +   else if (server_version_num >= 140000)
>> +       relation_sql = relstats_query_14;
>> +   else
>> +       relation_sql = relstats_query_default;
>>
>> I think that having the definition for each of relstats_query_18,
>> relstats_query_14 and relstats_query_default makes the code redundant
>> and the maintenance hard.  To avoid that, how about building the
>> relation_sql query dynamically as done for the query to fetch all
>> table data from the remote server in postgresImportForeignSchema().
>> Same for the attribute_sql query.

> It may be a moot point. If we're not fetching relallfrozen, then the 14 & 18 cases are now the same, and since the pre-14 case concerns differentiating between analyzed and unanalyzed tables, we would just map that to 0 IF we kept those stats, but we almost never would because an unanalyzed remote table would not have the attribute stats necessary to qualify as a good remote fetch. So we're down to just one static query.

You are right.  As the relation_sql query is only used in
fetch_remote_statistics(), shouldn't the query be defined within that
function?

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-06T14:12:50Z

>
>
> I think that the FDW API that I proposed could actually allow us to
> fall back to sampling, by modifying StatisticsAreImportable so that it
> also checks if 1) there are statistics on the remote server and 2) the
> data is fresh enough, and if so, returns true; otherwise, returns
> false; in the latter case we could fall back to sampling.  And if we
> modified it as such, I think we could change the default to true.
> (Checking #2 is necessary to avoid importing stale data, which would
> degrade plan quality.)
>

So while I haven't checked for "freshness" of the statistics, I have added
checks that ensure that every asked-for column in the local table with
attstattarget != 0 will be send in the column filter, and we:

1. Find remote stats for all the columns that made our list
2. Do not get any stats over the wire with no matching target column.
3. Sort the list of expected remote column names, which means the list
matching is effectively a merge, so O(N) vs O(N^2). This is done with a
name+attnum structure, but it could just as easily have been done with a
local_name+remote_name, as pg_restore_attribute_stats() will take either
attnum or attname as a parameter.

Aside from a pre-emptive ANALYZE, how would you propose we check for and/or
measure "freshness" of the remote statistics?


> Remote ANALYZE would be an interesting idea, but to get the automatic
> improvement, I think we should first work on the issue I mentioned
> above.  So I still think we should leave this for future work.
>
> From a different perspective, even without the automatic improvement
> including remote ANALYZE, I think this feature is useful for many
> users.
>

I'm still hoping to hear from Nathan on this subject.


> >> I think we should retrieve the attribute statistics for only the
> >> referenced columns of the remote table, not all the columns of it, to
> >> reduce the data transfer and the cost of matching local/remote
> >> attributes in import_fetched_statistics().
>
> > I thought about this, and decided that we wanted to 1) avoid per-column
> round trips and 2) keep the remote queries simple. It's not such a big deal
> to add "AND s.attname = ANY($3)" and construct a '{att1,att2,"ATt3"}'
> string, as we already do in pg_dump in a few places.
>
> Yeah, I was also thinking of modifying the query as you proposed.
>

That is done in the patch attached.


> > It may be a moot point. If we're not fetching relallfrozen, then the 14
> & 18 cases are now the same, and since the pre-14 case concerns
> differentiating between analyzed and unanalyzed tables, we would just map
> that to 0 IF we kept those stats, but we almost never would because an
> unanalyzed remote table would not have the attribute stats necessary to
> qualify as a good remote fetch. So we're down to just one static query.
>
> You are right.  As the relation_sql query is only used in
> fetch_remote_statistics(), shouldn't the query be defined within that
> function?
>

Oddly enough, I already moved it inside of fetch_remote_statistics() in the
newest patch.

I wasn't planning on posting this patch until we had heard back from
Nathan, but since I'd already been working on a few of the items you
mentioned in your last email, I thought I'd show you that work in
progress. Some issues like the documentation haven't been updated, so it's
more of a work in progress, but it does pass the tests.

Summary of key points of this  v7 WIP:

* Reduced columns on relstats query, query moved inside calling function.
* Per-column filters on attstats queries, filtering on destination
attstattarget != 0.
* Verification that all filtered-for columns have stats, and that all stats
in the result set have a matching target column.
* Expected column list is now sorted by remote-side attname, allowing a
merge join of the two lists.
* No changes to the documentation, but I know they are needed.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Matheus Alcantara <matheusssilv97@gmail.com> — 2026-01-06T21:38:04Z

Hi, thanks for working on this. Generally I think that this is a good
idea to avoid fetching rows from a foreign table to compute statistics
that may already be available on the foreign server.

I started reviewing the v7 patch and here are my initial comments. I
still want to do another round of review and run some more tests.

+		if (fdwroutine->ImportStatistics != NULL &&
+			fdwroutine->StatisticsAreImportable != NULL &&
+			fdwroutine->StatisticsAreImportable(onerel))
+			import_stats = true;
+		else
+		{
+			if (fdwroutine->AnalyzeForeignTable != NULL)
+				ok = fdwroutine->AnalyzeForeignTable(onerel,
+													 &acquirefunc,
+													 &relpages);
+
+			if (!ok)
+			{
+				ereport(WARNING,
+						errmsg("skipping \"%s\" -- cannot analyze this foreign table.",
+							   RelationGetRelationName(onerel)));
+				relation_close(onerel, ShareUpdateExclusiveLock);
+				return;
+			}
+		}
+
 		if (fdwroutine->AnalyzeForeignTable != NULL)
 			ok = fdwroutine->AnalyzeForeignTable(onerel,
 												 &acquirefunc,
												 &relpages);

		if (!ok)
		{
			ereport(WARNING,
					(errmsg("skipping \"%s\" --- cannot analyze this foreign table",
							RelationGetRelationName(onerel))));
			relation_close(onerel, ShareUpdateExclusiveLock);
			return;
		}

It seems that we have the same code within the else branch after the if/else
check, is this correct?

---

+	 * Every row of the result should be an attribute that we specificially

s/specificially/specifically

---

+		if (TupleDescAttr(tupdesc, i)->attisdropped)
+			continue;
+
+		/* Ignore generated columns. */
+		if (TupleDescAttr(tupdesc, i)->attgenerated)
+			continue;
+
+		attname = NameStr(TupleDescAttr(tupdesc, i)->attname);

Wouldn't be better to call TupleDescAttr a single time and save the value?
		Form_pg_attribute attr = TupleDescAttr(tupdesc, i - 1);

		/* Ignore dropped columns. */
		if (attr->attisdropped)
			continue;
		...

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-01-07T02:18:04Z

Hi Matheus,

On Wed, Jan 7, 2026 at 6:38 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
> +               if (fdwroutine->ImportStatistics != NULL &&
> +                       fdwroutine->StatisticsAreImportable != NULL &&
> +                       fdwroutine->StatisticsAreImportable(onerel))
> +                       import_stats = true;
> +               else
> +               {
> +                       if (fdwroutine->AnalyzeForeignTable != NULL)
> +                               ok = fdwroutine->AnalyzeForeignTable(onerel,
> +                                                                                                        &acquirefunc,
> +                                                                                                        &relpages);
> +
> +                       if (!ok)
> +                       {
> +                               ereport(WARNING,
> +                                               errmsg("skipping \"%s\" -- cannot analyze this foreign table.",
> +                                                          RelationGetRelationName(onerel)));
> +                               relation_close(onerel, ShareUpdateExclusiveLock);
> +                               return;
> +                       }
> +               }
> +
>                 if (fdwroutine->AnalyzeForeignTable != NULL)
>                         ok = fdwroutine->AnalyzeForeignTable(onerel,
>                                                                                                  &acquirefunc,
>                                                                                                  &relpages);
>
>                 if (!ok)
>                 {
>                         ereport(WARNING,
>                                         (errmsg("skipping \"%s\" --- cannot analyze this foreign table",
>                                                         RelationGetRelationName(onerel))));
>                         relation_close(onerel, ShareUpdateExclusiveLock);
>                         return;
>                 }
>
> It seems that we have the same code within the else branch after the if/else
> check, is this correct?

No.  This should be something like the attached in [1].  (I didn't
look at the core changes in v6...)

Thanks!

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/CAPmGK17Dfjy_zLH1yjPqybpSueHWP7Gy_xBZXA2NpRso1qya7A%40mail.gmail.com

Re: Import Statistics in postgres_fdw before resorting to sampling.

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

>
> > I agree, if there is no fallback, then the default should be false. When
> I was initially brainstorming this patch, Nathan Bossart had suggested
> making it the default because 1) that would be an automatic benefit to
> users and 2) the cost for attempting to import stats was small in
> comparison to a table stample, so it was worth the attempt. I still want
> users to get that automatic benefit, but if there is no fallback to
> sampling then the default only makes sense as false.
>
> I think that the FDW API that I proposed could actually allow us to
> fall back to sampling, by modifying StatisticsAreImportable so that it
> also checks if 1) there are statistics on the remote server and 2) the
> data is fresh enough, and if so, returns true; otherwise, returns
> false; in the latter case we could fall back to sampling.  And if we
> modified it as such, I think we could change the default to true.
> (Checking #2 is necessary to avoid importing stale data, which would
> degrade plan quality.)


I've given this some more thought.

First, we'd have to add the va_cols param to StatisticsAreImportable, which
isn't itself terrible.

Then, we'd have to determine that there are stats available for every
mapped column (filtered by va_cols, if any). But the only way to do that is
to query the pg_stats view on the remote end, and if we have done that,
then we've already fetched the stats. Yes, we could avoid selecting the
actual statistical values, and that would save some network bandwidth at
the cost of having to do the query again with stats. So I don't really see
the savings.

Also, the pg_stats view is our security-barrier black box into statistics,
and it gives no insight into how recently those stats were acquired. We
could poke pg_stat_all_tables, assuming we can even query it, and then make
a value judgement on the value of (CURRENT_TIMESTAMP -
GREATEST(last_analyze, last_autoanalyze), but that value is highly
subjective.

I suppose we could move all of the statistics fetching
into StatisticsAreImportable, And carry those values forward if they are
satisfactory. That would leave ImportStatistics() with little to do other
than form up the calls to pg_restore_*_stats(), but those could still fail,
and at that point we'd have no way to fall back to sampling and analysis.

I really want to make sampling fallback possible.

Anyway, here's v8, incorporating the documentation feedback and Matheus's
notes.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Matheus Alcantara <matheusssilv97@gmail.com> — 2026-01-07T15:17:38Z

On Wed Jan 7, 2026 at 3:04 AM -03, Corey Huinker wrote:
>>
>> > I agree, if there is no fallback, then the default should be false. When
>> I was initially brainstorming this patch, Nathan Bossart had suggested
>> making it the default because 1) that would be an automatic benefit to
>> users and 2) the cost for attempting to import stats was small in
>> comparison to a table stample, so it was worth the attempt. I still want
>> users to get that automatic benefit, but if there is no fallback to
>> sampling then the default only makes sense as false.
>>
>> I think that the FDW API that I proposed could actually allow us to
>> fall back to sampling, by modifying StatisticsAreImportable so that it
>> also checks if 1) there are statistics on the remote server and 2) the
>> data is fresh enough, and if so, returns true; otherwise, returns
>> false; in the latter case we could fall back to sampling.  And if we
>> modified it as such, I think we could change the default to true.
>> (Checking #2 is necessary to avoid importing stale data, which would
>> degrade plan quality.)
>
>
> I've given this some more thought.
>
> First, we'd have to add the va_cols param to StatisticsAreImportable, which
> isn't itself terrible.
>
> Then, we'd have to determine that there are stats available for every
> mapped column (filtered by va_cols, if any). But the only way to do that is
> to query the pg_stats view on the remote end, and if we have done that,
> then we've already fetched the stats. Yes, we could avoid selecting the
> actual statistical values, and that would save some network bandwidth at
> the cost of having to do the query again with stats. So I don't really see
> the savings.
>
> Also, the pg_stats view is our security-barrier black box into statistics,
> and it gives no insight into how recently those stats were acquired. We
> could poke pg_stat_all_tables, assuming we can even query it, and then make
> a value judgement on the value of (CURRENT_TIMESTAMP -
> GREATEST(last_analyze, last_autoanalyze), but that value is highly
> subjective.
>
> I suppose we could move all of the statistics fetching
> into StatisticsAreImportable, And carry those values forward if they are
> satisfactory. That would leave ImportStatistics() with little to do other
> than form up the calls to pg_restore_*_stats(), but those could still fail,
> and at that point we'd have no way to fall back to sampling and analysis.
>
> I really want to make sampling fallback possible.
>
> Anyway, here's v8, incorporating the documentation feedback and Matheus's
> notes.

Thanks for the new version.

+static bool
+postgresStatisticsAreImportable(Relation relation)
+...
+
+	/*
+	 * Server-level options can be overridden by table-level options, so check
+	 * server-level first.
+	 */
+	foreach(lc, server->options)
+	{
+		DefElem    *def = (DefElem *) lfirst(lc);
+
+		if (strcmp(def->defname, "fetch_stats") == 0)
+		{
+			fetch_stats = defGetBoolean(def);
+			break;
+		}
+	}
+
+	foreach(lc, table->options)
+	{
+		DefElem    *def = (DefElem *) lfirst(lc);
+
+		if (strcmp(def->defname, "fetch_stats") == 0)
+		{
+			fetch_stats = defGetBoolean(def);
+			break;
+		}
+	}
+
I don't think that it's good to make StatisticsAreImportable() routine
check if fetch_stats is enabled on foreign server/table options because
if so, every fdw implementation would need this same block of code and
also fdw implementations may forget or bypass these options which I
don't think that it would be a desired behavior. What about move this
check to analyze_rel()? Perhaps create a function that just check if the
fetch_stats is enabled.

If the above statement make sense, it seems to me that
StatisticsAreImportable() may not be needed at all. 

I think that we could make analyze_rel() check if fetch_stats is enable
on the foreign server/table and then call ImportStatistics() which could
return true or false. If it returns true it means that the statistics
was imported successfully, otherwise if it returns false we could
fallback to table sampling as we already do today. ImportStatistics
could return false if the foreign server don't have statistics for the
requested table, even  after running ANALYZE if remote_analyze is true.

Is that make sense? Any thoughts?

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-07T19:17:36Z

>
> +
> I don't think that it's good to make StatisticsAreImportable() routine
> check if fetch_stats is enabled on foreign server/table options because
> if so, every fdw implementation would need this same block of code and
> also fdw implementations may forget or bypass these options which I
> don't think that it would be a desired behavior. What about move this
> check to analyze_rel()? Perhaps create a function that just check if the
> fetch_stats is enabled.
>

StatisticsAreImportable() is a virtual function whose goal is to determine
if this specific table supports stats exporting.

postgresStatisticsAreImportable() is the postgres_fdw implementation of
that virtual function.

Any other FDWs that want to implement stats import will need to invent
their own tests and configurations to determine if that is possible.


>
> If the above statement make sense, it seems to me that
> StatisticsAreImportable() may not be needed at all.
>

It wasn't there, initially.


>
> I think that we could make analyze_rel() check if fetch_stats is enable
> on the foreign server/table and then call ImportStatistics() which could
> return true or false.


That we can't do, because there's a chance that those FDWs already have a
setting named fetch_stats.


> If it returns true it means that the statistics
> was imported successfully, otherwise if it returns false we could
> fallback to table sampling as we already do today. ImportStatistics
> could return false if the foreign server don't have statistics for the
> requested table, even  after running ANALYZE if remote_analyze is true.
>
> Is that make sense? Any thoughts?
>

That sounds very similar to the design that was presented in v1.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Matheus Alcantara <matheusssilv97@gmail.com> — 2026-01-07T19:27:11Z

On Wed Jan 7, 2026 at 4:17 PM -03, Corey Huinker wrote:
>>
>> +
>> I don't think that it's good to make StatisticsAreImportable() routine
>> check if fetch_stats is enabled on foreign server/table options because
>> if so, every fdw implementation would need this same block of code and
>> also fdw implementations may forget or bypass these options which I
>> don't think that it would be a desired behavior. What about move this
>> check to analyze_rel()? Perhaps create a function that just check if the
>> fetch_stats is enabled.
>>
>
> StatisticsAreImportable() is a virtual function whose goal is to determine
> if this specific table supports stats exporting.
>
> postgresStatisticsAreImportable() is the postgres_fdw implementation of
> that virtual function.
>
> Any other FDWs that want to implement stats import will need to invent
> their own tests and configurations to determine if that is possible.
>
Ok, now I understand. I thought that fetch_stats and remote_analyze was
a generally fdw option and not only specific to postgres_fdw. Now I
understand that is up to the fdw implementation decide how this should
be enabled or disabled. Thanks for making it clear now.

>> If it returns true it means that the statistics
>> was imported successfully, otherwise if it returns false we could
>> fallback to table sampling as we already do today. ImportStatistics
>> could return false if the foreign server don't have statistics for the
>> requested table, even  after running ANALYZE if remote_analyze is true.
>>
>> Is that make sense? Any thoughts?
>>
>
> That sounds very similar to the design that was presented in v1.
>
Yeah, I think that my suggestion don't make sense, I miss understood the
feature. Sorry about the noise, I'll continue reviewing the v8 patch.

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

Re: Import Statistics in postgres_fdw before resorting to sampling.

Matheus Alcantara <matheusssilv97@gmail.com> — 2026-01-08T19:42:47Z

On Wed Jan 7, 2026 at 3:04 AM -03, Corey Huinker wrote:
> Anyway, here's v8, incorporating the documentation feedback and Matheus's
> notes.
>

+CREATE FOREIGN TABLE remote_analyze_ftable (id int, a text, b bigint)
+       SERVER loopback
+       OPTIONS (table_name 'remote_analyze_table',
+                fetch_stats 'true',
+                remote_analyze 'true');

I think that it would be good also to have a test case where
remote_analyze is false. The test could manually execute an ANALYZE on
the target table and ensure that an ANALYZE on the foreign table fetch
the statistics correctly.

---

If the table don't have columns it fails to fetch the statistics with
remote_analyze=false even if the target table has statistics:
ERROR:  P0002: Failed to import statistics from remote table public.t2, no statistics found.

And if I set remote_analyze=true it fails with the following error:

postgres=# analyze t2_fdw;
ERROR:  08006: could not obtain message string for remote error
CONTEXT:  remote SQL command: SELECT DISTINCT ON (s.attname) attname,
s.null_frac, s.avg_width, s.n_distinct, s.most_common_vals,
s.most_common_freqs, s.histogram_bounds, s.correlation,
s.most_common_elems, s.most_common_elem_freqs, s.elem_count_histogram,
s.range_length_histogram, s.range_empty_frac, s.range_bounds_histogram
FROM pg_catalog.pg_stats AS s WHERE s.schemaname = $1 AND s.tablename =
$2 AND s.attname = ANY($3::text[]) ORDER BY s.attname, s.inherited DESC
LOCATION:  pgfdw_report_internal, connection.c:1037

---

If we try to run ANALYZE on a specific table column that don't exists we
get:
postgres=# analyze t(c);
ERROR:  42703: column "c" of relation "t" does not exist
LOCATION:  do_analyze_rel, analyze.c:412

With fetch_stats=false we get the same error:

postgres=# ALTER FOREIGN TABLE t_fdw OPTIONS (add fetch_stats 'false');
ALTER FOREIGN TABLE

postgres=# ANALYZE t_fdw(c);
ERROR:  42703: column "c" of relation "t_fdw" does not exist

But with fetch_stats=true we get a different error:

postgres=# ALTER FOREIGN TABLE t_fdw OPTIONS (drop fetch_stats);
ALTER FOREIGN TABLE

postgres=# ANALYZE t_fdw(c);
ERROR:  P0002: Failed to import statistics from remote table public.t, no statistics found.

Should all these errors be consistency?

---

I hope that these comments are more useful now. Thanks.

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

Re: Import Statistics in postgres_fdw before resorting to sampling.

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-01-09T09:35:11Z

On Wed, Jan 7, 2026 at 11:34 AM Corey Huinker <corey.huinker@gmail.com> wrote:

>
> Anyway, here's v8, incorporating the documentation feedback and Matheus's notes.

I went through the patches. I have one question: The column names of
the foreign table on the local server are sorted using qsort, which
uses C sorting. The column names the result obtained from the foreign
server are sorted using ORDER BY clause. If the default collation on
the foreign server is different from the one used by qsort(), the
merge sort in import_fetched_statistics() may fail. Shouldn't these
two sorts use the same collation?

Some minor comments

/* avoid including explain_state.h here */
typedef struct ExplainState ExplainState;
-

unintended line deletion?

fetch_remote_statistics() fetches the statistics twice if the first
attempt fails. I think we need same check after the second attempt as
well. The second attempt should not fail, but I think we need some
safety checks and Assert at least, in case the foreign server
misbehaves.

-- 
Best Wishes,
Ashutosh Bapat

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-09T16:21:55Z

On Fri, Jan 9, 2026 at 4:35 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

> On Wed, Jan 7, 2026 at 11:34 AM Corey Huinker <corey.huinker@gmail.com>
> wrote:
>
> >
> > Anyway, here's v8, incorporating the documentation feedback and
> Matheus's notes.
>
> I went through the patches. I have one question: The column names of
> the foreign table on the local server are sorted using qsort, which
> uses C sorting. The column names the result obtained from the foreign
> server are sorted using ORDER BY clause. If the default collation on
> the foreign server is different from the one used by qsort(), the
> merge sort in import_fetched_statistics() may fail. Shouldn't these
> two sorts use the same collation?
>

I wondered about that, but somehow thought that because they're of type
pg_catalog.name rather than text/varchar that they weren't subject to
collation settings. We definitely should explicitly sort by C collation
regardless.

>
> Some minor comments
>
> /* avoid including explain_state.h here */
> typedef struct ExplainState ExplainState;
> -
>
> unintended line deletion?
>

Yeah, must have been.

>
> fetch_remote_statistics() fetches the statistics twice if the first
> attempt fails. I think we need same check after the second attempt as
> well. The second attempt should not fail, but I think we need some
> safety checks and Assert at least, in case the foreign server
> misbehaves.
>

I think the only test *not* done on re-fetch is checking the relkind of the
relation, are you speaking of another check? It's really no big deal to do
that one again, but I made the distinction early on in the coding, and in
retrospect there are relatively few checks we'd consider skipping on the
second pass, so maybe it's not worth the distinction.

Since you're joining the thread, we have an outstanding debate about what
the desired basic workflow should be, and I think we should get some
consensus before we paint ourselves into a corner.

1. The Simplest Possible Model

* There is no remote_analyze functionality
* fetch_stats defaults to false
* Failure to fetch stats results in a failure, no failover to sampling.

2. Simplest Model, but with Failover

* Same as #1, but if we aren't satisfied with the stats we get from the
remote, we issue a WARNING, then fall back to sampling, trusting that the
user will eventually turn off fetch_stats on tables where it isn't working.

3. Analyze and Retry

* Same as #2, but we add remote_analyze option (default false).
* If the first attempt fails AND remote_analyze is set on, then we send the
remote analyze, then retry. Only if that fails do we fall back to sampling.

4. Analyze and Retry, Optimistic

* Same as #3, but fetch_stats defaults to ON, because the worst case
scenario is that we issue a few queries that return 0-1 rows before giving
up and just sampling.
* This is the option that Nathan advocated for in our initial conversation
about the topic, and I found it quite persuasive at the time, but he's been
slammed with other stuff and hasn't been able to add to this thread.

5. Fetch With Retry Or Sample, Optimisitc

* If fetch_stats is on, AND the remote table is seemingly capable of
holding stats, attempt to fetch them, possibly retrying after ANALYZE
depending on remote_analyze.
* If fetching stats failed, just error, as a way to prime the user into
changing the table's setting.
* This is what's currently implemented, and it's not quite what anyone
wants. Defaulting fetch_stats to true doesn't seem great, but not
defaulting it to true will reduce adoption of this feature.

6. Fetch With Retry Or Sample, Pessimistic

* Same as #5, but with fetch_stats = false.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-14T19:41:34Z

>
> Since you're joining the thread, we have an outstanding debate about what
> the desired basic workflow should be, and I think we should get some
> consensus before we paint ourselves into a corner.
>
> 1. The Simplest Possible Model
>
> * There is no remote_analyze functionality
> * fetch_stats defaults to false
> * Failure to fetch stats results in a failure, no failover to sampling.
>
> 2. Simplest Model, but with Failover
>
> * Same as #1, but if we aren't satisfied with the stats we get from the
> remote, we issue a WARNING, then fall back to sampling, trusting that the
> user will eventually turn off fetch_stats on tables where it isn't working.
>
> 3. Analyze and Retry
>
> * Same as #2, but we add remote_analyze option (default false).
> * If the first attempt fails AND remote_analyze is set on, then we send
> the remote analyze, then retry. Only if that fails do we fall back to
> sampling.
>
> 4. Analyze and Retry, Optimistic
>
> * Same as #3, but fetch_stats defaults to ON, because the worst case
> scenario is that we issue a few queries that return 0-1 rows before giving
> up and just sampling.
> * This is the option that Nathan advocated for in our initial conversation
> about the topic, and I found it quite persuasive at the time, but he's been
> slammed with other stuff and hasn't been able to add to this thread.
>
> 5. Fetch With Retry Or Sample, Optimisitc
>
> * If fetch_stats is on, AND the remote table is seemingly capable of
> holding stats, attempt to fetch them, possibly retrying after ANALYZE
> depending on remote_analyze.
> * If fetching stats failed, just error, as a way to prime the user into
> changing the table's setting.
> * This is what's currently implemented, and it's not quite what anyone
> wants. Defaulting fetch_stats to true doesn't seem great, but not
> defaulting it to true will reduce adoption of this feature.
>
> 6. Fetch With Retry Or Sample, Pessimistic
>
> * Same as #5, but with fetch_stats = false.
>


Rebased after adding the COLLATE argument to the ORDER-BY statements.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-01-15T13:47:34Z

On Fri, Jan 9, 2026 at 9:52 PM Corey Huinker <corey.huinker@gmail.com> wrote:
>
> On Fri, Jan 9, 2026 at 4:35 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>>
>> On Wed, Jan 7, 2026 at 11:34 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>>
>> >
>> > Anyway, here's v8, incorporating the documentation feedback and Matheus's notes.
>>
>> I went through the patches. I have one question: The column names of
>> the foreign table on the local server are sorted using qsort, which
>> uses C sorting. The column names the result obtained from the foreign
>> server are sorted using ORDER BY clause. If the default collation on
>> the foreign server is different from the one used by qsort(), the
>> merge sort in import_fetched_statistics() may fail. Shouldn't these
>> two sorts use the same collation?
>
>
> I wondered about that, but somehow thought that because they're of type pg_catalog.name rather than text/varchar that they weren't subject to collation settings. We definitely should explicitly sort by C collation regardless.
>
>>
>>
>> Some minor comments
>>
>> /* avoid including explain_state.h here */
>> typedef struct ExplainState ExplainState;
>> -
>>
>> unintended line deletion?
>
>
> Yeah, must have been.
>
>
>>
>>
>> fetch_remote_statistics() fetches the statistics twice if the first
>> attempt fails. I think we need same check after the second attempt as
>> well. The second attempt should not fail, but I think we need some
>> safety checks and Assert at least, in case the foreign server
>> misbehaves.
>
>
> I think the only test *not* done on re-fetch is checking the relkind of the relation, are you speaking of another check? It's really no big deal to do that one again, but I made the distinction early on in the coding, and in retrospect there are relatively few checks we'd consider skipping on the second pass, so maybe it's not worth the distinction.
>
> Since you're joining the thread, we have an outstanding debate about what the desired basic workflow should be, and I think we should get some consensus before we paint ourselves into a corner.
>
> 1. The Simplest Possible Model
>
> * There is no remote_analyze functionality
> * fetch_stats defaults to false
> * Failure to fetch stats results in a failure, no failover to sampling.
>

Instead of fetch_stats, I would prefer has_stats. But if majority is
on fetch_stats, I am ok with it.

> 2. Simplest Model, but with Failover
>
> * Same as #1, but if we aren't satisfied with the stats we get from the remote, we issue a WARNING, then fall back to sampling, trusting that the user will eventually turn off fetch_stats on tables where it isn't working.
>
> 3. Analyze and Retry
>
> * Same as #2, but we add remote_analyze option (default false).
> * If the first attempt fails AND remote_analyze is set on, then we send the remote analyze, then retry. Only if that fails do we fall back to sampling.
>
> 4. Analyze and Retry, Optimistic
>
> * Same as #3, but fetch_stats defaults to ON, because the worst case scenario is that we issue a few queries that return 0-1 rows before giving up and just sampling.
> * This is the option that Nathan advocated for in our initial conversation about the topic, and I found it quite persuasive at the time, but he's been slammed with other stuff and hasn't been able to add to this thread.
>
> 5. Fetch With Retry Or Sample, Optimisitc
>
> * If fetch_stats is on, AND the remote table is seemingly capable of holding stats, attempt to fetch them, possibly retrying after ANALYZE depending on remote_analyze.
> * If fetching stats failed, just error, as a way to prime the user into changing the table's setting.
> * This is what's currently implemented, and it's not quite what anyone wants. Defaulting fetch_stats to true doesn't seem great, but not defaulting it to true will reduce adoption of this feature.
>
> 6. Fetch With Retry Or Sample, Pessimistic
>
> * Same as #5, but with fetch_stats = false.

I have no strong opinion about whether fetch_stats should be ON or off
by default. In my simple view, the remote relation that the foreign
table points is usually a regular table or a partitioned table. So
default ON makes sense to me. But there's a possibility that in an
OLAP system most of the foreign objects are views exposing only some
data and not all. I think defaulting to off is backward compatible and
requires cautious opt-in. Or else suddenly all ANALYZE on foreign
server will be costly since they are running a couple queries before
falling back to sampling.

Why do we need remote_analyze as a flag? I might have missed some
conversation which led to including this flag. We fetch relkind, so we
know whether the object on the foreign server supports statistics or
not. If it supports statistics, we run ANALYZE automatically. I mean
the user wants us to ANALYZE the foreign table and has also indicated
that the remote object is capable of storing stats. We should document
the fact that we analyze if we do not find existing statistics. Why
would users want to disable running ANALYZE? One possibility is they
don't want to disturb query plans because of suddenly changing
statistics. But then they will need to switch auto-analysis off. So
maybe there is a way to know when it's not acceptable to run ANALYZE
on the remote server. I feel this flag will put another burden on the
user who already has to set so many flags in postgres_fdw. That is
especially true, when the flag is useful only the first time when the
remote object doesn't have stats - usually there will be stats. If at
all we have to introduce remote_analyze, let it default to the value
of fetch_stats.

If both - importing statistics and fallback analysis - fail, I think
we should resort to sampling - since the user has asked for an ANALYZE
table and sampling will achieve that. As you said, giving warnings at
every failure will nudge the user to set their options correctly.

Does that make sense?

-- 
Best Wishes,
Ashutosh Bapat

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-15T17:57:38Z

>
> Instead of fetch_stats, I would prefer has_stats. But if majority is
> on fetch_stats, I am ok with it.
>

I don't think there are any strong feelings about the names of the
parameters, only their meanings and their defaults.


> I have no strong opinion about whether fetch_stats should be ON or off
> by default. In my simple view, the remote relation that the foreign
> table points is usually a regular table or a partitioned table. So
> default ON makes sense to me. But there's a possibility that in an
> OLAP system most of the foreign objects are views exposing only some
> data and not all. I think defaulting to off is backward compatible and
> requires cautious opt-in.


If the remote object is a postgres view, we're never going to get stats, so
default of fetch_stats = on coupled with automatic failover to regular
sampling would result in some trivial network traffic and some
NOTICE/WARNING log messages until the owner sets fetch_stats = off on the
local table.


> Or else suddenly all ANALYZE on foreign
> server will be costly since they are running a couple queries before
> falling back to sampling.
>

In all scenarios, the remote_analyze setting defaults to false, and the
remote database should continue to have stats going forward from that
point. I can see value in having a more insistent NOTICE/WARNING message
for situations where a remote ANALYZE fails or the table continues to have
inadequate stats after the remote ANALYZE.


> Why do we need remote_analyze as a flag? I might have missed some
> conversation which led to including this flag. We fetch relkind, so we
> know whether the object on the foreign server supports statistics or
> not.


Well, sorta. We know the database on the other end talks libpq and has the
table pg_class. There are more than a few forks, Redshift and Vertica come
to mind, where this check will succeed, but the design diverges after that
and doesn't have pg_statistic or similar. We have to do deeper checks of
the remote system (matching pg_statistic rows to all the va_cols we expect,
etc) to verify that we _probably_ have good stats, but we don't actually
truly know if the stats will actually be there until we fetch and import
them, and at that point we're done. And that is part of my discomfort with
StatisticsAreImportable in that it can identify some of the cases where we
_can't_ import stats, but to identify whether we _can_ import the stats we
have just moved all of the functionality of ImportStatistics into
StatisticsAreImportable.


> If it supports statistics, we run ANALYZE automatically. I mean
> the user wants us to ANALYZE the foreign table and has also indicated
> that the remote object is capable of storing stats. We should document
> the fact that we analyze if we do not find existing statistics. Why
> would users want to disable running ANALYZE? One possibility is they
> don't want to disturb query plans because of suddenly changing
> statistics. But then they will need to switch auto-analysis off. So
> maybe there is a way to know when it's not acceptable to run ANALYZE
> on the remote server. I feel this flag will put another burden on the
> user who already has to set so many flags in postgres_fdw. That is
> especially true, when the flag is useful only the first time when the
> remote object doesn't have stats - usually there will be stats. If at
> all we have to introduce remote_analyze, let it default to the value
> of fetch_stats.
>

I may have answered this question in the above responses, but
remote_analyze does not default to ON, nor do I envision making it the
default. I think the primary purpose for the flag is situations where the
remote database is often "swapped out" with one that has a fresher set of
the data (via DNS swapping, port swapping, etc) and the new database was
put online before vacuum got around to those tables. I would agree that
running the remote analyze is pulling a big lever, hence only trying it if
we failed to find sufficient existing stats.


> If both - importing statistics and fallback analysis - fail, I think
> we should resort to sampling - since the user has asked for an ANALYZE
> table and sampling will achieve that. As you said, giving warnings at
> every failure will nudge the user to set their options correctly.
>

Sorry if my description was confusing, I should have been more clear that
"fallback analysis" is the regular row sampling. In scenario #4 (the one I
advocate and Nathan advocate[d?]), the decision tree is this:

1. if fetch_stats = off OR StatisticsAreImportable = false THEN
resort_to_regular_row_sampling and exit normally.
2. attempt_to_fetch_remote_stats. If we fetched and imported remote stats,
THEN exit normally.
3. if remote_analyze = false THEN resort_to_regular_row_sampling and exit
normally.
4. send_remote_analyze_command, if that fails then THEN WARN and
resort_to_regular_row_sampling and exit normally.
5. (step #2, again, essentially) attempt_to_fetch_remote_stats. If we
fetched and imported remote stats, THEN exit normally.
6. WARN user of potentially intractable
problem, resort_to_regular_row_sampling and exit normally.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-01-16T07:10:50Z

On Thu, Jan 15, 2026 at 11:27 PM Corey Huinker <corey.huinker@gmail.com> wrote:
>
>>
>> Or else suddenly all ANALYZE on foreign
>> server will be costly since they are running a couple queries before
>> falling back to sampling.
>
>
> In all scenarios, the remote_analyze setting defaults to false, and the remote database should continue to have stats going forward from that point. I can see value in having a more insistent NOTICE/WARNING message for situations where a remote ANALYZE fails or the table continues to have inadequate stats after the remote ANALYZE.
>
>>
>> Why do we need remote_analyze as a flag? I might have missed some
>> conversation which led to including this flag. We fetch relkind, so we
>> know whether the object on the foreign server supports statistics or
>> not.
>
>
> Well, sorta. We know the database on the other end talks libpq and has the table pg_class. There are more than a few forks, Redshift and Vertica come to mind, where this check will succeed, but the design diverges after that and doesn't have pg_statistic or similar. We have to do deeper checks of the remote system (matching pg_statistic rows to all the va_cols we expect, etc) to verify that we _probably_ have good stats, but we don't actually truly know if the stats will actually be there until we fetch and import them, and at that point we're done. And that is part of my discomfort with StatisticsAreImportable in that it can identify some of the cases where we _can't_ import stats, but to identify whether we _can_ import the stats we have just moved all of the functionality of ImportStatistics into StatisticsAreImportable.
>
>>
>> If it supports statistics, we run ANALYZE automatically. I mean
>> the user wants us to ANALYZE the foreign table and has also indicated
>> that the remote object is capable of storing stats. We should document
>> the fact that we analyze if we do not find existing statistics. Why
>> would users want to disable running ANALYZE? One possibility is they
>> don't want to disturb query plans because of suddenly changing
>> statistics. But then they will need to switch auto-analysis off. So
>> maybe there is a way to know when it's not acceptable to run ANALYZE
>> on the remote server. I feel this flag will put another burden on the
>> user who already has to set so many flags in postgres_fdw. That is
>> especially true, when the flag is useful only the first time when the
>> remote object doesn't have stats - usually there will be stats. If at
>> all we have to introduce remote_analyze, let it default to the value
>> of fetch_stats.
>
>
> I may have answered this question in the above responses, but remote_analyze does not default to ON, nor do I envision making it the default. I think the primary purpose for the flag is situations where the remote database is often "swapped out" with one that has a fresher set of the data (via DNS swapping, port swapping, etc) and the new database was put online before vacuum got around to those tables. I would agree that running the remote analyze is pulling a big lever, hence only trying it if we failed to find sufficient existing stats.
>

Assuming following conditions to be true
1. object on the other side usually has statistics
2. it didn't when we queried.

The reason for that situation is that the object was not analyzed
before for the reasons you mention. Then why not just run ANALYZE and
instantiate the statistics. That will happen only rarely. Why do we
need a table and server option to control that behaviour? Maybe you
have already explained and I am not able to understand your answer.
Sorry.

-- 
Best Wishes,
Ashutosh Bapat

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-16T16:49:30Z

>
> Assuming following conditions to be true
> 1. object on the other side usually has statistics
> 2. it didn't when we queried.
>
> The reason for that situation is that the object was not analyzed
> before for the reasons you mention. Then why not just run ANALYZE and
> instantiate the statistics. That will happen only rarely.


I agree.


> Why do we
> need a table and server option to control that behaviour? Maybe you
> have already explained and I am not able to understand your answer.
>

I probably didn't explain it, but one reason for having the option is that
the role used to connect to the remote database might not have the
permissions to analyze tables in general, or that table in particular.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-18T02:53:17Z

On Fri, Jan 16, 2026 at 11:49 AM Corey Huinker <corey.huinker@gmail.com>
wrote:

> Assuming following conditions to be true
>> 1. object on the other side usually has statistics
>> 2. it didn't when we queried.
>>
>> The reason for that situation is that the object was not analyzed
>> before for the reasons you mention. Then why not just run ANALYZE and
>> instantiate the statistics. That will happen only rarely.
>
>
> I agree.
>
>
>> Why do we
>> need a table and server option to control that behaviour? Maybe you
>> have already explained and I am not able to understand your answer.
>>
>
> I probably didn't explain it, but one reason for having the option is that
> the role used to connect to the remote database might not have the
> permissions to analyze tables in general, or that table in particular.
>

Changes in this release, aside from rebasing:

- The generic analyze and fdw.h changes are in their own patch (0001) that
ignores contrib/postgres_fdw entirely.
- The option for remote_analyze has been moved to its own patch (0003).
- The errors raised are now warnings, to ensure that we can always fall
back to row sampling.
- All local attributes with attstatarget > 0 must get matching remote
statistics or the import is considered a failure.
- The pg_restore_attribute_stats() call has been turned into a prepared
statement, for clarity and some minor parsing savings.
- The calls to pg_restore_relation_stats() are parameterized, but not
prepared as this is rarely called more than once.
- postgresStatisticsAreImportable will now disqualify a table if has
extended statistics objects, because we can't compute those without a row
sample.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-21T20:51:23Z

>
> Changes in this release, aside from rebasing:
>
> - The generic analyze and fdw.h changes are in their own patch (0001) that
> ignores contrib/postgres_fdw entirely.
> - The option for remote_analyze has been moved to its own patch (0003).
> - The errors raised are now warnings, to ensure that we can always fall
> back to row sampling.
> - All local attributes with attstatarget > 0 must get matching remote
> statistics or the import is considered a failure.
> - The pg_restore_attribute_stats() call has been turned into a prepared
> statement, for clarity and some minor parsing savings.
> - The calls to pg_restore_relation_stats() are parameterized, but not
> prepared as this is rarely called more than once.
> - postgresStatisticsAreImportable will now disqualify a table if has
> extended statistics objects, because we can't compute those without a row
> sample.
>

Rebase

Re: Import Statistics in postgres_fdw before resorting to sampling.

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-01-22T10:15:46Z

On Thu, Jan 22, 2026 at 2:21 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>>
>> Changes in this release, aside from rebasing:
>>
>> - The generic analyze and fdw.h changes are in their own patch (0001) that ignores contrib/postgres_fdw entirely.
>> - The option for remote_analyze has been moved to its own patch (0003).
>> - The errors raised are now warnings, to ensure that we can always fall back to row sampling.
>> - All local attributes with attstatarget > 0 must get matching remote statistics or the import is considered a failure.
>> - The pg_restore_attribute_stats() call has been turned into a prepared statement, for clarity and some minor parsing savings.
>> - The calls to pg_restore_relation_stats() are parameterized, but not prepared as this is rarely called more than once.
>> - postgresStatisticsAreImportable will now disqualify a table if has extended statistics objects, because we can't compute those without a row sample.
>

Thanks Corey for breaking down these patches. It makes reviewing easier.

analyze_rel() and acquire_inherited_sample_rows() both call
fdwroutine->AnalyzeForeignTable() but only the first one uses the
statistics import facility. Is that intentional? Typical use case of
sharding will create a partitioned table with foreign tables as
partitions. The partitions will be analyzed by the second function.
Thus a big use case of postgres_fdw won't be able to use the import
statistics facility. That seems like a major drawback of this patch.
Thinking more about it, acquire_inherited_sample_rows() accumulates
the sample rows from the child tables and extracts statistics from
those rows and then updates corresponding pg_statistics rows. Doing
that through import statistics seems a bit tricky since we need to be
able to combine statistics from multiple relations. Can we do that?
There's an advantage if we can combine stats across multiple relations
- we don't have to sample children twice when analyzing the parent
without ONLY. Instead we could produce parent statistics by combining
statistics across children and the parent. To me this looks like
altogether a different beast just like partial aggregates.

It will be good to fix this drawback. If not, at least we should
figure out (plan/POC) how to deal with the child tables? We need to at
least document this drawback - the documentation in the current patch
reads as if all foreign tables will use this facility when available.

-- 
Best Wishes,
Ashutosh Bapat

Re: Import Statistics in postgres_fdw before resorting to sampling.

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-01-22T11:29:12Z

On Thu, Jan 22, 2026 at 3:45 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Jan 22, 2026 at 2:21 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> >>
> >> Changes in this release, aside from rebasing:
> >>
> >> - The generic analyze and fdw.h changes are in their own patch (0001) that ignores contrib/postgres_fdw entirely.
> >> - The option for remote_analyze has been moved to its own patch (0003).
> >> - The errors raised are now warnings, to ensure that we can always fall back to row sampling.
> >> - All local attributes with attstatarget > 0 must get matching remote statistics or the import is considered a failure.
> >> - The pg_restore_attribute_stats() call has been turned into a prepared statement, for clarity and some minor parsing savings.
> >> - The calls to pg_restore_relation_stats() are parameterized, but not prepared as this is rarely called more than once.
> >> - postgresStatisticsAreImportable will now disqualify a table if has extended statistics objects, because we can't compute those without a row sample.
> >

The patches fail to build the document. Attached diff fixes that.


-- 
Best Wishes,
Ashutosh Bapat

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-22T22:20:12Z

On Thu, Jan 22, 2026 at 5:16 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

> On Thu, Jan 22, 2026 at 2:21 AM Corey Huinker <corey.huinker@gmail.com>
> wrote:
> >>
> >> Changes in this release, aside from rebasing:
> >>
> >> - The generic analyze and fdw.h changes are in their own patch (0001)
> that ignores contrib/postgres_fdw entirely.
> >> - The option for remote_analyze has been moved to its own patch (0003).
> >> - The errors raised are now warnings, to ensure that we can always fall
> back to row sampling.
> >> - All local attributes with attstatarget > 0 must get matching remote
> statistics or the import is considered a failure.
> >> - The pg_restore_attribute_stats() call has been turned into a prepared
> statement, for clarity and some minor parsing savings.
> >> - The calls to pg_restore_relation_stats() are parameterized, but not
> prepared as this is rarely called more than once.
> >> - postgresStatisticsAreImportable will now disqualify a table if has
> extended statistics objects, because we can't compute those without a row
> sample.
> >
>
> Thanks Corey for breaking down these patches. It makes reviewing easier.
>
> analyze_rel() and acquire_inherited_sample_rows() both call
> fdwroutine->AnalyzeForeignTable() but only the first one uses the
> statistics import facility. Is that intentional? Typical use case of
> sharding will create a partitioned table with foreign tables as
> partitions. The partitions will be analyzed by the second function.
> Thus a big use case of postgres_fdw won't be able to use the import
> statistics facility. That seems like a major drawback of this patch.
> Thinking more about it, acquire_inherited_sample_rows() accumulates
> the sample rows from the child tables and extracts statistics from
> those rows and then updates corresponding pg_statistics rows. Doing
> that through import statistics seems a bit tricky since we need to be
> able to combine statistics from multiple relations. Can we do that?
>

We can't synthesize sample rows from imported statistics, no.


> There's an advantage if we can combine stats across multiple relations
> - we don't have to sample children twice when analyzing the parent
> without ONLY. Instead we could produce parent statistics by combining
> statistics across children and the parent. To me this looks like
> altogether a different beast just like partial aggregates.
>

I think this patch is only ever going to get us out of 1 of the 2 samples,
which isn't ideal but it is a savings.


>
> It will be good to fix this drawback. If not, at least we should
> figure out (plan/POC) how to deal with the child tables? We need to at
> least document this drawback - the documentation in the current patch
> reads as if all foreign tables will use this facility when available.
>

 Yes, we will have to note the limitation. I have made that note, as well
as the documentation fix attached.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-01-23T15:43:19Z

On Fri, Jan 23, 2026 at 3:50 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>
> On Thu, Jan 22, 2026 at 5:16 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>>
>> On Thu, Jan 22, 2026 at 2:21 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>> >>
>> >> Changes in this release, aside from rebasing:
>> >>
>> >> - The generic analyze and fdw.h changes are in their own patch (0001) that ignores contrib/postgres_fdw entirely.
>> >> - The option for remote_analyze has been moved to its own patch (0003).
>> >> - The errors raised are now warnings, to ensure that we can always fall back to row sampling.
>> >> - All local attributes with attstatarget > 0 must get matching remote statistics or the import is considered a failure.
>> >> - The pg_restore_attribute_stats() call has been turned into a prepared statement, for clarity and some minor parsing savings.
>> >> - The calls to pg_restore_relation_stats() are parameterized, but not prepared as this is rarely called more than once.
>> >> - postgresStatisticsAreImportable will now disqualify a table if has extended statistics objects, because we can't compute those without a row sample.
>> >
>>
>> Thanks Corey for breaking down these patches. It makes reviewing easier.
>>
>> analyze_rel() and acquire_inherited_sample_rows() both call
>> fdwroutine->AnalyzeForeignTable() but only the first one uses the
>> statistics import facility. Is that intentional? Typical use case of
>> sharding will create a partitioned table with foreign tables as
>> partitions. The partitions will be analyzed by the second function.
>> Thus a big use case of postgres_fdw won't be able to use the import
>> statistics facility. That seems like a major drawback of this patch.
>> Thinking more about it, acquire_inherited_sample_rows() accumulates
>> the sample rows from the child tables and extracts statistics from
>> those rows and then updates corresponding pg_statistics rows. Doing
>> that through import statistics seems a bit tricky since we need to be
>> able to combine statistics from multiple relations. Can we do that?
>
>
> We can't synthesize sample rows from imported statistics, no.
>
>>
>> There's an advantage if we can combine stats across multiple relations
>> - we don't have to sample children twice when analyzing the parent
>> without ONLY. Instead we could produce parent statistics by combining
>> statistics across children and the parent. To me this looks like
>> altogether a different beast just like partial aggregates.
>
>
> I think this patch is only ever going to get us out of 1 of the 2 samples, which isn't ideal but it is a savings.
>

I am not suggesting to synthesize sample rows. Calculate the
statistics of the parent table from that of its children.

>>
>>
>> It will be good to fix this drawback. If not, at least we should
>> figure out (plan/POC) how to deal with the child tables? We need to at
>> least document this drawback - the documentation in the current patch
>> reads as if all foreign tables will use this facility when available.
>
>
>  Yes, we will have to note the limitation. I have made that note, as well as the documentation fix attached.

The note just mentions partition table but the limitation applies to
any foreign child table.

-- 
Best Wishes,
Ashutosh Bapat

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-23T17:15:40Z

>
> >> There's an advantage if we can combine stats across multiple relations
> >> - we don't have to sample children twice when analyzing the parent
> >> without ONLY. Instead we could produce parent statistics by combining
> >> statistics across children and the parent. To me this looks like
> >> altogether a different beast just like partial aggregates.
> >
> >
> > I think this patch is only ever going to get us out of 1 of the 2
> samples, which isn't ideal but it is a savings.
> >
>
> I am not suggesting to synthesize sample rows. Calculate the
> statistics of the parent table from that of its children.
>

I'm not sure we can actually do that. The functions that compute the
statistics are all based off of row samples, not already computed
statistics. I don't think we can synthesize a rowsample from the imported
statistics, at least not accurately. If I'm misunderstanding what you're
suggesting, please correct me.


> The note just mentions partition table but the limitation applies to
> any foreign child table.
>

Noted. Will fix in next revision.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-01-27T05:47:08Z

On Fri, Jan 23, 2026 at 10:45 PM Corey Huinker <corey.huinker@gmail.com> wrote:
>>
>> >> There's an advantage if we can combine stats across multiple relations
>> >> - we don't have to sample children twice when analyzing the parent
>> >> without ONLY. Instead we could produce parent statistics by combining
>> >> statistics across children and the parent. To me this looks like
>> >> altogether a different beast just like partial aggregates.
>> >
>> >
>> > I think this patch is only ever going to get us out of 1 of the 2 samples, which isn't ideal but it is a savings.
>> >
>>
>> I am not suggesting to synthesize sample rows. Calculate the
>> statistics of the parent table from that of its children.
>
>
> I'm not sure we can actually do that. The functions that compute the statistics are all based off of row samples, not already computed statistics. I don't think we can synthesize a rowsample from the imported statistics, at least not accurately. If I'm misunderstanding what you're suggesting, please correct me.

I am comparing the calculation of statistics to the calculation of
aggregates. We have code to compute aggregates on a partitioned table
from the partial aggregates computed from the individual partitions.
(Even though I am mentioning the partitioned table, the technique can
be used for an inheritance hierarchy.) Similarly if we could come up
with a representation of partial statistics, we could get partial
statistics computed for the children (and the parent in
non-partitioned inheritance). Use the partial statistics to compute
the statistics for the parent without the need to synthesize row
samples from the children. I haven't looked at all the kinds of
statistics to see whether this is feasible.

--
Best Wishes,
Ashutosh Bapat

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-27T08:04:52Z

>
> > I'm not sure we can actually do that. The functions that compute the
> statistics are all based off of row samples, not already computed
> statistics. I don't think we can synthesize a rowsample from the imported
> statistics, at least not accurately. If I'm misunderstanding what you're
> suggesting, please correct me.
>
> I am comparing the calculation of statistics to the calculation of
> aggregates. We have code to compute aggregates on a partitioned table
> from the partial aggregates computed from the individual partitions.
> (Even though I am mentioning the partitioned table, the technique can
> be used for an inheritance hierarchy.) Similarly if we could come up

with a representation of partial statistics, we could get partial
> statistics computed for the children (and the parent in
> non-partitioned inheritance). Use the partial statistics to compute
> the statistics for the parent without the need to synthesize row
> samples from the children. I haven't looked at all the kinds of
> statistics to see whether this is feasible.
>

We're limited to the existing data from pg_class and the security-barrier
view pg_stats. We also have pg_stats_ext and pg_stats_ext_exprs as well,
but those are for extended stats objects, which aren't useful to us in this
context.

I've been a part of some research into the feasibility of caching the row
samples fetched, allowing the planner to generate on-the-fly statistics for
OLAP queries. If we ever got that functionality, we'd need a means of
exposing those row samples externally, and even then we'd have to wait for
the remote postgresql server to have that feature. So for now we are
limited to what pg_class and pg_stats tell us.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-01-29T11:03:00Z

On Tue, Jan 27, 2026 at 1:35 PM Corey Huinker <corey.huinker@gmail.com> wrote:
>>
>> > I'm not sure we can actually do that. The functions that compute the statistics are all based off of row samples, not already computed statistics. I don't think we can synthesize a rowsample from the imported statistics, at least not accurately. If I'm misunderstanding what you're suggesting, please correct me.
>>
>> I am comparing the calculation of statistics to the calculation of
>> aggregates. We have code to compute aggregates on a partitioned table
>> from the partial aggregates computed from the individual partitions.
>> (Even though I am mentioning the partitioned table, the technique can
>> be used for an inheritance hierarchy.) Similarly if we could come up
>>
>> with a representation of partial statistics, we could get partial
>> statistics computed for the children (and the parent in
>> non-partitioned inheritance). Use the partial statistics to compute
>> the statistics for the parent without the need to synthesize row
>> samples from the children. I haven't looked at all the kinds of
>> statistics to see whether this is feasible.
>
>
> We're limited to the existing data from pg_class and the security-barrier view pg_stats. We also have pg_stats_ext and pg_stats_ext_exprs as well, but those are for extended stats objects, which aren't useful to us in this context.
>
> I've been a part of some research into the feasibility of caching the row samples fetched, allowing the planner to generate on-the-fly statistics for OLAP queries. If we ever got that functionality, we'd need a means of exposing those row samples externally, and even then we'd have to wait for the remote postgresql server to have that feature. So for now we are limited to what pg_class and pg_stats tell us.

The way this is implemented, it will favour the usecases where foreign
tables are not child tables. That leaves out the sharding use case
which I believe is also a significant usecase. I think we need to
think, how can we make that usecase benefit from this optimization. In
case of sharding the foreign tables will be child tables pointing to
very large tables on the remote side or at least the parent table they
are part of will have very large data spread across multiple remotes.
Not being able to use statistics available on the remote side seems a
major limitation. But I don't have a better solution than to think of
supporting some kind of partial statistics.

-- 
Best Wishes,
Ashutosh Bapat

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-01-29T19:20:26Z

>
>
> The way this is implemented, it will favour the usecases where foreign
> tables are not child tables.


It is true that this feature does not benefit the recursive
do_analyze_rel() case. But it does help when those same tables are analyzed
directly.


> That leaves out the sharding use case
> which I believe is also a significant usecase. I think we need to
> think, how can we make that usecase benefit from this optimization.


I agree that we should find a way to do that, but this handles the other
case, and doesn't prevent us from later teaching
postgresAnalyzeForeignTable() to use cache the rowsample locally for later
use, which postgresImportStatistics() could then consider the relative
benefits of using that local cached sample vs the already formed remote
statistics. Even in that case, I'm guessing that the remote table's stats
will be based on a larger and therefore better sample size then the sample
we are able to pull across the wire and cache locally, so the remotely
computed statistics would be better.

Not being able to use statistics available on the remote side seems a
> major limitation. But I don't have a better solution than to think of
> supporting some kind of partial statistics.


I'm not against trying to fetch and cache rowsamples, or cache some
partially aggregated results of a rowsample, but this patch does not cover
that. This patch should, at least in theory, reduce the number of table
samples pulled across the wire by 50% and that seems worthwhile.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-02-12T14:29:34Z

On Thu, Jan 29, 2026 at 2:20 PM Corey Huinker <corey.huinker@gmail.com>
wrote:

>
>> The way this is implemented, it will favour the usecases where foreign
>> tables are not child tables.
>
>
> It is true that this feature does not benefit the recursive
> do_analyze_rel() case. But it does help when those same tables are analyzed
> directly.
>
>
>> That leaves out the sharding use case
>> which I believe is also a significant usecase. I think we need to
>> think, how can we make that usecase benefit from this optimization.
>
>
> I agree that we should find a way to do that, but this handles the other
> case, and doesn't prevent us from later teaching
> postgresAnalyzeForeignTable() to use cache the rowsample locally for later
> use, which postgresImportStatistics() could then consider the relative
> benefits of using that local cached sample vs the already formed remote
> statistics. Even in that case, I'm guessing that the remote table's stats
> will be based on a larger and therefore better sample size then the sample
> we are able to pull across the wire and cache locally, so the remotely
> computed statistics would be better.
>
> Not being able to use statistics available on the remote side seems a
>> major limitation. But I don't have a better solution than to think of
>> supporting some kind of partial statistics.
>
>
> I'm not against trying to fetch and cache rowsamples, or cache some
> partially aggregated results of a rowsample, but this patch does not cover
> that. This patch should, at least in theory, reduce the number of table
> samples pulled across the wire by 50% and that seems worthwhile.
>
>

Rebase with some error message cleanups.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-03-12T11:02:43Z

On Sun, Jan 18, 2026 at 11:53 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> Changes in this release, aside from rebasing:
>
> - The generic analyze and fdw.h changes are in their own patch (0001) that ignores contrib/postgres_fdw entirely.
> - The option for remote_analyze has been moved to its own patch (0003).

Thanks for splitting the patch!  That makes reviewing easy!

> - The errors raised are now warnings, to ensure that we can always fall back to row sampling.

Falling back to sampling improves the usability, so I don't object to
adding it anymore, but I'm concerned about changes made to the error
handling of libpq functions in fetch_relstats() and fetch_attstats(),
like this bit in the former function:

+   if (!PQsendQueryParams(conn, sql, 2, NULL, params, NULL, NULL, 0))
+   {
+       pgfdw_report(WARNING, NULL, conn, sql);
+       return NULL;
+   }
+
+   res = pgfdw_get_result(conn);
+
+   if (res == NULL
+       || PQresultStatus(res) != PGRES_TUPLES_OK
+       || PQntuples(res) != 1
+       || PQnfields(res) != RELSTATS_NUM_FIELDS
+       || PQgetisnull(res, 0, RELSTATS_RELKIND))
+   {
+       pgfdw_report(WARNING, res, conn, sql);
+       PQclear(res);
+       return NULL;
+   }

By the errors-to-warnings change, even when those libpq functions
fail, we fall back to the normal ANALYZE processing, but I don't think
that that is OK, because if those functions fail for some reasons
(network-related issues like network disconnection or memory-related
issues like out of memory), then libpq functions called later for the
normal processing would also be likely to fail for the same reasons,
causing the same failure again, which I don't think is good.  To avoid
that, when libpq functions in fetch_relstats() and fetch_attstats()
fail, shouldn't we just throw an error, as before?

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-03-12T19:41:54Z

>
> By the errors-to-warnings change, even when those libpq functions
> fail, we fall back to the normal ANALYZE processing, but I don't think
> that that is OK, because if those functions fail for some reasons
> (network-related issues like network disconnection or memory-related
> issues like out of memory), then libpq functions called later for the
> normal processing would also be likely to fail for the same reasons,
> causing the same failure again, which I don't think is good.  To avoid
> that, when libpq functions in fetch_relstats() and fetch_attstats()
> fail, shouldn't we just throw an error, as before?
>

Right now the code doesn't differentiate on the kind of an error that was
encountered. If it was a network error, then not only do we expect fallback
to also fail, but simple fdw queries will fail as well. If, however it were
a permission issue (no SELECT permission on pg_stats, for instance, or
pg_stats doesn't exist because the remote database is not a regular
Postgres like redshift or something) then I definitely want to fall back.
Currently, the code makes no judgement on the details of the error, it just
trusts that fallback-analyze will either succeed (because it was
permissions related) or it will quickly encounter the same insurmountable
effort, and one extra PQsendQuery isn't that much overhead.

If you think that inspecting the error that we get and matching against a
list of errors that should skip the retry or skip the fallback, I'd be in
favor of that. It's probably easier to start with a list of errorcodes that
we feel are hopeless and should remain ERRORs not WARNINGs.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-03-13T12:16:49Z

On Fri, Mar 13, 2026 at 4:42 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>>
>> By the errors-to-warnings change, even when those libpq functions
>> fail, we fall back to the normal ANALYZE processing, but I don't think
>> that that is OK, because if those functions fail for some reasons
>> (network-related issues like network disconnection or memory-related
>> issues like out of memory), then libpq functions called later for the
>> normal processing would also be likely to fail for the same reasons,
>> causing the same failure again, which I don't think is good.  To avoid
>> that, when libpq functions in fetch_relstats() and fetch_attstats()
>> fail, shouldn't we just throw an error, as before?

> Right now the code doesn't differentiate on the kind of an error that was encountered. If it was a network error, then not only do we expect fallback to also fail, but simple fdw queries will fail as well. If, however it were a permission issue (no SELECT permission on pg_stats, for instance, or pg_stats doesn't exist because the remote database is not a regular Postgres like redshift or something) then I definitely want to fall back. Currently, the code makes no judgement on the details of the error, it just trusts that fallback-analyze will either succeed (because it was permissions related) or it will quickly encounter the same insurmountable effort, and one extra PQsendQuery isn't that much overhead.

I'm not sure if that's really true; if the error is the one that
doesn't take a time to detect, the extra function call wouldn't
probably be a problem, but if the error is the one that takes a long
time to detect, the function call would make the situation worse.

> If you think that inspecting the error that we get and matching against a list of errors that should skip the retry or skip the fallback, I'd be in favor of that. It's probably easier to start with a list of errorcodes that we feel are hopeless and should remain ERRORs not WARNINGs.

That's an improvement, but I think that that's nice-to-have, not
must-have.  As there's not much time left until the feature freeze,
I'd like to propose to make the first cut as simple as possible and
thus leave that for future work, if this is intended for v19.  If no
objections from you (or anyone else), I'd like to update the patch as
I proposed.

Another thing I noticed is related to this comment in fetch_relstats():

+   /*
+    * Before v14, a reltuples value of 0 was ambiguous: it could either mean
+    * the relation is empty, or it could mean that it hadn't yet been
+    * vacuumed or analyzed.  (Newer versions use -1 for the latter case).
+    *
+    * We can ignore this change, because if the remote table wasn't analyzed,
+    * then it would have no attribute stats, and thus we wouldn't have stats
+    * that we would try to import. So we can take the reltuples value as-is.
+    */

I might have misread the patch, but for v14 or later the patch tries
to import remote stats even when we have reltuples = -1.  Is that
intentional?  In that case the remote table has never yet been
vacuumed/analyzed, so I think we should just fall back to the normal
processing.  Also, for v13 or earlier it also tries to import remote
stats even when we have reltuples = 0, but in that case the remote
table might not have been vacuumed/analyzed, so to avoid possibly
useless processing, I think we should just fall back to it in that
case as well.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-03-13T15:36:31Z

On Fri, Mar 13, 2026 at 8:17 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

> On Fri, Mar 13, 2026 at 4:42 AM Corey Huinker <corey.huinker@gmail.com>
> wrote:
> >>
> >> By the errors-to-warnings change, even when those libpq functions
> >> fail, we fall back to the normal ANALYZE processing, but I don't think
> >> that that is OK, because if those functions fail for some reasons
> >> (network-related issues like network disconnection or memory-related
> >> issues like out of memory), then libpq functions called later for the
> >> normal processing would also be likely to fail for the same reasons,
> >> causing the same failure again, which I don't think is good.  To avoid
> >> that, when libpq functions in fetch_relstats() and fetch_attstats()
> >> fail, shouldn't we just throw an error, as before?
>
> > Right now the code doesn't differentiate on the kind of an error that
> was encountered. If it was a network error, then not only do we expect
> fallback to also fail, but simple fdw queries will fail as well. If,
> however it were a permission issue (no SELECT permission on pg_stats, for
> instance, or pg_stats doesn't exist because the remote database is not a
> regular Postgres like redshift or something) then I definitely want to fall
> back. Currently, the code makes no judgement on the details of the error,
> it just trusts that fallback-analyze will either succeed (because it was
> permissions related) or it will quickly encounter the same insurmountable
> effort, and one extra PQsendQuery isn't that much overhead.
>
> I'm not sure if that's really true; if the error is the one that
> doesn't take a time to detect, the extra function call wouldn't
> probably be a problem, but if the error is the one that takes a long
> time to detect, the function call would make the situation worse.
>

I'm struggling to think of what kind of error that would be that would take
a long time to detect. Network timeouts could be one thing, but such a
situation would affect normal query operations as well, not just analyzes.
If you have a specific error in mind, please let me know.


>
> > If you think that inspecting the error that we get and matching against
> a list of errors that should skip the retry or skip the fallback, I'd be in
> favor of that. It's probably easier to start with a list of errorcodes that
> we feel are hopeless and should remain ERRORs not WARNINGs.
>
> That's an improvement, but I think that that's nice-to-have, not
> must-have.  As there's not much time left until the feature freeze,
> I'd like to propose to make the first cut as simple as possible and
> thus leave that for future work, if this is intended for v19.  If no
> objections from you (or anyone else), I'd like to update the patch as
> I proposed.
>

I'd like to get this into 19 as well, so I'm likely to agree to your
proposal.


>
> Another thing I noticed is related to this comment in fetch_relstats():
>
> +   /*
> +    * Before v14, a reltuples value of 0 was ambiguous: it could either
> mean
> +    * the relation is empty, or it could mean that it hadn't yet been
> +    * vacuumed or analyzed.  (Newer versions use -1 for the latter case).
> +    *
> +    * We can ignore this change, because if the remote table wasn't
> analyzed,
> +    * then it would have no attribute stats, and thus we wouldn't have
> stats
> +    * that we would try to import. So we can take the reltuples value
> as-is.
> +    */
>
> I might have misread the patch, but for v14 or later the patch tries
> to import remote stats even when we have reltuples = -1.  Is that
> intentional?  In that case the remote table has never yet been
> vacuumed/analyzed, so I think we should just fall back to the normal
> processing.


For cases where we affirmatively have -1, it makes sense to immediately go
to the ANALYZE option (if available) and fall back if not. I'll make that
change.


>   Also, for v13 or earlier it also tries to import remote
> stats even when we have reltuples = 0, but in that case the remote
> table might not have been vacuumed/analyzed, so to avoid possibly
> useless processing, I think we should just fall back to it in that
> case as well.
>

+1

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-03-14T11:40:52Z

On Sat, Mar 14, 2026 at 12:36 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> On Fri, Mar 13, 2026 at 8:17 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> On Fri, Mar 13, 2026 at 4:42 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>> >>
>> >> By the errors-to-warnings change, even when those libpq functions
>> >> fail, we fall back to the normal ANALYZE processing, but I don't think
>> >> that that is OK, because if those functions fail for some reasons
>> >> (network-related issues like network disconnection or memory-related
>> >> issues like out of memory), then libpq functions called later for the
>> >> normal processing would also be likely to fail for the same reasons,
>> >> causing the same failure again, which I don't think is good.  To avoid
>> >> that, when libpq functions in fetch_relstats() and fetch_attstats()
>> >> fail, shouldn't we just throw an error, as before?
>>
>> > Right now the code doesn't differentiate on the kind of an error that was encountered. If it was a network error, then not only do we expect fallback to also fail, but simple fdw queries will fail as well. If, however it were a permission issue (no SELECT permission on pg_stats, for instance, or pg_stats doesn't exist because the remote database is not a regular Postgres like redshift or something) then I definitely want to fall back. Currently, the code makes no judgement on the details of the error, it just trusts that fallback-analyze will either succeed (because it was permissions related) or it will quickly encounter the same insurmountable effort, and one extra PQsendQuery isn't that much overhead.
>>
>> I'm not sure if that's really true; if the error is the one that
>> doesn't take a time to detect, the extra function call wouldn't
>> probably be a problem, but if the error is the one that takes a long
>> time to detect, the function call would make the situation worse.
>
> I'm struggling to think of what kind of error that would be that would take a long time to detect. Network timeouts could be one thing, but such a situation would affect normal query operations as well, not just analyzes. If you have a specific error in mind, please let me know.

I too was thinking of network timeouts (and in such a case I thought
that even if PQsendQuery was run successfully, the following
PQgetResult would have to block for a long time).  And yes, that
situation can occur for normal queries, but in such a query, the extra
functions call is done only when aborting the remote transaction, in a
way libpq functions don't have to wait for a network timeout.

>> > If you think that inspecting the error that we get and matching against a list of errors that should skip the retry or skip the fallback, I'd be in favor of that. It's probably easier to start with a list of errorcodes that we feel are hopeless and should remain ERRORs not WARNINGs.
>>
>> That's an improvement, but I think that that's nice-to-have, not
>> must-have.  As there's not much time left until the feature freeze,
>> I'd like to propose to make the first cut as simple as possible and
>> thus leave that for future work, if this is intended for v19.  If no
>> objections from you (or anyone else), I'd like to update the patch as
>> I proposed.
>
> I'd like to get this into 19 as well, so I'm likely to agree to your proposal.

Ok, will update the patch.

>> Another thing I noticed is related to this comment in fetch_relstats():
>>
>> +   /*
>> +    * Before v14, a reltuples value of 0 was ambiguous: it could either mean
>> +    * the relation is empty, or it could mean that it hadn't yet been
>> +    * vacuumed or analyzed.  (Newer versions use -1 for the latter case).
>> +    *
>> +    * We can ignore this change, because if the remote table wasn't analyzed,
>> +    * then it would have no attribute stats, and thus we wouldn't have stats
>> +    * that we would try to import. So we can take the reltuples value as-is.
>> +    */
>>
>> I might have misread the patch, but for v14 or later the patch tries
>> to import remote stats even when we have reltuples = -1.  Is that
>> intentional?  In that case the remote table has never yet been
>> vacuumed/analyzed, so I think we should just fall back to the normal
>> processing.
>
> For cases where we affirmatively have -1, it makes sense to immediately go to the ANALYZE option (if available) and fall back if not. I'll make that change.
>
>>   Also, for v13 or earlier it also tries to import remote
>> stats even when we have reltuples = 0, but in that case the remote
>> table might not have been vacuumed/analyzed, so to avoid possibly
>> useless processing, I think we should just fall back to it in that
>> case as well.
>
> +1

I forgot to mention this, but when we have reltuples = 0 for v14 or
later, the patch tries to import both relstats and attstats, but in
that case I think it would be OK to do so only for the former for the
consistency with the normal processing.  What do you think about that?

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-03-14T19:51:29Z

>
>
> I too was thinking of network timeouts (and in such a case I thought
> that even if PQsendQuery was run successfully, the following
> PQgetResult would have to block for a long time).  And yes, that
> situation can occur for normal queries, but in such a query, the extra
> functions call is done only when aborting the remote transaction, in a
> way libpq functions don't have to wait for a network timeout.
>

I've copied the ERROR-behavior that was already done
in postgresGetAnalyzeInfoForForeignTable(), which I assume will be to your
liking.

Structural error checks (how many columns does the result set have, etc)
are now elog errors, bad PQresultStatus-es are now errors as well.

The warnings when the query ran fine but no data was found are now NOTICE
level.


> I forgot to mention this, but when we have reltuples = 0 for v14 or
> later, the patch tries to import both relstats and attstats, but in
> that case I think it would be OK to do so only for the former for the
> consistency with the normal processing.  What do you think about that?
>

I've update the logic that we don't try to fetch attrstats if the reltuples
is 0 or -1, and though the comments still mention the difference in server
version, the code behaves the same in new versions and old. My thinking is
that either value means "you are not going to find attstats" and our
reaction is the same either way.

I've also moved the column matching - finding which rows of the attstats
result set match to which columns in the local table - up into the fetch
portion, something you had mentioned wanting to see as well. This resulted
in some significant refactoring, but overall I think you will find the
changes for the better.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-03-14T20:05:41Z

>
>
> I've also moved the column matching - finding which rows of the attstats
> result set match to which columns in the local table - up into the fetch
> portion, something you had mentioned wanting to see as well. This resulted
> in some significant refactoring, but overall I think you will find the
> changes for the better.
>

I've added a reset of the result set indexes. Probably not necessary given
that we only keep the result set if all the columns found homes, but good
for peace of mind.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-03-18T01:56:54Z

On Sat, Mar 14, 2026 at 4:05 PM Corey Huinker <corey.huinker@gmail.com>
wrote:

>
>> I've also moved the column matching - finding which rows of the attstats
>> result set match to which columns in the local table - up into the fetch
>> portion, something you had mentioned wanting to see as well. This resulted
>> in some significant refactoring, but overall I think you will find the
>> changes for the better.
>>
>
> I've added a reset of the result set indexes. Probably not necessary given
> that we only keep the result set if all the columns found homes, but good
> for peace of mind.
>

And rebased to make the CI happy.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-03-30T09:32:38Z

On Wed, Mar 18, 2026 at 10:57 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> On Sat, Mar 14, 2026 at 4:05 PM Corey Huinker <corey.huinker@gmail.com> wrote:

>>> I've also moved the column matching - finding which rows of the attstats result set match to which columns in the local table - up into the fetch portion, something you had mentioned wanting to see as well. This resulted in some significant refactoring, but overall I think you will find the changes for the better.

>> I've added a reset of the result set indexes. Probably not necessary given that we only keep the result set if all the columns found homes, but good for peace of mind.

> And rebased to make the CI happy.

Thanks for updating patches!

I have been reviewing 0001 and 0002.  Here are my comments/proposals I
have for now:

Core side:

* I proposed the StatisticsAreImportable callback function, but the
ImportStatistics callback function is now allowed to fall back to
analyzing, so I don't think StatisticsAreImportable has much value
anymore.  I'll withdraw my proposal.  Attached is an updated patch
removing it, in which 0001 is merged into 0002 for ease of review
(0001 can't be tested without 0002).

* I modified analyze.c to start progress monitor a bit earlier to
monitor the work of ImportStatistics (and AnalyzeForeignTable) as part
of initialization.

* I noticed this odd behavior:

create table t1 (id int, data text);
create foreign table ft1 (id int, data text) server loopback options
(table_name 't1');

alter foreign table ft1 options (add fetch_stats 'false');
analyze ft1 (ctid);
ERROR:  column "ctid" of relation "ft1" does not exist

Looks good, but:

alter foreign table ft1 options (set fetch_stats 'true');
analyze ft1 (ctid);
NOTICE:  remote table public.t1 has no statistics to import
HINT:  Falling back to ANALYZE with regular row sampling.
ERROR:  column "ctid" of relation "ft1" does not exist

postgres_fdw is doing the remote access without any need to do so,
which isn't good.

The cause is that the patches fail to validate the va_cols list given
to analyze_rel before importing stats.  To fix, I modified analyze.c
to do so before calling ImportStatistics.  I think this is also useful
when analyzing inheritance trees, as it avoids validating the list
repeatedly in that case.

* I separated a bit of code in examine_attribute that checks whether
the given column is analyzable into a new extern function so that FDWs
can use it.

postgres_fdw side:

* In fetch_remote_statistics, if we get reltuples=0 for v14 or later,
I think we should update only the relation stats with that info, and
avoid resorting to analyzing, for efficiency, as I proposed before.  I
modified that function (and import_fetched_statistics) that way.

* I noticed that importing stats for a foreign table repeatedly like
this leads to incorrect attribute stats:

insert into t1 values ('aaa'), ('bbb'), ('ccc');
analyze t1;
analyze ft1;
select histogram_bounds from pg_stats where tablename = 'ft1';
 histogram_bounds
------------------
 {aaa,bbb,ccc}
(1 row)

delete from t1;
insert into t1 values ('foo'), ('foo'), ('foo');
analyze t1;
analyze ft1;
select histogram_bounds from pg_stats where tablename = 'ft1';
 histogram_bounds
------------------
 {aaa,bbb,ccc}
(1 row)

Old stats are remaining.

The root cause is that passing NULL to pg_restore_attribute_stats
leaves the stats unchanged.  To fix, I modified
import_fetched_statistics to do pg_clear_attribute_stats before the
restore function.

* I don't think the error handling in import_fetched_statistics (and
match_attrmap) is safe:

+   plan = SPI_prepare(attimport_sql, ATTIMPORT_SQL_NUM_FIELDS,
+                      (Oid *) attimport_argtypes);
+
+   if (plan == NULL || SPI_result < 0)
+   {
+       ereport(WARNING,
+               errmsg("import attribute statistics prepare failed %s "
+                      "with error code %d",
+                      attimport_sql, SPI_result));
+       goto import_cleanup;
+   }

In general, I think that if an error that should not happen happens,
continuing the processing isn't safe anymore; in such a case we should
throw an error to give the user a chance to decide what to do (eg,
retry after fixing issues or retry after setting the fetch_size option
to false if that's considered safe or ...).  I think this is
consistent with the error handling of libpq functions we discussed
before.  So I modified the handling in both functions to throw an
error in such cases.

* The matching logic in match_attrmap seems correct to me, but I think
it's still complicated than necessary: you are doing a merge join of
the RemoteAttributeMapping array and the attstats set by placing the
latter in the outer side, but it makes it simpler to do the join the
opposite way (ie, the former in the outer side), like the attached,
which doesn't need an inner for-loop anymore, as the inner side (ie,
the attstats set) consists of *distinct* columns.

* I moved table_has_extended_stats to extended_stats.c and renamed it
to match other functions in it so that other FDWs too can use it.

* The 0002 patch is using different levels (LOG, NOTICE, and WARNING)
for logging in postgresImportStatisitcs, but I think it's appropriate
to use elevel or WARNING for it.  So I modified the patch to use
WARNING.  I modified the wording as well to match analyze.c.

* I added at the top/end of postgresImportStatisitcs log messages that
show the start/end of importing stats.

That's it.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-03-30T09:42:09Z

On Sun, Mar 15, 2026 at 4:51 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>> I too was thinking of network timeouts (and in such a case I thought
>> that even if PQsendQuery was run successfully, the following
>> PQgetResult would have to block for a long time).  And yes, that
>> situation can occur for normal queries, but in such a query, the extra
>> functions call is done only when aborting the remote transaction, in a
>> way libpq functions don't have to wait for a network timeout.

> I've copied the ERROR-behavior that was already done in postgresGetAnalyzeInfoForForeignTable(), which I assume will be to your liking.
>
> Structural error checks (how many columns does the result set have, etc) are now elog errors, bad PQresultStatus-es are now errors as well.

The patch looks good to me other than this bit in fetch_relstats/fetch_attstats:

+   res = pgfdw_get_result(conn);
+
+   if (PQresultStatus(res) != PGRES_TUPLES_OK)
+       pgfdw_report_error(NULL, conn, sql);

You forgot to pass the res to pgfdw_report_error for proper logging.
I fixed this in the patch I posted just before.

>> I forgot to mention this, but when we have reltuples = 0 for v14 or
>> later, the patch tries to import both relstats and attstats, but in
>> that case I think it would be OK to do so only for the former for the
>> consistency with the normal processing.  What do you think about that?

> I've update the logic that we don't try to fetch attrstats if the reltuples is 0 or -1, and though the comments still mention the difference in server version, the code behaves the same in new versions and old. My thinking is that either value means "you are not going to find attstats" and our reaction is the same either way.

OK, but I modified this further in the patch.  Please check it.

Thanks again!

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-03-30T20:04:46Z

>
> * I proposed the StatisticsAreImportable callback function, but the
> ImportStatistics callback function is now allowed to fall back to
> analyzing, so I don't think StatisticsAreImportable has much value
> anymore.  I'll withdraw my proposal.  Attached is an updated patch
> removing it, in which 0001 is merged into 0002 for ease of review
> (0001 can't be tested without 0002).
>

Thanks.


> * I modified analyze.c to start progress monitor a bit earlier to
> monitor the work of ImportStatistics (and AnalyzeForeignTable) as part
> of initialization.
>
> * I noticed this odd behavior:
>
> create table t1 (id int, data text);
> create foreign table ft1 (id int, data text) server loopback options
> (table_name 't1');
>
> alter foreign table ft1 options (add fetch_stats 'false');
> analyze ft1 (ctid);
> ERROR:  column "ctid" of relation "ft1" does not exist
>
> Looks good, but:
>
> alter foreign table ft1 options (set fetch_stats 'true');
> analyze ft1 (ctid);
> NOTICE:  remote table public.t1 has no statistics to import
> HINT:  Falling back to ANALYZE with regular row sampling.
> ERROR:  column "ctid" of relation "ft1" does not exist
>
> postgres_fdw is doing the remote access without any need to do so,
> which isn't good.
>
> The cause is that the patches fail to validate the va_cols list given
> to analyze_rel before importing stats.  To fix, I modified analyze.c
> to do so before calling ImportStatistics.  I think this is also useful
> when analyzing inheritance trees, as it avoids validating the list
> repeatedly in that case.
>

I see this change and I like it.


> * I separated a bit of code in examine_attribute that checks whether
> the given column is analyzable into a new extern function so that FDWs
> can use it.
>

Interesting. I wouldn't have dared make a change that affected regular
analyze, but it makes sense.

postgres_fdw side:
>
> * In fetch_remote_statistics, if we get reltuples=0 for v14 or later,
> I think we should update only the relation stats with that info, and
> avoid resorting to analyzing, for efficiency, as I proposed before.  I
> modified that function (and import_fetched_statistics) that way.
>

This will miss out on the case where the remote table did get analyzed
once, when empty, but now isn't empty. I realize that shouldn't happen very
often, but the cost of rowsampling a table that is empty is very low.


> The root cause is that passing NULL to pg_restore_attribute_stats
> leaves the stats unchanged.  To fix, I modified
> import_fetched_statistics to do pg_clear_attribute_stats before the
> restore function.
>

+1


> * The matching logic in match_attrmap seems correct to me, but I think
> it's still complicated than necessary: you are doing a merge join of
> the RemoteAttributeMapping array and the attstats set by placing the
> latter in the outer side, but it makes it simpler to do the join the
> opposite way (ie, the former in the outer side), like the attached,
> which doesn't need an inner for-loop anymore, as the inner side (ie,
> the attstats set) consists of *distinct* columns.
>

The new match_attrmap seems reasonable.


> * I moved table_has_extended_stats to extended_stats.c and renamed it
> to match other functions in it so that other FDWs too can use it.
>

+1


> * The 0002 patch is using different levels (LOG, NOTICE, and WARNING)
> for logging in postgresImportStatisitcs, but I think it's appropriate
> to use elevel or WARNING for it.  So I modified the patch to use
> WARNING.  I modified the wording as well to match analyze.c.
>

+1


> * I added at the top/end of postgresImportStatisitcs log messages that
> show the start/end of importing stats.
>


+1


> That's it.
>

I see that remote_analyze didn't make it as a part of this patch. Is that
something you'd repackaged as a follow-on patch, or are you just done with
it?

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-03-30T20:05:43Z

>
> You forgot to pass the res to pgfdw_report_error for proper logging.
> I fixed this in the patch I posted just before.
>

+1


>
> > I've update the logic that we don't try to fetch attrstats if the
> reltuples is 0 or -1, and though the comments still mention the difference
> in server version, the code behaves the same in new versions and old. My
> thinking is that either value means "you are not going to find attstats"
> and our reaction is the same either way.
>

Agree.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-04-01T11:39:19Z

On Tue, Mar 31, 2026 at 5:04 AM Corey Huinker <corey.huinker@gmail.com> wrote:

>> postgres_fdw side:
>>
>> * In fetch_remote_statistics, if we get reltuples=0 for v14 or later,
>> I think we should update only the relation stats with that info, and
>> avoid resorting to analyzing, for efficiency, as I proposed before.  I
>> modified that function (and import_fetched_statistics) that way.
>
> This will miss out on the case where the remote table did get analyzed once, when empty, but now isn't empty. I realize that shouldn't happen very often, but the cost of rowsampling a table that is empty is very low.

I think that that would be the user's fault, as it's the user's
responsibility to ensure that the existing stats for the remote table
are up-to-date.  From another perspective, not all users will be able
to operate in such a way, so I'm thinking of disabling this feature by
default.

> I see that remote_analyze didn't make it as a part of this patch. Is that something you'd repackaged as a follow-on patch, or are you just done with it?

As just reviewing/polishing the 0001/0002 patches is a lot of work, I
didn't have time to look at the remote_analyze patch.  We are running
out of time, so I'm afraid that I won't be able to have time for that.

I modified the patch further:

* Modified postgresImportStatistics to create RemoteAttributeMapping if needed.

* The query executed in fetch_relstats is almost the same as the one
executed in postgresGetAnalyzeInfoForForeignTable.  To avoid code
duplication, I modified it to use the latter query.  I also changed it
to use PQsendQuery, not PQsendQueryParams, for efficiency.

* Modified import_spi_query_ok to get the result of an import query by
using SPI_getbinval, not SPI_getvalue, for efficiency.

Attached is a new version of the patch.

Thanks for reviewing!

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-04-01T18:53:50Z

>
> I think that that would be the user's fault, as it's the user's
> responsibility to ensure that the existing stats for the remote table
> are up-to-date.  From another perspective, not all users will be able
> to operate in such a way, so I'm thinking of disabling this feature by
> default.
>

I'd like it to remain on by default, and the handling of this edge case can
be made configurable in the future, so I'm fine with it for now.


> > I see that remote_analyze didn't make it as a part of this patch. Is
> that something you'd repackaged as a follow-on patch, or are you just done
> with it?
>
> As just reviewing/polishing the 0001/0002 patches is a lot of work, I
> didn't have time to look at the remote_analyze patch.  We are running
> out of time, so I'm afraid that I won't be able to have time for that.
>

Fair enough.


> I modified the patch further:
>
> * Modified postgresImportStatistics to create RemoteAttributeMapping if
> needed.
>

+1


> * The query executed in fetch_relstats is almost the same as the one
> executed in postgresGetAnalyzeInfoForForeignTable.  To avoid code
> duplication, I modified it to use the latter query.


+1. That always bothered me.


> * Modified import_spi_query_ok to get the result of an import query by
> using SPI_getbinval, not SPI_getvalue, for efficiency.


+1.

Everything checks out over here.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-04-02T11:05:41Z

On Thu, Apr 2, 2026 at 3:54 AM Corey Huinker <corey.huinker@gmail.com> wrote:

>> I think that that would be the user's fault, as it's the user's
>> responsibility to ensure that the existing stats for the remote table
>> are up-to-date.  From another perspective, not all users will be able
>> to operate in such a way, so I'm thinking of disabling this feature by
>> default.
>
> I'd like it to remain on by default, and the handling of this edge case can be made configurable in the future, so I'm fine with it for now.

The problem is not limited to this special case.  Consider cases when
1) the remote table that has many rows are heavily updated after it
got analyzed, and then 2) postgres_fdw imports its stats before it
gets re-analyzed.  The stats postgres_fdw imports would be stale,
causing plan degradation.  I don't think we should enable this feature
by default until we guarantee stats freshness in some way.

> Everything checks out over here.

Thanks for reviewing!

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-04-02T17:14:35Z

>
> The problem is not limited to this special case.  Consider cases when
> 1) the remote table that has many rows are heavily updated after it
> got analyzed, and then 2) postgres_fdw imports its stats before it
> gets re-analyzed.  The stats postgres_fdw imports would be stale,
> causing plan degradation.  I don't think we should enable this feature
> by default until we guarantee stats freshness in some way.


So it seems like we have the following configurations desired by at least
somebody:

0. Row Sampling Only
1. Fetch stats and fall back to row sampling.
2. Always analyze remote table (assuming it is a table that can hold
stats), then fetch stats, and fall back if necessary.
3. Fetch stats, and if that turned up 0 attribute stats try an analyze,
then try to refetch and if it still fails go to row sampling.

With the following interpretation of reltuples = 0:

a. The table is definitively empty, stop.
b. The table is missing stats and running an analyze is cheap (assuming
remote analysis is even enabled)
c. if remote version >= 14 then a else b

I'm of the opinion that 3c is the best configuration for most tables, and
you have advocated for 1a without an analyze option and 2a with one. Option
2 seems a bit heavy handed to me, but I could see checking the remote
pg_stat_all_tables and making an analyze/no-analyze judgement call based on
that, perhaps call that analyze_stale_vacuum_interval or something like
that. That could be a neat feature for v20, and so whatever default we
choose for fetch_stats, I ask that we choose values that keep our options
open for all 4x3 configurations enumerated above.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-04-03T13:55:54Z

On Fri, Apr 3, 2026 at 2:14 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>>
>> The problem is not limited to this special case.  Consider cases when
>> 1) the remote table that has many rows are heavily updated after it
>> got analyzed, and then 2) postgres_fdw imports its stats before it
>> gets re-analyzed.  The stats postgres_fdw imports would be stale,
>> causing plan degradation.  I don't think we should enable this feature
>> by default until we guarantee stats freshness in some way.
>
> So it seems like we have the following configurations desired by at least somebody:
>
> 0. Row Sampling Only
> 1. Fetch stats and fall back to row sampling.
> 2. Always analyze remote table (assuming it is a table that can hold stats), then fetch stats, and fall back if necessary.
> 3. Fetch stats, and if that turned up 0 attribute stats try an analyze, then try to refetch and if it still fails go to row sampling.
>
> With the following interpretation of reltuples = 0:
>
> a. The table is definitively empty, stop.
> b. The table is missing stats and running an analyze is cheap (assuming remote analysis is even enabled)
> c. if remote version >= 14 then a else b
>
> I'm of the opinion that 3c is the best configuration for most tables, and you have advocated for 1a without an analyze option and 2a with one. Option 2 seems a bit heavy handed to me, but I could see checking the remote pg_stat_all_tables and making an analyze/no-analyze judgement call based on that, perhaps call that analyze_stale_vacuum_interval or something like that. That could be a neat feature for v20, and so whatever default we choose for fetch_stats, I ask that we choose values that keep our options open for all 4x3 configurations enumerated above.

Before discussing this, let's wrap up the work for v19.  Here is a new
version of the patch.  Changes are:

* As above, I think we should disable this feature by default for now,
so I modified the patch as such.

* You are defining a remote query per version at the top of the file
that is used in fetch_attstats, but I think that that reduces
readability, making maintenance hard, so I modified that function to
build the query dynamically in it, as I proposed before.  I also
modified that function to use PQsendQuery, not PQsendQueryParams, as
in fetch_relstats, for efficiency.

* I moved the code for opening/closing the connection from
postgresImportStatistics to fetch_remote_statistics, as the connection
is only used in the latter function.  I also removed useless cleanup
processing in that function.

* I added regression tests.

* I fixed a minor bug I noticed while adding those tests.

* I did some cleanup and editorialization including re-ordering
functions in logical order.

One thing I'm wondering is: we should rename ImportStatistics to
ImportForeignStatistics, for consistency with ImportForeignSchema?
Also, we should rename fetch_stats to import_stats, for consistency
with eg, import_default?

Anyway I think the patch is now in good shape, except comments/docs,
which I will work on next.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

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

>
> * As above, I think we should disable this feature by default for now,
> so I modified the patch as such.
>

+1 so long as it doesn't paint us into a corner.


> * You are defining a remote query per version at the top of the file
> that is used in fetch_attstats, but I think that that reduces
> readability, making maintenance hard, so I modified that function to
> build the query dynamically in it, as I proposed before.  I also
> modified that function to use PQsendQuery, not PQsendQueryParams, as
> in fetch_relstats, for efficiency.
>

No objections. I find full-formed queries more readable, but I'm clearly in
the minority with that opinion.

I'm curious, is the efficiency of PQsendQuery over PQsendQueryParams that
significant?


> * I moved the code for opening/closing the connection from
> postgresImportStatistics to fetch_remote_statistics, as the connection
> is only used in the latter function.  I also removed useless cleanup
> processing in that function.
>

+1


> * I added regression tests.
>

+1


> * I fixed a minor bug I noticed while adding those tests.
>
> * I did some cleanup and editorialization including re-ordering
> functions in logical order.
>

This was momentarily confusing as I apply your patch to a separate branch
and then compare whole branches, but it checks out.


> One thing I'm wondering is: we should rename ImportStatistics to
> ImportForeignStatistics, for consistency with ImportForeignSchema?
>

ImportForeignStatistics makes sense to me.


> Also, we should rename fetch_stats to import_stats, for consistency
> with eg, import_default?
>

restore_stats is probably the better name, given that it is calling the
pg_restore_*() functions.


> Anyway I think the patch is now in good shape, except comments/docs,
> which I will work on next.


+1!

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-04-05T11:05:42Z

On Sat, Apr 4, 2026 at 1:01 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>>
>> * As above, I think we should disable this feature by default for now,
>> so I modified the patch as such.

> +1 so long as it doesn't paint us into a corner.

Given that we can't guarantee the freshness, I think that that's the
only choice.

>> * You are defining a remote query per version at the top of the file
>> that is used in fetch_attstats, but I think that that reduces
>> readability, making maintenance hard, so I modified that function to
>> build the query dynamically in it, as I proposed before.  I also
>> modified that function to use PQsendQuery, not PQsendQueryParams, as
>> in fetch_relstats, for efficiency.

> I'm curious, is the efficiency of PQsendQuery over PQsendQueryParams that significant?

No, it isn't, but note that PQsendQuery uses simple query protocol
while PQsendQueryParams uses extended query protocol, and that the
overhead incurred by the former protocol is smaller than that by the
latter protocol, so it's better to use PQsendQuery here.

>> * I fixed a minor bug I noticed while adding those tests.
>>
>> * I did some cleanup and editorialization including re-ordering
>> functions in logical order.

> This was momentarily confusing as I apply your patch to a separate branch and then compare whole branches, but it checks out.

Sorry, but I believe that that makes the code easy to read.

>> One thing I'm wondering is: we should rename ImportStatistics to
>> ImportForeignStatistics, for consistency with ImportForeignSchema?

> ImportForeignStatistics makes sense to me.

Will rename.

>> Also, we should rename fetch_stats to import_stats, for consistency
>> with eg, import_default?

> restore_stats is probably the better name, given that it is calling the pg_restore_*() functions.

That sounds better, so will rename.

Thanks for reviewing!

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-04-07T10:09:46Z

On Sun, Apr 5, 2026 at 8:05 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Sat, Apr 4, 2026 at 1:01 AM Corey Huinker <corey.huinker@gmail.com> wrote:

> >> One thing I'm wondering is: we should rename ImportStatistics to
> >> ImportForeignStatistics, for consistency with ImportForeignSchema?
>
> > ImportForeignStatistics makes sense to me.
>
> Will rename.

Done.

> >> Also, we should rename fetch_stats to import_stats, for consistency
> >> with eg, import_default?
>
> > restore_stats is probably the better name, given that it is calling the pg_restore_*() functions.
>
> That sounds better, so will rename.

Done.

Here are other changes:

* I removed the implementation details from the docs in
postgres-fdw.sgml, as I think it is overly technical for
postgres-fdw.sgml, which is intended for end users.  Also, I added a
note about resorting to sampling if stats import failed, and another
note about the user's responsibility to ensure that remote stats are
up-to-date.

* I added the docs about the new callback routine to fdwhandler.sgml.

* I added/tweaked comments.

* I Added one more test case.

* I did further cleanup and editorialization.

I think the patch is committable, so I will push it if no objections.
(Maybe we could add/improve some more comments, but I think we could
do that later if needed.)

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-04-08T11:24:00Z

On Tue, Apr 7, 2026 at 7:09 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> I think the patch is committable, so I will push it if no objections.

Pushed.  I closed the CF entry as well.

Corey, if you intend to continue working on the remote_analyze patch,
please start a new thread about it and create a new CF entry with that
thread.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-04-08T13:11:23Z

>
> Pushed.  I closed the CF entry as well.
>

Hooray!


> Corey, if you intend to continue working on the remote_analyze patch,
> please start a new thread about it and create a new CF entry with that
> thread.
>

I will, but I want to incorporate your feedback about how analyze should
work and check in with some people at pgconf.dev first.

Thanks again!

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-04-12T06:31:01Z

On Wed, Apr 8, 2026 at 10:11 PM Corey Huinker <corey.huinker@gmail.com> wrote:

>> Corey, if you intend to continue working on the remote_analyze patch,
>> please start a new thread about it and create a new CF entry with that
>> thread.
>
> I will, but I want to incorporate your feedback about how analyze should work and check in with some people at pgconf.dev first.

Ok, thanks!

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Tom Lane <tgl@sss.pgh.pa.us> — 2026-04-12T18:15:00Z

Etsuro Fujita <etsuro.fujita@gmail.com> writes:
> Pushed.  I closed the CF entry as well.

Coverity doesn't like this patch's uses of strncpy():

>>>     CID 1691464:           (BUFFER_SIZE)
>>>     Calling "strncpy" with a maximum size argument of 64 bytes on destination array "remattrmap[attrcnt].local_attname" of size 64 bytes might leave the destination string unterminated.
5960     		strncpy(remattrmap[attrcnt].local_attname, attname, NAMEDATALEN);
5961     		strncpy(remattrmap[attrcnt].remote_attname, remote_attname, NAMEDATALEN);

I think it's dead right to complain: remote_attname, in particular,
could certainly be longer than NAMEDATALEN.

AFAICS, postgres_fdw's subsequent uses of those strings only need
them to be nul-terminated C strings, so strncpy's property of
zero-filling the whole buffer is not needed here.  I recommend
s/strncpy/strlcpy/.

It's probably also appropriate to think about using pg_mbcliplen()
to ensure that this code doesn't result in a broken multibyte
character.

			regards, tom lane

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-04-13T04:53:05Z

On Sun, Apr 12, 2026 at 11:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Coverity doesn't like this patch's uses of strncpy():
>
> >>>     CID 1691464:           (BUFFER_SIZE)
> >>>     Calling "strncpy" with a maximum size argument of 64 bytes on destination array "remattrmap[attrcnt].local_attname" of size 64 bytes might leave the destination string unterminated.
> 5960                    strncpy(remattrmap[attrcnt].local_attname, attname, NAMEDATALEN);
> 5961                    strncpy(remattrmap[attrcnt].remote_attname, remote_attname, NAMEDATALEN);
>
> I think it's dead right to complain: remote_attname, in particular,
> could certainly be longer than NAMEDATALEN.
>
> AFAICS, postgres_fdw's subsequent uses of those strings only need
> them to be nul-terminated C strings, so strncpy's property of
> zero-filling the whole buffer is not needed here.  I recommend
> s/strncpy/strlcpy/.

Seems like a good idea.

> It's probably also appropriate to think about using pg_mbcliplen()
> to ensure that this code doesn't result in a broken multibyte
> character.

Will do.

I am currently overseas and will be attending an event this week, so I
will work on this after returning home.  In the meantime, I created an
entry for it in the open items list.

Thanks!

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-04-30T18:04:22Z

>
> > AFAICS, postgres_fdw's subsequent uses of those strings only need
> > them to be nul-terminated C strings, so strncpy's property of
> > zero-filling the whole buffer is not needed here.  I recommend
> > s/strncpy/strlcpy/.
>
> Seems like a good idea.
>
> > It's probably also appropriate to think about using pg_mbcliplen()
> > to ensure that this code doesn't result in a broken multibyte
> > character.
>
> Will do.
>
> I am currently overseas and will be attending an event this week, so I
> will work on this after returning home.  In the meantime, I created an
> entry for it in the open items list.
>
> Thanks!


I've attached 2 different fixes.

The first one swtiches to pg_mbcliplen+memcpy, which is probably overkill
but better safe than sorry.

The second gets rid of the string buffers entirely, and instead frees the
nested object.

I have no preference between the two, though I suspect that the nested-free
solution will be preferred by the community.

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-05-12T11:36:25Z

On Fri, May 1, 2026 at 3:04 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> I've attached 2 different fixes.

Thanks for the patches!

> The first one swtiches to pg_mbcliplen+memcpy, which is probably overkill but better safe than sorry.

I noticed that RemoteAttributeMapping.remote_attname is used in
match_attrmap(), which assumes that it fully contains a remote
attribute's name, so I think this solution is actually unsafe.

> The second gets rid of the string buffers entirely, and instead frees the nested object.
>
> I have no preference between the two, though I suspect that the nested-free solution will be preferred by the community.

ISTM that the second solution is the right direction to go in.

+static void
+free_remattrmap(RemoteAttributeMapping *map, int len)
+{
+   if (!map)
+       return;
+
+   for (int i = 0; i < len; i++)
+   {
+       if (map[i].local_attname)
+           pfree(map[i].local_attname);
+       if (map[i].remote_attname)
+           pfree(map[i].remote_attname);
+   }

The if tests for local_attname/remote_attname should be assertions?

It took more time than expected to return to this issue.  My apologies for that.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-05-13T11:40:57Z

On Tue, May 12, 2026 at 8:36 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Fri, May 1, 2026 at 3:04 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> > The second gets rid of the string buffers entirely, and instead frees the nested object.
> >
> > I have no preference between the two, though I suspect that the nested-free solution will be preferred by the community.
>
> ISTM that the second solution is the right direction to go in.
>
> +static void
> +free_remattrmap(RemoteAttributeMapping *map, int len)
> +{
> +   if (!map)
> +       return;
> +
> +   for (int i = 0; i < len; i++)
> +   {
> +       if (map[i].local_attname)
> +           pfree(map[i].local_attname);
> +       if (map[i].remote_attname)
> +           pfree(map[i].remote_attname);
> +   }
>
> The if tests for local_attname/remote_attname should be assertions?

I mean that by the definition of build_remattrmap(), the
local_attname/remote_attname cannot be NULL if the given map isn't
NULL, so the if tests are just a waste of cycles.  So I modified the
patch as such.  Updated patch attached.  I also removed an extra
change made to RemoteAttributeMapping, which seemed irrelevant to this
issue to me.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-05-14T11:35:30Z

On Wed, May 13, 2026 at 8:40 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Tue, May 12, 2026 at 8:36 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Fri, May 1, 2026 at 3:04 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> > > The second gets rid of the string buffers entirely, and instead frees the nested object.
> > >
> > > I have no preference between the two, though I suspect that the nested-free solution will be preferred by the community.
> >
> > ISTM that the second solution is the right direction to go in.
> >
> > +static void
> > +free_remattrmap(RemoteAttributeMapping *map, int len)
> > +{
> > +   if (!map)
> > +       return;
> > +
> > +   for (int i = 0; i < len; i++)
> > +   {
> > +       if (map[i].local_attname)
> > +           pfree(map[i].local_attname);
> > +       if (map[i].remote_attname)
> > +           pfree(map[i].remote_attname);
> > +   }
> >
> > The if tests for local_attname/remote_attname should be assertions?
>
> I mean that by the definition of build_remattrmap(), the
> local_attname/remote_attname cannot be NULL if the given map isn't
> NULL, so the if tests are just a waste of cycles.  So I modified the
> patch as such.  Updated patch attached.  I also removed an extra
> change made to RemoteAttributeMapping, which seemed irrelevant to this
> issue to me.

I'll push the updated patch if no objections from you (or anyone else).

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-05-16T09:10:00Z

On Thu, May 14, 2026 at 8:35 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> I'll push the updated patch if no objections from you (or anyone else).

Pushed.  I closed the open item as well.

Best regards,
Etsuro Fujita

Re: Import Statistics in postgres_fdw before resorting to sampling.

Corey Huinker <corey.huinker@gmail.com> — 2026-05-26T20:31:47Z

On Sat, May 16, 2026 at 5:10 AM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

> On Thu, May 14, 2026 at 8:35 PM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> > I'll push the updated patch if no objections from you (or anyone else).
>
> Pushed.  I closed the open item as well.
>
> Best regards,
> Etsuro Fujita
>

My own apologies for a late reply. I agree that the second method was the
better fix, and I agree with your changes (making free_remattrmap tolerant
of null maps, and moving the if-tests to asserts).