Re: warn if GUC set to an invalid shared library

Thanks for picking this back up, Justin.

>I've started to think that we should really WARN whenever a (set of) GUC is set
>in a manner that the server will fail to start - not just for shared libraries.

+0.5. I think it's a reasonable change, but I've never broken my
server with anything other than shared_preload_libraries, so I'd
rather see an improvement here first rather than expanding scope. I
think shared_preload_libraries (and friends) is especially tricky due
to the syntax, and more likely to lead to problems.

On the update patch itself, I have some minor feedback about message wording

postgres=# set local_preload_libraries=xyz;
SET

Great, it's nice that this no longer gives a warning.

postgres=# alter role bob set local_preload_libraries = xyz;
WARNING: could not access file "xyz"
DETAIL: New sessions will currently fail to connect with the new setting.
ALTER ROLE

The warning makes sense, but the detail feels a little awkward. I
think "currently" is sort of redundant with "new setting". And it
could be clearer that the setting did in fact take effect (I know the
ALTER ROLE command tag echo tells you that, but we could reinforce
that in the warning).

Also, I know I said last time that the scope of the warning is clear
from the setting, but looking at it again, I think we could do better.
I guess because when we're generating the error, we don't know whether
the source was ALTER DATABASE or ALTER ROLE, we can't give a more
specific message? Ideally, I think the DETAIL would be something like
"New sessions for this role will fail to connect due to this setting".
Maybe even with a HINT of "Run ALTER ROLE again with a valid value to
fix this"? If that's not feasible, maybe "New sessions for this role
or database will fail to connect due to this setting"? That message is
not as clear about the impact of the change as it could be, but
hopefully you know what command you just ran, so that should make it
unambiguous. I do think without qualifying that, it suggests that all
new sessions are affected.

Hmm, or maybe just "New sessions affected by this setting will fail to
connect"? That also makes the scope clear without the warning having
to be aware of the scope: if you just ran ALTER DATABASE it's pretty
clear that what is affected by the setting is the database. I think
this is probably the way to go, but leaving my thought process above
for context.

postgres=# alter system set shared_preload_libraries = lol;
WARNING: could not access file "$libdir/plugins/lol"
DETAIL: The server will currently fail to start with this setting.
HINT: If the server is shut down, it will be necessary to manually
edit the postgresql.auto.conf file to allow it to start again.
ALTER SYSTEM

I think this works. Tom's copy edit above omitted "currently" from the
DETAIL and did not include the "$libdir/plugins/" prefix. I don't feel
strongly about these either way.

2022-07-22 10:37:50.217 PDT [1131187] LOG: database system is shut down
2022-07-22 10:37:50.306 PDT [1134058] WARNING: could not access file
"$libdir/plugins/lol"
2022-07-22 10:37:50.306 PDT [1134058] DETAIL: The server will
currently fail to start with this setting.
2022-07-22 10:37:50.306 PDT [1134058] HINT: If the server is shut
down, it will be necessary to manually edit the postgresql.auto.conf
file to allow it to start again.
2022-07-22 10:37:50.312 PDT [1134058] FATAL: could not access file
"lol": No such file or directory
2022-07-22 10:37:50.312 PDT [1134058] CONTEXT: while loading shared
libraries for setting "shared_preload_libraries"
from /home/maciek/code/aux/postgres/tmpdb/postgresql.auto.conf:3
2022-07-22 10:37:50.312 PDT [1134058] LOG: database system is shut down

Hmm, I guess this is a side effect of where these messages are
emitted, but at this point, lines 4-6 here are a little confusing, no?
The server was already shut down, and we're trying to start it back
up. If there's no reasonable way to avoid that output, I think it's
okay, but it'd be better to skip it (or adjust it to the new context).

Thanks,
Maciek