On 3/22/25 23:49, Tom Lane wrote:
> I spent awhile reviewing the v5 patch, and here's a proposed v6.
> Some notes:
>
> * I didn't like depending on offsetof(Pg_magic_struct, module_extra)
> to determine which parts of the struct are checked for compatibility.
> It just seems way too easy to break that with careless insertion
> of new fields, and such breakage might not cause obvious failures.
> I think the right thing is to break out the ABI-checking fields as
> their own sub-struct, rather than breaking out the new fields as a
> sub-struct.
Agree. It is a clear approach. I like it.
>
> * I renamed the inquiry function to pg_get_loaded_modules, since
> it only works on loaded modules but that's hardly clear from the
> previous name.
+1
>
> * It is not clear to me what permission restrictions we should put
> on pg_get_loaded_modules, ...
I vote for the idea of stripping the full path to just a filename. My
initial use cases were:
1. User reports the issue and need to provide me all loaded modules at
the moment of query execution. Higher privileges needs administrative
procedures that is a long way and not all the time possible.
2. A module needs to detect another loaded module - it is not a frequent
case so far, but concurrency on queryId with pg_stat_statements is at
least one of my examples happening sometimes.
Also, permissions here should be in agreement with permissions on
pg_available_extensions(), right?
>
> * I didn't like anything about the test setup. ...
Ok, thanks. I just played with alternatives.
>
> Still TBD:
>
> * I'm not happy with putting pg_get_loaded_modules into dfmgr.c.
> It feels like the wrong layer to have a SQL-callable function,
> and the large expansion in its #include list is evidence that we're
> adding functionality that doesn't belong there. But I'm not quite
> sure where to put it instead. Also, the naive way to do that would
> require exporting DynamicFileList which doesn't feel nice either.
> Maybe we could make dfmgr.c export some sort of iterator function?
I just attempted to reduce number of exported objects here. If it is ok
to introduce an iterator, the pg_get_loaded_modules() may live in
extension.c
>
> * Should we convert our existing modules to use PG_MODULE_MAGIC_EXT?
> I'm mildly in favor of that, but I think we'd need some automated way
> to manage their version strings, and I don't know what that ought to
> look like. Maybe it'd be enough to make all the in-core modules use
> PG_VERSION as their version string, but I think that might put a dent
> in the idea of the version strings following semantic versioning
> rules.
Yes, additional burden to bump version string was a stopper for me to
propose such a brave idea.
--
regards, Andrei Lepikhov
