On Sun, Jan 18, 2026 at 11:53 AM Corey Huinker <[email protected]> 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
