Hi,
On 3/6/23 5:30 PM, Melanie Plageman wrote:
> Thanks for the review!
>
> On Tue, Feb 28, 2023 at 4:49 AM Drouvot, Bertrand
> <bertranddrouvot.pg@gmail.com> wrote:
>> On 2/26/23 5:03 PM, Melanie Plageman wrote:
>>> The timings will only be non-zero when track_io_timing is on
>>
>> That could lead to incorrect interpretation if one wants to divide the timing per operations, say:
>>
>> - track_io_timing is set to on while there is already operations
>> - or set to off while it was on (and the number of operations keeps growing)
>>
>> Might be worth to warn/highlight in the "track_io_timing" doc?
>
> This is a good point. I've added a note to the docs for pg_stat_io.
Thanks!
Now I've a second thought: what do you think about resetting the related number
of operations and *_time fields when enabling/disabling track_io_timing? (And mention it in the doc).
That way it'd prevent bad interpretation (at least as far the time per operation metrics are concerned).
Thinking that way as we'd loose some (most?) benefits of the new *_time columns
if one can't "trust" their related operations and/or one is not sampling pg_stat_io frequently enough (to discard the
samples
where the track_io_timing changes occur).
But well, resetting the operations could also lead to bad interpretation about the operations...
Not sure about which approach I like the most yet, what do you think?
>> That leads to pgstat_count_io_time() to be called before pgstat_count_io_op() (for the IOOP_EXTEND case) and
>> after pgstat_count_io_op() (for the IOOP_READ case).
>>
>> What about calling them in the same order and so that pgstat_count_io_time() is called before pgstat_count_io_op()?
>>
>> If so, the ordering would also need to be changed in:
>>
>> - FlushRelationBuffers()
>> - register_dirty_segment()
>
> Yes, good point. I've updated the code to use this suggested ordering in
> attached v3.
>
Thanks, this looks good to me.
>> As pgstat_bktype_io_stats_valid() is called only in Assert(), I think that would be a good idea
>> to also check that if counts are not Zero then times are not Zero.
>
> Yes, I think adding some validation around the relationship between
> counts and timing should help prevent developers from forgetting to call
> pg_stat_count_io_op() when calling pgstat_count_io_time() (as relevant).
>
> However, I think that we cannot check that if IO counts are non-zero
> that IO times are non-zero, because the user may not have
> track_io_timing enabled.
Yeah, right.
> We can check that if IO times are not zero, IO
> counts are not zero. I've done this in the attached v3.
>
Thanks, looks good to me.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
