Re: pgsql: Fix double-release of spinlock

Hi,

On 2024-07-29 12:33:13 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2024-07-29 11:31:56 -0400, Tom Lane wrote:
> >> There was some recent discussion about getting rid of
> >> --disable-spinlocks on the grounds that nobody would use
> >> hardware that lacked native spinlocks.  But now I wonder
> >> if there is a testing/debugging reason to keep it.
> 
> > Seems it'd be a lot more straightforward to just add an assertion to the
> > x86-64 spinlock implementation verifying that the spinlock isn't already free?

FWIW, I quickly hacked that up, and it indeed quickly fails with 0393f542d72^
and passes with 0393f542d72.


> I dunno, is that the only extra check that the --disable-spinlocks
> implementation is providing?

I think it also provides the (valuable!) check that spinlocks were actually
initialized. But that again seems like something we'd be better off adding
more general infrastructure for - nobody runs --disable-spinlocks locally, we
shouldn't need to run this on the buildfarm to find problems like this.


> I'm kind of allergic to putting Asserts into spinlocked code segments,
> mostly on the grounds that it violates the straight-line-code precept.
> I suppose it's not really that bad for tests that you don't expect
> to fail, but still ...

I don't think the spinlock implementation itself is really affected by that
rule - after all, the --disable-spinlocks implementation actually consists out
of several layers of external function calls (including syscalls in some
cases!).

Greetings,

Andres Freund