Orphaned wait event

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]commit dee663f7843902535a15ae366cede8b4089f1144 Author: Thomas Munro <tmunro@postgresql.org> Date: Fri Sep 25 18:49:43 2020 +1200 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
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.

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]/messages/by-id/CA+hUKG+ewEpxm=hPNXyupRUB_SKGh-6tO86viaco0g-P_pm_Cw@mail.gmail.com 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]: /messages/by-id/CA+hUKG+ewEpxm=hPNXyupRUB_SKGh-6tO86viaco0g-P_pm_Cw@mail.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] /messages/by-id/CA+hUKG+ewEpxm=hPNXyupRUB_SKGh-6tO86viaco0g-P_pm_Cw@mail.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