Обсуждение: 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
Вложения
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..
Вложения
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
Вложения
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
Вложения
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
Вложения
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