Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 9 Jan 2024 10:09:31 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Adding facility for injection points (or probe points?) for more
 advanced tests
Message-ID: <ZZycyzpgphnbTd2U@paquier.xyz>
References: <ZWACtHPetBFIvP61@paquier.xyz>
 <CAExHW5usVs10M4i5RD68r5fpbr3mdzePnT589nT4X3cm7L64sQ@mail.gmail.com>
 <ZWCuG1GpB0bi49kk@paquier.xyz>
 <CAExHW5vyURNaht1YYJfZMk-Kyh=TD6FhE4NEi5zTAhnOj0qwcA@mail.gmail.com>
 <ZWUaACRWoH4WddLY@paquier.xyz>
 <CAFiTN-u+xrjmgK+3-TCjug_kqwWwUB3CXiOVjXNE1=vcf4JRUQ@mail.gmail.com>
 <ZXbaDg5fBl8GLy2u@paquier.xyz>
 <CAFiTN-s88VUSVXWGKxDgHwcycVtM=ZEOMBNtPU0FRPRq4bMgDw@mail.gmail.com>
 <ZXg5qUjKSe5XQffQ@paquier.xyz>
 <CAFiTN-uTQ1CytzDfx3_-wjtJ3OTPG0T2tgehi5k4BaVm0FJGXw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Zw72eCXfldru36gj"
Content-Disposition: inline
In-Reply-To: <CAFiTN-uTQ1CytzDfx3_-wjtJ3OTPG0T2tgehi5k4BaVm0FJGXw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZZycyzpgphnbTd2U%40paquier.xyz>
Precedence: bulk


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

On Fri, Jan 05, 2024 at 03:00:25PM +0530, Dilip Kumar wrote:
> Some comments in 0001, mostly cosmetics
>=20
> 1.
> +/* utilities to handle the local array cache */
> +static void
> +injection_point_cache_add(const char *name,
> +   InjectionPointCallback callback)
>=20
> I think the comment for this function should be more specific about
> adding an entry to the local injection_point_cache_add.  And add
> comments for other functions as well e.g. injection_point_cache_get

And it is not an array anymore.  Note InjectionPointArrayEntry that
still existed.

> 2.
> +typedef struct InjectionPointEntry
> +{
> + char name[INJ_NAME_MAXLEN]; /* hash key */
> + char library[INJ_LIB_MAXLEN]; /* library */
> + char function[INJ_FUNC_MAXLEN]; /* function */
> +} InjectionPointEntry;
>=20
> Some comments would be good for the structure

Sure.  I've spent more time documenting things in injection_point.c,
addressing any inconsistencies.

> 3.
>=20
> +static bool
> +file_exists(const char *name)
> +{
> + struct stat st;
> +
> + Assert(name !=3D NULL);
> + if (stat(name, &st) =3D=3D 0)
> + return !S_ISDIR(st.st_mode);
> + else if (!(errno =3D=3D ENOENT || errno =3D=3D ENOTDIR))
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not access file \"%s\": %m", name)));
> + return false;
> +}
>=20
> dfmgr.c has a similar function so can't we reuse it by making that
> function external?

Yes.  Note that jit.c has an extra copy of it.  I was holding on the
refactoring, but let's bite the bullet and have a single routine.
I've moved that into a 0001 that builds on top of the rest.

> 4.
> + if (found)
> + {
> + LWLockRelease(InjectionPointLock);
> + elog(ERROR, "injection point \"%s\" already defined", name);
> + }
> +
> ...
> +#else
> + elog(ERROR, "Injection points are not supported by this build");
>=20
> Better to use similar formatting for error output, Injection vs
> injection (better not to capitalize the first letter for consistency
> pov)

Fixed.

> 5.
> + * Check first the shared hash table, and adapt the local cache
> + * depending on that as it could be possible that an entry to run
> + * has been removed.
> + */
>=20
> What if the entry is removed after we have released the
> InjectionPointLock? Or this would not cause any harm?

With an entry found in the shmem table?  I don't really think that we
need to care about such cases, TBH, because the injection point would
have been found in the table to start with.  This comes down to if we
should try to hold InjectionPointLock while calling the callback, and
that may not be a good idea in some cases if you'd expect a high
concurrency on the callback running.

> 0004:
>=20
> I think
> test_injection_points_wake() and test_injection_wait() can be moved as
> part of 0002

Nah.  I intend to keep the introduction of this API where it becomes
relevant.  Perhaps this could also use an isolation test?  This could
always be polished once we agree on 0001 and 0002.

(I'll post a v6 a bit later, there are more comments posted here and
there.)
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmWcnMsACgkQnvQgOdby
QH0uig/+LuM1QIbd6rJwwYTJTZRRCm0ba16TfXrd8PFrlW7JRTEF5i3Kn5mN/IwU
swgR/UYYVJmI+KMaMc8Wa0JIHwI6dqpoF3QI1wNTjKsqvxadbh8A6RizzoeN2jRh
JZEmJmthIKIJDxHATb7DIrC4OctJJ0UMH8fMp966AexQvZLUrVVA19OG9qJfMAtr
u4ZCXiNf8qx2BrBDVeD5bp6lnfoYogIcJU+zoaiC4lVijPA2PwEl6ETSLoXTB5G1
8GyqjKZDoxbIqp0FIMH7xmzGPbBinjKkrQ1GZChpx4fEqAPhDXB9ERl7wBak8xQB
bjbB08c6YYKt3WHgsG2PrQs86tPA4Bvj7wCu++PzVJNejuKDX0ETNwAo7RQrTvbO
yWBP0+wmJm9fl6zAsIE8B00Z8dweMLRTwad5wCu0hU49W3MNVEh5go9ghZ2XBX70
dPMYGYv+4RWyaowhvWZtaqy4gxclzPgAlEW+V8kUzigEHYr9vGc1GLTHx9PwBX1E
R7ZePD55JDz2QVCDLY7Hyo7Ka+wvNptkQxDLWK1tDIZbMHtLbhG2qEfpZiV3TOE8
qlX+FC+XKoL79sSc/y5TdnZzSSaUPhnhNoLB7o3tiiG5SK6CIxLj0Xo42BQ3I5z6
x2P+srNB46UdIqYzOggHgOoCjxG6xrPqUVHGnnlPbSfpt6F75lM=
=Jp2Q
-----END PGP SIGNATURE-----

--Zw72eCXfldru36gj--