> I've attached a revised patch reflecting this change, and it also includes
> the documentation.
Thanks fo the update!
I have some comments:
1/
+pgstat_report_skipped_vacuum_analyze(Oid relid, bits8 flags)
using bit8 is fine here, but I would have just used int. For this
case, it's just a matter of prefernace.
2/
+/* flags for pgstat_flush_backend() */
+#define PGSTAT_REPORT_SKIPPED_VACUUM (1 << 0) /* vacuum is skipped */
+#define PGSTAT_REPORT_SKIPPED_ANALYZE (1 << 1) /* analyze is skipped */
+#define PGSTAT_REPORT_SKIPPED_AUTOVAC (1 << 2) /* skipped
during autovacuum/autoanalyze */
+#define PGSTAT_REPORT_SKIPPED_ANY (PGSTAT_REPORT_SKIPPED_VACUUM |
PGSTAT_REPORT_SKIPPED_ANALYZE)
can we just have 4 flags, SKIPPED_VACUUM, SKIPPED_ANALYZE,
SKIPPED_AUTOVACUUM, SKIPPED_AUTOANALYZE,
which can then remove the nested if/else and makes the mapping more obvious
+ if (flags & PGSTAT_REPORT_SKIPPED_AUTOVAC)
+ {
+ if (flags & PGSTAT_REPORT_SKIPPED_VACUUM)
+ {
+ tabentry->last_skipped_autovacuum_time = ts;
+ tabentry->skipped_autovacuum_count++;
+ }
+ if (flags & PGSTAT_REPORT_SKIPPED_ANALYZE)
+ {
+ tabentry->last_skipped_autoanalyze_time = ts;
+ tabentry->skipped_autoanalyze_count++;
+ }
+ }
+ else
+ {
+ if (flags & PGSTAT_REPORT_SKIPPED_VACUUM)
+ {
+ tabentry->last_skipped_vacuum_time = ts;
+ tabentry->skipped_vacuum_count++;
+ }
+ if (flags & PGSTAT_REPORT_SKIPPED_ANALYZE)
+ {
+ tabentry->last_skipped_analyze_time = ts;
+ tabentry->skipped_analyze_count++;
+ }
+ }
3/
For the sake of consistency, can we rename the fields from
skipped_vacuum_count to vacuum_skipped_count, etc. ? to be similar
to fields like vacuum_count
4/
field documentation could be a bit better to match existing phrasing
For example, the timestamp fields:
- Last time a manual vacuum on this table was attempted but skipped due to
- lock unavailability (not counting <command>VACUUM FULL</command>)
+ The time of the last manual vacuum on this table that was skipped
+ due to lock unavailability (not counting <command>VACUUM FULL</command>)
and the counter fields
- Number of times vacuums on this table have been attempted but skipped
+ Number of times a manual vacuum on this table has been skipped
5/
Partitioned table asymmetry between vacuum_count and vacuum_skipped_count.
vacuum_count never increments on a the parenttable, because the parent is never
pocessed. On the other hand, if the manual VACUUM/ANALYZE is on the
parent table,
then we will skip all the children. So, we should still report the skip on the
parent table, but we should add a Notes section in the docs perhaps to
document this caveat?
6/
It would be nice to add a test for this, but this requires concurrency and I'm
not sure it's woth it.
Also, can you create a CF entry in
https://commitfest.postgresql.org/59/, please.
Thanks!
--
Sami Imseih
Amazon Web Services (AWS)
