> > What is the defense of making all these warnings rather than errors? It's > one > thing to e.g. warn that analyze skipped a relation due to locks, but doing > some catalog updates but not doing everything that the catalog updates > depended on seems like a really bad idea. Transactions exist for a > reason... >
This is more an explanation than a defense, but here it goes... It is that way because that's what the existing statistics import functions (relation and attribute) do: they treat certain errors (cannot find that table/column, cannot modify stats while in recovery, etc) as regular errors, but in the case of the statistical values parameters they attempt to make sense of them, and if some of the stats do not make sense (example: most common values provided but most common frequencies not _also_ provided) it issues a warning and bypasses that stat, while still trying to set other stat values for that column. That was the desired behavior for pg_upgrade/pg_restore, where we didn't want to stop the whole operation because the stats values somehow didn't line up with expectations of a newer version. A bit of historical trivia, there were patches that had two different functions that did treat everything as an error, those were the pg_set_* functions, but those were discarded in the development process in favor of just using the pg_restore_* functions. Each restore function does, however, return whether or not the import of stats went flawlessly or not, and this flag allows stats import for foreign data wrapper tables to fall back to sampling if the stats were less than immaculate. In the normal run of things, these bad remote stats would be rejected, and the bad remote data would have also failed, exposing that the schema of the foreign table had violated first normal form as well as the recommended practice of making the foreign table columns identical to the remote table column. Had the remote table user done a remediation that left at least some rows in the table, then the table sample would have overwritten the stats rows that were written, but in this corner case no rows remained in the table and do_analyze_rel() decides to leave the existing stats as-is, oblivious to the changes already made. That's the explanation, what follows is a recap of options of what we can do in the future. That decision of the analyze.c code to leave existing stats as-is when it gets an empty table sample is curious to me, as I'm not sure how ANALYZE could ever reflect when a foreign table is actually empty once it has been populated at least once, and if that's genuinely the case then perhaps we should address that, but that would have implications outside of this feature, so I'm highly reluctant to do that. I can foresee several possible courses of action if we choose to reopen this item. 1. Do nothing, as this is a corner case resulting from a misdesigned foreign table and a remote table in an explicitly unsupported state (modified to empty but not analyzed), and the situation will resolve itself when the remote table is repopulated, or analyzed, or the column data types are brought into alignment, whichever comes first. 2. Consider whether do_analyze_rel should do something (like clear the pg_statistic rows for the relation) in the case where numrows returned from the acquirefunc is zero. If I'm reading it correctly, there's no way for ANALYZE to set stats on a truly empty table, though I can see where it would make practical sense to assume that a truly empty table is only temporarily empty, and query plans for a table with rows in it when the stats stay it's empty are much worse than the query plans for an empty table that the stats say still has rows, and thus keep the known bad stats around because they're likely to be fixed soonish. 3. Add in a subtransaction and rollback like Nikolai's patch did, though oddly enough we wouldn't want the try/catch part of it, because the decision to roll back lies entirely with the boolean result from the function, and we would not want the actual ERROR-level errors to be caught. 4. Do a substransaction but inside analyze_rel(), as we'd want this behavior for all FDWs that do stats import, not just postgres_fdw. 5. Instead of a subtransaction, we could instead opt to clear the pg_statistics for the relation and zero out the relpages/reltuples, basically presuming that the table is empty until told otherwise by the next step, which regular sampling. If sampling succeeds, then we get all of our relation/attribute stats back, and if it doesn't then there is something preventing regular querying of the table, so the 0 rows reported isn't that far off. Option 1 is where we're at now. It leaves open the chance of inconsistent stats data being shown for a table designed in a such a way that ordinary queries can generate errors AND someone has taken steps to hinder its remediation. Option 2 requires more investigation to determine why we opt to leave stats as is when we get a numrows = 0 sample. Is it because we assume that all methods of emptying a table already wipe out stats (e.g. truncate)? Is it because we're hoping the table gets repopulated soonish? The viability of this option depends on the answers to those questions. Option 3 has the most control as it allows us to start the subtransaction only when we are at risk of actually wanting to roll back rows, and it gives us the most chances to avoid the overhead of the subtransaction (we'd only start it once we knew that the table had the option enabled and we successfully fetched stats data for all the columns), but it's still subtransaction overhead to cover a case that shouldn't happen and whose negative consequences are minimal. Option 4 feels cleaner than Option 3, but it currently lacks the controls to see whether the table in question is configured to import stats, so we'd be incurring subtransaction overhead for all tables from FDWs that support the feature whether or not the table is using the feature. Option 5 avoids the subtransaction, which removes the overhead from properly configured tables and all other situations that don't match the corner case. The only downside is that we have to document the change in behavior.
