Re: warn if GUC set to an invalid shared library - Mailing list pgsql-hackers

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

I tried the latest version of the patch, and it works as discussed. There is no documentation, but I think that's moot
forthis warning (we may want to note something in the setting docs, but even if so, I think we should figure out the
messagefirst and then decide if it merits additional explanation in the docs). I do not know whether it is
spec-compliant,but I doubt the spec has much to say on something like this.
 

I tried running ALTER SYSTEM and got the warnings as expected:

postgres=# alter system set shared_preload_libraries = no_such_library,not_this_one_either;
WARNING:  could not access file "$libdir/plugins/no_such_library"
WARNING:  could not access file "$libdir/plugins/not_this_one_either"
ALTER SYSTEM

I think this is great, but it would be really helpful to also indicate that at this point the server will fail to come
backup after a restart. In my mind, that's a big part of the reason for having a warning here. Having made this mistake
acouple of times, I would be able to read between the lines, as would many other users, but if you're not familiar with
Postgresthis might still be pretty opaque. I think if I'm reading the code correctly, this warning path is shared
betweenALTER SYSTEM and a SET of local_preload_libraries so it might be tricky to word this in a way that works in all
situations,but it could make the precarious situation a lot clearer. I don't really know a good wording here, but maybe
ahint like "The server or session will not be able to start if it has been configured to use libraries that cannot be
loaded."?

Also, there are two sides to this: one is actually applying the possibly-bogus setting, and the other is when that
settingtakes effect (e.g., attempting to start the server or to start a new session). I think Robert had good feedback
regardingthe latter:
 

On Fri, Jan 28, 2022 at 6:42 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Dec 28, 2021 at 12:45 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > 0002 adds context when failing to start.
> >
> >         2021-12-27 17:01:12.996 CST postmaster[1403] WARNING:  could not load library: $libdir/plugins/asdf: cannot
openshared object file: No such file or directory
 
> >         2021-12-27 17:01:14.938 CST postmaster[1403] FATAL:  could not access file "asdf": No such file or
directory
> >         2021-12-27 17:01:14.938 CST postmaster[1403] CONTEXT:  guc "shared_preload_libraries"
> >         2021-12-27 17:01:14.939 CST postmaster[1403] LOG:  database system is shut down
>
> -1 from me on using "guc" in any user-facing error message. And even
> guc -> setting isn't a big improvement. If we're going to structure
> the reporting this way there, we should try to use a meaningful phrase
> there, probably beginning with the word "while"; see "git grep
> errcontext.*while" for interesting precedents.
>
> That said, that series of messages seems a bit suspect to me, because
> the WARNING seems to be stating the same problem as the subsequent
> FATAL and CONTEXT lines. Ideally we'd tighten that somehow.

Maybe we don't even need the WARNING in this case? At this point, it's clear what the problem is. I think the CONTEXT
linedoes actually help, because otherwise it's not clear why the server failed to start, but the warning does seem
superfluoushere. I do agree that GUC is awkward here, and I like the "while..." wording suggested both for consistency
withother messages and how it could work here:
 

CONTEXT: while loading "shared_preload_libraries"

I think that would be pretty clear. In the ALTER SYSTEM case, you still need to know to edit the file in spite of the
warningtelling you not to edit it, but I think that's still better. Based on Cary's feedback, maybe that could be
clearer,too (like you, I'm not sure if I understood what he did correctly), but I think that could certainly be future
work.

Thanks,
Maciek