On Mon, Jul 31, 2023 at 03:53:27PM +0900, Masahiro Ikeda wrote:
> I think the order in which they are mentioned should be matched. I mean that
> - so an <literal>LWLock</literal> or <literal>Extension</literal> wait
> + so an <literal>Extension</literal> or <literal>LWLock</literal> wait
Makes sense.
> /* This should only be called for user-defined wait event. */
> if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
> ereport(ERROR,
> errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("invalid wait event ID %u", eventId));
>
> I was just wondering if it should also check the eventId
> that has been allocated though it needs to take the spinlock
> and GetWaitEventExtensionIdentifier() doesn't take it into account.
What kind of extra check do you have in mind? Once WAIT_EVENT_ID_MASK
is applied, we already know that we don't have something larger than
PG_UNIT16_MAX, or perhaps you want to cross-check this number with
what nextId holds in shared memory and that we don't have a number
between nextId and PG_UNIT16_MAX? I am not sure that we need to care
much about that this much in this code path, and I'd rather avoid
taking an extra time the spinlock just for a cross-check.
Attaching a v11 based on Bharath's feedback and yours, for now. I
have also applied the addition of the two masking variables in
wait_event.c separately with 7395a90.
--
Michael