From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Heikki Linnakangas <heikki.linnakangas@iki.fi>,
        pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Fix double-release of spinlock
In-reply-to: <20240729174609.jbfei3sokdobeeeo@awork3.anarazel.de>
References: <E1sYSF2-001lEB-D1@gemulon.postgresql.org>
 <2600757.1722267116@sss.pgh.pa.us>
 <20240729161846.kgiafbe2dtgabxgb@awork3.anarazel.de>
 <2802449.1722270793@sss.pgh.pa.us>
 <20240729164026.yurg37tej34o76uk@awork3.anarazel.de>
 <2803949.1722271519@sss.pgh.pa.us>
 <20240729174609.jbfei3sokdobeeeo@awork3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Mon, 29 Jul 2024 10:46:09 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2812375.1722275765.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 29 Jul 2024 13:56:05 -0400
Message-ID: <2812376.1722275765@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2812376.1722275765%40sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
> On 2024-07-29 12:45:19 -0400, Tom Lane wrote:
>> Hmm, but how?

> ...
> I.e. the version using 2 as the locked state uses a three byte instructi=
on vs
> a two byte instruction before.
> *If* we are worried about this, we could
> a) Change the representation only for assert enabled builds, but that'd =
have
>    ABI issues again.

Agreed, that would be a very bad idea.  It would for example break the
case of a non-assert-enabled extension used with an assert-enabled
core or vice versa, which is something we've gone out of our way to
allow.

> b) Instead define the spinlock to have 1 as the unlocked state and 0 as =
the
>    locked state. That makes it a bit harder to understand that initializ=
ation
>    is missing, compared to a dedicated state, as the first use of the sp=
inlock
>    just blocks.

This option works for me.

> To make 1) b) easier to understand it might be worth changing the slock_=
t
> typedef to be something like

> typedef struct slock_t
> {
>         char is_free;
> } slock_t;

+1

How much of this would we change across platforms, and how much
would be x86-only?  I think there are enough people developing on
ARM (e.g. Mac) now to make it worth covering that, but maybe we
don't care so much about anything else.

			regards, tom lane