Re: Track the amount of time waiting due to cost_delay

Hi,

On Fri, Dec 13, 2024 at 10:06:08PM -0600, Nathan Bossart wrote:
> I spent some time preparing v12 for commit and made the following larger
> changes:

Thanks!

> * I renamed the column to delay_time and changed it back to repoting
> milliseconds to match other stats views like pg_stat_io.

Okay better to be consistent.

> * I optimized the code in vacuum_delay_point a bit. Notably, we're now
> just storing the nanoseconds value in the pgstat param,

Right, using nanoseconds induces less computation/conversions in the C code.

> so we now have to divide by 1,000,000 in the views.

So, reading the output one could get the number of nanoseconds waiting on
cost delay. I'm not sure it's needed to give this level of precision for the
delay time (while I fully agree it has to be done for the I/O related timing).

OTOH, it does not hurt to give this level of precision, so I'm fine with it.

> * I added a track_cost_delay_timing parameter that is off by default. The
> new timing code is only used when this parameter is turned on. This is
> meant to match parameters like track_io_timing. I felt that this was
> important since this is relatively hot code.

Fully agree.

> * I also added delay_time to pg_stat_progress_analyze. It seems to use the
> same vacuum_delay_point() function, so we actually need to do a bit of
> refactoring to make sure the right pgstat param is incremented.

Good idea!

> I think this has been discussed in the thread a bit already, but I do think
> we should consider also adding this information to the vacuum/analyze log
> messages and to the output of VACUUM/ANALYZE (VERBOSE). That needn't hold
> up this patch, though.

Yes, that would be a nice next step to do.

> Finally, I can't help but feel that the way we are adding this information
> is a bit weird, both in how we are doing it and where we are presenting the
> results. I don't see any reason that pgstat_progress_incr_param() and
> friends can't handle this information, but I don't see any existing uses
> for timing information. Plus, IMHO it's debatable whether the delay time
> is really "progress" information, although I haven't thought of a better
> place (existing or new) for it.
>

I agree that pgstat_progress_incr_param() was originally designed for progress
counters rather than timing data. I also agree that "it's not real" progress
information. I see it more like "let's look at it while checking the progress".

I think that we have is a pragmatic approach: use the existing progress reporting
system even though it's not a perfect conceptual fit, rather than creating new
"infrastructure" just for this timing data.

I think that's fine as we could still change our mind should new "timing data"
be added in the future.

A few comments about the patch:

=== 1

+ /* accumulate the delay time */

s/accumulate/Accumulate/ to be consistent with the code around. Did it that
way in v14 attached and for other places.

=== 2

+ S.param10 AS indexes_processed,
+ S.param11 / 1000000::double precision AS delay_time

The output looks like "167.100142". As said above, I'm not sure it's needed to
give this level of precision for the delay time. But that does not hurt.

=== 3

+#define PARALLEL_VACUUM_WORKER_DELAY_REPORT_INTERVAL_NS (NS_PER_S)

Did not changed in v14, but "PARALLEL_VACUUM_REPORT_INTERVAL_NS" could be
an option as well. I think it keeps the key concepts while being more concise (
WORKER is somehow implicit in the context).

=== 4

-vacuum_delay_point(void)
+static void
+vacuum_delay_point_internal(bool is_analyze)

Updated the comment on top of it accordingly.

=== 5

+ if (INSTR_TIME_GET_NANOSEC(time_since_last_report) >=
+ PARALLEL_VACUUM_WORKER_DELAY_REPORT_INTERVAL_NS)
+ {
+ pgstat_progress_parallel_incr_param(PROGRESS_VACUUM_DELAY_TIME,

Added a comment to mention that PROGRESS_ANALYZE_DELAY_TIME is out of interest
here.

v14 also fixes the typo mentioned by Sergei in [1].

[1]: https://www.postgresql.org/message-id/1983281734169163%40sjg23nxaikj7vz54.iva.yp-c.yandex.net

Regards,

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