Re: Re: Proposal for Allow postgresql.conf values to be changed via SQL [review] - Mailing list pgsql-hackers

On Monday, March 11, 2013 12:18 PM Amit Kapila wrote:
> 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,
> > 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.

I have not updated code to address 1 and 2, because as per my observation
the 
reason for your test-7 is that you have called pg_reload_conf(), after each
test.
So I feel calling directly in code will also lead to same problem.
I will hold to change code for this till that issue is resolved.

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

Fixed both 2a and 2b.

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

Fixed by parsing the file and if include_dir is missing, then give Warning
to user.
In case user deletes the folder, ERROR will be thrown, because in anycase it
cannot perform the 
Command if folder is deleted.
If persistent.auto.conf file is missing, then don't raise any error as SET
PERSISTENT command will
create the file.
> > 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.

Now for reload there will be no error, as we handled the same in SET
PERSISTENT command.
at time of startup it will LOG the 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.

Fixed. Now all values will be in quotes, as at that point it is difficult to
distinguish
as per current implementation.

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

I suspect this is due to pg_reload_conf(). I am looking into it.
However could you please try once without pg_reload_conf(). 

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

Done as explained above.

With Regards,
Amit Kapila.