On Tue, Dec 28, 2021 at 11:15 PM Justin Pryzby <pry...@telsasoft.com> wrote: > > forking <ca+tgmoawonzqewe-gqmkerny1ug0z1qhbzkhda158xftohk...@mail.gmail.com> > > On Mon, Dec 13, 2021 at 09:01:57AM -0500, Robert Haas wrote: > > On Thu, Dec 9, 2021 at 2:32 AM Maciek Sakrejda <m.sakre...@gmail.com> wrote: > > > > Considering the vanishingly small number of actual complaints we've > > > > seen about this, that sounds ridiculously over-engineered. > > > > A documentation example should be sufficient. > > > > > > I don't know if this will tip the scales, but I'd like to lodge a > > > belated complaint. I've gotten myself in this server-fails-to-start > > > situation several times (in development, for what it's worth). The > > > syntax (as Bharath pointed out in the original message) is pretty > > > picky, there are no guard rails, and if you got there through ALTER > > > SYSTEM, you can't fix it with ALTER SYSTEM (because the server isn't > > > up). If you go to fix it manually, you get a scary "Do not edit this > > > file manually!" warning that you have to know to ignore in this case > > > (that's if you find the file after you realize what the fairly generic > > > "FATAL: ... No such file or directory" error in the log is telling > > > you). Plus you have to get the (different!) quoting syntax right or > > > cut your losses and delete the change. > > > > +1. I disagree that trying to detect this kind of problem would be > > "ridiculously over-engineered." I don't know whether it can be done > > elegantly enough that we'd be happy with it and I don't know whether > > it would end up just garden variety over-engineered. But there's > > nothing ridiculous about trying to prevent people from putting their > > system into a state where it won't start. > > > > (To be clear, I also think updating the documentation is sensible, > > without taking a view on exactly what that update should look like.) > > Yea, I think documentation won't help to avoid this issue: > > If ALTER SYSTEM gives an ERROR, someone will likely to check the docs after a > few minutes if they know that they didn't get the correct syntax. > But if it gives no error nor warning, then most likely they won't know to > check > the docs. > > We should check session_preload_libraries too, right ? It's PGC_SIGHUP, so if > someone sets the variable and sends sighup, clients will be rejected, and they > had no good opportunity to avoid that. > > 0001 adds WARNINGs when doing SET: > > postgres=# SET local_preload_libraries=xyz; > WARNING: could not load library: xyz: cannot open shared object > file: No such file or directory > SET > > postgres=# ALTER SYSTEM SET shared_preload_libraries =asdf; > WARNING: could not load library: $libdir/plugins/asdf: cannot open > shared object file: No such file or directory > ALTER SYSTEM > > 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 > > But I wonder whether it'd be adequate context if dlopen were to fail rather > than stat() ? > > Before 0003: > 2021-12-18 23:13:57.861 CST postmaster[11956] FATAL: could not > access file "asdf": No such file or directory > 2021-12-18 23:13:57.862 CST postmaster[11956] LOG: database system > is shut down > > After 0003: > 2021-12-18 23:16:05.719 CST postmaster[13481] FATAL: could not load > library: asdf: cannot open shared object file: No such file or directory > 2021-12-18 23:16:05.720 CST postmaster[13481] LOG: database system > is shut down
Overall the idea looks good to me. A warning on ALTER SYSTEM SET seems reasonable than nothing. ERROR isn't the way to go as it limits the users of setting the extensions in shared_preload_libraries first, installing them later. Is NOTICE here a better idea than WARNING? I haven't looked at the patches though. Regards, Bharath Rupireddy.
