Re: Refactor GetLockStatusData() by skipping unused backends and groups

On 2024/10/21 16:32, Bertrand Drouvot wrote:
> A few random comments on it:

Thanks for the review!

> 1 ===
>
> + /* Skip backends with pid=0, as they don't hold fast-path locks */
> + if (proc->pid == 0)
> + continue;
>
> What about adding a few words in the comment that it represents prepared
> transactions? Or maybe add a new macro (say IS_PREPARED_TRANSACTION(proc)) and
> use it in the few places where we check for "PGPROC"->pid == 0 or "PGPROC"->pid != 0?

I understand that PGPROC entries with pid=0 are typically those not yet allocated to
any backends. Yes, as you mentioned, prepared transactions also have pid=0. However,
GetLockStatusData() loops up to ProcGlobal->allProcCount, which is MaxBackends plus
NUM_AUXILIARY_PROCS, excluding prepared transactions. Therefore, GetLockStatusData()
doesn't seem to check PGPROC entries for prepared transactions at all.

In proc.c
--------------
/* XXX allProcCount isn't really all of them; it excludes prepared xacts */
ProcGlobal->allProcCount = MaxBackends + NUM_AUXILIARY_PROCS;
--------------

> One remark about the comment, what about?
>
> s/Skip unallocated groups/Skip groups without registered fast-path locks./?

I've updated the source comment accordingly.

> or at least add a "." at the end to be consistent with:
>
> "/* Skip unallocated slots. */"

I removed the period at the end to match the usual convention in the codebase
for single-line comment.

I've attached v2 patch.

> 3 ===
>
> One thing that worry me a bit is that we "lost" the FP_LOCK_SLOTS_PER_BACKEND usage,
> so that if there is a change on it (for wathever reason) then we probably need to
> be careful that the change would be reflected here too.
>
> So, what about to add an Assert to check that we overall iterated over FP_LOCK_SLOTS_PER_BACKEND?

You mean adding an assertion check to ensure that the slot ID calculated by
FAST_PATH_SLOT() is less than FP_LOCK_SLOTS_PER_BACKEND? But GetLockStatusData()
already calls FAST_PATH_GET_BITS() right after FAST_PATH_SLOT(),
and FAST_PATH_GET_BITS() has an assertion that validates this. So, probably
we can consider that this check is already in place. If it’s still worth adding,
perhaps placing it inside the FAST_PATH_SLOT() macro could be an option...
Or current assertion check is enough? Thought?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION