Re: Re: Extended Statistics set/restore/clear functions. - Mailing list pgsql-hackers

Indeed.  These exact typos existed in some of the previous iterations
of the patch posted on this thread.

1) statatt_get_type(), that retrieves a set of information from a
relation and one of its attributes.  This is lightly documented in
extended_statistics_update() from 0002, with an argument claiming that
we do the same as attribute statistics.  If one then looks at
attribute_stats.c, we have a not-really-helpful "derive information
from attribute".  The trick is that we have an implied dependency
here: statatt_get_type() needs to be called *before*
statatt_get_elem_type() for attribute *and* extended stats,
statatt_get_elem_type() being fed data from statatt_get_type().

2) statatt_get_elem_type(), interesting piece of the puzzle that acts
as a wrapper for the type cache.  An important part here, it seems to
me, would be to at least tell that this relies on data retrieved from
statatt_get_type(), initially.  Once I've noticed this dependency the
logic felt a bit cleaner to understand, but we have no docs explaining
any of that..

3) statatt_init_empty_tuple(), that initializes a set of data to be
used for updates and/or insertion.  My first thought here is about
...
not apply to extended stats.  But one would have to guess this fact
after knowing that this relates to the catalog pg_statistic..

4) statatt_set_slot is slightly simpler.  Still here, it is really easy
to miss that this routine should be fed data retrieved by
statatt_get_type().  staop can also be an eq or an lt operator.
stakind is one of the STATISTIC_KIND_* values, etc, etc.

As far as I can see, there is nothing in these functions that require
them to be located in attribute_stats.c anymore.  Let's move that to
stats_utils.c, common place for all the shared facilities used by
these stats modules.  Or it could also be a new, separate patch, for
the manipulation and extraction of the tuple data related to
the pg_statistic tuples.

Perhaps you know what these imply because you have written this code
originally in ce207d2a7901, but exposing them also means that it is
very important to document at least some concepts and assumptions
around them so as it is possible to guide somebody that may want to
use this code:
- What are the input arguments from?
- And what are the results?
- In which circumstances should these be used?

Then about 0002.

There are a bunch of assumptions embedded inside import_expressions()
that are interdependent with the calls of these attribute routines
...
each STATISTIC_KIND_*.  Too much is let to the reader to guess, making
the code complicated to maintain as written, at least IMHO.

Is there any need to locate these new functions and code in
extended_stats.c, actually?  A separation into a new file seems like
it would be a cleaner result, leaving extended_stats.c to deal with
the import of the new data, including the fetch and deletion of any
existing data in pg_stat_ext that would be updated or inserted.
Perhaps name that extended_stats_funcs.c, as these are about the
direct SQL functions, import and deletion.

For the tests, it looks like it would be better to have everything in
a new file, like a stats_ext_import.sql for the clear and restore
bits.

A lot of the tests are copy-pastes of surrounding queries.  Perhaps it
would be better to use a SQL function wrapper or an IN clause with
multiple relations?

  The patch has a lot of bloat with these repeated
queries..  Perhaps we should use a split for the elements in the
extended stats array instead of jsonb_pretty().  The results get quite
long here.

As of 0002, there are actually two independent pieces dealt with: the
deletion of extended stats with pg_clear_extended_stats(), and the
insert/update of extended stats with pg_restore_extended_stats().  I'd
suggest to split that into two parts, with the clear being first in
rank.  The deletion is simpler, and getting that in first simplifies
the review of the import part with less input to deal with.