Re: POC: track vacuum/analyze cumulative time per relation - Mailing list pgsql-hackers

Hi,

On Thu, Jan 02, 2025 at 12:24:06PM -0600, Sami Imseih wrote:
> Having the total (auto)vacuum elapsed time
> along side the existing (auto)vaccum_count
> allows a user to track the average time an
> operating overtime and to find vacuum tuning
> opportunities.
> 
> The same can also be said for (auto)analyze.

I think that makes sense to expose those metrics through SQL as you're proposing
here. The more we expose through SQL the better I think.

> attached a patch ( without doc changes)
> that adds 4 new columns:

Thanks!

A few random comments:

=== 1

+       endtime = GetCurrentTimestamp();
        pgstat_report_vacuum(RelationGetRelid(rel),
                                                 rel->rd_rel->relisshared,
                                                 Max(vacrel->new_live_tuples, 0),
                                                 vacrel->recently_dead_tuples +
-                                                vacrel->missed_dead_tuples);
+                                                vacrel->missed_dead_tuples,
+                                                starttime,
+                                                endtime);
        pgstat_progress_end_command();

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

What about keeping the endtime assignment after the pgstat_progress_end_command()
call? I think that it makes more sense that way.

=== 2

        pgstat_report_vacuum(RelationGetRelid(rel),
                                                 rel->rd_rel->relisshared,
                                                 Max(vacrel->new_live_tuples, 0),
                                                 vacrel->recently_dead_tuples +
-                                                vacrel->missed_dead_tuples);
+                                                vacrel->missed_dead_tuples,
+                                                starttime,
+                                                endtime);

What about doing the elapsedtime computation prior the this call and passed
it as a parameter (then remove the starttime one and keep the endtime as it
is needed)?

=== 3

        pgstat_report_analyze(onerel, totalrows, totaldeadrows,
-                             (va_cols == NIL));
+                             (va_cols == NIL), starttime, endtime);

Same as 2 for pgstat_report_analyze() except that the endtime could be removed
too.

=== 4

+/* pg_stat_get_vacuum_time */
+PG_STAT_GET_RELENTRY_INT64(total_vacuum_time)
+
+/* pg_stat_get_autovacuum_time */
+PG_STAT_GET_RELENTRY_INT64(total_autovacuum_time)
+
+/* pg_stat_get_analyze_time */
+PG_STAT_GET_RELENTRY_INT64(total_analyze_time)
+
+/* pg_stat_get_autoanalyze_time */
+PG_STAT_GET_RELENTRY_INT64(total_autoanalyze_time)

I wonder if it wouldn't be better to use FLOAT8 here (to match things like
pg_stat_get_checkpointer_write_time(), pg_stat_get_checkpointer_sync_time() among
others).

=== 5

+
+       PgStat_Counter total_vacuum_time;       /* user initiated vacuum */
+       PgStat_Counter total_autovacuum_time;   /* autovacuum initiated */
+       PgStat_Counter total_analyze_time;      /* user initiated vacuum */
+       PgStat_Counter total_autoanalyze_time;  /* autovacuum initiated */

Those comments look weird to me.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com