Обсуждение: pgsql: Add wait event for fsync of WAL segments
Add wait event for fsync of WAL segments This has been visibly a forgotten spot in the first implementation of wait events for I/O added by 249cf07, and what has been missing is a fsync call for WAL segments which is a wrapper reacting on the value of GUC wal_sync_method. Reported-by: Konstantin Knizhnik Author: Konstantin Knizhnik Reviewed-by: Craig Ringer, Michael Paquier Discussion: https://postgr.es/m/4a243897-0ad8-f471-aa40-242591f2476e@postgrespro.ru Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/c55de5e5123ce58ee19a47c08425949599285041 Modified Files -------------- doc/src/sgml/monitoring.sgml | 4 ++++ src/backend/access/transam/xlog.c | 2 ++ src/backend/postmaster/pgstat.c | 3 +++ src/include/pgstat.h | 1 + 4 files changed, 10 insertions(+)
On 2018-Jul-02, Michael Paquier wrote: > Add wait event for fsync of WAL segments > > This has been visibly a forgotten spot in the first implementation of > wait events for I/O added by 249cf07, and what has been missing is a > fsync call for WAL segments which is a wrapper reacting on the value of > GUC wal_sync_method. I wonder if we should backpatch this one all the way to pg10. I don't see no reason not to. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jul 02, 2018 at 12:23:35PM -0400, Alvaro Herrera wrote: > I wonder if we should backpatch this one all the way to pg10. I don't > see no reason not to. ABI breakage (if that's the correct wording?). Simply cherry-picking the patch from master to back-branches would cause extensions and plugins already compiled with those versions to be confused by the ordering of the enum WaitEventIO. Well, one simple solution is to simply put the new event purposefully at the bottom of the list. If that sounds right, I could do that on back-branches but I'd rather let HEAD on it current state with the event set correctly ordered. -- Michael
Вложения
On 2018-Jul-03, Michael Paquier wrote: > On Mon, Jul 02, 2018 at 12:23:35PM -0400, Alvaro Herrera wrote: > > I wonder if we should backpatch this one all the way to pg10. I don't > > see no reason not to. > > ABI breakage (if that's the correct wording?). Simply cherry-picking > the patch from master to back-branches would cause extensions and > plugins already compiled with those versions to be confused by the > ordering of the enum WaitEventIO. Well, one simple solution is to > simply put the new event purposefully at the bottom of the list. If > that sounds right, I could do that on back-branches Are the numerical values actually exposed to the world? I thought the only way to this info was through the system views, which surely expose the names, not the numbers. Actually, comment on pgstat_report_wait_start talks about WaitClass as if it were a thing, which it seems not to be. There is a comment "Wait Classes" but that term is not used anywhere else. If reading wait events is actually possible, then it seems easy to backpatch this in pg10 by putting the new value at the end of the relevant enum, yeah. > but I'd rather let HEAD on it current state with the event set > correctly ordered. Sure. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2018-07-03 15:13:20 -0400, Alvaro Herrera wrote: > Are the numerical values actually exposed to the world? I thought the > only way to this info was through the system views, which surely expose > the names, not the numbers. There's at least some work at high frequency sampling of these, and I assume that's not going through SQL, but rather C functions. > If reading wait events is actually possible, then it seems easy to > backpatch this in pg10 by putting the new value at the end of the > relevant enum, yeah. Honestly, I don't really see an argument for backpatching this. If people started measuring it might even invalidate their stats. Greetings, Andres Freund
On 2018-Jul-03, Andres Freund wrote: > On 2018-07-03 15:13:20 -0400, Alvaro Herrera wrote: > > Are the numerical values actually exposed to the world? I thought the > > only way to this info was through the system views, which surely expose > > the names, not the numbers. > > There's at least some work at high frequency sampling of these, and I > assume that's not going through SQL, but rather C functions. You're right. > > If reading wait events is actually possible, then it seems easy to > > backpatch this in pg10 by putting the new value at the end of the > > relevant enum, yeah. > > Honestly, I don't really see an argument for backpatching this. If > people started measuring it might even invalidate their stats. Hmm, if the stats are really invalidated by this change, then surely the original numbers were incorrect anyway. Anyway, it's not a huge deal to me. If Michael doesn't want to backpatch it, it's his call, and I don't have the cycles to do it myself right now either. If some other committer cares about it, well ... -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jul 03, 2018 at 04:12:23PM -0400, Alvaro Herrera wrote: > On 2018-Jul-03, Andres Freund wrote: > > > On 2018-07-03 15:13:20 -0400, Alvaro Herrera wrote: > > > Are the numerical values actually exposed to the world? I thought the > > > only way to this info was through the system views, which surely expose > > > the names, not the numbers. > > > > There's at least some work at high frequency sampling of these, and I > > assume that's not going through SQL, but rather C functions. > > You're right. My take is a background worker which connects to shared memory and scans directly ProcArray entries. > Anyway, it's not a huge deal to me. If Michael doesn't want to > backpatch it, it's his call, and I don't have the cycles to do it myself > right now either. If some other committer cares about it, well ... New wait events on HEAD are adapted to HEAD in my opinion, so I would keep the code as it is now. If anybody wishes to back-patch the change, of course feel free to do so. -- Michael
Вложения
Hi Michael, On Mon, Jul 2, 2018 at 3:23 PM, Michael Paquier <michael@paquier.xyz> wrote: > Add wait event for fsync of WAL segments > > Modified Files > -------------- > doc/src/sgml/monitoring.sgml | 4 ++++ I just noticed that the number of rows for the IO wait event type documentation hasn't been updated, see https://www.postgresql.org/docs/devel/static/monitoring-stats.html#WAIT-EVENT-TABLE. Trivial patch attached.
Вложения
On Sun, Jul 08, 2018 at 10:23:37PM +0200, Julien Rouhaud wrote: > I just noticed that the number of rows for the IO wait event type > documentation hasn't been updated, see > https://www.postgresql.org/docs/devel/static/monitoring-stats.html#WAIT-EVENT-TABLE. > > Trivial patch attached. Thanks, Julien! Fixed. Indeed the table format was weird.. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > On Sun, Jul 08, 2018 at 10:23:37PM +0200, Julien Rouhaud wrote: >> I just noticed that the number of rows for the IO wait event type >> documentation hasn't been updated, see >> https://www.postgresql.org/docs/devel/static/monitoring-stats.html#WAIT-EVENT-TABLE. >> Trivial patch attached. > Thanks, Julien! Fixed. Indeed the table format was weird.. Hm, do we need that "morerows" thing at all? It seems impossible that we'll remember to keep it up to date in future. How do you even check that it's right now? regards, tom lane
On 2018-Jul-09, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: > > On Sun, Jul 08, 2018 at 10:23:37PM +0200, Julien Rouhaud wrote: > >> I just noticed that the number of rows for the IO wait event type > >> documentation hasn't been updated, see > >> https://www.postgresql.org/docs/devel/static/monitoring-stats.html#WAIT-EVENT-TABLE. > >> Trivial patch attached. > > > Thanks, Julien! Fixed. Indeed the table format was weird.. > > Hm, do we need that "morerows" thing at all? It seems impossible > that we'll remember to keep it up to date in future. How do you > even check that it's right now? Maybe we should add an XML comment <!-- remember to update "morerows" above --> at the end of the table :-) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jul 9, 2018 at 11:24 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Maybe we should add an XML comment > <!-- remember to update "morerows" above --> > at the end of the table :-) +1. I made the same mistake at one point. -- Peter Geoghegan
On Mon, Jul 09, 2018 at 12:12:42PM -0700, Peter Geoghegan wrote: > On Mon, Jul 9, 2018 at 11:24 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > Maybe we should add an XML comment > > <!-- remember to update "morerows" above --> > > at the end of the table :-) > > +1. I made the same mistake at one point. Another idea that I have here, is to rework the page for monitoring stats so as we create one sub-section for each system view, and also one for the table of wait events. For the wait events, we could then completely remove the first category column which has morerows and divide the section into on table per event category. -- Michael
Вложения
On Mon, Jul 9, 2018 at 4:41 PM, Michael Paquier <michael@paquier.xyz> wrote: > Another idea that I have here, is to rework the page for monitoring > stats so as we create one sub-section for each system view, and also one > for the table of wait events. For the wait events, we could then > completely remove the first category column which has morerows and > divide the section into on table per event category. +1 from me. I think I proposed that before. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company