On Thu, Jun 13, 2024 at 07:42:25PM -0700, Noah Misch wrote:
> On Fri, Jun 14, 2024 at 09:58:59AM +0900, Michael Paquier wrote:
>> 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 add a default for consistency as this is just a few extra
lines, but if you feel strongly about that, I'm OK as well. It makes
a bit easier the detection of incorrect wait event numbers set
incorrectly in extensions depending on the class wanted.
> 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.
It's one of the areas where I think that we should have more
documentation, not less of it, so I'd rather keep it and maintaining
it is not really a pain (?). The backend gets complicated enough
these days that limiting what developers have to guess on their own is
a better long-term approach because the Postgres out-of-core ecosystem
is expanding a lot (aka have also in-core documentation for hooks,
even if there's been a lot of reluctance historically about having
them).
--
Michael
