On Fri, Jan 05, 2024 at 03:00:25PM +0530, Dilip Kumar wrote:
> 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
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;
>
> Some comments would be good for the structure
Sure. I've spent more time documenting things in injection_point.c,
addressing any inconsistencies.
> 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?
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");
>
> 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.
> + */
>
> 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:
>
> 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