Re: wait events for disk I/O

Thanks Rahila for reviewing this patch.

On Tue, Mar 14, 2017 at 8:13 PM, Rahila Syed <rahilasyed90(at)gmail(dot)com> wrote:

> Hello,
>
> I applied and tested this patch on latest sources and it works fine.
>
> Following are some comments,
>
> >+ /* Wait event for SNRU */
> >+ WAIT_EVENT_READ_SLRU_PAGE,
> Typo in the comment.
>
>
Fixed.

> >FileWriteback(v->mdfd_vfd, seekpos, (off_t) BLCKSZ * nflush,
> WAIT_EVENT_FLUSH_DATA_BLOCK);
> This call is inside mdwriteback() which can flush more than one block so
> should WAIT_EVENT _FLUSH_DATA_BLOCK
> be renamed to WAIT_EVENT_FLUSH_DATA_BLOCKS?
>
>
Changed with WAIT_EVENT_FLUSH_DATA_BLOCKS.

> Should calls to write() in following functions be tracked too?
> qtext_store() - This is related to pg_stat_statements
>
>

I am not quite sure about this, as this is for stat statements. Also part
from the
place you found there are many other fwrite() call into pg_stat_statements,
and
I intentionally haven't added event here as it is very small write about
stat, and
doesn't look like we should add for those call.

> dsm_impl_mmap() - This is in relation to creating dsm segments.
>
>
Added new event here. Actually particular write call is zero-filling the
DSM file.

> write_auto_conf_file()- This is called when updated configuration
> parameters are
> written to a temp file.
>
>
write_auto_conf_file() is getting called during the ALTER SYSTEM call. Here
write
happen only when someone explicitly run the ALTER SYSTEM call. This is
administrator call and so doesn't seem like necessary to add separate wait
event
for this.

PFA latest patch with other fixes.

>
> On Wed, Mar 8, 2017 at 4:50 PM, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
> wrote:
>
>>
>>
>> On Wed, Mar 8, 2017 at 8:23 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
>> wrote:
>>
>>> On Tue, Mar 7, 2017 at 9:32 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>>> wrote:
>>> > On Tue, Mar 7, 2017 at 9:16 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
>>> wrote:
>>> >> On Mon, Mar 6, 2017 at 9:09 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>>> wrote:
>>> >>> Sure, if you think both Writes and Reads at OS level can have some
>>> >>> chance of blocking in obscure cases, then we should add a wait event
>>> >>> for them.
>>> >>
>>> >> I think writes have a chance of blocking in cases even in cases that
>>> >> are not very obscure at all.
>>> >
>>> > Point taken for writes, but I think in general we should have some
>>> > criteria based on which we can decide whether to have a wait event for
>>> > a particular call. It should not happen that we have tons of wait
>>> > events and out of which, only a few are helpful in most of the cases
>>> > in real-world scenarios.
>>>
>>> Well, the problem is that if you pick and choose which wait events to
>>> add based on what you think will be common, you're actually kind of
>>> hosing yourself. Because now when something uncommon happens, suddenly
>>> you don't get any wait event data and you can't tell what's happening.
>>> I think the number of new wait events added by Rushabh's patch is
>>> wholly reasonable. Yeah, some of those are going to be a lot more
>>> common than others, but so what? We add wait events so that we can
>>> find out what's going on. I don't want to sometimes know when a
>>> backend is blocked on an I/O. I want to ALWAYS know.
>>>
>>>
>> Yes, I agree with Robert. Knowing what we want and what we don't
>> want is difficult to judge. Something which we might think its not useful
>> information, and later of end up into situation where we re-think about
>> adding those missing stuff is not good. Having more information about
>> the system, specially for monitoring purpose is always good.
>>
>> I am attaching another version of the patch, as I found stupid mistake
>> in the earlier version of patch, where I missed to initialize initial
>> value to
>> WaitEventIO enum. Also earlier version was not getting cleanly apply on
>> the current version of sources.
>>
>>
>>
>> --
>> Rushabh Lathia
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>>
>

Regards,
Rushabh Lathia
www.EnterpriseDB.com
The Enterprise PostgreSQL Company