I'd like to spend some more time reviewing this one, but here are a couple
of comments.
On Tue, Dec 12, 2023 at 11:44:57AM +0100, Michael Paquier wrote:
> +/*
> + * Allocate shmem space for dynamic shared hash.
> + */
> +void
> +InjectionPointShmemInit(void)
> +{
> +#ifdef USE_INJECTION_POINTS
> + HASHCTL info;
> +
> + /* key is a NULL-terminated string */
> + info.keysize = sizeof(char[INJ_NAME_MAXLEN]);
> + info.entrysize = sizeof(InjectionPointEntry);
> + InjectionPointHash = ShmemInitHash("InjectionPoint hash",
> +
> INJECTION_POINT_HASH_INIT_SIZE,
> +
> INJECTION_POINT_HASH_MAX_SIZE,
> +
> &info,
> +
> HASH_ELEM | HASH_STRINGS);
> +#endif
> +}
Should we specify HASH_FIXED_SIZE, too? This hash table will be in the
main shared memory segment and therefore won't be able to expand too far
beyond the declared maximum size.
> + /*
> + * Check if the callback exists in the local cache, to avoid unnecessary
> + * external loads.
> + */
> + injection_callback = injection_point_cache_get(name);
> + if (injection_callback == NULL)
> + {
> + char path[MAXPGPATH];
> +
> + /* Found, so just run the callback registered */
> + snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
> + entry_by_name->library, DLSUFFIX);
> +
> + if (!file_exists(path))
> + elog(ERROR, "could not find injection library \"%s\"",
> path);
> +
> + injection_callback = (InjectionPointCallback)
> + load_external_function(path, entry_by_name->function,
> true, NULL);
> +
> + /* add it to the local cache when found */
> + injection_point_cache_add(name, injection_callback);
> + }
I'm wondering how important it is to cache the callbacks locally.
load_external_function() won't reload an already-loaded library, so AFAICT
this is ultimately just saving a call to dlsym().
> + <literal>name</literal> is the name of the injection point, that
> + will execute the <literal>function</literal> loaded from
> + <literal>library</library>.
> + Injection points are saved in a hash table in shared memory, and
> + last until the server is shut down.
> + </para>
I think </library> is supposed to be </literal> here.
> +++
> b/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl
0003 and 0004 add tests to the test_injection_points module. Is the idea
that we'd add any tests that required injection points here? I think it'd
be better if we could move the tests closer to the logic they're testing,
but perhaps that is difficult because you also need to define the callback
functions somewhere. Hm...
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
