Horton

Thread

Commits

  1. Move ProcStructLock to the ProcGlobal struct

  1. Little cleanup: Move ProcStructLock to the ProcGlobal struct

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-10T17:39:17Z

    For some reason, the ProcStructLock spinlock is allocated in a shared 
memory area of its own:

     /* Create ProcStructLock spinlock, too */
     ProcStructLock = (slock_t *) ShmemInitStruct("ProcStructLock spinlock",
                                                  sizeof(slock_t),
                                                  &found);
     SpinLockInit(ProcStructLock);

I believe that's just for historical reasons. A long long time ago, 
spinlocks had to be allocated separately rather than embedded in other 
structs.

The spinlock protects the freeProcs list and some other fields in 
ProcGlobal, so let's put it together with those fields. It's good for 
cache locality to have it next to the thing it protects, and just makes 
more sense anyway.

Any objections?

- Heikki

  2. Re: Little cleanup: Move ProcStructLock to the ProcGlobal struct

    Chao Li <li.evan.chao@gmail.com> — 2026-02-11T03:16:07Z

    
> On Feb 11, 2026, at 01:39, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
> For some reason, the ProcStructLock spinlock is allocated in a shared memory area of its own:
> 
>    /* Create ProcStructLock spinlock, too */
>    ProcStructLock = (slock_t *) ShmemInitStruct("ProcStructLock spinlock",
>                                                 sizeof(slock_t),
>                                                 &found);
>    SpinLockInit(ProcStructLock);
> 
> I believe that's just for historical reasons. A long long time ago, spinlocks had to be allocated separately rather than embedded in other structs.
> 
> The spinlock protects the freeProcs list and some other fields in ProcGlobal, so let's put it together with those fields. It's good for cache locality to have it next to the thing it protects, and just makes more sense anyway.
> 
> Any objections?
> 
> - Heikki<0001-Move-ProcStructLock-to-the-ProcGlobal-struct.patch>

Hi Heikki,

I took a quick review. You moved ProcStructLock into PROC_HDR as freeProcsLock, and deleted:
```
ProcStructLock = ShmemInitStruct(...);
SpinLockInit(ProcStructLock);
```

But I don’t see a replacement like SpinLockInit(&ProcGlobal->freeProcsLock);

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  3. Re: Little cleanup: Move ProcStructLock to the ProcGlobal struct

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-02-11T11:51:55Z

    On Wed, Feb 11, 2026 at 8:46 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Feb 11, 2026, at 01:39, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >
> > For some reason, the ProcStructLock spinlock is allocated in a shared memory area of its own:
> >
> >    /* Create ProcStructLock spinlock, too */
> >    ProcStructLock = (slock_t *) ShmemInitStruct("ProcStructLock spinlock",
> >                                                 sizeof(slock_t),
> >                                                 &found);
> >    SpinLockInit(ProcStructLock);
> >
> > I believe that's just for historical reasons. A long long time ago, spinlocks had to be allocated separately rather than embedded in other structs.
> >
> > The spinlock protects the freeProcs list and some other fields in ProcGlobal, so let's put it together with those fields. It's good for cache locality to have it next to the thing it protects, and just makes more sense anyway.
> >
> > Any objections?
> >
> > - Heikki<0001-Move-ProcStructLock-to-the-ProcGlobal-struct.patch>
>
> Hi Heikki,
>
> I took a quick review. You moved ProcStructLock into PROC_HDR as freeProcsLock, and deleted:
> ```
> ProcStructLock = ShmemInitStruct(...);
> SpinLockInit(ProcStructLock);
> ```
>
> But I don’t see a replacement like SpinLockInit(&ProcGlobal->freeProcsLock);

Good catch. I think the spinlock needs to be initialized somewhere in
the code block starting with
/*
* Initialize the data structures.
*/

I also checked many other shared structures which contain spinlocks in
them. All of them embed the spinlock instead of pointer to the
spinlock. This change looks inline with that.

-- 
Best Wishes,
Ashutosh Bapat

  4. Re: Little cleanup: Move ProcStructLock to the ProcGlobal struct

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-02-11T14:52:56Z

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> On Wed, Feb 11, 2026 at 8:46 AM Chao Li <li.evan.chao@gmail.com> wrote:
>> But I don’t see a replacement like SpinLockInit(&ProcGlobal->freeProcsLock);

> Good catch.

Undoubtedly, this escaped Heikki's notice because on all supported
platforms SpinLockInit() initializes the spinlock value to zero,
but shared memory starts out zeroes anyway.

We used to have better odds of catching such mistakes.  My old HPPA
dinosaur would have caught it by dint of needing a nonzero initial
value, but that hardware is long gone.  The test infrastructure
we used to have for emulating spinlocks with SysV semaphores would
have caught it too, I think, but that's also gone.

This is not a great situation.  I wonder if we can put back some
mode that could be used by a few BF members to catch such oversights.

			regards, tom lane

  5. Re: Little cleanup: Move ProcStructLock to the ProcGlobal struct

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-11T15:02:47Z

    On 11/02/2026 13:51, Ashutosh Bapat wrote:
> On Wed, Feb 11, 2026 at 8:46 AM Chao Li <li.evan.chao@gmail.com> wrote:
>> I took a quick review. You moved ProcStructLock into PROC_HDR as freeProcsLock, and deleted:
>> ```
>> ProcStructLock = ShmemInitStruct(...);
>> SpinLockInit(ProcStructLock);
>> ```
>>
>> But I don’t see a replacement like SpinLockInit(&ProcGlobal->freeProcsLock);
> 
> Good catch. I think the spinlock needs to be initialized somewhere in
> the code block starting with
> /*
> * Initialize the data structures.
> */
> 
> I also checked many other shared structures which contain spinlocks in
> them. All of them embed the spinlock instead of pointer to the
> spinlock. This change looks inline with that.

Fixed the initialization and pushed. Thanks!

- Heikki

  6. Re: Little cleanup: Move ProcStructLock to the ProcGlobal struct

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-11T15:05:27Z

    On 11/02/2026 16:52, Tom Lane wrote:
> Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
>> On Wed, Feb 11, 2026 at 8:46 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>> But I don’t see a replacement like SpinLockInit(&ProcGlobal->freeProcsLock);
> 
>> Good catch.
> 
> Undoubtedly, this escaped Heikki's notice because on all supported
> platforms SpinLockInit() initializes the spinlock value to zero,
> but shared memory starts out zeroes anyway.

Right.

> We used to have better odds of catching such mistakes.  My old HPPA
> dinosaur would have caught it by dint of needing a nonzero initial
> value, but that hardware is long gone.  The test infrastructure
> we used to have for emulating spinlocks with SysV semaphores would
> have caught it too, I think, but that's also gone.
> 
> This is not a great situation.  I wonder if we can put back some
> mode that could be used by a few BF members to catch such oversights.

Do we still support any architectures where initializing the spinlock to 
all-zeros doesn't do the right thing? Could we accept that all-zeros is 
a valid initialization of a spinlock? This reminds me that Thomas was 
working on re-implementing spinlocks with atomics [1]. With that least, 
I presume we could.

[1] 
https://www.postgresql.org/message-id/CA%2BhUKGKFvu3zyvv3aaj5hHs9VtWcjFAmisOwOc7aOZNc5AF3NA%40mail.gmail.com

- Heikki

  7. Re: Little cleanup: Move ProcStructLock to the ProcGlobal struct

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-02-11T15:29:00Z

    Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 11/02/2026 16:52, Tom Lane wrote:
>> This is not a great situation.  I wonder if we can put back some
>> mode that could be used by a few BF members to catch such oversights.

> Do we still support any architectures where initializing the spinlock to 
> all-zeros doesn't do the right thing? Could we accept that all-zeros is 
> a valid initialization of a spinlock?

I'm not terribly comfortable with that: it seems short-sighted.
Even today, on platforms where we use __sync_lock_test_and_set /
__sync_lock_release, the gcc manual does not quite promise that
the released state is all-zero.

			regards, tom lane