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. >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? Should calls to write() in following functions be tracked too? qtext_store() - This is related to pg_stat_statements dsm_impl_mmap() - This is in relation to creating dsm segments. write_auto_conf_file()- This is called when updated configuration parameters are written to a temp file. Thank you, Rahila Syed 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 (email@example.com) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >