Re: Import Statistics in postgres_fdw before resorting to sampling. - Mailing list pgsql-hackers

* I proposed the StatisticsAreImportable callback function, but the
ImportStatistics callback function is now allowed to fall back to
analyzing, so I don't think StatisticsAreImportable has much value
anymore.  I'll withdraw my proposal.  Attached is an updated patch
removing it, in which 0001 is merged into 0002 for ease of review
(0001 can't be tested without 0002).

* I modified analyze.c to start progress monitor a bit earlier to
monitor the work of ImportStatistics (and AnalyzeForeignTable) as part
of initialization.

* I noticed this odd behavior:

create table t1 (id int, data text);
create foreign table ft1 (id int, data text) server loopback options
(table_name 't1');

alter foreign table ft1 options (add fetch_stats 'false');
analyze ft1 (ctid);
ERROR:  column "ctid" of relation "ft1" does not exist

Looks good, but:

alter foreign table ft1 options (set fetch_stats 'true');
analyze ft1 (ctid);
NOTICE:  remote table public.t1 has no statistics to import
HINT:  Falling back to ANALYZE with regular row sampling.
ERROR:  column "ctid" of relation "ft1" does not exist

postgres_fdw is doing the remote access without any need to do so,
which isn't good.

The cause is that the patches fail to validate the va_cols list given
to analyze_rel before importing stats.  To fix, I modified analyze.c
to do so before calling ImportStatistics.  I think this is also useful
when analyzing inheritance trees, as it avoids validating the list
repeatedly in that case.

* I separated a bit of code in examine_attribute that checks whether
the given column is analyzable into a new extern function so that FDWs
can use it.

postgres_fdw side:

* In fetch_remote_statistics, if we get reltuples=0 for v14 or later,
I think we should update only the relation stats with that info, and
avoid resorting to analyzing, for efficiency, as I proposed before.  I
modified that function (and import_fetched_statistics) that way.

The root cause is that passing NULL to pg_restore_attribute_stats
leaves the stats unchanged.  To fix, I modified
import_fetched_statistics to do pg_clear_attribute_stats before the
restore function.

* The matching logic in match_attrmap seems correct to me, but I think
it's still complicated than necessary: you are doing a merge join of
the RemoteAttributeMapping array and the attstats set by placing the
latter in the outer side, but it makes it simpler to do the join the
opposite way (ie, the former in the outer side), like the attached,
which doesn't need an inner for-loop anymore, as the inner side (ie,
the attstats set) consists of *distinct* columns.

* I moved table_has_extended_stats to extended_stats.c and renamed it
to match other functions in it so that other FDWs too can use it.

* The 0002 patch is using different levels (LOG, NOTICE, and WARNING)
for logging in postgresImportStatisitcs, but I think it's appropriate
to use elevel or WARNING for it.  So I modified the patch to use
WARNING.  I modified the wording as well to match analyze.c.

* I added at the top/end of postgresImportStatisitcs log messages that
show the start/end of importing stats.

That's it.