Defects with invalid stats data for expressions in extended stats

Hi all,
(Adding Tomas in CC., as primary author of the feature dealt with
here, and Corey, due to his work for the restore of extstats.)

While looking at the proposed patch for the restore of extended
statistics on expressions, I have bumped into two defects that exist
since this feature has been introduced in v15. First, I have thought
that this was a problem only related to the proposed patch, but after
more analysis, I have found that this issue is independent, and can be
triggered without restoring any stats.

First, one thing that is important to know is that when defining an
extstat object with N expressions, what we finish by storing in the
catalogs is an array of N pg_statistics tuples. Some expressions can
have invalid data, symbolized by this code in extended_stats.c
if (!stats->stats_valid)
{
astate = accumArrayResult(astate,
(Datum) 0,
true,
typOid,
CurrentMemoryContext);
continue;
}

There is nothing wrong with that. Having N elements in the array of
pg_statistics tuples is a requirement, and the code clearly intends
that. There should be no more and no less elements, and this is used
as a marker to let the code that loads this catalog data that nothing
could be computed. This data is inserted when we run ANALYZE.

Some code paths are unfortunately not water-proof with this NULL-ness
handling, and I have found two of them as fixed by 0001.

1) When building statistics, lookup_var_attr_stats() has missed the
fact that computing stats for an expression could lead to invalid
stats being generated. examine_attribute() deals with this case by
returning NULL if either:
1-1) the typanalyze callback returns false,
1-2) The number of rows returned is negative.
1-3) For whatever reason in a custom type implementation, the
compute_stats callback is not set.
lookup_var_attr_stats() has been dealing with the case of invalid
stats for attributes, but it has missed the mark after calling
examine_attribute() for each expression. Note that
examine_attribute() exists in both extended_stats.c and analyze.c,
they are very close in shape, and need to rely on the same assumptions
in terms of what the typanalyze callback can return.
lookup_var_attr_stats() has two callers, both are able to deal with
NULL data (aka invalid stats). A consequence of this issue is a set
of NULL pointer dereferences for MCV, ndistinct and dependencies, as
all these code paths expect something to exist for each expression.
As there is no stats data, each of them would crash. At least one
needs to be specified when creating an extstat object.

2) When loading statistics, statext_expressions_load() missed that
some elements in the pg_statistics array could be NULL. This issue
can only be triggered if we have some invalid data stored in the
catalogs. This issue can be triggered on any branches with a
typanalyze callback that returns true, where stats_valid is set to
false when computing the stats (all the in-core callbacks set this
flag to true, nobody in their right mind would do that except me here,
I suspect). The restore of extended stats for expressions makes
this defect more easily reachable by *bypassing* the build, but as the
previous sentence describes it is *not* a mandatory requirement
depending on what a typanalyze callback does. Hence, we have patch it
in all the stable branches anyway. The code allows NULL data to exist
for some expressions, but the load does not cope with it. This is
reflected by fixing two code paths:
2-1) statext_expressions_load() needs to be able to load NULL data.
2-2) examine_variable() in selfuncs.c needs to lead with this possible
consequence.
All the callers of examine_variable() have an exit path in selfuncs.c
if there is no stats data available, assuming some defaults, in the
event where statsTuple is NULL (aka invalid). Note that it is
possible to reach this path with a typanalyze that returns true,
meaning that some data is available and that we store NULL data to be
stored, but the compute_stats callback has the idea to set stats_valid
to false. We never set stats_valid to false in any of the in-core
callbacks.

In order to demonstrate these two bugs, I have implemented a test
module called test_custom_types, as of 0002 (btree operator bloats the
module, it is required for extended stats), that includes a simple
custom type with two buggy typanalyze callbacks (one for the build
defect, and one for the load defect). I think that it would be a good
thing to backpatch this module with the main fix, so as we can cover
these fancy scenarios for all released branches. This could be
enlarged for more cases if we have more defects detected in the future
in this area of the code. This affects versions down to v15.

In order to finish the business with the restore of extended stats for
this release, these defects have to be addressed first. It does not
impact what has been already committed for the restore of extended
stats, fortunately: we have not touched the expressions yet and the
patch is still floating around in this CF.

I am registering this item in the final CF, as a bug fix. The
typanalyze requirements may sound it like something worth only fixing
on HEAD, but I don't really see a reason why back-branches should not
be fixed as well.

So, thoughts or comments?
--
Michael