On Fri, Mar 13, 2026 at 7:41 AM Heikki Linnakangas <[email protected]> wrote: > Yeah. IMHO the existing shmem_request/startup_hook mechanism is pretty > awkward too, and in most cases, the new mechanism is more convenient. It > might be slightly less convenient for some things, but mostly it's > better. Would you agree with that, or do you actually like the old hooks > and ShmemInitStruct() better?
I'd say it's not massively different one way or the other. Looking at the pg_stat_statements changes in particular, I feel like in v2, it was actually worse, because you didn't get manage to get rid of pgss_shmem_request(), but it was no longer the thing requesting shmem. v3 is better in that regard: you get rid of some complexity in exchange for what you add. It's not amazing, though: pgss_shmem_startup() now has nothing to do with the name of the hook, which is not this patch's fault but also isn't solved by it. I wonder why in the world somebody decided to jam all of this non-shmem-related logic into this function, and why they didn't add a comment explaining why it was here and not someplace else. One of the worst things about this area is that I often end up having to trace through a bunch of postmaster startup logic to figure out whether any given bit of code is actually in the right place, and that makes me feel like the hooks are badly designed. pgss_shmem_startup() is a good example of that, and the fact that it needs an IsUnderPostmaster check is another. We should somehow try to make it clear what happens with this new mechanism if a module is loaded via shared_preload_libraries vs. if it's loaded via LOAD or session_preload_libraries or whatever. Writing modules that don't require shared_preload_libraries is a boon to users, because they can be added to a production system without a server restart. I wonder whether this new facility handles that case better, worse, or the same as existing facilities. > One such wrinkle with ShmemRegisterStruct() in the patch now is that > it's harder to do initialization that touches multiple structs or hash > tables. Currently the callbacks are called in the same order that the > structs are registered, so you can do all the initialization in the last > struct's callback. The single pair of shmem_request/startup_hooks per > module was more clear in that aspect. Fortunately, that kind of > cross-struct dependencies are pretty rare. So I think it's fine. (The > order that the callbacks are called needs be documented explicitly though). > > If we want to improve on that, one idea would be to introduce a > ShmemRegisterCallbacks() function to register callbacks that are not > tied to any particular struct and are called after all the per-struct > callbacks. I think the whole idea of ShmemInitHash() and ShmemInitStruct() is relatively poorly designed in this regard. Ideally, I want to initialize all the shared memory that belongs to me, and that might include arbitrary data structures, but the current design kind of thinks that I want one struct or one hash table and nothing else. If we're redesigning this mechanism, it would sure be nice to improve on that. Looking just at the pg_stat_statements changes, I think my overall view on this right now is that it's not terrible, but I'm also not that happy about introducing yet another way to do it for this amount of gain. To me, it doesn't yet rise to the level of a clear win. -- Robert Haas EDB: http://www.enterprisedb.com
