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

Thanks for the review!

> === 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.

Yes, that can be done. I see a few more examples in
which we report stats after end_command, i.e.
AbortTransaction

> 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)?

Yes, makes sense. It will keep the changes to pgstat_report_ to a minimum.

> === 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

I see the checkpointer write_time for example is converted
to double "for presentation". I am not sure why, expect for maybe
historical reasons. I will do the same for this new cumulative
time field for the sake of consistency. There is no macro for
relentry to return a stat as FLOAT8, so adding it as part of this
patch.

Datum
pg_stat_get_checkpointer_write_time(PG_FUNCTION_ARGS)
{
/* time is already in msec, just convert to double for presentation */
PG_RETURN_FLOAT8((double)
pgstat_fetch_stat_checkpointer()->write_time);
}

> +
> +       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.
>

Ok, Will remove these.

I also updated the comments for the instrument code path to reflect the
fact starttime is now set for all cases.Also, added documentation.

See the attached v2.

Regards,

Sami