Re: POC: track vacuum/analyze cumulative time per relation

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