Re: [PG19][PATCH] Make postgres_fdw statistics import atomic

>
> 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.