Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 20 Jan 2025 12:25:17 +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: <Z43CHdLQvUWwTSrM@paquier.xyz>
References: <CAA5RZ0uVOGBYmPEeGF2d1B_67tgNjKx_bKDuL+oUftuoz+=Y1g@mail.gmail.com>
 <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="/zk1wQqtB809JthL"
Content-Disposition: inline
In-Reply-To: <CAA5RZ0vKWKULGZ_V8DOOtFHXtsOLMVLCpkJ+guniB66T-o_eZA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z43CHdLQvUWwTSrM%40paquier.xyz>
Precedence: bulk


--/zk1wQqtB809JthL
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmeNwh0ACgkQnvQgOdby
QH07MQ//V9f232Lnk0dYQ4rtdcPRnPEQmNmKkV2UgbK5+d7PRyCGIzUXIaHq+/ZK
PhZnTHql8FSpoWyg0dF2kmNX/I+SAVW9dF9HAfyfq4bv0vWOfdlwryOVF1n4ygEm
AUuOUjYn+I7GyiLjKdd25EyaZoVPgnMVtk5OvfOyoz6U0pa+ua01Gr5BDXY3XeFl
WnYebeu4sW47S8pJ2lUKXj5FFRClu0atLOy/7mJ+Lp4apvbEsCgxtgdyFTjAHZ6O
SrxU3t1eHjPdd933m4FaDCsyWXsmMmnxJp9DILoTH3oW+dgrQoACp3KLYIn1Pidl
l+Sx16JQJywuQ8PmrXusgVo/zLl/SZZRLgegBG5Vnn0JTuihkoseEPXyNHg/FCoK
D89Tn2tM+gSLfFFM9SI7ALnifQimPQUZ1Mr0jjm4VOHY9HBM5uQ1opniAgJxB1kE
ZN+xm7Zn3jr4J/rwTJwdVi8wQIvSjnPA+qKKDqSXJAFGU2Pd0ezrunlsevufBiDX
lQpf6MeUhL9HICGC5hmVwvB78FG0G1Lc2ncpQg6i+C35NPicCNPnPiN/Bsd/m2KO
ZPP4beslId0oEsmeT5gawiHK32FhjnwT5z0BBN1INKrylvgC37CQUSeo14L/u9a0
85sS8KLPbF7pv+h0nMbwODtF8NUMKLTahQ5+JMAZINu8nR6ZMOU=
=AypN
-----END PGP SIGNATURE-----

--/zk1wQqtB809JthL--