warn if GUC set to an invalid shared library

forking <CA+TgmoawONZqEwe-GqmKERNY1ug0z1QhBzkHdA158xfToHKN9w(at)mail(dot)gmail(dot)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(dot)sakrejda(at)gmail(dot)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

--
Justin