Re: Track the amount of time waiting due to cost_delay

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