Thread: Re: warn if GUC set to an invalid shared library
On Tue, Dec 28, 2021 at 12:45 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > 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 -1 from me on using "guc" in any user-facing error message. And even guc -> setting isn't a big improvement. If we're going to structure the reporting this way there, we should try to use a meaningful phrase there, probably beginning with the word "while"; see "git grep errcontext.*while" for interesting precedents. That said, that series of messages seems a bit suspect to me, because the WARNING seems to be stating the same problem as the subsequent FATAL and CONTEXT lines. Ideally we'd tighten that somehow. -- Robert Haas EDB: http://www.enterprisedb.com
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: not tested Hello I tested the patches on master branch on Ubuntu 18.04 and regression turns out fine. I did a manual test following the queryexamples in this email thread and I do see the warnings when I attempted: these queries: postgres=# SET local_preload_libraries=xyz.so; 2022-01-28 15:11:00.592 PST [13622] WARNING: could not access file "xyz.so" WARNING: could not access file "xyz.so" SET postgres=# ALTER SYSTEM SET shared_preload_libraries=abc.so; 2022-01-28 15:11:07.729 PST [13622] WARNING: could not access file "$libdir/plugins/abc.so" WARNING: could not access file "$libdir/plugins/abc.so" ALTER SYSTEM This is fine as this is what these patches are aiming to provide. However, when I try to restart the server, it fails tostart because abc.so and xyz.so do not exist. Setting the parameters "local_preload_libraries" and "local_preload_libraries"to something else in postgresql.conf does not seem to take effect either. It still complains shared_preload_libraries abc.so does not exist even though I have already set shared_preload_librariesto something else in postgresql.conf. This seems a little strange to me thank you Cary
Thanks for loooking On Fri, Jan 28, 2022 at 11:36:20PM +0000, Cary Huang wrote: > This is fine as this is what these patches are aiming to provide. However, when I try to restart the server, it fails tostart because abc.so and xyz.so do not exist. Setting the parameters "local_preload_libraries" and "local_preload_libraries"to something else in postgresql.conf does not seem to take effect either. > It still complains shared_preload_libraries abc.so does not exist even though I have already set shared_preload_librariesto something else in postgresql.conf. This seems a little strange to me Could you show exactly what you did and the output ? The patches don't entirely prevent someone from putting the server config into a bad state. It only aims to tell them if they've done that, so they can fix it, rather than letting someone (else) find the error at some later (probably inconvenient) time. ALTER SYSTEM adds config into postgresql.auto.conf. If you stop the server after adding bad config there (after getting a warning), the server won't start. Once the server is off, you have to remove it manually. The goal of the patch is to 1) warn someone that they've put a bad config in place, so they don't leave it there; and, 2) if the server fails to start for such a reason, provide a CONTEXT line to help them resolve it quickly. Maybe you know all that and I didn't understand what you're saying. -- Justin
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation: not tested I tried the latest version of the patch, and it works as discussed. There is no documentation, but I think that's moot forthis warning (we may want to note something in the setting docs, but even if so, I think we should figure out the messagefirst and then decide if it merits additional explanation in the docs). I do not know whether it is spec-compliant,but I doubt the spec has much to say on something like this. I tried running ALTER SYSTEM and got the warnings as expected: postgres=# alter system set shared_preload_libraries = no_such_library,not_this_one_either; WARNING: could not access file "$libdir/plugins/no_such_library" WARNING: could not access file "$libdir/plugins/not_this_one_either" ALTER SYSTEM I think this is great, but it would be really helpful to also indicate that at this point the server will fail to come backup after a restart. In my mind, that's a big part of the reason for having a warning here. Having made this mistake acouple of times, I would be able to read between the lines, as would many other users, but if you're not familiar with Postgresthis might still be pretty opaque. I think if I'm reading the code correctly, this warning path is shared betweenALTER SYSTEM and a SET of local_preload_libraries so it might be tricky to word this in a way that works in all situations,but it could make the precarious situation a lot clearer. I don't really know a good wording here, but maybe ahint like "The server or session will not be able to start if it has been configured to use libraries that cannot be loaded."? Also, there are two sides to this: one is actually applying the possibly-bogus setting, and the other is when that settingtakes effect (e.g., attempting to start the server or to start a new session). I think Robert had good feedback regardingthe latter: On Fri, Jan 28, 2022 at 6:42 AM Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Dec 28, 2021 at 12:45 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > 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 > > -1 from me on using "guc" in any user-facing error message. And even > guc -> setting isn't a big improvement. If we're going to structure > the reporting this way there, we should try to use a meaningful phrase > there, probably beginning with the word "while"; see "git grep > errcontext.*while" for interesting precedents. > > That said, that series of messages seems a bit suspect to me, because > the WARNING seems to be stating the same problem as the subsequent > FATAL and CONTEXT lines. Ideally we'd tighten that somehow. Maybe we don't even need the WARNING in this case? At this point, it's clear what the problem is. I think the CONTEXT linedoes actually help, because otherwise it's not clear why the server failed to start, but the warning does seem superfluoushere. I do agree that GUC is awkward here, and I like the "while..." wording suggested both for consistency withother messages and how it could work here: CONTEXT: while loading "shared_preload_libraries" I think that would be pretty clear. In the ALTER SYSTEM case, you still need to know to edit the file in spite of the warningtelling you not to edit it, but I think that's still better. Based on Cary's feedback, maybe that could be clearer,too (like you, I'm not sure if I understood what he did correctly), but I think that could certainly be future work. Thanks, Maciek
On Tue, Feb 1, 2022 at 11:06 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
I tried running ALTER SYSTEM and got the warnings as expected:
postgres=# alter system set shared_preload_libraries = no_such_library,not_this_one_either;
WARNING: could not access file "$libdir/plugins/no_such_library"
WARNING: could not access file "$libdir/plugins/not_this_one_either"
ALTER SYSTEM
I think this is great, but it would be really helpful to also indicate that at this point the server will fail to come back up after a restart. In my mind, that's a big part of the reason for having a warning here. Having made this mistake a couple of times, I would be able to read between the lines, as would many other users, but if you're not familiar with Postgres this might still be pretty opaque.
+1
I would at least consider having the UX go something like:
postgres=# ALTER SYSTEM SET shared_preload_libraries = not_such_library;
ERROR: <paraphrase: your system will not reboot in its current state as that library is not present>.
HINT: to bypass the error please add FORCE before SET
postgres=# ALTER SYSTEM FORCE SET shared_preload_libraries = no_such_library;
NOTICE: Error suppressed while setting shared_preload_libraries.
That is, have the user express their desire to leave the system in a precarious state explicitly before actually doing so.
Upon startup, if the system already can track each separate location that shared_preload_libraries is set, printing out those locations and current values would be useful context. Seeing ALTER SYSTEM in that listing would be helpful.
David J.
On Wed, Feb 2, 2022 at 7:39 AM David G. Johnston <david.g.johnston@gmail.com> wrote:
I would at least consider having the UX go something like:postgres=# ALTER SYSTEM SET shared_preload_libraries = not_such_library;ERROR: <paraphrase: your system will not reboot in its current state as that library is not present>.HINT: to bypass the error please add FORCE before SETpostgres=# ALTER SYSTEM FORCE SET shared_preload_libraries = no_such_library;NOTICE: Error suppressed while setting shared_preload_libraries.That is, have the user express their desire to leave the system in a precarious state explicitly before actually doing so.
While I don't have a problem with that behavior, given that there are currently no such facilities for asserting "yes, really" with ALTER SYSTEM, I don't think it's worth introducing that just for this patch. A warning seems like a reasonable first step. This can always be expanded later. I'd rather see a warning ship than move the goalposts out of reach.
On Wed, Feb 9, 2022 at 5:58 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > FYI, it has said "while..." and hasn't said "guc" since the 2nd revision of the > patch. The v3-0001 attached above had "while... for GUC..."--sorry I wasn't clear. 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'm not familiar with that setting (reading the docs, it's like a non-superuser session_preload_libraries for compatible modules?), but given nothing is being persisted here with ALTER SYSTEM, this seems incorrect. Changing session_preload_libraries emits a similar warning: postgres=# set session_preload_libraries = foo; WARNING: could not access file "$libdir/plugins/foo" HINT: New sessions will fail with the existing configuration. SET This is also not persisted, so I think this is also incorrect, right? (I'm not sure what setting session_preload_libraries without an ALTER ROLE or ALTER DATABASE accomplishes, given a new session is required for the change to take effect, but I thought I'd point this out.) I'm guessing this may be due to trying to have the warning for ALTER ROLE? postgres=# alter role bob set session_preload_libraries = foo; WARNING: could not access file "$libdir/plugins/foo" HINT: New sessions will fail with the existing configuration. ALTER ROLE This is great. Ideally, we'd qualify this with "New sessions for user..." or "New sessions for database..." but given you get the warning right after running the relevant command, maybe that's clear enough. > $ ./tmp_install/usr/local/pgsql/bin/postgres -D src/test/regress/tmp_check/data -clogging_collector=on > 2022-02-09 19:53:48.034 CST postmaster[30979] FATAL: could not access file "a": No such file or directory > 2022-02-09 19:53:48.034 CST postmaster[30979] CONTEXT: while loading shared libraries for setting "shared_preload_libraries" > from /home/pryzbyj/src/postgres/src/test/regress/tmp_check/data/postgresql.auto.conf:3 > 2022-02-09 19:53:48.034 CST postmaster[30979] LOG: database system is shut down > > Maybe it's enough to show the GucSource rather than file:line... This is great. I think the file:line output is helpful here. Thanks, Maciek
Maciek Sakrejda <m.sakrejda@gmail.com> 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
On Wed, Mar 23, 2022 at 03:02:23PM -0400, Tom Lane wrote: > 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. It seems to me that there is no objection to the proposed patch, but an update is required. Justin? -- Michael
Attachment
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
Finally returning to this .. rebased and updated per feedback. I'm not sure of a good place to put test cases for this..
Attachment
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
Maciek Sakrejda <m.sakrejda@gmail.com> writes:
>> 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.
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.
> 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?
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
On Fri, Jul 22, 2022 at 01:53:21PM -0400, Tom Lane wrote: > > 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.conffile 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? > > 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. 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
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Fri, Jul 22, 2022 at 01:53:21PM -0400, Tom Lane wrote:
>> 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.
> 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..
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
On Fri, Jul 22, 2022 at 03:00:23PM -0400, Tom Lane wrote: > Justin Pryzby <pryzby@telsasoft.com> writes: > > On Fri, Jul 22, 2022 at 01:53:21PM -0400, Tom Lane wrote: > >> 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. > > > 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.. > > 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. 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 manuallyedit 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
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Fri, Jul 22, 2022 at 03:00:23PM -0400, Tom Lane wrote:
>> Shouldn't you be doing this when the source is PGC_S_TEST, instead?
> 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
On Fri, Jul 22, 2022 at 03:26:47PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > On Fri, Jul 22, 2022 at 03:00:23PM -0400, Tom Lane wrote:
> >> Shouldn't you be doing this when the source is PGC_S_TEST, instead?
>
> > 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.
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
On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > 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
Attachment
On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > 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. 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
On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > > 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. > > I'm hoping for some guidance ; this simple change may be naive, but I'm not > 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
On Sun, Oct 30, 2022 at 04:12:33PM -0700, Maciek Sakrejda wrote:
> On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> > > 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.
> >
> > I'm hoping for some guidance ; this simple change may be naive, but I'm not
> > sure what a wider change would look like.
>
> 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 ?
> 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.
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
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Sun, Oct 30, 2022 at 04:12:33PM -0700, Maciek Sakrejda wrote:
>> 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.
> 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).
Yeah, I am afraid that you will break assorted dump/restore and
pg_upgrade scenarios if you insist on that.
regards, tom lane
On Mon, Oct 31, 2022 at 08:31:20AM -0500, Justin Pryzby wrote:
> On Sun, Oct 30, 2022 at 04:12:33PM -0700, Maciek Sakrejda wrote:
> > On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> > > > 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.
> > >
> > > I'm hoping for some guidance ; this simple change may be naive, but I'm not
> > > sure what a wider change would look like.
> >
> > 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 ?
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
Attachment
On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > > > > > 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. > > > > > > > > I'm hoping for some guidance ; this simple change may be naive, but I'm not > > > > sure what a wider change would look like. I'm still hoping. > > PGC_S_TEST is a better fit, so my question is whether it's really that > > simple ? > > 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. > From defdb57fe0ec373c1eea8df42f0e1831b3f9c3cc Mon Sep 17 00:00:00 2001 > From: Justin Pryzby <pryzbyj@telsasoft.com> > Date: Fri, 22 Jul 2022 15:52:11 -0500 > Subject: [PATCH v6 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not > FILE > > WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE > > Since the value didn't come from a file. Or maybe we should have > another PGC_S_ value for this, or a flag for 'is a test'. > --- > src/backend/utils/misc/guc.c | 2 +- > src/include/utils/guc.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c > index 6f21752b844..ae8810591d6 100644 > --- a/src/backend/utils/misc/guc.c > +++ b/src/backend/utils/misc/guc.c > @@ -4435,7 +4435,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) > > /* 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)) > ereport(ERROR, > (errcode(ERRCODE_INVALID_PARAMETER_VALUE), This is rebased over my own patch to enable checks for REGRESSION_TEST_NAME_RESTRICTIONS. -- Justin
Attachment
On Thu, Dec 28, 2023 at 10:54 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > > > > > > 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. > > > > > > > > > > I'm hoping for some guidance ; this simple change may be naive, but I'm not > > > > > sure what a wider change would look like. > > I'm still hoping. > > > > PGC_S_TEST is a better fit, so my question is whether it's really that > > > simple ? > > > > 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. > > > From defdb57fe0ec373c1eea8df42f0e1831b3f9c3cc Mon Sep 17 00:00:00 2001 > > From: Justin Pryzby <pryzbyj@telsasoft.com> > > Date: Fri, 22 Jul 2022 15:52:11 -0500 > > Subject: [PATCH v6 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not > > FILE > > > > WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE > > > > Since the value didn't come from a file. Or maybe we should have > > another PGC_S_ value for this, or a flag for 'is a test'. > > --- > > src/backend/utils/misc/guc.c | 2 +- > > src/include/utils/guc.h | 1 + > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c > > index 6f21752b844..ae8810591d6 100644 > > --- a/src/backend/utils/misc/guc.c > > +++ b/src/backend/utils/misc/guc.c > > @@ -4435,7 +4435,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) > > > > /* 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)) > > ereport(ERROR, > > (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > > This is rebased over my own patch to enable checks for > REGRESSION_TEST_NAME_RESTRICTIONS. > I was reviewing the Patch and came across a minor issue that the Patch does not apply on the current Head. Please provide the updated version of the patch. Thanks and Regards, Shubham Khanna.
On Thu, Dec 28, 2023 at 12:27 PM Shubham Khanna <khannashubham1197@gmail.com> wrote: > > I was reviewing the Patch and came across a minor issue that the Patch > does not apply on the current Head. Please provide the updated version > of the patch. For your information, the commitfest manager has the ability to send private messages to authors about procedural issues like this. There is no need to tell the whole list about it.
On Fri, Jul 22, 2022 at 03:26:47PM -0400, Tom Lane wrote: > 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. On Thu, Jul 06, 2023 at 03:15:20PM -0500, Justin Pryzby wrote: > On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > > > > > > 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. > > > > > > > > > > I'm hoping for some guidance ; this simple change may be naive, but I'm not > > > > > sure what a wider change would look like. > > I'm still hoping. @cfbot: rebased
Attachment
On Thu, Jul 6, 2023 at 4:15 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > I'm still hoping. Hi, I got asked to take a look at this thread. First, I want to explain why I think this thread hasn't gotten as much feedback as Justin was hoping. It is always possible for any thread to have that problem just because people are busy or not interested. However, I think in this case an aggravating factor is that the discussion is very "high context"; it's hard to understand what the open problems are without reading a lot of emails and understanding how they all relate to each other. One of the key questions is whether we should replace PGC_S_FILE with PGC_S_TEST in AlterSystemSetConfigFile. I originally thought, based on reading one of the emails, that the question was whether we should do that out of some sense of intellectual purity, and my answer was "probably not, because that would change the behavior in a way that doesn't seem good." But then I realized, reading another email, that Justin already knew that the behavior would change, or at least I'm 90% certain that he knows that. So now I think the question is whether we want that behavior change, but he only provided one example of how the behavior changes, and it's not clear how many other scenarios are affected or in what way, so it's still a bit hard to answer. Plus, it took me 10 minutes to figure out what the question was. I think that if the question had been phrased in a way that was easily understandable to any experienced PostgreSQL user, it's a lot more likely that one or more people would have had an opinion on whether it was good or bad. As it is, I think most people probably didn't understand the question, and the people who did understand the question may not have wanted to spend the time to do the research that they would have needed to do to come up with an intelligent answer. I'm not saying any of this to criticize Justin or to say that he did anything wrong, but I think we have lots of examples of stuff like this on the mailing list, where people are sad because they didn't get an answer, but don't always realize that there might be things they could do to improve their chances. On the behavior change itself, it seems to me that there's a big difference between shared_preload_libraries=bogus and work_mem=bogus. The former is valid or invalid according to whether bogus.so exists in an appropriate directory on the local machine, but the latter is categorically invalid. I'm not sure to what degree we have the infrastructure to distinguish those cases, but to the extent that we do, handling them differently is completely defensible. It's reasonable to allow the first one on the theory that the presently-invalid configuration may at a later time become valid, but that's not reasonable in the second case. So if changing PGC_S_FILE to PGC_S_TEST in AlterSystemSetConfigFile is going to have the effect of allowing garbage values into postgresql.auto.conf that would currently get blocked, I think that's a bad plan and we shouldn't do it. But it's quite possible I'm not fully understanding the situation. -- Robert Haas EDB: http://www.enterprisedb.com
On Fri, May 24, 2024 at 09:26:54AM -0400, Robert Haas wrote: > On Thu, Jul 6, 2023 at 4:15 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > But then I realized, reading another email, that Justin already knew > that the behavior would change, or at least I'm 90% certain that he > knows that. You give me too much credit.. > On the behavior change itself, it seems to me that there's a big > difference between shared_preload_libraries=bogus and work_mem=bogus. .. > So if changing PGC_S_FILE to > PGC_S_TEST in AlterSystemSetConfigFile is going to have the effect of > allowing garbage values into postgresql.auto.conf that would currently > get blocked, I think that's a bad plan and we shouldn't do it. Right - this is something I'd failed to realize. We can't change it in the naive way because it allows bogus values, and not just missing libraries. Specifically, for GUCs with assign hooks conditional on PGC_TEST. We don't want to change the behavior to allow this to succeed -- it would allow leaving the server in a state that it fails to start (rather than helping to avoid doing so, as intended by this thread). regression=# ALTER SYSTEM SET default_table_access_method=abc; NOTICE: table access method "abc" does not exist ALTER SYSTEM Maybe there should be a comment explaning why PGC_FILE is used, and maybe there should be a TAP test for the behavior, but they're pretty unrelated to this thread. So, I've dropped the 001 patch. -- Justin
Attachment
On Fri, May 24, 2024 at 11:48 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> You give me too much credit..
Gee, usually I'm very good at avoiding that mistake. :-)
> We don't want to change the behavior to allow this to succeed -- it
> would allow leaving the server in a state that it fails to start (rather
> than helping to avoid doing so, as intended by this thread).
+1.
> Maybe there should be a comment explaning why PGC_FILE is used, and
> maybe there should be a TAP test for the behavior, but they're pretty
> unrelated to this thread. So, I've dropped the 001 patch.
+1 for that, too.
+ /* Note that filename was already canonicalized */
I see that this comment is copied from load_libraries(), but I don't
immediately see where the canonicalization actually happens. Do you
know, or can you find out? Because that's crucial here, else stat()
might not target the real filename. I wonder if it will anyway. Like,
couldn't the library be versioned, and might not dlopen() try a few
possibilities?
+ errdetail("The server will currently fail to start with this setting."),
+ errdetail("New sessions will currently fail to connect with the new
setting."));
I understand why these messages have the word "currently" in them, but
I bet the user won't. I'm not sure exactly what to recommend at the
moment (and I'm quite busy today due to the conference upcoming) but I
think we should try to find some way to rephrase these.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Fri, May 24, 2024 at 01:15:13PM -0400, Robert Haas wrote: > + /* Note that filename was already canonicalized */ > > I see that this comment is copied from load_libraries(), but I don't > immediately see where the canonicalization actually happens. Do you > know, or can you find out? Because that's crucial here, else stat() > might not target the real filename. I wonder if it will anyway. Like, > couldn't the library be versioned, and might not dlopen() try a few > possibilities? This comment made me realize that we've been fixated on the warning. But the patch was broken, and would've always warned. I think almost all of the previous patch versions had this issue - oops. I added a call to expand_dynamic_library_name(), which seems to answer your question. And added a preparatory patch to distinguish ALTER USER/DATABASE SET from SET in a function, to avoid warning in that case. -- Justin
Attachment
I had a quick glance at this, and I agree with Robert's comment earlier that this requires a lot of context to understand. Does this change the behavior of what is accepted or not? What are those cases? Can you give some examples, please? When do you get the new warnings? Examples please. Does this require documentation changes? On 22/07/2024 19:28, Justin Pryzby wrote: > And added a preparatory patch to distinguish ALTER USER/DATABASE SET > from SET in a function, to avoid warning in that case. The comment above enum GucSource needs updating. PGC_S_TEST_FUNCTION is missing from GucSource_Names (there's a reminder of this in the comment above enum GucSource). Are there other places where PGC_S_TEST_FUNCTION is missing? Do extensions need any changes for this? Consider renaming PGC_S_TEST too, to intentionally break any code that might now need to also check for PGC_S_TEST_FUNCTION. Or perhaps there's a better way to distinguish between ALTER DATABASE/ROLE and CREATE FUNCTION settings. We already have different GucSource codes for per-database and per-user settings; I wonder if it would be better to use those codes and a separate "is_testing" flag. That would also naturally allow the combination of "source == PGC_S_FILE && is_testing==true", if some settings would need different checks in ALTER SYSTEM for example. -- Heikki Linnakangas Neon (https://neon.tech)