Re: Further cleanup related to statistics import support in postgres_fdw

Hi,

On Wed, Aug 19, 2026 at 4:44 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)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