Thread: 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
Attachment
On Thu, Mar 23, 2023 at 7:43 AM Thomas Munro <thomas.munro@gmail.com> 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 <tmunro@postgresql.org>
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
On Thu, Mar 23, 2023 at 8:10 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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.
Attachment
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
On Fri, Mar 24, 2023 at 3:31 AM Thomas Munro <thomas.munro@gmail.com> wrote: > > On Thu, Mar 23, 2023 at 8:10 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> 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
On Fri, Mar 24, 2023 at 12:00 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Fri, Mar 24, 2023 at 3:31 AM Thomas Munro <thomas.munro@gmail.com> wrote: > > > > On Thu, Mar 23, 2023 at 8:10 PM Bharath Rupireddy > > <bharath.rupireddyforpostgres@gmail.com> 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
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