From: Noah Misch <noah@leadboat.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Smolkin Grigory <smallkeen@gmail.com>
Subject: Re: race condition in pg_class
Date: Thu, 13 Jun 2024 19:42:25 -0700
Message-ID: <20240614024225.d6@rfd.leadboat.com> (raw)
In-Reply-To: <ZmuV01mBRz-0QuNt@paquier.xyz>
References: <20231027214946.79.nmisch@google.com>
<1617596.1698446455@sss.pgh.pa.us>
<20231027232612.99.nmisch@google.com>
<20231102030915.d3.nmisch@google.com>
<20240512232923.aa.nmisch@google.com>
<20240605181706.fe.nmisch@google.com>
<CA+TgmobfMU5pdXP36D5iAwxV5WKE_vuDLtp_1QyH+H5jMMt21g@mail.gmail.com>
<20240611024525.9f.nmisch@google.com>
<20240614003549.c2.nmisch@google.com>
<ZmuV01mBRz-0QuNt@paquier.xyz>
On Fri, Jun 14, 2024 at 09:58:59AM +0900, Michael Paquier wrote:
> Looking at inplace031-inj-wait-event..
>
> The comment at the top of GetWaitEventCustomNames() requires an
> update, still mentioning extensions.
Thanks. Fixed locally.
> GetWaitEventCustomIdentifier() is incorrect, and should return
> "InjectionPoint" in the default case of this class name, no?
I intentionally didn't provide a default event ID for InjectionPoint.
PG_WAIT_EXTENSION needs a default case for backward compatibility, if nothing
else. For this second custom type, it's needless complexity. The value
0x0B000000U won't just show up like PG_WAIT_EXTENSION does.
GetLWLockIdentifier() also has no default case. How do you see it?
> I would
> just pass the classID to GetWaitEventCustomIdentifier().
As you say, that would allow eventId==0 to raise "could not find custom wait
event" for PG_WAIT_INJECTIONPOINT instead of wrongly returning "Extension".
Even if 0x0B000000U somehow does show up, having pg_stat_activity report
"Extension" instead of an error, in a developer test run, feels unimportant to
me.
> It is suboptimal to have pg_get_wait_events() do two scans of
> WaitEventCustomHashByName. Wouldn't it be better to do a single scan,
> returning a set of (class_name,event_name) fed to the tuplestore of
> this SRF?
Micro-optimization of pg_get_wait_events() doesn't matter. I did consider
that or pushing more of the responsibility into wait_events.c, but I
considered it on code repetition grounds, not performance grounds.
> uint32
> WaitEventExtensionNew(const char *wait_event_name)
> {
> + return WaitEventCustomNew(PG_WAIT_EXTENSION, wait_event_name);
> +}
> +
> +uint32
> +WaitEventInjectionPointNew(const char *wait_event_name)
> +{
> + return WaitEventCustomNew(PG_WAIT_INJECTIONPOINT, wait_event_name);
> +}
>
> Hmm. The advantage of two routines is that it is possible to control
> the class IDs allowed to use the custom wait events. Shouldn't the
> second routine be documented in xfunc.sgml?
The patch added to xfunc.sgml an example of using it. I'd be more inclined to
delete the WaitEventExtensionNew() docbook documentation than to add its level
of detail for WaitEventInjectionPointNew(). We don't have that kind of
documentation for most extension-facing C functions.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, michael@paquier.xyz, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, smallkeen@gmail.com
Subject: Re: race condition in pg_class
In-Reply-To: <20240614024225.d6@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox