Horton

Re: Statistics Import and Export

Corey Huinker <corey.huinker@gmail.com>

View thread
From: Corey Huinker <corey.huinker@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>, Bruce Momjian <bruce@momjian.us>, Tom Lane <tgl@sss.pgh.pa.us>, Nathan Bossart <nathandbossart@gmail.com>, Magnus Hagander <magnus@hagander.net>, Stephen Frost <sfrost@snowman.net>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Peter Smith <smithpb2250@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Tomas Vondra <tomas.vondra@enterprisedb.com>, alvherre@alvh.no-ip.org
Date: 2024-07-20T01:58:33Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Change pg_dump default for statistics export.

  2. pg_dump: Adjust reltuples from 0 to -1 for dumps of older versions.

  3. vacuumdb: Don't skip empty relations in --missing-stats-only mode.

  4. pg_dump: Fix query for gathering attribute stats on older versions.

  5. Prevent redeclaration of typedef TocEntry.

  6. Remove unused function parameters in pg_backup_archiver.c.

  7. pg_dump: Retrieve attribute statistics in batches.

  8. pg_dump: Reduce memory usage of dumps with statistics.

  9. Skip second WriteToc() call for custom-format dumps without data.

  10. Add relallfrozen to pg_dump statistics.

  11. Matview statistics depend on matview data.

  12. Add pg_dump --with-{schema|data|statistics} options.

  13. Stats: use schemaname/relname instead of regclass.

  14. CREATE INDEX: do update index stats if autovacuum=off.

  15. Don't convert to and from floats in pg_dump.

  16. CREATE INDEX: don't update table stats if autovacuum=off.

  17. Organize and deduplicate statistics import tests.

  18. Address stats export review comments.

  19. Address stats import review comments.

  20. Add relallfrozen to pg_class

  21. Fix pg_strtof() to not crash on NULL endptr.

  22. Use attnum to identify index columns in pg_restore_attribute_stats().

  23. pg_dump: prepare attribute stats query.

  24. Avoid unnecessary relation stats query in pg_dump.

  25. Remove redundant pg_set_*_stats() variants.

  26. Do not use in-place updates for statistics import.

  27. Fix confusion about data type of pg_class.relpages and relallvisible.

  28. Documentation fixups for dumping statistics.

  29. Trial fix for old cross-version upgrades.

  30. Transfer statistics during pg_upgrade.

  31. Lock table in ShareUpdateExclusive when importing index stats.

  32. Use in-place updates for pg_restore_relation_stats().

  33. Improve error message for replication of generated columns.

  34. pg_dump: Add dumpSchema and dumpData derivative flags.

  35. Disallow modifying statistics on system columns.

  36. Add missing CommandCounterIncrement() in stats import functions.

  37. Add functions pg_restore_relation_stats(), pg_restore_attribute_stats().

  38. Documentation fixup.

  39. Add functions pg_set_attribute_stats() and pg_clear_attribute_stats().

  40. Change pg_*_relation_stats() functions to return type to void.

  41. Disable autovacuum for tables in stats import tests.

  42. Allow pg_set_relation_stats() to set relpages to -1.

  43. Fixup for pg_set_relation_stats().

  44. Create functions pg_set_relation_stats, pg_clear_relation_stats.

  45. Add memory/disk usage for Window aggregate nodes in EXPLAIN.

  46. Improve performance of dumpSequenceData().

  47. Add INJECTION_POINT_CACHED() to run injection points directly from cache

  48. Improve performance of binary_upgrade_set_pg_class_oids().

  49. Improve assertion in mdwritev()

  50. CREATE INDEX: do not update stats during binary upgrade.

  51. Redefine pg_class.reltuples to be -1 before the first VACUUM or ANALYZE.

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