Message-ID: <53c844e1-a21a-e481-cca5-2184c8aa47b9@iki.fi>
Date: Thu, 28 Sep 2023 12:58:12 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: Latches vs lwlock contention
Content-Language: en-US
To: Thomas Munro <thomas.munro@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Yura Sokolov <y.sokolov@postgrespro.ru>,
 Andres Freund <andres@anarazel.de>
References: <CA+hUKGKmO7ze0Z6WXKdrLxmvYa=zVGGXOO30MMktufofVwEm1A@mail.gmail.com>
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <CA+hUKGKmO7ze0Z6WXKdrLxmvYa=zVGGXOO30MMktufofVwEm1A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/53c844e1-a21a-e481-cca5-2184c8aa47b9%40iki.fi>
Precedence: bulk

On 28/10/2022 06:56, Thomas Munro wrote:
> One example is heavyweight lock wakeups.  If you run BEGIN; LOCK TABLE
> t; ... and then N other sessions wait in SELECT * FROM t;, and then
> you run ... COMMIT;, you'll see the first session wake all the others
> while it still holds the partition lock itself.  They'll all wake up
> and begin to re-acquire the same partition lock in exclusive mode,
> immediately go back to sleep on*that*  wait list, and then wake each
> other up one at a time in a chain.  We could avoid the first
> double-bounce by not setting the latches until after we've released
> the partition lock.  We could avoid the rest of them by not
> re-acquiring the partition lock at all, which ... if I'm reading right
> ... shouldn't actually be necessary in modern PostgreSQL?  Or if there
> is another reason to re-acquire then maybe the comment should be
> updated.

ISTM that the change to not re-aqcuire the lock in ProcSleep is 
independent from the other changes. Let's split that off to a separate 
patch.

I agree it should be safe. Acquiring a lock just to hold off interrupts 
is overkill anwyway, HOLD_INTERRUPTS() would be enough. 
LockErrorCleanup() uses HOLD_INTERRUPTS() already.

There are no CHECK_FOR_INTERRUPTS() in GrantAwaitedLock(), so cancel/die 
interrupts can't happen here. But could we add HOLD_INTERRUPTS(), just 
pro forma, to document the assumption? It's a little awkward: you really 
should hold interrupts until the caller has done "awaitedLock = NULL;". 
So it's not quite enough to add a pair of HOLD_ and RESUME_INTERRUPTS() 
at the end of ProcSleep(). You'd need to do the HOLD_INTERRUPTS() in 
ProcSleep() and require the caller to do RESUME_INTERRUPTS(). In a 
sense, ProcSleep downgrades the lock on the partition to just holding 
off interrupts.

Overall +1 on this change to not re-acquire the partition lock.

-- 
Heikki Linnakangas
Neon (https://neon.tech)