Re: warn if GUC set to an invalid shared library

On Tue, Dec 28, 2021 at 11:15 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> forking <CA+TgmoawONZqEwe-GqmKERNY1ug0z1QhBzkHdA158xfToHKN9w@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.sakrejda@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
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
>
> 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.