Re: Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

On 3/7/13 2:42 AM, Amit Kapila wrote:
> I also think the tests added for regression may be more than required...
> If you think above optimization's to reduce number of tests are okay, then I
> will update the patch.

I was not trying to get you to remove regression tests. I was just
pointing out to everyone that the patch seems longer than it really is,
because you put so many of them in there. This is not a problem, and I
didn't bring it up to say you should remove some of them.

This feature is close to being functional enough to make a lot of people
happy. Most of it works the way I've wanted it to for a few years now,
an end point several people have pushed toward in pieces for years now.
There is a decent sized list of issues that I found under careful
testing though. Maybe all these can be resolved quickly and this moves
on to commit candidate. I'd like to see this feature hit the code base,
but it feels like a good amount of work is left to reach there to me
right now.

I'm focusing on functional rather than code quality review here. My
opinion on how the implementation is coded isn't worth very much
anyway--I have a bad track record for correctly judging whether
something is good quality when it starts messing with the GUC system.
The patch has shrunk since original submission a bit, and it has many
regression tests it passes. It may still be objectionably long. There
is a good amount of cleanup in comments and documentation needed, and I
will be happy to work on that part myself. I wanted to get the
functional issues outlined first though. I would be shocked if this
goes in without someone else wanting more code cleanup or
simplification. I don't know who is going to do that other than one of
the committers though.

If Amit can address the functional ones, I can tweak the text/level of
the log messages myself during the cleanup round I do.

= Functional changes =

There are a number of individually small but serious blocker items I
think need to be changed in how this works. I see 7 functional changes
to be made before there's no surprising behavior here. I have a lot of
commentary about how the log/error messages from this might be improved
in addition to that.

1) When you change a sighup or user setting, it writes the config file
out. But it does not signal for a reload. Example:

$ psql -c "show work_mem" -x
work_mem | 1MB
$ psql -c "set persistent work_mem='2MB'"
SET
$ psql -c "show work_mem" -x
work_mem | 1MB

That the above doesn't leave work_mem set to 2MB for new sessions is
surprising. SET PERSISTENT can't do anything about postmaster
parameters like shared_buffers. But it should be able to change
work_mem or a sighup parameter like checkpoint_segments. The regression
test examples call pg_reload_conf() in order to activate the changes.
As a user, I would expect the config reload to be automatic after
executing SET PERSISTENT on parameters that can be changed dynamically.

2) Once automatic activation is happening, a hint is really needed in
cases where it can't happen. I'd like to see a warning about that.
There's one like this in the code already:

LOG: parameter "shared_buffers" cannot be changed without restarting
the server

Something like this would be appropriate for SET PERSISTENT:

LOG: parameter "shared_buffers" cannot be changed without restarting
the server. The changed setting has been saved to
config/postgresql.auto.conf.

3) The config/postgresql.auto.conf file (which I'd like to see named
'config/persistent.auto.conf') is completely overwritten when a change
is made. When the file is written out, both at initdb time and when
changes are made, it should have this as its first line:

# Do not edit this file manually! It is overwritten by the SET
PERSISTENT command.

4) There is one bad problem case left if I try to make this not work.
If I do this:

-rm -rf config/
-Remove "include_dir config"
-Restart the server. It gives me a warning that SET PERSISTENT won't
work because at least one of the multiple things it needs are missing.
-mkdir config
-psql -c "set persistent checkpoint_segments='32'"

That command happily writes out postgresql.auto.conf, but since I
removed the include_dir it doesn't matter. It will never become active.

The check for the include_dir entry needs to happen each time SET
PERSISTENT runs. This is more important than noticing it at server
start time. In fact, I think if this problem case includes the WARNING
about "include_dir config" not being in the postgresql.conf, the check
that happens at server start (shown below) might even go away. People
who object to it as log file garbage will be happier, and users will be
told about the feature being broken if they try to use it.

5) An error message appears every time you reload configuration, if some
part of the SET PERSISTENT mechanism isn't functional. This is going to
be too much for some people. And it's confusing when it happens as part
an interactive psql session. I have an example way down at the very end
of this message.

6) Putting spaces into memory units results in broken config files:

$ psql -c "set persistent work_mem='2 MB'"
SET
$ cat $PGDATA/config/postgresql.auto.conf
work_mem = 2 MB

This is wrong and you'll get this on reload:

LOG: syntax error in file
"/Users/gsmith/pgwork/data/autoconf/config/postgresql.auto.conf" line 1,
near token "MB"
LOG: configuration file
"/Users/gsmith/pgwork/data/autoconf/postgresql.conf" contains errors; no
changes were applied

It needs to look like this:

work_mem = '2 MB'

A regression test for some cases with spaces in the middle should be
added too. This case is really weird, due to how the code is reading
the existing postgresql.auto.conf file. If I first manually fix the
file, and then run a change again, it stays fixed! Look at this weird
sequence:

$ psql -c "set persistent work_mem='2 MB'"
SET
$ cat $PGDATA/config/postgresql.auto.conf
work_mem = 2 MB
[Here this setting is broken]
$ vi $PGDATA/config/postgresql.auto.conf
[Add quotes around the value, now it works]
$ cat $PGDATA/config/postgresql.auto.conf
work_mem = '2 MB'
$ pg_ctl reload
server signaled
$ psql -c "set persistent work_mem='4 MB'"
SET
$ cat $PGDATA/config/postgresql.auto.conf
work_mem = '4 MB'

Below I'll rant some more about how looking at what's in the existing
file, rather than the in-memory GUCs, has some other surprising properties.

7) If I run SET PERSISTENT a lot concurrently, something happens that
slows down connecting to the server. Restarting the server makes it go
away. I have a pgbench test case demonstrating the problem below, in
the "Concurrency" section. I haven't tried to replicate it on another
system yet.

= Tested features that work fine =

Entries added here are tracked as you'd expect:

$ psql -c "set persistent checkpoint_segments='32'"
$ pg_ctl reload
$ psql -xc "select name,setting,sourcefile,sourceline from pg_settings
where name='checkpoint_segments'"
name | checkpoint_segments
setting | 32
sourcefile | /Users/gsmith/pgwork/data/autoconf/config/postgresql.auto.conf
sourceline | 1

When the postgresql.auto.conf file is missing and you use SET
PERSISTENT, it quietly creates it when writing out the new setting.

= Concurrency and performance slowdown =

I made two pgbench scripts that adjust a guc, one user and the other
sighup, to a random value:

random-join-limit.sql:

\setrandom limit 1 8
set persistent join_collapse_limit=:limit;
select pg_reload_conf();

random-segments.sql:

\setrandom segments 3 16
set persistent checkpoint_segments=:segments;
select pg_reload_conf();

I then fired off a bunch of these in parallel:

pgbench -n -f random-join-limit.sql -f random-segments.sql -c 8 -T 60

This ran at 1547 TPS on my 8 core Mac, so that's not bad. No assertions
and the config file was still valid at the end, which is a good sign the
locking method isn't allowing utter chaos. Without the pg_reload_conf()
in the test files, it also completed. With the reload happening in one
file but not the other, things were also fine.

However, one thing I saw is that the server got significantly slower the
more I ran this test script. After a few minutes it was down to only
400 TPS. The delay shows up between when I run psql and when I get the
prompt back. Here's normal behavior:

$ time psql -c "select 1"
real 0m0.006s

And here's what I get after a single run of this pgbench hammering:

$ time psql -c "select 1"
real 0m0.800s

800ms? The slowdown is all for psql to start and connect, it's not in
the executor:

$ time psql -c "explain analyze select 1"
QUERY PLAN
------------------------------------------------------------------------------------
Result (cost=0.00..0.01 rows=1 width=0) (actual time=0.001..0.001
rows=1 loops=1)
Total runtime: 0.039 ms
(2 rows)

real 0m0.807s

Stopping and restarting the server brings the performance back to
normal. Maybe this is one of those assertion build issues, but it
smells like there could be nasty memory leak somewhere instead.
Clearing whatever weirdness is going on here is a blocking issue.

= Loss of in-memory changes =

In this example, I change two settings, corrupt the file, and then
change one of them a second time:

$ psql -c "set persistent checkpoint_segments='32'"
$ psql -c "set persistent work_mem='2MB'"
$ cat postgresql.auto.conf
checkpoint_segments = 32
work_mem = 2MB
$ rm postgresql.auto.conf
$ psql -c "set persistent work_mem='2MB'"
SET
$ cat postgresql.auto.conf
work_mem = 2MB
$ psql -xc "show checkpoint_segments"
checkpoint_segments | 3

That this happens isn't unreasonable, and I can live with the
limitation. The change to work_mem is lost, but it didn't have to be.
A user might expect that in this case the last SET PERSISTENT would have
written out a file with both the settings still intact. The running GUC
entries certainly knew that postgresql.auto.conf had two lines with
these entries at that point. All in-memory persistent changes could be
dumped out to postgresql.auto.conf. That's what I had hoped was
possible in the implementation.

Amit may have this right though, because I think the code he's using is
simpler and reliable than what I had in mind. I'm really not sure which
way is better. This one isn't a blocker, and if this gets committed it
could easily be nudged around later, as an internal change without a
catversion bump.

= Error messages =

If you remove postgresql.auto.conf and restart the server, it gives a
warning that SET PERSISTENT won't work until you put it back. The error
in this and several similar cases is pretty generic too:

WARNING: File "postgresql.auto.conf" is not accessible, either file
"postgresql.auto.conf" or folder "config" doesn't exist. or default
"config" is not present in postgresql.conf.

It would be nice if the error were more specific, individually
identifying which of these is the actual problem. I can rewrite that
long text entry to be more readable, but I think it should be a series
of smaller error checks with their own individual messages instead.

If you remove postgresql.auto.conf then exeute "pg_ctl reload", it gives
this error 6 times, which seems excessive. Reducing how often it
appears in the reload case would be nice.

Deleting the whole config directory gives this:

LOG: could not open configuration directory
"/Users/gsmith/pgwork/data/autoconf/config": No such file or directory
LOG: configuration file
"/Users/gsmith/pgwork/data/autoconf/postgresql.conf" contains errors; no
changes were applied

If you now try to use the feature, the error message could be better.

$ psql -c "set persistent checkpoint_segments='32'"
ERROR: Failed to open auto conf temp file
"/Users/gsmith/pgwork/data/autoconf/config/postgresql.auto.conf.temp":
No such file or directory

It would be nice to complain about the config directory being missing,
as a first check before the file is opened. Restarting the server in
this situation throws the correct error in your face though:

LOG: could not open configuration directory
"/Users/gsmith/pgwork/data/autoconf/config": No such file or directory
FATAL: configuration file
"/Users/gsmith/pgwork/data/autoconf/postgresql.conf" contains errors

If you render this whole system unavailable by removing the "include_dir
config", at every server start you'll see this:

WARNING: File "postgresql.auto.conf" is not accessible, either file
"postgresql.auto.conf" or folder "config" doesn't exist. or default
"config" is not present in postgresql.conf.
Configuration parameters changed before start/reload of server with SET
PERSISTENT command will not be effective.
LOG: database system was shut down at 2013-03-09 23:55:03 EST

This is a debatable design choice. Some people might not want the file
and remove it, but don't want to be nagged about it. If people want to
wipe out the file or directory and work the old way, without this
feature available, that's fine and they can. To me, helping users who
accidentally break this is more important than reducing harmless warning
messages for things people did intentionally. WARNING might not be the
right level for this though. The existing checks like this I showed
above use LOG for this sort of thing.

The bigger problem is that this message shows up whenever you reload the
config too. Watch this bizarre sequence:

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

gsmith=#
gsmith=# select 1;
WARNING: File "postgresql.auto.conf" is not accessible, either file
"postgresql.auto.conf" or folder "config" doesn't exist. or default
"config" is not present in postgresql.conf.
Configuration parameters changed before start/reload of server with SET
PERSISTENT command will not be effective.
?column?
----------
1

And as I commented above, shifting more of the checks to SET PERISTENT
time could eliminate this check from running at server start and reload
altogether. I would be fine with them *also* happening at server start.
But I could understand that other people might not like that. And
having this pop up on every reload, appearing to a client next to
another statement altogether, that isn't acceptable though.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com