Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 21 Feb 2024 07:08:03 +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: <ZdUiw17n4Vy7aKMs@paquier.xyz>
References: <ZdLuxBk5hGpol91B@paquier.xyz>
 <ZdMIkfJnBj6rBQrL@paquier.xyz>
 <ZdNldOxVx4SL3+f0@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZdPkHNMK8s4o0tV4@paquier.xyz>
 <ZdTLXCnN0EV0e/KF@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="Phy/pLWPXlcqd0hk"
Content-Disposition: inline
In-Reply-To: <ZdTLXCnN0EV0e/KF@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/ZdUiw17n4Vy7aKMs%40paquier.xyz>
Precedence: bulk


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

On Tue, Feb 20, 2024 at 03:55:08PM +0000, Bertrand Drouvot wrote:
> Okay, makes sense to keep this as it is as a "template" in case more stuf=
f is
> added.=20
>=20
> +       /* Counter advancing when injection_points_wake() is called */
> +       int                     wait_counts;
>=20
> In that case what about using an unsigned instead? (Nit)

uint32.  Sure.

> 1 =3D=3D=3D
>=20
> +void
> +injection_wait(const char *name)
>=20
> Looks like name is not used in the function. I guess the reason it is a p=
arameter
> is because that's the way the callback function is being called in
> InjectionPointRun()?

Right.  The callback has to define this argument.

> 2 =3D=3D=3D
>=20
> +PG_FUNCTION_INFO_V1(injection_points_wake);
> +Datum
> +injection_points_wake(PG_FUNCTION_ARGS)
> +{
>=20
> I think that This function will wake up all the "wait" injection points.
> Would that make sense to implement some filtering based on the name? That=
 could
> be useful for tests that would need multiple wait injection points and th=
at want
> to wake them up "sequentially".
>=20
> We could think about it if there is such a need in the future though.

Well, both you and Andrey are asking for it now, so let's do it.  The
implementation is simple:
- Store in InjectionPointSharedState an array of wait_counts and an
array of names.  There is only one condition variable.
- When a point wants to wait, it takes the spinlock and looks within
the array of names until it finds a free slot, adds its name into the
array to reserve a wait counter at the same position, releases the
spinlock.  Then it loops on the condition variable for an update of
the counter it has reserved.  It is possible to make something more
efficient, but at a small size it would not really matter.
- The wakeup takes a point name in argument, acquires the spinlock,
and checks if it can find the point into the array, pinpoints the
location of the counter to update and updates it.  Then it broadcasts
the change.
- The wait loop checks its counter, leaves its loop, cancels the
sleep, takes the spinlock to unregister from the array, and leaves.

I would just hardcode the number of points that can wait, say 5 of
them tracked in shmem?  Does that look like what you are looking at?

> +# Register a injection point on the standby so as the follow-up
>=20
> typo: "an injection"?

Oops.  Fixed locally.

> +for (my $i =3D 0; $i < 3000; $i++)
> +{
>=20
> is 3000 due to?:
>=20
> +checkpoint_timeout =3D 30s
>=20
> If so, would that make sense to reduce the value for both?

That had better be based on PostgreSQL::Test::Utils::timeout_default,
actually, as in something like:
foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default)
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmXVIsMACgkQnvQgOdby
QH2v0g//Z08I7bNJVrzX0jSKVP5AIZAaRwk73Ix7wK063BGfQSnr5ylvbRTrw/Nx
nbTc6LbGQ6jmbNgPnaxpBRkKIJkuTixu9/TA6T5DZ2nvUwgOzHebZRKVuexd0+I3
re4RxKpBD2KEIHvuYxBWenr8/cLUxPEphzt+BbY2R5cKY0ggBIeV0C+pVq2LqIgn
hHkngNYJn/t60OA64KFz+xJCtmRDdEQSWpxnpbLk++lTH3nz02QocAXs51xdYEcZ
aAfE0bN24mhpXng8PEbk8eFsZwFJqJM5oea1JM6etfNtyotiwhQj05aHEWMjZHfc
fmhFUpEx8bkUH7Gp4P/JlsrhwcGtrZLvTlkUyz4kIlw/uxu3iEuBARFE6ZU/jfrw
2uQxnRrcNGRxuxI2RsdNK5mfPnrCPt8I7sQuosBbXpgfXnpH+x7oMGzezHnGzN1G
JCgYo+xr9adSfCHDh4MJS6NFYIvh/outSR6ohoBpN7y8GhI4PFldPdXVv59CiYYI
lYqLcf8yLoqQAvPhYarBaCemIBa3C+vpzD/rDw6iGsxxNcpp+B5RDe6Vmz3yUrn1
unAhDhe0SsNgz0nNcqQf31RuoZ84XAil1H+NwyiQsKyzl70Ji7rjlupQzdzuxMZs
u7bFmAHL2x4baNxW2/RbP9oVY+XIGufrNP3P++5y1B6hmLvRE8I=
=F9Mw
-----END PGP SIGNATURE-----

--Phy/pLWPXlcqd0hk--