Re: POC: track vacuum/analyze cumulative time per relation

On Fri, Jan 17, 2025 at 09:42:13AM -0600, Sami Imseih wrote:
> Indeed. Corrected in v5

I can get behind the idea of the patch.

+ PgStat_Counter total_vacuum_time;
+ PgStat_Counter total_autovacuum_time;
+ PgStat_Counter total_analyze_time;
+ PgStat_Counter total_autoanalyze_time;
} PgStat_StatTabEntry;

These are time values in milliseconds. It would be good to document
that as a comment with these fields, like PgStat_CheckpointerStats and
others. Perhaps they should be grouped with their related counters
within PgStat_StatTabEntry? Or not.

The patch makes sense here in terms of passing down to
pgstat_report_vacuum() the elapsed time and the end time to avoid an
extra GetCurrentTimestamp() when calculating if the logging should
happen. However in analyze.c..

+++ b/src/backend/commands/analyze.c
[...]
+ TimestampTz endtime = 0;
[...]
- TimestampTz endtime = GetCurrentTimestamp();
-
if (verbose || params->log_min_duration == 0 ||
TimestampDifferenceExceeds(starttime, endtime,

This is incorrect, endtime is never set. I would give priority to the
symmetry of the code here, using for the analyze reporting the same
arguments as the vacuum bits, so as we don't call GetCurrentTimestamp
in the proposed changes for pgstat_report_analyze().

+ if (AmAutoVacuumWorkerProcess())
+ tabentry->total_autovacuum_time += elapsedtime;
+ else
+ tabentry->total_vacuum_time += elapsedtime;
[...]
+ if (AmAutoVacuumWorkerProcess())
+ tabentry->total_autoanalyze_time += elapsedtime;
+ else
+ tabentry->total_analyze_time += elapsedtime;

These could be grouped with their previous blocks.
--
Michael