Date: Thu, 24 Oct 2024 02:12:07 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Refactor GetLockStatusData() by skipping unused backends and
 groups
Message-ID: <Zxms9xnKd8SKOgVx@ip-10-97-1-34.eu-west-3.compute.internal>
References: <a0a00c44-31e9-4c67-9846-fb9636213ac9@oss.nttdata.com>
 <ZxYDgginZZGFPxqp@ip-10-97-1-34.eu-west-3.compute.internal>
 <5a0f78d1-0f56-439b-b4d3-2bae9d262c6c@oss.nttdata.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5a0f78d1-0f56-439b-b4d3-2bae9d262c6c@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/Zxms9xnKd8SKOgVx%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

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