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
