Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 24 Jan 2025 16:32:45 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Sami Imseih <samimseih@gmail.com>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: POC: track vacuum/analyze cumulative time per relation
Message-ID: <Z5NCHYaTBML1Chfk@paquier.xyz>
References: <Z4E4PkpEAsB2SDjP@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0tP6my7Mbr_syP7hRAiFANC_bG8SyoQF=-vfV9VawpV=g@mail.gmail.com>
 <Z4U3hkWuEgByhGgJ@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0v9060p7hjtLPY9i0Me6SoaRKYwxATqt_DnZr8dGSnUhQ@mail.gmail.com>
 <Z4ez/l7kbFd6Ppmq@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0s8=wd-X4GgMcFQO1WbwPtw4HjTtMcDahsnegZ8p6tYPQ@mail.gmail.com>
 <Z4peXI1UtEl477DY@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0vKWKULGZ_V8DOOtFHXtsOLMVLCpkJ+guniB66T-o_eZA@mail.gmail.com>
 <Z43CHdLQvUWwTSrM@paquier.xyz>
 <CAA5RZ0su2Unc+iJkhL6QEfsbjj6-WXjRWyf2E0b4BotUH=o4FA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="/u52U/I5WAMPALk5"
Content-Disposition: inline
In-Reply-To: <CAA5RZ0su2Unc+iJkhL6QEfsbjj6-WXjRWyf2E0b4BotUH=o4FA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z5NCHYaTBML1Chfk%40paquier.xyz>
Precedence: bulk


--/u52U/I5WAMPALk5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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 =3D (verbose || (AmAutoVacuumWorkerProcess() &&
                               params->log_min_duration >=3D 0));
+
     if (instrument)

Some noise.

     if (instrument)
     {
-        TimestampTz endtime =3D 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

--/u52U/I5WAMPALk5
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmeTQh0ACgkQnvQgOdby
QH0TWA/5AeXPkaXn2n3imiwwcnaGAOFER6oPhptaZoCVz0ywlea2rxlFfHttj38b
9vDq40FgyxRvdQCb88dyW78Q+VXbV4atSTGr2IqZorNIQzSxLybE+b0I6n42no/H
cHwlDNmj39pvvDwctklYLN6GE5M+YAo4CiZoonpTaXQYBlO1V07L8k7fC5bhTgZk
mFDrLHgRJ8aVki91++cCU8UhL9Jfp5zrr9sDfdU8t5PMNnu3nchv8NHcEA8KicY3
sKVOpqnSYHHGj/+fTuiiqIkz4YOf0571AvL7uVx/Fj+dkwixni4QibnrKhQ+HVjb
ooZAyyf1SvQ++JEFTrk8avxOKd2bcNA+Y4aHwpdF3ITM5+IoBtxpSuLQ5PaAllX9
mXjHGFLPY1AoogxiD8+mCmCwnTZEPYlt9fMEXXzqxnoAjI7C7+xPAxyb2Ha3XfA4
fqSoeYN4JXOKweOAeUUEKZS69fQNu/wK7kwZf6tSJNVIuOUt1a2fS/Lnk11p+0Qy
jmxCCrJBqIOKOSaqSTtG+C/Du3C6ABv/729ZUY8Hc9YGISWjnZJNPACsMl0G6P+w
dltQl02cpbU1hWGc+5VARsECsiub9e0dadDBsvOJOWsFhi2Ot8rFZYr/NZ2R7y7R
TlzjaXD8i+0lQUUhtZELLC3vUPTdiC1DLSKkFQ+bU60vO2DpEio=
=zss1
-----END PGP SIGNATURE-----

--/u52U/I5WAMPALk5--