Re: Adding facility for injection points (or probe points?) for more advanced tests

On Tue, Dec 12, 2023 at 4:15 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Dec 12, 2023 at 10:27:09AM +0530, Dilip Kumar wrote:
> > Oops, I only included the code changes where I am adding injection
> > points and some comments to verify that, but missed the actual test
> > file. Attaching it here.
>
> I see.  Interesting that this requires persistent connections to work.
> That's something I've found clunky to rely on when the scenarios a
> test needs to deal with are rather complex.  That's an area that could
> be made easier to use outside of this patch..  Something got proposed
> by Andrew Dunstan to make the libpq routines usable through a perl
> module, for example.
>
> > Note:  I think the latest patches are conflicting with the head, can you rebase?
>
> Indeed, as per the recent manipulations in ipci.c for the shmem
> initialization areas.  Here goes a v6.

Some comments in 0001, mostly cosmetics

1.
+/* utilities to handle the local array cache */
+static void
+injection_point_cache_add(const char *name,
+   InjectionPointCallback callback)

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


2.
+typedef struct InjectionPointEntry
+{
+ char name[INJ_NAME_MAXLEN]; /* hash key */
+ char library[INJ_LIB_MAXLEN]; /* library */
+ char function[INJ_FUNC_MAXLEN]; /* function */
+} InjectionPointEntry;

Some comments would be good for the structure

3.

+static bool
+file_exists(const char *name)
+{
+ struct stat st;
+
+ Assert(name != NULL);
+ if (stat(name, &st) == 0)
+ return !S_ISDIR(st.st_mode);
+ else if (!(errno == ENOENT || errno == ENOTDIR))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not access file \"%s\": %m", name)));
+ return false;
+}

dfmgr.c has a similar function so can't we reuse it by making that
function external?

4.
+ if (found)
+ {
+ LWLockRelease(InjectionPointLock);
+ elog(ERROR, "injection point \"%s\" already defined", name);
+ }
+
...
+#else
+ elog(ERROR, "Injection points are not supported by this build");

Better to use similar formatting for error output, Injection vs
injection (better not to capitalize the first letter for consistency
pov)

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.
+ */

What if the entry is removed after we have released the
InjectionPointLock? Or this would not cause any harm?


0004:

I think
test_injection_points_wake() and test_injection_wait() can be moved as
part of 0002



--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com