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
