Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 20 Feb 2024 08:28:28 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>,
	Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Subject: Re: Injection points: some tools to wait and wake
Message-ID: <ZdPkHNMK8s4o0tV4@paquier.xyz>
References: <ZdLuxBk5hGpol91B@paquier.xyz>
 <ZdMIkfJnBj6rBQrL@paquier.xyz>
 <ZdNldOxVx4SL3+f0@ip-10-97-1-34.eu-west-3.compute.internal>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="AykIMfLkuoKn5BRT"
Content-Disposition: inline
In-Reply-To: <ZdNldOxVx4SL3+f0@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/ZdPkHNMK8s4o0tV4%40paquier.xyz>
Precedence: bulk


--AykIMfLkuoKn5BRT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 19, 2024 at 02:28:04PM +0000, Bertrand Drouvot wrote:
> +CREATE FUNCTION injection_points_wake()
>=20
> what about injection_points_wakeup() instead?

Sure.

> +/* Shared state information for injection points. */
> +typedef struct InjectionPointSharedState
> +{
> +       /* protects accesses to wait_counts */
> +       slock_t         lock;
> +
> +       /* Counter advancing when injection_points_wake() is called */
> +       int                     wait_counts;
>=20
> If slock_t protects "only" one counter, then what about using pg_atomic_u=
int64
> or pg_atomic_uint32 instead? And btw do we need wait_counts at all? (see =
comment=20
> number 4)

We could, indeed, even if we use more than one counter.  Still, I
would be tempted to keep it in case more data is added to this
structure as that would make for less stuff to do while being normally
non-critical.  This sentence may sound pedantic, though..

> + * SQL function for waking a condition variable
>=20
> waking up instead?

Okay.

> I'm wondering if this loop and wait_counts are needed, why not doing some=
thing
> like (and completely get rid of wait_counts)?
>=20
> "
>     ConditionVariablePrepareToSleep(&inj_state->wait_point);
>     ConditionVariableSleep(&inj_state->wait_point, injection_wait_event);
>     ConditionVariableCancelSleep();
> "
>=20
> It's true that the comment above ConditionVariableSleep() mentions that:

Perhaps not, but it encourages good practices around the use of
condition variables, and we need to track all that in shared memory
anyway.  Ashutosh has argued in favor of the approach taken by the
patch in the original thread when I've sent a version doing exactly
what you are saying now to not track a state in shmem.
--
Michael

--AykIMfLkuoKn5BRT
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmXT5BwACgkQnvQgOdby
QH2ovg//fNIRkfVrCrobrTVEAiEcdehSgj7nnBbknOZC74ZJW5hnh0cj1w8s4417
zXmFMxgusKke3zTF9idHQK/Wpz+owUhrMr4sR6054cosMlf30NMNHnfAfxL4pDZE
s4mRkbQQBzTf0Geh7QngtLDsopSkloUq1aSYTwhICXiAPG5F7KdKA6XvjoKxGz+L
YgpD49N4O4LUEe+/caLva0aBSqraK5qxnRRjy5eqRfuqYfsDcP56HPgvI3kANo1o
noXW3pn4dlIONVgCvS3o108vM/FBg3PwnCuoNfVHyKlcdihQMkvank4IjXcCNP/j
Xd9ENo5z77/5MqRjTpNw7eMn6UilKSpXiCN+NqIrF4aLIlMiLdkNayCxrwWDW3hp
nMVSQjvtg0IvSNx3WKuKZ4lBHuiC2J83g9MbRTz0wcUj8nTzqM/1ppbsbb6apsPN
1i28zbxLO8ozL57t3YYmKqFWgGswAeSdaciCML9oACNj7aGSeRsnQXwGoAsT96Gp
qXYmvUD2bXgh9pQJqzACJzH4tGB9IojqUB4JFtTByhbsRz4wLeV6T6QWGmBLcULl
50AeqPPlWOgPtZgihIxP0HoFZX3LFWUg2jmBCvnT7K4iQjSmzj/XT3Fv102/8HeE
zFeP2fxd+872rmkQ5hyVTHKAcaDhyJywOQvc6WCs6O75zoQ5Qwk=
=2PZC
-----END PGP SIGNATURE-----

--AykIMfLkuoKn5BRT--