Subject: Re: warn if GUC set to an invalid shared library

From: Tom Lane <[email-masked]>
Date: Wed, 23 Mar 2022 15:02:23 -0400

Maciek Sakrejda <[email-masked]> writes:
> In v4, the message looks fine to me for shared_preload_libraries
> (except there is a doubled "is"). However, I also get the message for
> a simple SET with local_preload_libraries:

> postgres=# set local_preload_libraries=xyz;
> WARNING:  could not access file "xyz"
> HINT:  The server will fail to start with the existing configuration.
> If it is is shut down, it will be necessary to manually edit the
> postgresql.auto.conf file to allow it to start.
> SET

I agree with Maciek's concerns about these HINTs being emitted
inappropriately, but I also have a stylistic gripe: they're only
halfway hints.  Given that we fix things so they only print when they
should, the complaint about the server not starting is not a hint,
it's a fact, which per style guidelines means it should be errdetail.
So I think this ought to look more like

WARNING:  could not access file "xyz"
DETAIL:  The server will 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.

I adjusted the wording a bit too --- YMMV, but I think my text is clearer.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Thu, 16 Jun 2022 12:14:46 +0900

> I adjusted the wording a bit too --- YMMV, but I think my text is clearer.

It seems to me that there is no objection to the proposed patch, but
an update is required.  Justin?
--
Michael

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 15 Jun 2022 22:50:14 -0500

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.

In particular, I think the server should also warn if it's going to fail to
start like this:

2022-06-15 22:48:34.279 CDT postmaster[20782] FATAL:  WAL streaming (max_wal_senders > 0) requires wal_level "replica" or "logical"

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 21 Jul 2022 20:54:12 -0500

Finally returning to this .. rebased and updated per feedback.

I'm not sure of a good place to put test cases for this..

---

From: Maciek Sakrejda <[email-masked]>
Date: Fri, 22 Jul 2022 10:42:11 -0700

Thanks for picking this back up, Justin.

>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

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

---

From: Tom Lane <[email-masked]>
Date: Fri, 22 Jul 2022 13:53:21 -0400

> rather see an improvement here first rather than expanding scope.

Generally speaking, anything that tries to check a combination of
GUC settings is going to be so fragile as to be worthless.  We've
learned that lesson the hard way in the past.

> emitted, but at this point, lines 4-6 here are a little confusing, no?

This indicates that the warning is being issued in the wrong place.
It's okay if it comes out during ALTER SYSTEM.  It's not okay if it
comes out during server start; then it's just an annoyance.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 22 Jul 2022 13:35:57 -0500

On Fri, Jul 22, 2022 at 01:53:21PM -0400, Tom Lane wrote:
> > 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

This was a regression from the previous patch version, and I even noticed the
problem, but then forgot when returning to the patch :(

The previous patch version checked if (!IsUnderPostmaster()) before warning.
Is there a better way ?

ALTER SYSTEM uses PGC_S_FILE, the same as during startup..

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Fri, 22 Jul 2022 15:00:23 -0400

> ALTER SYSTEM uses PGC_S_FILE, the same as during startup..

Shouldn't you be doing this when the source is PGC_S_TEST, instead?
That's pretty much what it's for.  See check_default_table_access_method
and other examples.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 22 Jul 2022 14:14:43 -0500

> That's pretty much what it's for.  See check_default_table_access_method

That makes sense, but it doesn't work for ALTER SYSTEM, which uses PGC_S_FILE.

postgres=# ALTER SYSTEM SET shared_preload_libraries =a;
2022-07-22 14:07:25.489 CDT client backend[23623] psql WARNING:  source 3
WARNING:  source 3
2022-07-22 14:07:25.489 CDT client backend[23623] psql WARNING:  could not access file "$libdir/plugins/a"
2022-07-22 14:07:25.489 CDT client backend[23623] psql DETAIL:  The server will currently fail to start with this setting.
2022-07-22 14:07:25.489 CDT client backend[23623] psql 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.

postgres=# ALTER SYSTEM SET default_table_access_method=abc;
Breakpoint 1, check_default_table_access_method (newval=0x7ffe4c6fe820, extra=0x7ffe4c6fe828, source=PGC_S_FILE) at tableamapi.c:112

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Fri, 22 Jul 2022 15:26:47 -0400

> That makes sense, but it doesn't work for ALTER SYSTEM, which uses PGC_S_FILE.

Hmph.  I wonder if we shouldn't change that, because it's a lie.
The value isn't actually coming from the config file, at least
not yet.

We might need to invent a separate PGC_S_TEST_FILE value; or maybe it'd
be better to pass the "this is a test" flag separately?  But that'd
require changing the signature of all GUC check hooks, so probably
it's unduly invasive.  I'm not sure whether any users of the TEST
capability need to distinguish values proposed for postgresql.auto.conf
from those proposed for pg_db_role_setting ... but I guess it's
plausible that somebody might.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 2 Sep 2022 17:24:58 -0500

> Hmph.  I wonder if we shouldn't change that, because it's a lie.

I think so, and I was going to raise this question some months ago when
I first picked up the patch.

The question is, which behavior do we want ?

postgres=# ALTER SYSTEM SET default_table_access_method=abc;
2022-07-22 15:24:55.445 CDT client backend[27938] psql ERROR:  invalid value for parameter "default_table_access_method": "abc"
2022-07-22 15:24:55.445 CDT client backend[27938] psql DETAIL:  Table access method "abc" does not exist.
2022-07-22 15:24:55.445 CDT client backend[27938] psql STATEMENT:  ALTER SYSTEM SET default_table_access_method=abc;

That behavior differs from ALTER SYSTEM SET shared_preload_libraries,
which supports first seting the GUC and then installing the library.  If
that wasn't supported, I think we'd just throw an error and avoid the
possibility that the server can't start.

It caused no issue when I changed:

                        /* Check that it's acceptable for the indicated parameter */
                        if (!parse_and_validate_value(record, name, value,
-                                                     PGC_S_FILE, ERROR,
+                                                     PGC_S_TEST, ERROR,
                                                      &newval, &newextra))

I'm not sure where to go from here.

-- 
Justin

---

From: Michael Paquier <[email-masked]>
Date: Wed, 12 Oct 2022 14:34:09 +0900

> I'm not sure where to go from here.

Not sure either, but the thread has no activity for a bit more than 1
month, so marked as RwF for now.
--
Michael

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 29 Oct 2022 12:40:53 -0500

> I'm not sure where to go from here.

I'm hoping for some guidance ; this simple change may be naive, but I'm not
sure what a wider change would look like.

-- 
Justin

---

From: Maciek Sakrejda <[email-masked]>
Date: Sun, 30 Oct 2022 16:12:33 -0700

> sure what a wider change would look like.

I assume you mean guidance on implementation details here, and not on
design. I still think this is a useful patch and I'd be happy to
review and try out future iterations, but I don't have any useful
input here.

Also, for what it's worth, I think requiring the libraries to be in
place before running ALTER SYSTEM does not really seem that onerous. I
can't really think of use cases it precludes.

Thanks,
Maciek

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 31 Oct 2022 08:31:20 -0500

> I assume you mean guidance on implementation details here, and not on

ALTER SYSTEM tests the new/proposed setting using PGC_S_FILE ("which is
a lie").

It seems better to address that lie before attempting to change the
behavior of *_preload_libraries.

PGC_S_TEST is a better fit, so my question is whether it's really that
simple ?  

> can't really think of use cases it precludes.

Right now, it's allowed to set the GUC before installing the shlib.
That's a supported case (see the 11 month old messages toward the
beginning of this thread).

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Mon, 31 Oct 2022 09:43:05 -0400

> beginning of this thread).

Yeah, I am afraid that you will break assorted dump/restore and
pg_upgrade scenarios if you insist on that.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 1 Nov 2022 17:26:34 -0500

> PGC_S_TEST is a better fit, so my question is whether it's really that

I've added the trivial change as 0001 and re-opened the patch (which ended
up in January's CF)

If for some reason it's not really as simple as that, then 001 will
serve as a "straw-man patch" hoping to elicit discussion on that point.

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 7 Jan 2024 07:27:00 -0600

On Thu, Jul 06, 2023 at 03:15:20PM -0500, Justin Pryzby wrote:
> I'm still hoping.

@cfbot: rebased