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
