public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bertrand Drouvot <[email protected]>
To: Fujii Masao <[email protected]>
Cc: [email protected]
Subject: Re: Refactor GetLockStatusData() by skipping unused backends and groups
Date: Thu, 24 Oct 2024 02:12:07 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (5+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: Refactor GetLockStatusData() by skipping unused backends and groups
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox