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.lat...@gmail.com> wrote: > Thanks Rahila for reviewing this patch. > > On Tue, Mar 14, 2017 at 8:13 PM, Rahila Syed <rahilasye...@gmail.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.lat...@gmail.com> >> wrote: >> >>> >>> >>> On Wed, Mar 8, 2017 at 8:23 AM, Robert Haas <robertmh...@gmail.com> >>> wrote: >>> >>>> On Tue, Mar 7, 2017 at 9:32 PM, Amit Kapila <amit.kapil...@gmail.com> >>>> wrote: >>>> > On Tue, Mar 7, 2017 at 9:16 PM, Robert Haas <robertmh...@gmail.com> >>>> wrote: >>>> >> On Mon, Mar 6, 2017 at 9:09 PM, Amit Kapila <amit.kapil...@gmail.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@postgresql.org) >>> To make changes to your subscription: >>> http://www.postgresql.org/mailpref/pgsql-hackers >>> >>> >> > > > Regards, > Rushabh Lathia > www.EnterpriseDB.com > The Enterprise PostgreSQL Company >