Re: Statistics Import and Export

> * Doc build error and malformatting.
>

Looking into it.

> * I'm not certain that we want all changes to relation stats to be non-
> transactional. Are there transactional use cases? Should it be an
> option? Should it be transactional for pg_set_relation_stats() but non-
> transactional for pg_restore_relation_stats()?
>

It's non-transactional because that's how ANALYZE does it to avoid bloating
pg_class. We _could_ do it transactionally, but on restore we'd immediately
have a pg_class that was 50% bloat.

>
> * The documentation for the pg_set_attribute_stats() still refers to
> upgrade scenarios -- shouldn't that be in the
> pg_restore_attribute_stats() docs? I imagine the pg_set variant to be
> used for ad-hoc planner stuff rather than upgrades.
>

Noted.

>
> * For the "WARNING: stat names must be of type text" I think we need an
> ERROR instead. The calling convention of name/value pairs is broken and
> we can't safely continue.
>

They can't be errors, because any one error fails the whole pg_upgrade.

> * The huge list of "else if (strcmp(statname, mc_freqs_name) == 0) ..."
> seems wasteful and hard to read. I think we already discussed this,
> what was the reason we can't just use an array to map the arg name to
> an arg position type OID?
>

That was my overreaction to the dislike that the P_argname enum got in
previous reviews.

We'd need an array of struct like

argname (ex. "mc_vals")
argtypeoid (one of: int, text, real, rea[])
argtypename (name we want to call the argtypeoid (integer, text. real,
real[] about covers it).
argpos (position in the arg list of the corresponding pg_set_ function

>
> * How much error checking did we decide is appropriate? Do we need to
> check that range_length_hist is always specified with range_empty_frac,
> or should we just call that the planner's problem if one is specified
> and the other not? Similarly, range stats for a non-range type.
>

I suppose we can let that go, and leave incomplete stat pairs in there.

The big risk is that somebody packs the call with more than 5 statkinds,
which would overflow the struct.

> * I think most of the tests should be of pg_set_*_stats(). For
> pg_restore_, we just want to know that it's translating the name/value
> pairs reasonably well and throwing WARNINGs when appropriate. Then, for
> pg_dump tests, it should exercise pg_restore_*_stats() more completely.
>

I was afraid you'd suggest that, in which case I'd break up the patch into
the pg_sets and the pg_restores.

> * It might help to clarify which arguments are important (like
> n_distinct) vs not. I assume the difference is that it's a non-NULLable
> column in pg_statistic.
>

There are NOT NULL stats...now. They might not be in the future. Does that
change your opinion?

>
> * Some arguments, like the relid, just seem absolutely required, and
> it's weird to just emit a WARNING and return false in that case.

Again, we can't fail.Any one failure breaks pg_upgrade.

> * To clarify: a return of "true" means all settings were successfully
> applied, whereas "false" means that some were applied and some were
> unrecognized, correct? Or does it also mean that some recognized
> options may not have been applied?
>

True means "at least some stats were applied. False means "nothing was
modified".

> * pg_set_attribute_stats(): why initialize the output tuple nulls array
> to false? It seems like initializing it to true would be safer.
>

+1

>
> * please use a better name for "k" and add some error checking to make
> sure it doesn't overrun the available slots.
>

k was an inheritance from analzye.c, from whence the very first version was
cribbed. No objection to renaming.

> * the pg_statistic tuple is always completely replaced, but the way you
> can call pg_set_attribute_stats() doesn't imply that -- calling
> pg_set_attribute_stats(..., most_common_vals => ..., most_common_freqs
> => ...) looks like it would just replace the most_common_vals+freqs and
> leave histogram_bounds as it was, but it actually clears
> histogram_bounds, right? Should we make that work or should we document
> better that it doesn't?
>

That would complicate things. How would we intentionally null-out one stat,
while leaving others unchanged? However, this points out that I didn't
re-instate the re-definition that applied the NULL defaults.