From: | Sami Imseih <samimseih(at)gmail(dot)com> |
---|---|
To: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: POC: track vacuum/analyze cumulative time per relation |
Date: | 2025-01-14 23:01:52 |
Message-ID: | CAA5RZ0v9060p7hjtLPY9i0Me6SoaRKYwxATqt_DnZr8dGSnUhQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
thanks for the review!
> The comments do not reflect the function names ("total" is missing to give
> pg_stat_get_total_vacuum_time() and such).
fixed
> I think it's better to define the macro just before its first usage (meaning
> just after pg_stat_get_vacuum_count()): that would be consistent with the places
> the other macros are defined.
correct. I fixed this to match the placement of similar macros.
> s/int64/double/?
fixed
> Mention the unit?
fixed
> Out of curiosity, why this extra comment? To be somehow consistent with
> do_analyze_rel()?
Yes, I added it for consistency, but since the comment was not there
before, I just decided to remove it.
> I wonder if it wouldn't make more sense to put the GetCurrentTimestamp() call
> before the pgstat_progress_start_command() call. That would be aligned with the
> "endtime" being after the pgstat_progress_end_command() and where it was before
> the patch.
I agree. Fixed.
Please see the attached v3.
Regards,
Sami
Attachment | Content-Type | Size |
---|---|---|
v3-0001-Track-per-relation-cumulative-time-spent-in-vacuu.patch | application/octet-stream | 16.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2025-01-14 23:55:03 | Re: Reorder shutdown sequence, to flush pgstats later |
Previous Message | Sami Imseih | 2025-01-14 22:07:36 | Re: Sample rate added to pg_stat_statements |