Re: wait events for disk I/O

Thank you for the updated patch.

I have applied and tested it on latest sources and the patch looks good to
me.

>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.
I agree that this writes less amount of data. Although tracking this can
be useful too in scenarios where pg_stat_statements is lagging due to I/O
bottleneck.
I will leave this decision to the committer.

Thank you,
Rahila Syed

On Wed, Mar 15, 2017 at 1:03 PM, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
wrote:

> 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
>