Re: Further cleanup related to statistics import support in postgres_fdw

Hi Bharath,

On Thu, Aug 20, 2026 at 3:44 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> 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.

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