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

On Sunday, March 10, 2013 8:43 PM Greg Smith wrote:
> 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.

Thank you for a careful and detail testing.

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

Sure, I would address the functional issues.

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

I think we cannot guarantee even after calling pg_reload_conf(), as this is
an
asynchronous function call.
We already once had a discussion about this point and as I can understand it
is
concluded that it should not be handled. Refer the below mail:
http://www.postgresql.org/message-id/21869.1360683928@sss.pgh.pa.us

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

2 things, you want as part of this comment:
a. change the name of .auto file to persistent.auto.conf
b. new comment in beginning of persistent.auto.conf file.

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

This can be handled, but for this we need to parse the whole postgresql.conf
file
and then give the warning. This will increase the time of SET PERSISTENT
command.

Or the other way might be to note down the information about include dir at
startup and then use it,
but implementation can get complicated for scenarios if somebody change the
postgresql.conf and reload it.

I will check whichever way is less complicated, I will handle it that way.

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

I shall fix this behavior.

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

Okay, I shall look into this 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.

In-memory values are not guaranteed after running SET PERSISTENT, even
though we
do pg_reload_conf(), so IMO we should leave this behavior as it is.

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

You are right, we should change this by identifying actual problems.

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

Agreed, I will add the check at time of SET PERSISTENT command.
Also I will try to remove this message at time of reload.

With Regards,
Amit Kapila.