Thread: separating use of SerialSLRULock

Hello

In Dilip's patch to improve SLRU concurrency, there's a requirement to
prevent predicate.c's SLRU control lock from being used to control
access to another shared memory structure, SerialControlData.  This
struct is used to keep track of the areas of the SLRU that are valid.
Dilip just embedded that change into the patch he submitted, but I think
the patch is actually wrong in detail, because it's leaving the
SerialAdd() function unchanged to use SerialSLRULock, which is wrong
because that function depends heavily on the contents of
SerialControlData, and it also modifies it.  So if run concurrently with
the other functions that are using SerialControlLock, the result would
make no sense.

I came up with the attached.  If there are no objections, I'll be
pushing this shortly.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

On 2024-Jan-29, Alvaro Herrera wrote:

> In Dilip's patch to improve SLRU concurrency, there's a requirement to
> prevent predicate.c's SLRU control lock from being used to control
> access to another shared memory structure, SerialControlData.  This
> struct is used to keep track of the areas of the SLRU that are valid.
> Dilip just embedded that change into the patch he submitted, but I think
> the patch is actually wrong in detail, because it's leaving the
> SerialAdd() function unchanged to use SerialSLRULock, which is wrong
> because that function depends heavily on the contents of
> SerialControlData, and it also modifies it.

It's terrifying that SerialAdd() doesn't seem to be covered by any
tests, though.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I think my standards have lowered enough that now I think 'good design'
is when the page doesn't irritate the living f*ck out of me." (JWZ)

On 2024-Jan-29, Alvaro Herrera wrote:

> It's terrifying that SerialAdd() doesn't seem to be covered by any
> tests, though.

I realized that there's some coverage when compiling with
TEST_SUMMARIZE_SERIAL, so I tried that and it looks OK.

One other change I made was in the comment that explains the locking
order.  I had put the new lock at the top, but when I tested adding some
asserts to verify that the other locks are not held, they turn out to
fire soon enough ... and the conflicting lock is the last one of that
list.  So I added the new lock below it, and the SLRU lock further down,
because SerialAdd does it that way.

I pushed it now.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/