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
signature.asc
Description: PGP signature
