Hi,
On Tue, Dec 10, 2024 at 11:55:41AM -0600, Nathan Bossart wrote:
> On Mon, Dec 09, 2024 at 04:41:03PM +0000, Bertrand Drouvot wrote:
> > + <structfield>time_delayed</structfield> <type>bigint</type>
>
> I think it's also worth considering names like total_delay and
> cumulative_delay.
That's fine by me. Then I think that total_delay is the way to go (I don't see
any existing "cumulative_").
> > + Total amount of time spent in milliseconds waiting during <xref linkend="guc-vacuum-cost-delay"/>
> > + or <xref linkend="guc-autovacuum-vacuum-cost-delay"/>. In case of parallel
> > + vacuum the reported time is across all the workers and the leader. The
> > + workers update the column no more frequently than once per second, so it
> > + could show slightly old values.
>
> I wonder if it makes sense to provide this value as an interval instead of
> the number of milliseconds to make it more human-readable.
Yeah we could do so, but that would mean:
1. Write a dedicated "pg_stat_get_progress_info()" function for VACUUM. Indeed,
the current pg_stat_get_progress_info() is shared across multiple "commands" and
then we wouldn't be able to change it's output types in pg_proc.dat.
Or
2. Make use of make_interval() in the pg_stat_progress_vacuum view creation.
I don't like 1. that much and given that that would be as simple as:
"
select make_interval(secs => time_delayed / 1000) from pg_stat_progress_vacuum;
"
for an end user to display an interval, I'm not sure we should provide an interval
by default.
That said, I agree that milliseconds is not really human-readable and
does not provide that much added value (except flexibility), so I'd vote for 2.
if you feel we should provide an interval by default.
> I might also
> suggest some changes to the description:
>
> Total accumulated time spent sleeping due to the cost-based vacuum
> delay settings (e.g., vacuum_cost_delay, vacuum_cost_limit). This
> includes the time that any associated parallel workers have slept, too.
> However, parallel workers report their sleep time no more frequently
> than once per second, so the reported value may be slightly stale.
>
Yeah I like it, thanks! Now, I'm wondering if we should not also add something
like this:
"
Since multiple workers can sleep simultaneously, the total sleep time can exceed
the actual duration of the vacuum operation.
"
As that could be surprising to see this behavior in action.
Thoughts?
I'll provide an updated patch version once we agree on the above points.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
