Re: Import Statistics in postgres_fdw before resorting to sampling.

On Fri, Mar 13, 2026 at 4:42 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)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