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 for this warning (we may want to note something in the setting docs, but even if so, I think we should figure out the message first 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 back up after a restart. In my mind, that's a big part of the reason for having a warning here. Having made this mistake a couple of times, I would be able to read between the lines, as would many other users, but if you're not familiar with Postgres this might still be pretty opaque. I think if I'm reading the code correctly, this warning path is shared between ALTER 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 a hint 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 setting takes effect (e.g., attempting to start the server or to start a new session). I think Robert had good feedback regarding the latter: On Fri, Jan 28, 2022 at 6:42 AM Robert Haas <robertmh...@gmail.com> wrote: > On Tue, Dec 28, 2021 at 12:45 PM Justin Pryzby <pry...@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 open shared 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 line does actually help, because otherwise it's not clear why the server failed to start, but the warning does seem superfluous here. I do agree that GUC is awkward here, and I like the "while..." wording suggested both for consistency with other 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 warning telling 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
