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

On Mon, Jan 20, 2025 at 11:04:40AM -0600, Sami Imseih wrote:
>> + 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.
>
> done, including grouping all the vacuum counters together.

I was referring to the order of the fields in the structure itself,
but that's no big deal one way or the other.

> See the attached v6.

+ PgStat_Counter total_vacuum_time; /* times in milliseconds */
+ PgStat_Counter total_autovacuum_time; /* times in milliseconds */
+ PgStat_Counter total_analyze_time; /* times in milliseconds */
+ PgStat_Counter total_autoanalyze_time; /* times in milliseconds */

This should be one comment for the whole block, or this should use the
singular for each comment.

instrument = (verbose || (AmAutoVacuumWorkerProcess() &&
params->log_min_duration >= 0));
+
if (instrument)

Some noise.

if (instrument)
{
- TimestampTz endtime = GetCurrentTimestamp();

On HEAD in the ANALYZE path, the endtime is calculated after
index_vacuum_cleanup(). Your patch calculates it before
index_vacuum_cleanup(), which would result in an incorrect calculation
if the index cleanup takes a long time with less logs generated, no?
Sorry for not noticing that earlier on the thread.. Perhaps it would
be just better to pass the start time to pgstat_report_vacuum() and
pgstat_report_analyze() then let their internals do the elapsed time
calculations. Consistency of the arguments for both functions is
something worth having, IMO, even if it means a bit more
GetCurrentTimestamp() in this case.
--
Michael