should we document an example to set multiple libraries in shared_preload_libraries?

On Wed, Dec 01, 2021 at 04:20:52PM +0530, Bharath Rupireddy wrote:

It seems like users can try different ways to set multiple values for
shared_preload_libraries GUC even after reading the documentation
[1]), something like:

...

ALTER SYSTEM SET shared_preload_libraries = "auth_delay,pg_stat_statements,sepgsql"; --> wrong

The problem with the wrong parameter set command is that the ALTER
SYSTEM SET will not fail, but the server will not come up in case it
is restarted. In various locations in the documentation, we have shown
how a single value can be set, something like:
shared_preload_libraries = 'auth_delay'
shared_preload_libraries = 'pg_stat_statements'
shared_preload_libraries = 'sepgsql'

Isn't it better we document (in [1]) an example to set multiple values
to shared_preload_libraries?

+1 to document it, but it seems like the worse problem is allowing the admin to
write a configuration which causes the server to fail to start, without having
issued a warning.

I think you could fix that with a GUC check hook to emit a warning.
I'm not sure what objections people might have to this. Maybe it's confusing
to execute preliminary verification of the library by calling stat() but not do
stronger verification for other reasons the library might fail to load. Like
it doesn't have the right magic number, or it's built for the wrong server
version. Should factor out the logic from internal_load_library and check
those too ?

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

+1 to document it, but it seems like the worse problem is allowing the admin to
write a configuration which causes the server to fail to start, without having
issued a warning.

I think you could fix that with a GUC check hook to emit a warning.
I'm not sure what objections people might have to this. Maybe it's confusing
to execute preliminary verification of the library by calling stat() but not do
stronger verification for other reasons the library might fail to load. Like
it doesn't have the right magic number, or it's built for the wrong server
version. Should factor out the logic from internal_load_library and check
those too ?

Considering the vanishingly small number of actual complaints we've
seen about this, that sounds ridiculously over-engineered.
A documentation example should be sufficient.

regards, tom lane

On Wed, Dec 1, 2021 at 6:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

+1 to document it, but it seems like the worse problem is allowing the admin to
write a configuration which causes the server to fail to start, without having
issued a warning.

I think you could fix that with a GUC check hook to emit a warning.
I'm not sure what objections people might have to this. Maybe it's confusing
to execute preliminary verification of the library by calling stat() but not do
stronger verification for other reasons the library might fail to load. Like
it doesn't have the right magic number, or it's built for the wrong server
version. Should factor out the logic from internal_load_library and check
those too ?

Considering the vanishingly small number of actual complaints we've
seen about this, that sounds ridiculously over-engineered.
A documentation example should be sufficient.

Thanks. Here's the v1 patch adding examples in the documentation.

Regards,
Bharath Rupireddy.

On 12/1/21, 5:59 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Dec 1, 2021 at 6:45 PM Tom Lane <tgl@sss.pgh.pa.us> 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.

Thanks. Here's the v1 patch adding examples in the documentation.

I think the problems you noted upthread are shared for all GUCs with
type GUC_LIST_QUOTE (e.g., search_path, temp_tablespaces). Perhaps
the documentation for each of these GUCs should contain a short blurb
about how to properly SET a list of values.

Also upthread, I see that you gave the following example for an
incorrect way to set shared_preload_libraries:

ALTER SYSTEM SET shared_preload_libraries =
auth_delay,pg_stat_statements,sepgsql; --> wrong

Why is this wrong? It seems to work okay for me.

Nathan

On Fri, Dec 03, 2021 at 12:45:56AM +0000, Bossart, Nathan wrote:

I think the problems you noted upthread are shared for all GUCs with
type GUC_LIST_QUOTE (e.g., search_path, temp_tablespaces). Perhaps
the documentation for each of these GUCs should contain a short blurb
about how to properly SET a list of values.

Yeah, the approach taken by the proposed patch is not going to scale
and age well.

It seems to me that we should have something dedicated to lists around
the section for "Parameter Names and Values", and add a link in the
description of each parameters concerned back to the generic
description.

Also upthread, I see that you gave the following example for an
incorrect way to set shared_preload_libraries:

ALTER SYSTEM SET shared_preload_libraries =
auth_delay,pg_stat_statements,sepgsql; --> wrong

Why is this wrong? It seems to work okay for me.

Yep.
--
Michael

On Fri, Dec 3, 2021 at 6:33 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Dec 03, 2021 at 12:45:56AM +0000, Bossart, Nathan wrote:

I think the problems you noted upthread are shared for all GUCs with
type GUC_LIST_QUOTE (e.g., search_path, temp_tablespaces). Perhaps
the documentation for each of these GUCs should contain a short blurb
about how to properly SET a list of values.

Yeah, the approach taken by the proposed patch is not going to scale
and age well.

It seems to me that we should have something dedicated to lists around
the section for "Parameter Names and Values", and add a link in the
description of each parameters concerned back to the generic
description.

+1 to add here in the "Parameter Names and Values section", but do we
want to backlink every string parameter to this section? I think it
needs more effort. IMO, we can just backlink for
shared_preload_libraries alone. Thoughts?

<listitem>
<para>
<emphasis>String:</emphasis>
In general, enclose the value in single quotes, doubling any single
quotes within the value. Quotes can usually be omitted if the value
is a simple number or identifier, however.
</para>
</listitem>

Also upthread, I see that you gave the following example for an
incorrect way to set shared_preload_libraries:

ALTER SYSTEM SET shared_preload_libraries =
auth_delay,pg_stat_statements,sepgsql; --> wrong

Why is this wrong? It seems to work okay for me.

Yep.

My bad. Yes, it works.

Regards,
Bharath Rupireddy.

On 12/3/21, 6:21 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

+1 to add here in the "Parameter Names and Values section", but do we
want to backlink every string parameter to this section? I think it
needs more effort. IMO, we can just backlink for
shared_preload_libraries alone. Thoughts?

IMO this is most important for GUC_LIST_QUOTE parameters, of which
there are only a handful. I don't think adding a link to every string
parameter is necessary.

Nathan

On Fri, Dec 3, 2021 at 11:25 PM Bossart, Nathan <bossartn@amazon.com> wrote:

On 12/3/21, 6:21 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

+1 to add here in the "Parameter Names and Values section", but do we
want to backlink every string parameter to this section? I think it
needs more effort. IMO, we can just backlink for
shared_preload_libraries alone. Thoughts?

IMO this is most important for GUC_LIST_QUOTE parameters, of which
there are only a handful. I don't think adding a link to every string
parameter is necessary.

Agree.

Should we specify something like below in the "Parameter Names and
Values" section's "String:" para? Do we use generic terminology like
'name' and val1, val2, val3 and so on?

ALTER SYSTEM SET name = val1,val2,val3;
ALTER SYSTEM SET name = 'val1', 'val2', 'val3';
ALTER SYSTEM SET name = '"val 1"', '"val,2"', 'val3';

Another thing I observed is the difference between how the
postgresql.conf file and ALTER SYSTEM SET command is parsed for
GUC_LIST_QUOTE values.

For instance, in postgresql.conf file, by default search_path is
specified as follows:
search_path = '"$user", public',
postgres=# show search_path ;
search_path
-----------------
"$user", public
(1 row)

When I use the same style with ALTER SYSTEM SET command, the value is
treated as single string value:
postgres=# ALTER SYSTEM SET search_path = '"$user", public';
ALTER SYSTEM
postgres=# show search_path ;
search_path
-----------------
"$user", public
(1 row)

postgres=# select pg_reload_conf();
pg_reload_conf
----------------
t
(1 row)

postgres=# show search_path ;
search_path
---------------------
"""$user"", public"
(1 row)

Am I missing something here? Or is there a distinction between parsing
of postgresql.conf and ALTER SYSTEM SET command for GUC_LIST_QUOTE
values? If so, what is it?

Regards,
Bharath Rupireddy.

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:

Am I missing something here? Or is there a distinction between parsing
of postgresql.conf and ALTER SYSTEM SET command for GUC_LIST_QUOTE
values? If so, what is it?

One context is SQL, the other is not. The quoting rules are
really quite different.

regards, tom lane

On Mon, Dec 6, 2021 at 9:20 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Dec 3, 2021 at 11:25 PM Bossart, Nathan <bossartn@amazon.com> wrote:

On 12/3/21, 6:21 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

+1 to add here in the "Parameter Names and Values section", but do we
want to backlink every string parameter to this section? I think it
needs more effort. IMO, we can just backlink for
shared_preload_libraries alone. Thoughts?

IMO this is most important for GUC_LIST_QUOTE parameters, of which
there are only a handful. I don't think adding a link to every string
parameter is necessary.

Agree.

Should we specify something like below in the "Parameter Names and
Values" section's "String:" para? Do we use generic terminology like
'name' and val1, val2, val3 and so on?

ALTER SYSTEM SET name = val1,val2,val3;
ALTER SYSTEM SET name = 'val1', 'val2', 'val3';
ALTER SYSTEM SET name = '"val 1"', '"val,2"', 'val3';

Another thing I observed is the difference between how the
postgresql.conf file and ALTER SYSTEM SET command is parsed for
GUC_LIST_QUOTE values.

Since there's a difference in the way the params are set in the
postgresql.conf file and ALTER SYSTEM SET command (as pointed out by
Tom in this thread [1]/messages/by-id/3108541.1638806477@sss.pgh.pa.us), I'm now confused. If we were to add examples
to the "Parameter Names and Values" section, which examples should we
addd, postgresql.conf files ones or ALTER SYSTEM SET command ones?

Thoughts?

[1]: /messages/by-id/3108541.1638806477@sss.pgh.pa.us

Regards,
Bharath Rupireddy.

On Wed, Dec 1, 2021 at 5:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

+1 to document it, but it seems like the worse problem is allowing the admin to
write a configuration which causes the server to fail to start, without having
issued a warning.

I think you could fix that with a GUC check hook to emit a warning.
...

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.

I'm over-dramatizing this a bit, but I do think there are a lot of
opportunities to make mistakes here, and this behavior could be more
user-friendly beyond just documentation changes. If a config change is
bogus most likely due to a quoting mistake or a typo, a warning would
be fantastic (i.e., the stat() check Justin suggested). Or maybe the
FATAL log message on a failed startup could include the source of the
problem?

Thanks,
Maciek

On Thu, Dec 9, 2021 at 1:02 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:

On Wed, Dec 1, 2021 at 5:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

+1 to document it, but it seems like the worse problem is allowing the admin to
write a configuration which causes the server to fail to start, without having
issued a warning.

I think you could fix that with a GUC check hook to emit a warning.
...

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.

I'm over-dramatizing this a bit, but I do think there are a lot of
opportunities to make mistakes here, and this behavior could be more
user-friendly beyond just documentation changes. If a config change is
bogus most likely due to a quoting mistake or a typo, a warning would
be fantastic (i.e., the stat() check Justin suggested). Or maybe the
FATAL log message on a failed startup could include the source of the
problem?

How about ALTER SYSTEM SET shared_preload_libraries command itself
checking for availability of the specified libraries (after fetching
library names from the parsed string value) with stat() and then
report an error if any of the libraries doesn't exist? Currently, it
just accepts if the specified value passes the parsing.

Regards,
Bharath Rupireddy.

On Wed, Dec 1, 2021 at 5:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

+1 to document it, but it seems like the worse problem is allowing 
+the admin to
write a configuration which causes the server to fail to start, 
without having issued a warning.

I think you could fix that with a GUC check hook to emit a warning.
...

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.

I'm over-dramatizing this a bit, but I do think there are a lot of opportunities to make mistakes here, and this behavior could be more user-friendly beyond just documentation changes. If a config change is bogus most likely due to a quoting mistake or a typo, a warning would be fantastic (i.e., the stat() check Justin suggested). Or maybe the FATAL log message on a failed startup could include the source of the problem?

Thanks,
Maciek

I may have missed something in this stream, but is this a system controlled by Patroni? In any case I to have gotten stuck like this. If this is a Patroni system, I've discovered that patroni either ides or prevents "out of memory" messages from getting into the db log. If it is patroni controlled, I've solved this by turning off Patroni, starting the DB using pg_ctl and then I can examine the log messages. With pg_ctl, you can edit the postgresql.conf and see what you can do. Alternatively, with the DCS you can make 'dynamic edits' to the system configuration without the db running. Use the patroni control utility to do an 'edit-config' to make the changes. Then reload the config (same utility) and then you can bring up the db with Patroni...

Smiles,
phil

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

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Dec 9, 2021 at 7:42 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

How about ALTER SYSTEM SET shared_preload_libraries command itself
checking for availability of the specified libraries (after fetching
library names from the parsed string value) with stat() and then
report an error if any of the libraries doesn't exist? Currently, it
just accepts if the specified value passes the parsing.

That certainly would have helped me. I guess it technically breaks the
theoretical use case of "first change the shared_preload_libraries
setting, then install those libraries, then restart Postgres," but I
don't see why anyone would do that in practice.

For what it's worth, I don't even feel strongly that this needs to be
an error—just that the current flow around this is error-prone due to
several sharp edges and could be improved. I would be happy with an
error, but a warning or other mechanism could work, too. I do think
better documentation is not enough: the differences between a working
setting value and a broken one are pretty subtle.

Thanks,
Maciek

On Fri, Dec 10, 2021 at 10:10 AM Godfrin, Philippe E
<Philippe.Godfrin@nov.com> wrote:

I may have missed something in this stream, but is this a system controlled by Patroni?

In my case, no: it's a pretty vanilla PGDG install on Ubuntu 20.04.
Thanks for the context, though.

Thanks,
Maciek