On Thu, Mar 19, 2026 at 10:34 AM Heikki Linnakangas <[email protected]> wrote:
> > I suspect
> > that issue is also why pg_stat_statements has the
> > process_shared_preload_libraries_in_progress check at the top, because
> > it looks to me like everything else that the function does would be
> > completely fine to do later.
>
> I think a bigger problem is loading and saving the statistics file. The
> file needs to be saved on postmaster exit, where do you do that if the
> library was not in shared_preload_libraries?
Well, there's no way to install a hook in the postmaster in that case,
so you can't. But I'm not sure that justifies skipping everything
_PG_init() would have done. A problem with the status quo is that
every module author makes their own decision about how to handle the
s_p_l problem, and they don't all decide differently, even in our own
tree. For example, autoprewarm chooses to register the GUC that it can
while skipping the other one, while pg_stat_statements skips
everything, including hook installation. Maybe that's properly
considered flexibility that should be left to each individual author,
but to me it seems more like happenstance than anything else. I'd
favor a design that emphasizes severability - i.e. you always try to
do as much as possible, and defer errors until later. So you always
create the GUCs but then restrict setting them to values that you
can't support without a restart, instead of not creating them. You
install the hooks and then maybe they will have to no-op out. It's
just weird if you load a library and the GUCs aren't defined and
there's not even a diagnostic telling you why.
> If you squint a little, this is pretty much the same as my descriptor
> design. Let's start from your DefineShmemRegion function, but in order
> to have some flexibility to add optional optional in the future, without
> having to create DefineShmemRegionNew(), DefineShmemRegionExt() or
> similar functions, let's pass the arguments in a struct. So instead of:
>
> DefineShmemRegion("pg_stat_statements", sizeof(pgssSharedState), &pgss,
> &pgss,pgss_shmem_init, 0);
>
> you would call it like this:
>
> DefineShmemRegion(&(ShmemStructDesc) {
> .name = "pg_stat_statements",
> .size = sizeof(pgssSharedState),
> .ptr = (void **) &pgss,
> .init_fn = pgss_shmem_init,
> .flags = 0,
> });
>
> This flexibility will come handy as soon as we add the ability to resize.
I see your point. I'm not really convinced, though. In practice,
what's now going to happen is that you're probably going to move that
struct out of _PG_init() and define it elsewhere, so the logic is
getting split between multiple places, which does not improve
readability, and it also becomes much more worrying to what degree the
struct needs to be const, whereas if you just pass a bunch of
parameters by value you kind of understand what has to be happening.
Also, what flexibility have you really purchased? Sure, now you can
add arguments to the function call without breaking existing call
sites, but (1) there are other ways to do that, like by creating an
object first and then using methods to assign properties to it
afterward (i.e. RegisterCallbackOnShmemRegion) and (2) adding
arguments without breaking existing callers is not an unmixed blessing
in the first place. I know the world won't end if you go with this
style, but I guess I'm not much of a fan. I find this sort of thing
hard to read.
> Ok, we could add GetShmemRegion() in either design. Do we have any place
> where we'd use that though, instead the backend-private pointer global
> variable? I can't think of any examples where we currently call
> ShmemInitStruct() to get a pointer "on demand" like that.
>
> In pg_stat_statements, this would replace these tests:
>
> if (!pgss || !pgss_hash)
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("pg_stat_statements must be loaded via
> \"shared_preload_libraries\"")));
>
> But I don't think pg_stat_statements could still allocate the region
> after postmaster startup. GetShmemRegion() would just be a different way
> of throwing that error.
In that case, yes. But something like autoprewarm only needs a small
amount of shared memory, and can potentially initialize itself on the
fly. The not-yet-committed pg_collect_advice module has similar needs,
which it currently satisfies using GetNamedDSMSegment(); see in
particular pg_collect_advice_attach() if you feel like wandering over
to the pg_plan_advice thread.
> Thanks, this hasn't changed my opinions, but I really appreciate
> pressure-testing the design. I don't want to rewrite this again in a
> year, because we didn't get it quite right.
Yeah, I'm somewhat concerned about an ever-proliferating number of
ways to do things that all sort of suck in different ways.
--
Robert Haas
EDB: http://www.enterprisedb.com
