Re: Track the amount of time waiting due to cost_delay

Hi,

On Thu, Dec 05, 2024 at 05:51:11PM +0900, Masahiro Ikeda wrote:
> Hi,
>
> I recently encountered a case where having this feature would have been very
> helpful.

Oh great, thanks for the feedback!

> Thank you for developing it! I have a few questions and comments.
>
> Here are questions:
>
> After this patch is merged, are you considering adding delayed_time
> information to the logs output by log_autovacuum_min_duration?
> In the case I experienced, it would have been great to easily understand
> how much of the total execution time was spent on timed delays from the
> already executed VACUUM logs.

That's a good point. We already discussed adding some information in a dedicated
view ([1]) (and that's an idea I keep in mind). I also think your idea is worth
it and that it would make sense to start a dedicated thread once this one is
merged.

> Recently, this thread has not been active.

I think than Nathan wants to give time to others to interact on it like you
do ;-) (Nathan please correct me if I'm wrong).

> Here are minor comments on the v7 patch:

Thanks!

> Why not use the <xref> element, for example, <xref
> linkend="guc-autovacuum-vacuum-cost-delay"/>,
> as in the max_dead_tuple_bytes column?

There is multiple places where "<varname>vacuum_cost_delay</varname>" is
being used but I agree that's better to be consistent with how it is done for
this view. Done in v8 attached.

> IIUC, only the worker updates the column at a 1 Hz frequency. Would it be
> better to rephrase the following?"
> * The workers update the column no more frequently than once per second,
> so it could show slightly old values.

Yeah I like the re-wording, done that way in v8.

> + if (INSTR_TIME_GET_MILLISEC(time_since_last_report) >
> WORKER_REPORT_DELAY_INTERVAL)
> + {
> + pgstat_progress_parallel_incr_param(PROGRESS_VACUUM_TIME_DELAYED,
> + nap_time_since_last_report);
> + nap_time_since_last_report = 0;
> + last_report_time = delay_end;
> + }
>
> IIUC, unsent delayed_time will disappear when the parallel workers exit
> because they are not considered in parallel_vacuum_end(). I assume this
> is intentional behavior, as it is an acceptable error for the use cases.

Yeah, people would likely use this new field to monitor long running vacuum.
Long enough that this error should be acceptable. Do you agree?

> I didn't see any comments regarding this, so I wanted to confirm.

Added a comment to make it clear, thanks!

[1]: https://www.postgresql.org/message-id/CAD21AoDOu%3DDZcC%2BPemYmCNGSwbgL1s-5OZkZ1Spd5pSxofWNCw%40mail.gmail.com

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com