Further cleanup related to statistics import support in postgres_fdw

Hi,

On Wed, Aug 19, 2026 at 4:44 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Here is a patch for $SUBJECT. Changes are:

* Reorder struct definitions for readability.
* Reorder arguments for some functions for consistency.
* Reorder the conditions in an if-else block for efficiency/readability.

I'm not sure how much the reordering helps. It's hard to keep the
order in this form with new additions in the future. But if others
think otherwise, I'm fine with it.

* Rename a variable to match other places.
* Fix typos in comments.

These seem fine to me and can go separately.

* Add/Tweak some comments/docs for clarity.

A few comments:

1/
+/* Result sets that are returned from a foreign statistics scan */
+typedef struct
+{
+ PGresult   *rel; /* result for relation stats query */
+ PGresult   *att; /* result for attribute stats query */
+ double livetuples; /* livetuples estimates, for pgstat report */
+ double deadtuples; /* deadtuples estimates, for pgstat report */
+ int version; /* version of remote server */
+} RemoteStatsResults;
+
+/* Pairs of remote columns with local columns */
+typedef struct
+{
+ AttrNumber local_attnum; /* attribute number of local column */
+ char   *local_attname; /* attribute name of local column */
+ char   *remote_attname; /* attribute name of remote column */

The column names themselves are readable IMO, but having the comments
is fine by me.

2/
- /* Fetch relation stats. */
+ /* Fetch relation statistics. */
  remstats->rel = relstats = fetch_relstats(conn, relation);

 /*
- * Attempt to fetch remote attribute stats.
+ * Attempt to fetch remote attribute statistics.
  */
 static PGresult *
 fetch_attstats(PGconn *conn, int server_version_num,

These are fine as-is IMO, the use of "stats" is widespread in the code comments.

3/

- * Assume the remote schema/relation names are the same as the local name
+ * Assume the remote schema/table names are the same as the local name

Using "table names" is fine because "relation" in general includes
indexes and others.

3/
+/*
+ * Determine whether the column is analyzable.
+ *
+ * If the column is analyzable, its attstattarget value is returned into the
+ * output parameter p_attstattarget if it isn't NULL.
+ */
 bool
 attribute_is_analyzable(Relation onerel, int attnum, Form_pg_attribute attr,

How about just saying "If the column is analyzable, get its
attstattarget value if asked" without describing what the code does in
detail.

I think the changes need to be backported to PG19 to reduce the version diff.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Hi Bharath,

On Thu, Aug 20, 2026 at 3:44 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Aug 19, 2026 at 4:44 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Here is a patch for $SUBJECT. Changes are:

* Reorder struct definitions for readability.
* Reorder arguments for some functions for consistency.
* Reorder the conditions in an if-else block for efficiency/readability.

I'm not sure how much the reordering helps. It's hard to keep the
order in this form with new additions in the future. But if others
think otherwise, I'm fine with it.

Let's be careful to keep the order in the future development,
especially for the third one, the point of which is to put the if-test
"reltuples > 0" first, as it is true for most cases.

* Rename a variable to match other places.
* Fix typos in comments.

These seem fine to me and can go separately.

Cool! I will push/backpatch the typo-fix part separately. The
renaming part is a code change, so I'd like to merge it into other
code changes.

* Add/Tweak some comments/docs for clarity.

A few comments:

1/
+/* Result sets that are returned from a foreign statistics scan */
+typedef struct
+{
+ PGresult   *rel; /* result for relation stats query */
+ PGresult   *att; /* result for attribute stats query */
+ double livetuples; /* livetuples estimates, for pgstat report */
+ double deadtuples; /* deadtuples estimates, for pgstat report */
+ int version; /* version of remote server */
+} RemoteStatsResults;
+
+/* Pairs of remote columns with local columns */
+typedef struct
+{
+ AttrNumber local_attnum; /* attribute number of local column */
+ char   *local_attname; /* attribute name of local column */
+ char   *remote_attname; /* attribute name of remote column */

The column names themselves are readable IMO, but having the comments
is fine by me.

Check.

2/
- /* Fetch relation stats. */
+ /* Fetch relation statistics. */
remstats->rel = relstats = fetch_relstats(conn, relation);

/*
- * Attempt to fetch remote attribute stats.
+ * Attempt to fetch remote attribute statistics.
*/
static PGresult *
fetch_attstats(PGconn *conn, int server_version_num,

These are fine as-is IMO, the use of "stats" is widespread in the code comments.

Ok, I will refrain from changing.

3/

- * Assume the remote schema/relation names are the same as the local name
+ * Assume the remote schema/table names are the same as the local name

Using "table names" is fine because "relation" in general includes
indexes and others.

Cool!

3/
+/*
+ * Determine whether the column is analyzable.
+ *
+ * If the column is analyzable, its attstattarget value is returned into the
+ * output parameter p_attstattarget if it isn't NULL.
+ */
bool
attribute_is_analyzable(Relation onerel, int attnum, Form_pg_attribute attr,

How about just saying "If the column is analyzable, get its
attstattarget value if asked" without describing what the code does in
detail.

Seems like a good idea. Will change.

I think the changes need to be backported to PG19 to reduce the version diff.

Agreed. Will do.

Thanks for reviewing!

Best regards,
Etsuro Fujita

On Thu, Aug 20, 2026 at 8:55 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

On Thu, Aug 20, 2026 at 3:44 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Aug 19, 2026 at 4:44 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Here is a patch for $SUBJECT. Changes are:

* Rename a variable to match other places.
* Fix typos in comments.

These seem fine to me and can go separately.

Cool! I will push/backpatch the typo-fix part separately. The
renaming part is a code change, so I'd like to merge it into other
code changes.

I pushed/backpatched the typo fix.

* Add/Tweak some comments/docs for clarity.

A few comments:

2/
- /* Fetch relation stats. */
+ /* Fetch relation statistics. */
remstats->rel = relstats = fetch_relstats(conn, relation);

/*
- * Attempt to fetch remote attribute stats.
+ * Attempt to fetch remote attribute statistics.
*/
static PGresult *
fetch_attstats(PGconn *conn, int server_version_num,

These are fine as-is IMO, the use of "stats" is widespread in the code comments.

Ok, I will refrain from changing.

Removed.

3/
+/*
+ * Determine whether the column is analyzable.
+ *
+ * If the column is analyzable, its attstattarget value is returned into the
+ * output parameter p_attstattarget if it isn't NULL.
+ */
bool
attribute_is_analyzable(Relation onerel, int attnum, Form_pg_attribute attr,

How about just saying "If the column is analyzable, get its
attstattarget value if asked" without describing what the code does in
detail.

Seems like a good idea. Will change.

Modified. I tweaked the wording a little bit, though.

Attached is an updated version of the patch.

Best regards,
Etsuro Fujita

Hi,

On Fri, Aug 21, 2026 at 3:04 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Attached is an updated version of the patch.

The changes look good to me. pgindent wasn't happy about one thing
[1]: dev-dsk-rupiredd-2b-b0ec5a8e % git diff diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 07a80bf9365..bbadd58d893 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -393,7 +393,7 @@ typedef struct { AttrNumber local_attnum; /* attribute number of local column */ char *local_attname; /* attribute name of local column */ - char *remote_attname; /* attribute name of remote column */ + char *remote_attname; /* attribute name of remote column */ int res_index; /* index of row in attribute stats result */ } RemoteAttributeMapping;
being discussed in [2]/messages/by-id/CALj2ACVd3uEt9MpNT1zGYjOOXTFLB5qdNcr50YXyEAG=Go9TKQ@mail.gmail.com).

I think this also needs to be backpatched (unless others think
otherwise) because this code is new in PG19 and it's better to keep
the version diff to a minimum.

[1]
dev-dsk-rupiredd-2b-b0ec5a8e % git diff
diff --git a/contrib/postgres_fdw/postgres_fdw.c
b/contrib/postgres_fdw/postgres_fdw.c
index 07a80bf9365..bbadd58d893 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -393,7 +393,7 @@ typedef struct
 {
        AttrNumber      local_attnum;   /* attribute number of local column */
        char       *local_attname;      /* attribute name of local column */
-       char       *remote_attname;     /* attribute name of remote column */
+       char       *remote_attname; /* attribute name of remote column */
        int                     res_index;              /* index of
row in attribute stats result */
 } RemoteAttributeMapping;

[2]: /messages/by-id/CALj2ACVd3uEt9MpNT1zGYjOOXTFLB5qdNcr50YXyEAG=Go9TKQ@mail.gmail.com

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Hi Bharath,

On Sat, Aug 22, 2026 at 5:40 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Aug 21, 2026 at 3:04 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Attached is an updated version of the patch.

The changes look good to me. pgindent wasn't happy about one thing
[1], make check-world is fine (except a known issue in postgres_fdw
being discussed in [2]).

Thanks for looking! I fixed the indentation. Attached is an updated
patch for that.

(As for the regression test issue, I couldn't reproduce it in my
environment, so I didn't do anything about it. I want to leave it to
Alexander. I think he is probably busy with other stuff right now,
though.)

I think this also needs to be backpatched (unless others think
otherwise) because this code is new in PG19 and it's better to keep
the version diff to a minimum.

Agreed. I will push/backpatch the patch if no objections from others.

Best regards,
Etsuro Fujita

Hi,

On Sat, Aug 22, 2026 at 3:26 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

The changes look good to me. pgindent wasn't happy about one thing
[1], make check-world is fine (except a known issue in postgres_fdw
being discussed in [2]).

Thanks for looking! I fixed the indentation. Attached is an updated
patch for that.

I think this also needs to be backpatched (unless others think
otherwise) because this code is new in PG19 and it's better to keep
the version diff to a minimum.

Agreed. I will push/backpatch the patch if no objections from others.

V3 patch LGTM. Thanks.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Hi Bharath,

On Sun, Aug 23, 2026 at 5:58 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Sat, Aug 22, 2026 at 3:26 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

I will push/backpatch the patch if no objections from others.

V3 patch LGTM. Thanks.

Done after doing a bit more cleanup.

Thanks for looking!

Best regards,
Etsuro Fujita

I added Corey in CC, who is the author of this feature. I should have
done this from the beginning. Corey, sorry for that.

I continued to self-re-review the commit, and I found issues (by hand)
that can lead to plan changes depending on the ANALYZE method.

create table pt (a int, b int) partition by list (a);
create table p1 partition of pt for values in (1);
create table p2 partition of pt for values in (2);
insert into pt select 1, i from generate_series(1, 1000) i;
insert into pt select 2, i from generate_series(1, 1000) i;
create foreign table fpt (a int, b int) server loopback options
(table_name 'pt');
analyze pt;
analyze fpt;
select relpages, reltuples from pg_class where relname = 'fpt';
relpages | reltuples
----------+-----------
0 | 2000
(1 row)

alter foreign table fpt options (add import_stats 'true');
analyze fpt;
select relpages, reltuples from pg_class where relname = 'fpt';
relpages | reltuples
----------+-----------
-1 | 2000
(1 row)

The normal method produces relpages = 0, whereas the import method
produces relpages = -1. The reason is that to produce it, the former
uses pg_relation_size(), which returns 0 for a partition root, but the
latter imports relpages as-is from the remote pg_class. As relpages
is used for costing foreign paths, this divergence can lead to the
plan changes.

Another example is:

create table parent (a int, b int);
create table child (a int, b int) inherits (parent);
insert into parent select 1, i from generate_series(1, 1000) i;
insert into child select 2, i from generate_series(1, 1000) i;
create foreign table fparent (a int, b int) server loopback options
(table_name 'parent');
analyze parent;
analyze fparent;
select relpages, reltuples from pg_class where relname = 'fparent';
relpages | reltuples
----------+-----------
5 | 2000
(1 row)

alter foreign table fparent options (add import_stats 'true');
analyze fparent;
select relpages, reltuples from pg_class where relname = 'fparent';
relpages | reltuples
----------+-----------
5 | 1000
(1 row)

Both methods produce the same relpages, but not for reltuples. The
reason is that in the latter, the reltuples estimate, which is
imported as-is from the remote pg_class as in the case of relpages, is
just the one for the inheritance parent, not for the inheritance set.
reltuples is also used for costing foreign paths, so this can also
lead to the plan changes.

To fix, for the partitioning case, I modified postgres_fdw to import
relpages = 0, to match the normal case. For the inheritance case, we
could calculate the reltuples estimate for the inheritance set by
fetching those for child tables as well, but 1) changes for that isn't
small, and 2) IMO table inheritance hasn't been used that much these
days, so I just disabled the import method in that case.

Attached is a patch for that.

(The normal method's relpages estimation in these cases is also
broken, as pg_relation_size() just returns the size of the parent
table, not the total size of the partitioned/inherited table. But
that is another issue, so I'd like to leave it for future work.)

Best regards,
Etsuro Fujita

On Sun, Aug 30, 2026 at 8:50 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Attached is a patch for that.

I simplified the patch, fixing an oversight. I also revisited the
existing code, to remove redundant string-to-number conversion of
reltuples in fetch_remote_statistics() and
import_fetched_statistics(). Updated patch attached.

Best regards,
Etsuro Fujita

On Mon, Aug 31, 2026 at 8:22 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Updated patch attached.

Here is a new version of the patch, in which I fixed indentation after
tweaking comments/docs a little bit for clarity/readability. I will
push/backpatch this version if no objections.

Best regards,
Etsuro Fujita

On Thu, Sep 3, 2026 at 7:45 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Here is a new version of the patch, in which I fixed indentation after
tweaking comments/docs a little bit for clarity/readability. I will
push/backpatch this version if no objections.

Done.

Best regards,
Etsuro Fujita

On Fri, Sep 4, 2026 at 6:51 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

On Thu, Sep 3, 2026 at 7:45 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Here is a new version of the patch, in which I fixed indentation after
tweaking comments/docs a little bit for clarity/readability. I will
push/backpatch this version if no objections.

Done.

After the patch was committed, I observed that stats import could fail with
the following warning even though the remote table is no longer
inherited. Is this behavior intentional?

WARNING: could not import statistics for foreign table
"public.ft" --- remote table "public.t" is inherited

Here is the procedure to reproduce this situation. The point is that
the inheritance table is created and then dropped:

--------------------------------
CREATE TABLE t AS SELECT i FROM generate_series(1, 100) i;
ANALYZE t;
CREATE TABLE tt () INHERITS (t);
DROP TABLE tt;

CREATE EXTENSION postgres_fdw;
CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw;
CREATE USER MAPPING FOR public SERVER loopback;
CREATE FOREIGN TABLE ft (i int) SERVER loopback OPTIONS (table_name
't', import_stats 'true');

ANALYZE VERBOSE ft;
WARNING:  could not import statistics for foreign table "public.ft"
--- remote table "public.t" is inherited
--------------------------------

Regards,

--
Fujii Masao

On Fri, Sep 4, 2026 at 8:34 PM Fujii Masao <masao.fujii@gmail.com> wrote:

After the patch was committed, I observed that stats import could fail with
the following warning even though the remote table is no longer
inherited. Is this behavior intentional?

WARNING: could not import statistics for foreign table
"public.ft" --- remote table "public.t" is inherited

Here is the procedure to reproduce this situation. The point is that
the inheritance table is created and then dropped:

--------------------------------
CREATE TABLE t AS SELECT i FROM generate_series(1, 100) i;
ANALYZE t;
CREATE TABLE tt () INHERITS (t);
DROP TABLE tt;

CREATE EXTENSION postgres_fdw;
CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw;
CREATE USER MAPPING FOR public SERVER loopback;
CREATE FOREIGN TABLE ft (i int) SERVER loopback OPTIONS (table_name
't', import_stats 'true');

ANALYZE VERBOSE ft;
WARNING:  could not import statistics for foreign table "public.ft"
--- remote table "public.t" is inherited
--------------------------------

This is expected behavior, because postgres_fdw determines whether the
remote table is inherited or not, by checking only the table's
relhassubclass. We could check pg_inherits as well, to detect false
positives like this, but I didn't do so, because I don't think that
that is worth complicating the code, as table inheritance is not that
popular these days in the first place. Should we? I think another
option would be to add a note about this behavior.

Thanks for testing this!

Best regards,
Etsuro Fujita

On Fri, Sep 4, 2026 at 11:25 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

This is expected behavior, because postgres_fdw determines whether the
remote table is inherited or not, by checking only the table's
relhassubclass. We could check pg_inherits as well, to detect false
positives like this, but I didn't do so, because I don't think that
that is worth complicating the code, as table inheritance is not that
popular these days in the first place. Should we? I think another
option would be to add a note about this behavior.

We are really nearing the end of the beta stabilization phase, I feel
like just adding the note like this:

"Note that if the remote table was inherited in the past, postgres_fdw
currently considers it inherited even if it is not currently
inherited. This restriction might be fixed in a future release."

Best regards,
Etsuro Fujita

On Sat, Sep 5, 2026 at 1:23 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

We are really nearing the end of the beta stabilization phase, I feel
like just adding the note like this:

"Note that if the remote table was inherited in the past, postgres_fdw
currently considers it inherited even if it is not currently
inherited. This restriction might be fixed in a future release."

I expanded this to something like the attached.

Best regards,
Etsuro Fujita

On Sun, Sep 6, 2026 at 7:45 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

On Sat, Sep 5, 2026 at 1:23 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

We are really nearing the end of the beta stabilization phase, I feel
like just adding the note like this:

"Note that if the remote table was inherited in the past, postgres_fdw
currently considers it inherited even if it is not currently
inherited. This restriction might be fixed in a future release."

I expanded this to something like the attached.

Upon reconsideration, I think it's better to adjust the WARNING
message like this, rather than just adding the lengthy note.

                ereport(WARNING,
                                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                               errmsg("could not import statistics
for foreign table \"%s.%s\" --- remote table \"%s.%s\" is inherited",
+                               errmsg("could not import statistics
for foreign table \"%s.%s\" --- remote table \"%s.%s\" is (or was
once) inherited",
                                           local_schemaname, local_relname,
                                           remote_schemaname, remote_relname));

Attached is an updated patch for that, in which I simplified a near-by
code comment as well as that note. Comments welcome!

Best regards,
Etsuro Fujita

Attached is an updated patch for that, in which I simplified a near-by
code comment as well as that note. Comments welcome!

Best regards,
Etsuro Fujita

+1

On Wed, Sep 9, 2026 at 1:41 AM Corey Huinker <corey.huinker@gmail.com> wrote:

Attached is an updated patch for that, in which I simplified a near-by
code comment as well as that note. Comments welcome!

+1

Cool! Pushed/Backpatched after tweaking comments/docs a little bit.

Thanks for looking!

Best regards,
Etsuro Fujita