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

Hi,

On Wed, Oct 23, 2024 at 01:19:37AM +0900, Fujii Masao wrote:
> 
> 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;
> --------------

Oh right, thanks for pointing out!

> I removed the period at the end to match the usual convention in the codebase
> for single-line comment.
> 
> I've attached v2 patch.

Thanks for the new version!

> 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? 

Yes.

> But GetLockStatusData()
> already calls FAST_PATH_GET_BITS() right after FAST_PATH_SLOT(),
> and FAST_PATH_GET_BITS() has an assertion that validates this.

Oh right, it's "already" in FAST_PATH_GROUP() (and FAST_PATH_INDEX()).

> 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?

Given that it's already done in FAST_PATH_GET_BITS(), I think that's fine as it
is and v2 LGTM.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com