Horton
Menu

Thread

  1. warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-28T17:45:57Z

    forking <CA+TgmoawONZqEwe-GqmKERNY1ug0z1QhBzkHdA158xfToHKN9w@mail.gmail.com>

On Mon, Dec 13, 2021 at 09:01:57AM -0500, Robert Haas wrote:
> On Thu, Dec 9, 2021 at 2:32 AM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
> > > Considering the vanishingly small number of actual complaints we've
> > > seen about this, that sounds ridiculously over-engineered.
> > > A documentation example should be sufficient.
> >
> > I don't know if this will tip the scales, but I'd like to lodge a
> > belated complaint. I've gotten myself in this server-fails-to-start
> > situation several times (in development, for what it's worth). The
> > syntax (as Bharath pointed out in the original message) is pretty
> > picky, there are no guard rails, and if you got there through ALTER
> > SYSTEM, you can't fix it with ALTER SYSTEM (because the server isn't
> > up). If you go to fix it manually, you get a scary "Do not edit this
> > file manually!" warning that you have to know to ignore in this case
> > (that's if you find the file after you realize what the fairly generic
> > "FATAL: ... No such file or directory" error in the log is telling
> > you). Plus you have to get the (different!) quoting syntax right or
> > cut your losses and delete the change.
> 
> +1. I disagree that trying to detect this kind of problem would be
> "ridiculously over-engineered." I don't know whether it can be done
> elegantly enough that we'd be happy with it and I don't know whether
> it would end up just garden variety over-engineered. But there's
> nothing ridiculous about trying to prevent people from putting their
> system into a state where it won't start.
> 
> (To be clear, I also think updating the documentation is sensible,
> without taking a view on exactly what that update should look like.)

Yea, I think documentation won't help to avoid this issue:

If ALTER SYSTEM gives an ERROR, someone will likely to check the docs after a
few minutes if they know that they didn't get the correct syntax.
But if it gives no error nor warning, then most likely they won't know to check
the docs.

We should check session_preload_libraries too, right ?  It's PGC_SIGHUP, so if
someone sets the variable and sends sighup, clients will be rejected, and they
had no good opportunity to avoid that.

0001 adds WARNINGs when doing SET:

	postgres=# SET local_preload_libraries=xyz;
	WARNING:  could not load library: xyz: cannot open shared object file: No such file or directory
	SET

	postgres=# ALTER SYSTEM SET shared_preload_libraries =asdf;
	WARNING:  could not load library: $libdir/plugins/asdf: cannot open shared object file: No such file or directory
	ALTER SYSTEM

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 open shared 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

But I wonder whether it'd be adequate context if dlopen were to fail rather
than stat() ?

Before 0003:
	2021-12-18 23:13:57.861 CST postmaster[11956] FATAL:  could not access file "asdf": No such file or directory
	2021-12-18 23:13:57.862 CST postmaster[11956] LOG:  database system is shut down

After 0003:
	2021-12-18 23:16:05.719 CST postmaster[13481] FATAL:  could not load library: asdf: cannot open shared object file: No such file or directory
	2021-12-18 23:16:05.720 CST postmaster[13481] LOG:  database system is shut down

-- 
Justin

  2. Re: warn if GUC set to an invalid shared library

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-12-30T08:20:49Z

    On Tue, Dec 28, 2021 at 11:15 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> forking <CA+TgmoawONZqEwe-GqmKERNY1ug0z1QhBzkHdA158xfToHKN9w@mail.gmail.com>
>
> On Mon, Dec 13, 2021 at 09:01:57AM -0500, Robert Haas wrote:
> > On Thu, Dec 9, 2021 at 2:32 AM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
> > > > Considering the vanishingly small number of actual complaints we've
> > > > seen about this, that sounds ridiculously over-engineered.
> > > > A documentation example should be sufficient.
> > >
> > > I don't know if this will tip the scales, but I'd like to lodge a
> > > belated complaint. I've gotten myself in this server-fails-to-start
> > > situation several times (in development, for what it's worth). The
> > > syntax (as Bharath pointed out in the original message) is pretty
> > > picky, there are no guard rails, and if you got there through ALTER
> > > SYSTEM, you can't fix it with ALTER SYSTEM (because the server isn't
> > > up). If you go to fix it manually, you get a scary "Do not edit this
> > > file manually!" warning that you have to know to ignore in this case
> > > (that's if you find the file after you realize what the fairly generic
> > > "FATAL: ... No such file or directory" error in the log is telling
> > > you). Plus you have to get the (different!) quoting syntax right or
> > > cut your losses and delete the change.
> >
> > +1. I disagree that trying to detect this kind of problem would be
> > "ridiculously over-engineered." I don't know whether it can be done
> > elegantly enough that we'd be happy with it and I don't know whether
> > it would end up just garden variety over-engineered. But there's
> > nothing ridiculous about trying to prevent people from putting their
> > system into a state where it won't start.
> >
> > (To be clear, I also think updating the documentation is sensible,
> > without taking a view on exactly what that update should look like.)
>
> Yea, I think documentation won't help to avoid this issue:
>
> If ALTER SYSTEM gives an ERROR, someone will likely to check the docs after a
> few minutes if they know that they didn't get the correct syntax.
> But if it gives no error nor warning, then most likely they won't know to check
> the docs.
>
> We should check session_preload_libraries too, right ?  It's PGC_SIGHUP, so if
> someone sets the variable and sends sighup, clients will be rejected, and they
> had no good opportunity to avoid that.
>
> 0001 adds WARNINGs when doing SET:
>
>         postgres=# SET local_preload_libraries=xyz;
>         WARNING:  could not load library: xyz: cannot open shared object file: No such file or directory
>         SET
>
>         postgres=# ALTER SYSTEM SET shared_preload_libraries =asdf;
>         WARNING:  could not load library: $libdir/plugins/asdf: cannot open shared object file: No such file or directory
>         ALTER SYSTEM
>
> 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 open shared 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
>
> But I wonder whether it'd be adequate context if dlopen were to fail rather
> than stat() ?
>
> Before 0003:
>         2021-12-18 23:13:57.861 CST postmaster[11956] FATAL:  could not access file "asdf": No such file or directory
>         2021-12-18 23:13:57.862 CST postmaster[11956] LOG:  database system is shut down
>
> After 0003:
>         2021-12-18 23:16:05.719 CST postmaster[13481] FATAL:  could not load library: asdf: cannot open shared object file: No such file or directory
>         2021-12-18 23:16:05.720 CST postmaster[13481] LOG:  database system is shut down

Overall the idea looks good to me. A warning on ALTER SYSTEM SET seems
reasonable than nothing. ERROR isn't the way to go as it limits the
users of setting the extensions in shared_preload_libraries first,
installing them later. Is NOTICE here a better idea than WARNING?

I haven't looked at the patches though.

Regards,
Bharath Rupireddy.

  3. Re: warn if GUC set to an invalid shared library

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2022-01-08T21:29:24Z

    Thanks for working on this! I tried it out and it worked for me. I
reviewed the patch and didn't see any problems, but I'm not much of a
C programmer.

On Tue, Dec 28, 2021 at 9:45 AM 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 open shared 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

For whatever reason, I get slightly different (and somewhat redundant)
output on failing to start:

2022-01-08 12:59:36.784 PST [324482] WARNING:  could not load library:
$libdir/plugins/totally bogus: cannot open shared object file: No such
file or directory
2022-01-08 12:59:36.787 PST [324482] FATAL:  could not load library:
totally bogus: cannot open shared object file: No such file or
directory
2022-01-08 12:59:36.787 PST [324482] LOG:  database system is shut down

I'm on a pretty standard Ubuntu 20.04 (with PGDG packages installed
for 11, 12, and 13). I did configure to a --prefix and set
LD_LIBRARY_PATH and PATH. Not sure if this is an issue in my
environment or a platform difference?

Also, regarding the original warning:

2022-01-08 12:57:24.953 PST [324338] WARNING:  could not load library:
$libdir/plugins/totally bogus: cannot open shared object file: No such
file or directory

I think this is pretty clear to users familiar with
shared_preload_libraries. However, for someone less experienced with
Postgres and just following instructions on how to set up, e.g.,
auto_explain (and making a typo), it's not clear from this message
that your server will fail to start again after this if you shut it
down (or it crashes!), and how to get out of this situation. Should we
add a HINT to that effect?

Similarly, for

> 0001 adds WARNINGs when doing SET:
>
>         postgres=# SET local_preload_libraries=xyz;
>         WARNING:  could not load library: xyz: cannot open shared object file: No such file or directory
>         SET

This works for me, but should this explain the impact (especially if
used with something like ALTER ROLE)? I guess that's probably because
the context may not be easily accessible. I think
shared_preload_libraries is much more common, though, so I'm more
interested in a warning there.

Thanks,
Maciek

  4. Re: warn if GUC set to an invalid shared library

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2022-01-08T21:32:53Z

    On Thu, Dec 30, 2021 at 12:21 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> Overall the idea looks good to me. A warning on ALTER SYSTEM SET seems
> reasonable than nothing. ERROR isn't the way to go as it limits the
> users of setting the extensions in shared_preload_libraries first,
> installing them later. Is NOTICE here a better idea than WARNING?

I don't think so--I'm skeptical that "updated shared_preload_libraries
first, then install them" is much more than a theoretical use case. We
may not want to block that off completely, but I think a warning is
reasonable here, because you're *probably* doing something wrong if
you get to this message at all (and if you're not, you're probably
familiar enough with Postgres to know to ignore the warning).

Thanks,
Maciek

  5. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-08T22:07:02Z

    On Sat, Jan 08, 2022 at 01:29:24PM -0800, Maciek Sakrejda wrote:
> Thanks for working on this! I tried it out and it worked for me. I
> reviewed the patch and didn't see any problems, but I'm not much of a
> C programmer.

Thanks for looking at it.  I was just hacking on it myself.

Unfortunately, the output for dlopen() is not portable, which (I think) means
most of what I wrote can't be made to work..  Since it doesn't work to call
dlopen() when using SET, I tried using just stat().  But that also fails on
windows, since one of the regression tests has an invalid filename involving
unbalanced quotes, which cause it to return EINVAL rather than ENOENT.  So SET
cannot warn portably, unless it includes no details at all (or we specially
handle the windows case), or change the pre-existing regression test.  But
there's a 2nd instability, too, apparently having to do with timing.  So I'm
planning to drop the 0001 patch.

> On Tue, Dec 28, 2021 at 9:45 AM 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 open shared 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
> 
> For whatever reason, I get slightly different (and somewhat redundant)
> output on failing to start:
> 
> 2022-01-08 12:59:36.784 PST [324482] WARNING:  could not load library: $libdir/plugins/totally bogus: cannot open shared object file: No such file or directory
> 2022-01-08 12:59:36.787 PST [324482] FATAL:  could not load library: totally bogus: cannot open shared object file: No such file or directory
> 2022-01-08 12:59:36.787 PST [324482] LOG:  database system is shut down

I think the first WARNING is from the GUC mechanism "setting" the library.
And then the FATAL is from trying to apply the GUC.
It looks like you didn't apply the 0002 patch for that test so got no CONTEXT ?

$ ./tmp_install/usr/local/pgsql/bin/postgres -D src/test/regress/tmp_check/data -c shared_preload_libraries=asdf
2022-01-08 16:05:00.050 CST postmaster[2588] FATAL:  could not access file "asdf": No such file or directory
2022-01-08 16:05:00.050 CST postmaster[2588] CONTEXT:  while loading shared libraries for GUC "shared_preload_libraries"
2022-01-08 16:05:00.050 CST postmaster[2588] LOG:  database system is shut down

-- 
Justin

  6. Re: warn if GUC set to an invalid shared library

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2022-01-09T19:58:18Z

    On Sat, Jan 8, 2022 at 2:07 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> Unfortunately, the output for dlopen() is not portable, which (I think) means
> most of what I wrote can't be made to work..  Since it doesn't work to call
> dlopen() when using SET, I tried using just stat().  But that also fails on
> windows, since one of the regression tests has an invalid filename involving
> unbalanced quotes, which cause it to return EINVAL rather than ENOENT.  So SET
> cannot warn portably, unless it includes no details at all (or we specially
> handle the windows case), or change the pre-existing regression test.  But
> there's a 2nd instability, too, apparently having to do with timing.  So I'm
> planning to drop the 0001 patch.

Hmm. I think 001 is a big part of the usability improvement here.
Could we not at least warn generically, without relaying the
underlying error? The notable thing in this situation is that the
specified library could not be loaded (and that it will almost
certainly cause problems on restart). The specific error would be nice
to have, but it's less important. What is the timing instability?

> > On Tue, Dec 28, 2021 at 9:45 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > For whatever reason, I get slightly different (and somewhat redundant)
> > output on failing to start:
> >
> > 2022-01-08 12:59:36.784 PST [324482] WARNING:  could not load library: $libdir/plugins/totally bogus: cannot open shared object file: No such file or directory
> > 2022-01-08 12:59:36.787 PST [324482] FATAL:  could not load library: totally bogus: cannot open shared object file: No such file or directory
> > 2022-01-08 12:59:36.787 PST [324482] LOG:  database system is shut down
>
> I think the first WARNING is from the GUC mechanism "setting" the library.
> And then the FATAL is from trying to apply the GUC.
> It looks like you didn't apply the 0002 patch for that test so got no CONTEXT ?

I still had the terminal open where I tested this, and the scrollback
did show me applying the patch (and building after). I tried a make
clean and applying the patch again, and I do see the CONTEXT line now.
I'm not sure what the problem was but seems like PEBKAC--sorry about
that.

Thanks,
Maciek

  7. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-27T22:01:03Z

    On Sun, Jan 09, 2022 at 11:58:18AM -0800, Maciek Sakrejda wrote:
> On Sat, Jan 8, 2022 at 2:07 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > Unfortunately, the output for dlopen() is not portable, which (I think) means
> > most of what I wrote can't be made to work..  Since it doesn't work to call
> > dlopen() when using SET, I tried using just stat().  But that also fails on
> > windows, since one of the regression tests has an invalid filename involving
> > unbalanced quotes, which cause it to return EINVAL rather than ENOENT.  So SET
> > cannot warn portably, unless it includes no details at all (or we specially
> > handle the windows case), or change the pre-existing regression test.  But
> > there's a 2nd instability, too, apparently having to do with timing.  So I'm
> > planning to drop the 0001 patch.
> 
> Hmm. I think 001 is a big part of the usability improvement here.

I agree - it helps people avoid causing a disruption, rather than just helping
them to fix it faster.

> Could we not at least warn generically, without relaying the
> underlying error? The notable thing in this situation is that the
> specified library could not be loaded (and that it will almost
> certainly cause problems on restart). The specific error would be nice
> to have, but it's less important. What is the timing instability?

I saw regression diffs like this, showing that the warning could be displayed
before or after the SELECT was echoed.

https://cirrus-ci.com/task/6301672321318912
    -SELECT * FROM schema4.counted;
     WARNING:  could not load library: $libdir/plugins/worker_spi: cannot open shared object file: No such file or directory
    +SELECT * FROM schema4.counted;

It's certainly possible to show a static message without additional text from
errno.

On Tue, Dec 28, 2021 at 9:45 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > For whatever reason, I get slightly different (and somewhat redundant)
> > > output on failing to start:
> > >
> > > 2022-01-08 12:59:36.784 PST [324482] WARNING:  could not load library: $libdir/plugins/totally bogus: cannot open shared object file: No such file or directory
> > > 2022-01-08 12:59:36.787 PST [324482] FATAL:  could not load library: totally bogus: cannot open shared object file: No such file or directory
> > > 2022-01-08 12:59:36.787 PST [324482] LOG:  database system is shut down
> >
> > I think the first WARNING is from the GUC mechanism "setting" the library.
> > And then the FATAL is from trying to apply the GUC.
> > It looks like you didn't apply the 0002 patch for that test so got no CONTEXT ?
> 
> I still had the terminal open where I tested this, and the scrollback
> did show me applying the patch (and building after). I tried a make
> clean and applying the patch again, and I do see the CONTEXT line now.
> I'm not sure what the problem was but seems like PEBKAC--sorry about
> that.

Maybe you missed "make install" or similar.

I took the liberty of adding you as a reviewer here:
https://commitfest.postgresql.org/36/3482/

-- 
Justin

  8. Re: warn if GUC set to an invalid shared library

    Robert Haas <robertmhaas@gmail.com> — 2022-01-28T14:42:17Z

    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 open shared 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

  9. Re: warn if GUC set to an invalid shared library

    Cary Huang <cary.huang@highgo.ca> — 2022-01-28T23:36:20Z

    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 query examples 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 to start 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_libraries to something else in postgresql.conf. This seems a little strange to me 

thank you
Cary

  10. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-29T00:09:12Z

    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 to start 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_libraries to 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

  11. Re: warn if GUC set to an invalid shared library

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2022-02-02T06:06:01Z

    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 for this warning (we may want to note something in the setting docs, but even if so, I think we should figure out the message first 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 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. I think if I'm reading the code correctly, this warning path is shared between ALTER 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 a hint 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 setting takes effect (e.g., attempting to start the server or to start a new session). I think Robert had good feedback regarding the 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 open shared 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 line does actually help, because otherwise it's not clear why the server failed to start, but the warning does seem superfluous here. I do agree that GUC is awkward here, and I like the "while..." wording suggested both for consistency with other 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 warning telling 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

  12. Re: warn if GUC set to an invalid shared library

    David G. Johnston <david.g.johnston@gmail.com> — 2022-02-02T15:39:03Z

    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.

  13. Re: warn if GUC set to an invalid shared library

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2022-02-04T04:36:27Z

    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 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.
>

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.

  14. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-02-10T01:58:54Z

    On Fri, Jan 28, 2022 at 09:42:17AM -0500, Robert Haas wrote:
> -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.

On Wed, Feb 02, 2022 at 06:06:01AM +0000, Maciek Sakrejda wrote:
> I do agree that GUC is awkward here, and I like the "while..." wording suggested both for consistency with other messages and how it could work here:
> CONTEXT: while loading "shared_preload_libraries"

FYI, it has said "while..." and hasn't said "guc" since the 2nd revision of the
patch.

> 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.

I avoided the warning by checking IsUnderPostmaster, though I'm not sure if
that's the right condition..

On Wed, Feb 02, 2022 at 06:06:01AM +0000, Maciek Sakrejda wrote:
> 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.

> I don't really know a good wording here, but maybe a hint like "The server or session will not be able to start if it has been configured to use libraries that cannot be loaded."?

postgres=# ALTER SYSTEM SET shared_preload_libraries =a,b;
WARNING:  could not access file "$libdir/plugins/a"
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.
WARNING:  could not access file "$libdir/plugins/b"
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.
ALTER SYSTEM
postgres=# ALTER SYSTEM SET session_preload_libraries =c,d;
WARNING:  could not access file "$libdir/plugins/c"
HINT:  New sessions will fail with the existing configuration.
WARNING:  could not access file "$libdir/plugins/d"
HINT:  New sessions will fail with the existing configuration.
ALTER SYSTEM

$ ./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...

-- 
Justin

  15. Re: warn if GUC set to an invalid shared library

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2022-02-14T18:12:22Z

    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

  16. Re: warn if GUC set to an invalid shared library

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-23T19:02:23Z

    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

  17. Re: warn if GUC set to an invalid shared library

    Michael Paquier <michael@paquier.xyz> — 2022-06-16T03:14:46Z

    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

  18. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-06-16T03:50:14Z

    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

  19. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-07-22T01:54:12Z

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

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

  20. Re: warn if GUC set to an invalid shared library

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2022-07-22T17:42:11Z

    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

  21. Re: warn if GUC set to an invalid shared library

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-22T17:53:21Z

    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

  22. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-07-22T18:35:57Z

    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.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.

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

  23. Re: warn if GUC set to an invalid shared library

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-22T19:00:23Z

    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

  24. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-07-22T19:14:43Z

    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 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

  25. Re: warn if GUC set to an invalid shared library

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-22T19:26:47Z

    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

  26. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-09-02T22:24:58Z

    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

  27. Re: warn if GUC set to an invalid shared library

    Michael Paquier <michael@paquier.xyz> — 2022-10-12T05:34:09Z

    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

  28. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-10-29T17:40:53Z

    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

  29. Re: warn if GUC set to an invalid shared library

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2022-10-30T23:12:33Z

    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

  30. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-10-31T13:31:20Z

    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

  31. Re: warn if GUC set to an invalid shared library

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-10-31T13:43:05Z

    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

  32. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2022-11-01T22:26:34Z

    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

  33. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2023-07-06T20:15:19Z

    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

  34. Re: warn if GUC set to an invalid shared library

    Shubham Khanna <khannashubham1197@gmail.com> — 2023-12-28T05:26:53Z

    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.

  35. Re: warn if GUC set to an invalid shared library

    John Naylor <johncnaylorls@gmail.com> — 2024-01-01T12:16:02Z

    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.

  36. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2024-01-07T13:27:00Z

    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

  37. Re: warn if GUC set to an invalid shared library

    Robert Haas <robertmhaas@gmail.com> — 2024-05-24T13:26:54Z

    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

  38. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2024-05-24T15:48:34Z

    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

  39. Re: warn if GUC set to an invalid shared library

    Robert Haas <robertmhaas@gmail.com> — 2024-05-24T17:15:13Z

    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

  40. Re: warn if GUC set to an invalid shared library

    Justin Pryzby <pryzby@telsasoft.com> — 2024-07-22T16:28:23Z

    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

  41. Re: warn if GUC set to an invalid shared library

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-11-11T10:09:15Z

    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)

  42. Re: warn if GUC set to an invalid shared library

    Michael Paquier <michael@paquier.xyz> — 2024-12-11T05:12:17Z

    On Mon, Nov 11, 2024 at 12:09:15PM +0200, Heikki Linnakangas wrote:
> I had a quick glance at this, and I agree with Robert's comment earlier that
> this requires a lot of context to understand.

All this feedback was 4 weeks ago, and is unanswered, so I've marked
the CF entry as returned with feedback.  Feel free to resubmit if
necessary.
--
Michael