public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Sami Imseih <[email protected]>
Cc: Bertrand Drouvot <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: POC: track vacuum/analyze cumulative time per relation
Date: Fri, 24 Jan 2025 16:32:45 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAA5RZ0su2Unc+iJkhL6QEfsbjj6-WXjRWyf2E0b4BotUH=o4FA@mail.gmail.com>
References: <[email protected]>
<CAA5RZ0tP6my7Mbr_syP7hRAiFANC_bG8SyoQF=-vfV9VawpV=g@mail.gmail.com>
<[email protected]>
<CAA5RZ0v9060p7hjtLPY9i0Me6SoaRKYwxATqt_DnZr8dGSnUhQ@mail.gmail.com>
<Z4ez/[email protected]>
<CAA5RZ0s8=wd-X4GgMcFQO1WbwPtw4HjTtMcDahsnegZ8p6tYPQ@mail.gmail.com>
<[email protected]>
<CAA5RZ0vKWKULGZ_V8DOOtFHXtsOLMVLCpkJ+guniB66T-o_eZA@mail.gmail.com>
<[email protected]>
<CAA5RZ0su2Unc+iJkhL6QEfsbjj6-WXjRWyf2E0b4BotUH=o4FA@mail.gmail.com>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (19+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: POC: track vacuum/analyze cumulative time per relation
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox