Horton

Re: Statistics Import and Export

Stephen Frost <sfrost@snowman.net>

View thread
From: Stephen Frost <sfrost@snowman.net>
To: Corey Huinker <corey.huinker@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>, 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>
Date: 2024-03-11T10:00:32Z
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.

Greetings,

* Corey Huinker (corey.huinker@gmail.com) wrote:
> > > +/*
> > > + * Set statistics for a given pg_class entry.
> > > + *
> > > + * pg_set_relation_stats(relation Oid, reltuples double, relpages int)
> > > + *
> > > + * This does an in-place (i.e. non-transactional) update of pg_class,
> > just as
> > > + * is done in ANALYZE.
> > > + *
> > > + */
> > > +Datum
> > > +pg_set_relation_stats(PG_FUNCTION_ARGS)
> > > +{
> > > +     const char *param_names[] = {
> > > +             "relation",
> > > +             "reltuples",
> > > +             "relpages",
> > > +     };
> > > +
> > > +     Oid                             relid;
> > > +     Relation                rel;
> > > +     HeapTuple               ctup;
> > > +     Form_pg_class   pgcform;
> > > +
> > > +     for (int i = 0; i <= 2; i++)
> > > +             if (PG_ARGISNULL(i))
> > > +                     ereport(ERROR,
> > > +
> >  (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > > +                                      errmsg("%s cannot be NULL",
> > param_names[i])));
> >
> > Why not just mark this function as strict..?  Or perhaps we should allow
> > NULLs to be passed in and just not update the current value in that
> > case?
> 
> Strict could definitely apply here, and I'm inclined to make it so.

Having thought about it a bit more, I generally like the idea of being
able to just update one stat instead of having to update all of them at
once (and therefore having to go look up what the other values currently
are...).  That said, per below, perhaps making it strict is the better
plan.

> > Also, in some cases we allow the function to be called with a
> > NULL but then make it a no-op rather than throwing an ERROR (eg, if the
> > OID ends up being NULL).
> 
> Thoughts on it emitting a WARN or NOTICE before returning false?

Eh, I don't think so?

Where this is coming from is that we can often end up with functions
like these being called inside of larger queries, and having them spit
out WARN or NOTICE will just make them noisy.

That leads to my general feeling of just returning NULL if called with a
NULL OID, as we would get with setting the function strict.

> >   Not sure if that makes sense here or not
> > offhand but figured I'd mention it as something to consider.
> >
> > > +     pgcform = (Form_pg_class) GETSTRUCT(ctup);
> > > +     pgcform->reltuples = PG_GETARG_FLOAT4(1);
> > > +     pgcform->relpages = PG_GETARG_INT32(2);
> >
> > Shouldn't we include relallvisible?
> 
> Yes. No idea why I didn't have that in there from the start.

Ok.

> > Also, perhaps we should use the approach that we have in ANALYZE, and
> > only actually do something if the values are different rather than just
> > always doing an update.
> 
> That was how it worked back in v1, more for the possibility that there was
> no matching JSON to set values.
> 
> Looking again at analyze.c (currently lines 1751-1780), we just check if
> there is a row in the way, and if so we replace it. I don't see where we
> compare existing values to new values.

Well, that code is for pg_statistic while I was looking at pg_class (in
vacuum.c:1428-1443, where we track if we're actually changing anything
and only make the pg_class change if there's actually something
different):

vacuum.c:1531
    /* If anything changed, write out the tuple. */
    if (dirty)
        heap_inplace_update(rd, ctup);

Not sure why we don't treat both the same way though ... although it's
probably the case that it's much less likely to have an entire
pg_statistic row be identical than the few values in pg_class.

> > > +Datum
> > > +pg_set_attribute_stats(PG_FUNCTION_ARGS)
> >
> > > +     /* names of columns that cannot be null */
> > > +     const char *required_param_names[] = {
> > > +             "relation",
> > > +             "attname",
> > > +             "stainherit",
> > > +             "stanullfrac",
> > > +             "stawidth",
> > > +             "stadistinct",
> > > +             "stakind1",
> > > +             "stakind2",
> > > +             "stakind3",
> > > +             "stakind4",
> > > +             "stakind5",
> > > +     };
> >
> > Same comment here as above wrt NULL being passed in.
> 
> In this case, the last 10 params (stanumbersN and stavaluesN) can be null,
> and are NULL more often than not.

Hmm, that's a valid point, so a NULL passed in would need to set that
value actually to NULL, presumably.  Perhaps then we should have
pg_set_relation_stats() be strict and have pg_set_attribute_stats()
handles NULLs passed in appropriately, and return NULL if the relation
itself or attname, or other required (not NULL'able) argument passed in
cause the function to return NULL.

(What I'm trying to drive at here is a consistent interface for these
functions, but one which does a no-op instead of returning an ERROR on
values being passed in which aren't allowable; it can be quite
frustrating trying to get a query to work where one of the functions
decides to return ERROR instead of just ignoring things passed in which
aren't valid.)

> > > +     for (int k = 0; k < 5; k++)
> >
> > Shouldn't we use STATISTIC_NUM_SLOTS here?
> 
> Yes, I had in the past. Not sure why I didn't again.

No worries.

Thanks!

Stephen