Re: Orphaned wait event
On Tue, Apr 25, 2023 at 09:24:58PM +0530, Bharath Rupireddy wrote: > It looks like this patch attached upthread at [1] isn't in yet, > meaning WAIT_EVENT_SLRU_FLUSH_SYNC stays unused. IMO, it's worth > pushing it to the PG16 branch. It will help add a wait event for SLRU > page flushes. Thoughts? > > [1] > https://www.postgresql.org/message-id/CA%2BhUKG%2BewEpxm%3DhPNXyupRUB_SKGh-6tO86viaco0g-P_pm_Cw%40mail.gmail.com There could be the argument that some external code could abuse of this value for its own needs, but I don't really buy that. I'll go clean up that.. -- Michael signature.asc Description: PGP signature
Re: Orphaned wait event
On Fri, Mar 24, 2023 at 12:00 PM Bharath Rupireddy wrote: > > On Fri, Mar 24, 2023 at 3:31 AM Thomas Munro wrote: > > > > On Thu, Mar 23, 2023 at 8:10 PM Bharath Rupireddy > > wrote: > > > Yeah, commit [1] removed the last trace of it. I wonder if we can add > > > a WAIT_EVENT_SLRU_FLUSH_SYNC wait event in SlruSyncFileTag(), similar > > > to mdsyncfiletag. This way, we would have covered all sync_syncfiletag > > > fsyncs with wait events. > > > > Ahh, right. Thanks. The mistake was indeed that SlruSyncFileTag > > failed to report it while running pg_fsync(). > > Thanks. The attached patch looks good to me. It looks like this patch attached upthread at [1] isn't in yet, meaning WAIT_EVENT_SLRU_FLUSH_SYNC stays unused. IMO, it's worth pushing it to the PG16 branch. It will help add a wait event for SLRU page flushes. Thoughts? [1] https://www.postgresql.org/message-id/CA%2BhUKG%2BewEpxm%3DhPNXyupRUB_SKGh-6tO86viaco0g-P_pm_Cw%40mail.gmail.com -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Orphaned wait event
On Fri, Mar 24, 2023 at 3:31 AM Thomas Munro wrote: > > On Thu, Mar 23, 2023 at 8:10 PM Bharath Rupireddy > wrote: > > Yeah, commit [1] removed the last trace of it. I wonder if we can add > > a WAIT_EVENT_SLRU_FLUSH_SYNC wait event in SlruSyncFileTag(), similar > > to mdsyncfiletag. This way, we would have covered all sync_syncfiletag > > fsyncs with wait events. > > Ahh, right. Thanks. The mistake was indeed that SlruSyncFileTag > failed to report it while running pg_fsync(). Thanks. The attached patch looks good to me. > > > In case it's useful again, here's how I noticed: > > > > > > for X in ` grep WAIT_EVENT_ src/include/utils/wait_event.h | > > >sed '/^#/d;s/,//;s/ = .*//' ` > > > do > > > if ! ( git grep $X | > > > grep -v src/include/utils/wait_event.h | > > > grep -v src/backend/utils/activity/wait_event.c | > > > grep $X > /dev/null ) > > > then > > > echo "$X is not used" > > > fi > > > done > > > > Interesting. It might be an overkill to think of placing it as a > > compile-time script to catch similar miss-outs in future. > > Meh. Parsing C programs from shell scripts is fun for one-off > throw-away usage, but I think if we want proper automation here we > should look into a way to define wait events in a central file similar > to what we do for src/backend/storage/lmgr/lwlocknames.txt. It could > give the enum name, the display name, and the documentation sentence > on one tab-separated line, and we could generate all the rest from > that, or something like that? I suspect that downstream/monitoring > tools might appreciate the existence of such a file too. +1. So, with that approach, both wait_event.h and wait_event.c will be auto-generated I believe. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Orphaned wait event
Hi, On 3/23/23 11:00 PM, Thomas Munro wrote: I think if we want proper automation here we should look into a way to define wait events in a central file similar to what we do for src/backend/storage/lmgr/lwlocknames.txt. It could give the enum name, the display name, and the documentation sentence on one tab-separated line, and we could generate all the rest from that, or something like that? I suspect that downstream/monitoring tools might appreciate the existence of such a file too. Yeah, I think that makes sense. I'll look at this and start a new thread once I've a patch to share. FWIW, I'm also working on wait event "improvements" (mainly adding extra info per wait event) that 1) I'll share once ready 2) could also probably benefit from your proposal here. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Orphaned wait event
On Thu, Mar 23, 2023 at 8:10 PM Bharath Rupireddy wrote: > Yeah, commit [1] removed the last trace of it. I wonder if we can add > a WAIT_EVENT_SLRU_FLUSH_SYNC wait event in SlruSyncFileTag(), similar > to mdsyncfiletag. This way, we would have covered all sync_syncfiletag > fsyncs with wait events. Ahh, right. Thanks. The mistake was indeed that SlruSyncFileTag failed to report it while running pg_fsync(). > > In case it's useful again, here's how I noticed: > > > > for X in ` grep WAIT_EVENT_ src/include/utils/wait_event.h | > >sed '/^#/d;s/,//;s/ = .*//' ` > > do > > if ! ( git grep $X | > > grep -v src/include/utils/wait_event.h | > > grep -v src/backend/utils/activity/wait_event.c | > > grep $X > /dev/null ) > > then > > echo "$X is not used" > > fi > > done > > Interesting. It might be an overkill to think of placing it as a > compile-time script to catch similar miss-outs in future. Meh. Parsing C programs from shell scripts is fun for one-off throw-away usage, but I think if we want proper automation here we should look into a way to define wait events in a central file similar to what we do for src/backend/storage/lmgr/lwlocknames.txt. It could give the enum name, the display name, and the documentation sentence on one tab-separated line, and we could generate all the rest from that, or something like that? I suspect that downstream/monitoring tools might appreciate the existence of such a file too. From 13e8d3d10d773a07c5586c6f8efc4c526b5db1d0 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Fri, 24 Mar 2023 10:47:36 +1300 Subject: [PATCH] Fix missing WAIT_EVENT_SLRU_FLUSH_SYNC reporting. Commit dee663f7 moved fsync() calls for SLRU files into the checkpointer, but accidentally stopped reporting WAIT_EVENT_SLRU_FLUSH_SYNC as the wait event while in the system call. Fix, so that SlruSyncFileTag() mirrors the way the equivalent wait event for data files is reported by mdsyncfiletag(). Back-patch to 14, where dee663f7 landed. Reported-by: Bharath Rupireddy Discussion: https://postgr.es/m/CALj2ACVY3eA261mUDO8xcMJGDwj_OHpaxEUMc5L9jxkj3fvcpg%40mail.gmail.com --- src/backend/access/transam/slru.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c index 5ab86238a9..2a42f31ec2 100644 --- a/src/backend/access/transam/slru.c +++ b/src/backend/access/transam/slru.c @@ -1603,7 +1603,9 @@ SlruSyncFileTag(SlruCtl ctl, const FileTag *ftag, char *path) if (fd < 0) return -1; + pgstat_report_wait_start(WAIT_EVENT_SLRU_FLUSH_SYNC); result = pg_fsync(fd); + pgstat_report_wait_end(); save_errno = errno; CloseTransientFile(fd); -- 2.39.2
Re: Orphaned wait event
On Thu, Mar 23, 2023 at 7:43 AM Thomas Munro wrote: > > Hi, > > Commit dee663f7 made WAIT_EVENT_SLRU_FLUSH_SYNC redundant, so here's a > patch to remove it. Yeah, commit [1] removed the last trace of it. I wonder if we can add a WAIT_EVENT_SLRU_FLUSH_SYNC wait event in SlruSyncFileTag(), similar to mdsyncfiletag. This way, we would have covered all sync_syncfiletag fsyncs with wait events. > In case it's useful again, here's how I noticed: > > for X in ` grep WAIT_EVENT_ src/include/utils/wait_event.h | >sed '/^#/d;s/,//;s/ = .*//' ` > do > if ! ( git grep $X | > grep -v src/include/utils/wait_event.h | > grep -v src/backend/utils/activity/wait_event.c | > grep $X > /dev/null ) > then > echo "$X is not used" > fi > done Interesting. It might be an overkill to think of placing it as a compile-time script to catch similar miss-outs in future. [1] commit dee663f7843902535a15ae366cede8b4089f1144 Author: Thomas Munro Date: Fri Sep 25 18:49:43 2020 +1200 Defer flushing of SLRU files. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Orphaned wait event
Hi, Commit dee663f7 made WAIT_EVENT_SLRU_FLUSH_SYNC redundant, so here's a patch to remove it. In case it's useful again, here's how I noticed: for X in ` grep WAIT_EVENT_ src/include/utils/wait_event.h | sed '/^#/d;s/,//;s/ = .*//' ` do if ! ( git grep $X | grep -v src/include/utils/wait_event.h | grep -v src/backend/utils/activity/wait_event.c | grep $X > /dev/null ) then echo "$X is not used" fi done From 5bd5d5448b1fbd01833069af72bf12d19eef42dd Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Thu, 23 Mar 2023 14:36:27 +1300 Subject: [PATCH] Remove orphaned wait event. Commit dee663f7 made WAIT_EVENT_SLRU_FLUSH_SYNC redundant. diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 7ab4424bf1..45259efd0f 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -1468,11 +1468,6 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser ReplicationSlotWrite Waiting for a write to a replication slot control file. - - SLRUFlushSync - Waiting for SLRU data to reach durable storage during a checkpoint - or database shutdown. - SLRURead Waiting for a read of an SLRU page. diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c index 7940d64639..23a85345b7 100644 --- a/src/backend/utils/activity/wait_event.c +++ b/src/backend/utils/activity/wait_event.c @@ -672,9 +672,6 @@ pgstat_get_wait_io(WaitEventIO w) case WAIT_EVENT_REPLICATION_SLOT_WRITE: event_name = "ReplicationSlotWrite"; break; - case WAIT_EVENT_SLRU_FLUSH_SYNC: - event_name = "SLRUFlushSync"; - break; case WAIT_EVENT_SLRU_READ: event_name = "SLRURead"; break; diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h index 518d3b0a1f..14b7ac5cf6 100644 --- a/src/include/utils/wait_event.h +++ b/src/include/utils/wait_event.h @@ -207,7 +207,6 @@ typedef enum WAIT_EVENT_REPLICATION_SLOT_RESTORE_SYNC, WAIT_EVENT_REPLICATION_SLOT_SYNC, WAIT_EVENT_REPLICATION_SLOT_WRITE, - WAIT_EVENT_SLRU_FLUSH_SYNC, WAIT_EVENT_SLRU_READ, WAIT_EVENT_SLRU_SYNC, WAIT_EVENT_SLRU_WRITE, -- 2.39.2
