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

What's the use case of this? It sounds like it will just create a maintenance nightmare where some stuff you expect to lookup in in postgresql.conf is actually hiding in the .auto file. Assuming only super users/sysadmins would have the ability to change things in the config file, wouldn't they be more likely to just do it on the server and edit the .conf (which among other things, keeps it tidy and orderly).

Also, how would you propose to handle settings that require the server to be restarted, such as checkpoint_segments? It seems like by allowing these to be set via a command (which isn't really SQL) you're creating the impression that they will take immediate effect, which isn't the case.

Just my $0.02. Of course, I might be missing the point.

Il giorno 30/ott/2012, alle ore 00:31, Amit Kapila ha scritto:

> SYNTAX:
> ALTER SYSTEM SET configuration_parameter = value COMMENT 'value';
>
> DESIGN IDEA:
> (a) have a postgresql.conf.auto
> (b) add a default include for postgresql.conf.auto at the beginning of PostgreSQL.conf
> (c) SQL updates go to postgresql.conf.auto, which consists only of"setting = value #comments" .
> (d) We document that settings which are changed manually in postgresql.conf will override postgresql.conf.auto.
>
> IMPLEMENTATION IDEA:
>
> The main Idea is we create a lock file, it acts as lock to avoid concurrent edit into .conf auto file
> and also as an intermediate file where we keep all the new changes until we commit the alter system command.
>
> CCREATION OF AUTO FILE
> 1. during initdb we create the .auto file and it will be empty.
> 2. .conf file will have its first entry as follows
>
>
> #------------------------------------------------------------------------------
> # Postgresql.conf.auto inclusion
> #------------------------------------------------------------------------------
> # Do not edit postgresql.conf.auto file or remove the include.
> # You can Edit the settings below in this file which will override auto-generated file.
>
> include = 'postgresql.conf.auto'
>
>
>
>
> ALGORITHM for ALTER SYSTEM:
> 1. check whether the given key : value is valid.
> -- This is done so that next read from .auto file should not throw error.
> 2. get postgresql.conf.auto path. (always the data directory)
> -- Since the .auto file in data directory pg_basebackup will pick it up.
> 3. Create the postgresql.conf.auto.lock file( with O_EXCL flag).
> -- This act as a protection from other backends who are trying to edit this file.
> -- If already exist we wait for some time by retrying.
> 4. Open the postgresql.conf.auto file in read mode.
> 5. Write the new (key, value, comment) in to the postgresql.conf.auto.lock file by using below steps:
> a. read the contents of postgresql.conf.auto in to memory buffer line by line.
> b. Scan for key in postgresql.conf.auto file.
> if found get the line number in file such that where we have to insert the new (key,value).
> else we should write the new (key, value) pair to last line.
> c. add the new (key, value, comment) to memory buffer to the line as found in step b.
> d. Write the memory buffer into postgresql.conf.auto.lock file.
> -- here memory buffer represent the modified state of the postgresql.conf.auto file.
> e. Commit the .lock file.
> -- Here rename the lock file to auto file.
> -- If auto file is opened by other process (SIGHUP processing) then we retry rename for some time
> other wise alter system command fails.
> f. If any error in between rollback lock file
> -- here delete the lock file.
>
>
> CLARIFICATION
> 1. Tom, the below is mentioned by you in one of the discussions for this topic. I need small clarification:
> "About the only change I want to make immediately is that initdb ought to shove its settings into postgresql.auto instead of mucking with
> postgresql.conf."
> So do you mean to say the settings done by initdb (like max_connections, etc.) need to be in .auto file instead of .conf and let these
> parameters be commented in .conf?
> 2. Do .auto file needs to be included by default?
> 3. Can the path of .auto be fixed as data directory path?
>
> Note:
> 1. Only One backend can edit conf file at a time others wait.
> 2. Suppose .auto have invalid entry eg: listening port number mentioned is taken up by other application
> then if we try to restart the postgres it fails. This need manual intervention.
> 3. This command cannot be executed inside the transaction block. Not sure what to do for this part, whether it needs to be supported
> in a block?
> 4. currently command for reset or invalidation of (key, value) is not implemented.
>
>
> Comments/Suggestions about the value of this feature and Implementation Idea?
>
> With Regards,
> Amit Kapila.

On Monday, October 29, 2012 7:11 PM Chris Corbyn

> What's the use case of this? It sounds like it will just create a
maintenance nightmare where some stuff you expect to lookup in in
postgresql.conf is actually hiding in the .auto file. Assuming only super
users/sysadmins would have the ability to change things in the config file,
wouldn't they be more likely to just do it on the server and edit the .conf
(which among other things, keeps it tidy and orderly).

Basically after this user will have 2 options to change the postgresql.conf
parameters.

One is by directly editing the postgresql.conf file and

Other is by using SQL commands.

There will be nothing hidden in .auto file, it's just that it will create
separate file for parameters set by SQL command to avoid the hassles of
parsing the postgresql.conf during the processing of SQL command. Anything
changed by user in postgresql.conf will override the values in
postgresql.conf.auto

>Also, how would you propose to handle settings that require the server to
be restarted, such as checkpoint_segments? It seems like by allowing these
to be set via a command (which isn't really SQL) you're creating the
impression that they will take immediate effect, which isn't the case.

The values will take effect after server restart or by SIGHUP.

Il giorno 30/ott/2012, alle ore 00:31, Amit Kapila ha scritto:

SYNTAX:

ALTER SYSTEM SET configuration_parameter = value COMMENT 'value';

DESIGN IDEA:

(a) have a postgresql.conf.auto

(b) add a default include for postgresql.conf.auto at the beginning of
PostgreSQL.conf

(c) SQL updates go to postgresql.conf.auto, which consists only of"setting =
value #comments" .

(d) We document that settings which are changed manually in postgresql.conf
will override postgresql.conf.auto.

IMPLEMENTATION IDEA:

The main Idea is we create a lock file, it acts as lock to avoid concurrent
edit into .conf auto file

and also as an intermediate file where we keep all the new changes until we
commit the alter system command.

CCREATION OF AUTO FILE

1. during initdb we create the .auto file and it will be empty.

2. .conf file will have its first entry as follows

#---------------------------------------------------------------------------
---

# Postgresql.conf.auto inclusion

#---------------------------------------------------------------------------
---

# Do not edit postgresql.conf.auto file or remove the include.

# You can Edit the settings below in this file which will override
auto-generated file.

include = 'postgresql.conf.auto'

ALGORITHM for ALTER SYSTEM:

1. check whether the given key : value is valid.

-- This is done so that next read from .auto file should not
throw error.

2. get postgresql.conf.auto path. (always the data directory)

-- Since the .auto file in data directory pg_basebackup will
pick it up.

3. Create the postgresql.conf.auto.lock file( with O_EXCL flag).

-- This act as a protection from other backends who are
trying to edit this file.

-- If already exist we wait for some time by retrying.

4. Open the postgresql.conf.auto file in read mode.

5. Write the new (key, value, comment) in to the
postgresql.conf.auto.lock file by using below steps:

a. read the contents of postgresql.conf.auto in to memory buffer
line by line.

b. Scan for key in postgresql.conf.auto file.

if found get the line number in file such that where we
have to insert the new (key,value).

else we should write the new (key, value) pair to last
line.

c. add the new (key, value, comment) to memory buffer to the line
as found in step b.

d. Write the memory buffer into postgresql.conf.auto.lock file.

-- here memory buffer represent the modified state of the
postgresql.conf.auto file.

e. Commit the .lock file.

-- Here rename the lock file to auto file.

-- If auto file is opened by other process (SIGHUP
processing) then we retry rename for some time

other wise alter system command fails.

f. If any error in between rollback lock file

-- here delete the lock file.

CLARIFICATION

1. Tom, the below is mentioned by you in one of the discussions for this
topic. I need small clarification:

"About the only change I want to make immediately is that initdb ought to
shove its settings into postgresql.auto instead of mucking with

postgresql.conf."

So do you mean to say the settings done by initdb (like max_connections,
etc.) need to be in .auto file instead of .conf and let these

parameters be commented in .conf?

2. Do .auto file needs to be included by default?

3. Can the path of .auto be fixed as data directory path?

Note:

1. Only One backend can edit conf file at a time others wait.

2. Suppose .auto have invalid entry eg: listening port number mentioned is
taken up by other application

then if we try to restart the postgres it fails. This need manual
intervention.

3. This command cannot be executed inside the transaction block. Not sure
what to do for this part, whether it needs to be supported

in a block?

4. currently command for reset or invalidation of (key, value) is not
implemented.

Comments/Suggestions about the value of this feature and Implementation
Idea?

With Regards,

Amit Kapila.

On 10/29/12 6:40 AM, Chris Corbyn wrote:
> What's the use case of this? It sounds like it will just create a maintenance nightmare where some stuff you expect to lookup in in postgresql.conf is actually hiding in the .auto file. Assuming only super users/sysadmins would have the ability to change things in the config file, wouldn't they be more likely to just do it on the server and edit the .conf (which among other things, keeps it tidy and orderly).

The use is the ability to manage dozens, or hundreds, of PostgreSQL
servers via Port 5432. It would also make writing an auto-configurator
easier.

I agree that there's not much benefit if you're only managing a single
PostgreSQL server. There's a lot of benefit for those of us who have to
manage a lot of them though.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Tue, Oct 30, 2012 at 5:25 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 10/29/12 6:40 AM, Chris Corbyn wrote:
>> What's the use case of this? It sounds like it will just create a maintenance nightmare where some stuff you expect to lookup in in postgresql.conf is actually hiding in the .auto file. Assuming only super users/sysadmins would have the ability to change things in the config file, wouldn't they be more likely to just do it on the server and edit the .conf (which among other things, keeps it tidy and orderly).
>
> The use is the ability to manage dozens, or hundreds, of PostgreSQL
> servers via Port 5432. It would also make writing an auto-configurator
> easier.
>
> I agree that there's not much benefit if you're only managing a single
> PostgreSQL server. There's a lot of benefit for those of us who have to
> manage a lot of them though.

I rather think that the fact that postgresql.conf has supported an
"include directive" since at least as far back as 8.2 (likely further;
I'll not bother spelunking further into the docs) makes this extremely
troublesome.

We have long supported the notion that this configuration does not
have a Unique Place to be (e.g. - if you use INCLUDE, then there are
at least two possible places).

I should think that doing this requires heading back towards there
being a single unique configuration stream, and over the course of
several versions, deprecating the INCLUDE directive.

I imagine it means we'd want to come up with a representation that has
suitable semantics for being written to, make sure it is reasonably
expressive *without* INCLUDE, and establish a migration path between
the old and new forms. At some point, the old form can be treated as
vestigal, and be dropped.
--
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"

> I should think that doing this requires heading back towards there
> being a single unique configuration stream, and over the course of
> several versions, deprecating the INCLUDE directive.

Oh, maybe I should take a closer look at Amit's proposal then. I
thought we planned to make use of the INCLUDE facility for SET
PERSISTENT, including supporting include-if-exists. Possibly what he's
proposing and what I thought our last consensus were are highly divergent.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 10/29/2012 03:14 PM, Amit Kapila wrote:
>
> On Monday, October 29, 2012 7:11 PM Chris Corbyn
>
> > What's the use case of this? It sounds like it will just create a
> maintenance nightmare where some stuff you expect to lookup in in
> postgresql.conf is actually hiding in the .auto file. Assuming only
> super users/sysadmins would have the ability to change things in the
> config file, wouldn't they be more likely to just do it on the server
> and edit the .conf (which among other things, keeps it tidy and orderly).
>
> Basically after this user will have 2 options to change the
> postgresql.conf parameters.
>
> One is by directly editing the postgresql.conf file and
>
> Other is by using SQL commands.
>
> There will be nothing hidden in .auto file, it's just that it will
> create separate file for parameters set by SQL command to avoid the
> hassles of parsing the postgresql.conf during the processing of SQL
> command.
>
If interested I have somewhere pl/pythhonu functions for both looking at
and
changing parameters in postgresql.conf file,

It even keeps the old value and adds comments both to old and to the new
one abot who an when changed it.
Could also be extended to fpr example rotate last 10 postgreSQL conf
files and/or skip rewriting the file in case the effective value of GUC
did not change.

Cheers,
Hannu

Christopher Browne escribió:
> On Tue, Oct 30, 2012 at 5:25 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> > On 10/29/12 6:40 AM, Chris Corbyn wrote:
> >> What's the use case of this? It sounds like it will just create a maintenance nightmare where some stuff you expect to lookup in in postgresql.conf is actually hiding in the .auto file. Assuming only super users/sysadmins would have the ability to change things in the config file, wouldn't they be more likely to just do it on the server and edit the .conf (which among other things, keeps it tidy and orderly).
> >
> > The use is the ability to manage dozens, or hundreds, of PostgreSQL
> > servers via Port 5432. It would also make writing an auto-configurator
> > easier.
> >
> > I agree that there's not much benefit if you're only managing a single
> > PostgreSQL server. There's a lot of benefit for those of us who have to
> > manage a lot of them though.
>
> I rather think that the fact that postgresql.conf has supported an
> "include directive" since at least as far back as 8.2 (likely further;
> I'll not bother spelunking further into the docs) makes this extremely
> troublesome.

This is precisely the reason why "source file" and "source line" are now
tracked for configuration parameters. If a setting is in the auto file
(or any other file), it would be very simple to find.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> I should think that doing this requires heading back towards there
>> being a single unique configuration stream, and over the course of
>> several versions, deprecating the INCLUDE directive.

> Oh, maybe I should take a closer look at Amit's proposal then. I
> thought we planned to make use of the INCLUDE facility for SET
> PERSISTENT, including supporting include-if-exists. Possibly what he's
> proposing and what I thought our last consensus were are highly divergent.

I'm not convinced we ever *had* a consensus on this. There were
proposals, but I'm not sure a majority ever bought into any one of 'em.
The whole problem of intermixing manual editing and programmatic editing
is just a big can of worms, and not everybody is prepared to give up the
former to have the latter.

You can, if you are so inclined, implement something functionally
equivalent to Amit's proposal today using contrib/adminpack's
pg_file_write --- okay, it's much less convenient than a built-in
implementation would be, but you can drop some variable assignments into
a file and then put a suitable INCLUDE into the otherwise-static main
config file. The fact that this isn't being done by a large number of
people (is anybody at all actually doing it?) suggests to me that maybe
the demand isn't all that great.

regards, tom lane

On Tuesday, October 30, 2012 11:24:20 PM Tom Lane wrote:
> The fact that this isn't being done by a large number of
> people (is anybody at all actually doing it?) suggests to me that maybe
> the demand isn't all that great.

It might also be that the idea of implementing that yourself is quite scary.

Also you would need to parse the file to reset values which isn't exactly
easy... I think it only really becomes viable with the introduction of
directory includes where you can use one file per value.

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Tom,

> I'm not convinced we ever *had* a consensus on this. There were
> proposals, but I'm not sure a majority ever bought into any one of 'em.
> The whole problem of intermixing manual editing and programmatic editing
> is just a big can of worms, and not everybody is prepared to give up the
> former to have the latter.

Well, I think we have consensus that intermixing is impractical, which
is why every further proposal is around having a separate file for the
SQL-modified values. And yes, we have a certain amount of "You'll get
my carefully edited postgresql.conf when you pry it out of my cold, dead
hands" going on.

The real consensus problem, AFAICT, is that while we have consensus that
we would like something like SET PERSISTENT as an *option*, there's a
Hurricane Sandy-sized Bikeshedding Windstorm about how, exactly, people
would like it to work. Personally, I would prefer the implementation
which actually gets committed. ;-)

> You can, if you are so inclined, implement something functionally
> equivalent to Amit's proposal today using contrib/adminpack's
> pg_file_write --- okay, it's much less convenient than a built-in
> implementation would be, but you can drop some variable assignments into
> a file and then put a suitable INCLUDE into the otherwise-static main
> config file. The fact that this isn't being done by a large number of
> people (is anybody at all actually doing it?) suggests to me that maybe
> the demand isn't all that great.

It suggest nothing of the sort:

1. a tiny minority of our users even know about adminpack

2. implementing it the way you suggest would require a hacker's
understanding of Postgres, which is an even smaller minority.

On the other hand, the success of tools like Puppet have made having SET
PERSISTENT a lot less urgent for many large-scale installation managers.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wednesday, October 31, 2012 4:14 AM Josh Berkus wrote:
> Tom,
>
> > I'm not convinced we ever *had* a consensus on this. There were
> > proposals, but I'm not sure a majority ever bought into any one of
> 'em.
> > The whole problem of intermixing manual editing and programmatic
> editing
> > is just a big can of worms, and not everybody is prepared to give up
> the
> > former to have the latter.
>
> Well, I think we have consensus that intermixing is impractical, which
> is why every further proposal is around having a separate file for the
> SQL-modified values. And yes, we have a certain amount of "You'll get
> my carefully edited postgresql.conf when you pry it out of my cold, dead
> hands" going on.

I think for that part it was discussed that always postgresql.conf values will override the values of .auto.


> The real consensus problem, AFAICT, is that while we have consensus that
> we would like something like SET PERSISTENT as an *option*, there's a
> Hurricane Sandy-sized Bikeshedding Windstorm about how, exactly, people
> would like it to work. Personally, I would prefer the implementation
> which actually gets committed. ;-)

I think the original syntax is proposed by Robert Hass by reffering Oracle's syntax in below mail:
http://archives.postgresql.org/pgsql-hackers/2010-10/msg00953.php

and then finally the Syntax which I have used in my proposal was suggested by Tom in below mail:
http://archives.postgresql.org/pgsql-hackers/2010-10/msg00977.php

Do you see any discrepancy in the proposal I have sent and what have been concluded in previous discussions?

With Regards,
Amit Kapila.

On Wednesday, October 31, 2012 3:58 AM Andres Freund wrote:
> On Tuesday, October 30, 2012 11:24:20 PM Tom Lane wrote:
> > The fact that this isn't being done by a large number of
> > people (is anybody at all actually doing it?) suggests to me that
> maybe
> > the demand isn't all that great.
>
> It might also be that the idea of implementing that yourself is quite
> scary.

> Also you would need to parse the file to reset values which isn't
> exactly
> easy...

As this new file (postgresql.conf.auto) will not be edited by users, so it
might not be
difficult to handle it, as the code will now the exact format of file.
The problem can be there if we need to parse postgresql.conf to set/reset
values, as for that
the format is not fixed. However that is taken care by having 2 files.
Please point me, if I misunderstood the difficulty raised by you.

With Regards,
Amit Kapila.

On Wednesday, October 31, 2012 3:32 AM Hannu Krosing wrote:
On 10/29/2012 03:14 PM, Amit Kapila wrote:

On Monday, October 29, 2012 7:11 PM Chris Corbyn

> What's the use case of this? It sounds like it will just create a
maintenance nightmare where some stuff you expect to lookup in in
postgresql.conf is actually hiding in the .auto file. Assuming only super
users/sysadmins would have the ability to change things in the config file,
wouldn't they be more likely to just do it on the server and edit the .conf
(which among other things, keeps it tidy and orderly).

Basically after this user will have 2 options to change the postgresql.conf
parameters.

One is by directly editing the postgresql.conf file and

Other is by using SQL commands.

There will be nothing hidden in .auto file, it's just that it will create
separate file for parameters set by SQL command to avoid the hassles of
parsing the postgresql.conf during the processing of SQL command.

>If interested I have somewhere pl/pythhonu functions for both looking at
and
changing parameters in postgresql.conf file,

In the previous discussion about this feature, it was mentioned by many
people as postgresql.conf can be editied by users in many ways, it will be
difficult to come up with a reliable function which can handle all possible
cases. That is why I have taken the approach of having 2 separate files, one
user editable and other can be only edited by SQL Commands.

In anycase if you have those functions readily available then please send
them, it can be useful for me.

With Regards,

Amit Kapila.

On Wednesday, October 31, 2012 3:25 AM Josh Berkus
> > I should think that doing this requires heading back towards there
> > being a single unique configuration stream, and over the course of
> > several versions, deprecating the INCLUDE directive.
>
> Oh, maybe I should take a closer look at Amit's proposal then. I
> thought we planned to make use of the INCLUDE facility for SET
> PERSISTENT, including supporting include-if-exists. Possibly what he's
> proposing and what I thought our last consensus were are highly
> divergent.

Currently INCLUDE is used for including postgresql.conf.auto in postgresql.conf by default.
Can you please let me know what is the expectation?

Instead of INCLUDE,
1. include-if-exists can be used.
2. In code first read .auto file then .conf and override the values read from .auto by values from .conf.

With Regards,
Amit Kapila.

On Tue, Oct 30, 2012 at 11:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>>> I should think that doing this requires heading back towards there
>>> being a single unique configuration stream, and over the course of
>>> several versions, deprecating the INCLUDE directive.
>
>> Oh, maybe I should take a closer look at Amit's proposal then. I
>> thought we planned to make use of the INCLUDE facility for SET
>> PERSISTENT, including supporting include-if-exists. Possibly what he's
>> proposing and what I thought our last consensus were are highly divergent.
>
> I'm not convinced we ever *had* a consensus on this. There were
> proposals, but I'm not sure a majority ever bought into any one of 'em.

I thought there was a consensus. But given that the one I thought we
had consensus on was different, I'm not sure we can correctly call it
consensus.

What we discussed at that time was to have a *function* that changes
the permanent configuration, and not actually extend the syntax of the
system. As a starting point.

The idea at the time was to use the include *directory* functionality,
for say a "config.d" directory in pgdata. The builtin one would then
use a predictable filename in this directory, so that the DBA who
prefers it can drop files both before and after that file into the
directory.

> The whole problem of intermixing manual editing and programmatic editing
> is just a big can of worms, and not everybody is prepared to give up the
> former to have the latter.
>
> You can, if you are so inclined, implement something functionally
> equivalent to Amit's proposal today using contrib/adminpack's
> pg_file_write --- okay, it's much less convenient than a built-in
> implementation would be, but you can drop some variable assignments into
> a file and then put a suitable INCLUDE into the otherwise-static main
> config file. The fact that this isn't being done by a large number of
> people (is anybody at all actually doing it?) suggests to me that maybe
> the demand isn't all that great.

The demand for running something like thta manually isn't all that
great, I believe. This is why I think using a function for it is
perfectly OK, and we don't necessarily need ALTER SYSTEM or something
like that. (In fact, a function might be preferred in many cases since
you can feed it the result of a query, unlike an ALTER statement). But
a standardized way for how it's dealt with so that multiple tools
don't step on each other is a very good idea - and probably one reason
people don't build this stuff themselves.

Being able to automate it across many machines is bigger, but most
people solve that today with things like puppet and chef.

Being able to build a nice configuration interface into something like
pgadmin is something that a lot of people ask for - but that's at best
a secondary effect from having a change like this, which is why we're
not seeing direct demand for it.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Amit,

I think you can simplify this task by forgetting about parsing the .auto
file entirely when writing it. That is, the .auto file should be
regenerated, and should write out whatever has been set in pg_settings,
regardless of what was in the file beforehand. I don't see the value in
parsing the file before writing it out.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wednesday, October 31, 2012 10:21 PM Josh Berkus wrote:
Amit,

> I think you can simplify this task by forgetting about parsing the .auto
> file entirely when writing it. That is, the .auto file should be
> regenerated, and should write out whatever has been set in pg_settings,
> regardless of what was in the file beforehand. I don't see the value in
> parsing the file before writing it out.

In that case how the new value of config parameter as set by user, will go in .auto file.
Shall we change in guc, from where pg_settings take the values?

Another point is curretly pg_settings doesn't have comments, so user will not be allowed to give comments with new value of config parameter.
Is that okay?

With Regards,
Amit Kapila.

On Wednesday, October 31, 2012 5:47 PM Magnus Hagander wrote:
On Tue, Oct 30, 2012 at 11:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>>>> I should think that doing this requires heading back towards there
>>>> being a single unique configuration stream, and over the course of
>>>> several versions, deprecating the INCLUDE directive.
>
>>> Oh, maybe I should take a closer look at Amit's proposal then. I
>>> thought we planned to make use of the INCLUDE facility for SET
>>> PERSISTENT, including supporting include-if-exists. Possibly what he's
>>> proposing and what I thought our last consensus were are highly divergent.
>
> >I'm not convinced we ever *had* a consensus on this. There were
> >proposals, but I'm not sure a majority ever bought into any one of 'em.

> I thought there was a consensus. But given that the one I thought we
> had consensus on was different, I'm not sure we can correctly call it
> consensus.

> What we discussed at that time was to have a *function* that changes
> the permanent configuration, and not actually extend the syntax of the
> system. As a starting point.

Do you mean a function like pg_set_config(config_param,value)/pg_change_config(config_param,value)/pg_configure(config_param,value) to change the configuration values in file?

So till now below options are discussed which can be used to provide this functionality:

1. Set PERSISTENT --This has advantage that user can have one syntax (SET) to change values at different levels. But not sure if it is good incase COMMENTS also needs to be included.
2. ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'} COMMENT 'value';
This syntax is very much similar to what Oracle provides.
3. pg_set_config(config_param,value)/pg_change_config(config_param,value)
This is somewhat similar to SQL Server. Use sp_configure to display or change server-level settings. To change database-level settings, use ALTER DATABASE.
To change settings that affect only the current user session, use the SET statement.
http://msdn.microsoft.com/en-us/library/ms188787(v=sql.90).aspx
4. Any other better ideas for Syntax?

Please provide your suggestions which one is better?

> The idea at the time was to use the include *directory* functionality,
> for say a "config.d" directory in pgdata. The builtin one would then
> use a predictable filename in this directory, so that the DBA who
> prefers it can drop files both before and after that file into the
> directory.

Can you please explain in more detail how using this idea the whole implementation can be realized.
Do you see problems or improvements required in the design/implementation described in proposal mail?

>> The whole problem of intermixing manual editing and programmatic editing
>> is just a big can of worms, and not everybody is prepared to give up the
> former to have the latter.
>
>> You can, if you are so inclined, implement something functionally
>> equivalent to Amit's proposal today using contrib/adminpack's
>> pg_file_write --- okay, it's much less convenient than a built-in
>> implementation would be, but you can drop some variable assignments into
>> a file and then put a suitable INCLUDE into the otherwise-static main
>> config file. The fact that this isn't being done by a large number of
>> people (is anybody at all actually doing it?) suggests to me that maybe
>> the demand isn't all that great.

> The demand for running something like thta manually isn't all that
> great, I believe. This is why I think using a function for it is
> perfectly OK, and we don't necessarily need ALTER SYSTEM or something
> like that. (In fact, a function might be preferred in many cases since
> you can feed it the result of a query, unlike an ALTER statement). But
> a standardized way for how it's dealt with so that multiple tools
> don't step on each other is a very good idea - and probably one reason
> people don't build this stuff themselves.

> Being able to automate it across many machines is bigger, but most
> people solve that today with things like puppet and chef.

> Being able to build a nice configuration interface into something like
> pgadmin is something that a lot of people ask for - but that's at best
> a secondary effect from having a change like this, which is why we're
> not seeing direct demand for it.

I agree that may be demand is not high, but it is a useful feature considering that commercial databases (Oracle,SQL Server, etc.) and git provides this feature.

With Regards,
Amit Kapila.
As this feature

On 10/31/12 12:17 PM, Magnus Hagander wrote:

> The idea at the time was to use the include *directory* functionality,
> for say a "config.d" directory in pgdata. The builtin one would then
> use a predictable filename in this directory, so that the DBA who
> prefers it can drop files both before and after that file into the
> directory.

That's how I remember things as well. Unfortunately Amit's proposal
seems like an even more complicated version of ideas that were clearly
beaten down multiple times over many years now, partly for being too
complicated.

The only idea I remember ever crossing the gap between the "edit by
hand" and "tool config" crowd was based on the include directory
concept. The bugs in that implementation are finally worked out and the
include_dir feature committed recently, so now it's possible to consider
using it as a building block now.

Here is a much simpler proposal for example:

-Add a configuration subdirectory to the default installation. Needs to
follow the config file location, so things like the Debian relocation of
postgresql.conf still work. Maybe it has zero files; maybe it has one
that's named for this purpose, which defaults to the usual:

# Don't edit this file by hand! It's overwritten by...

-Have the standard postgresql.conf end by including that directory
-SQL parameter changes collect up all other active parameter changes,
rewrite that file, and signal the server. If any change requested
requires a full server restart. warn the user of that fact.

And that's basically it. Cranky old-timers can remove the include
directive and/or directory if they don't like it, act as if nothing has
changed, and move along. Everyone else gets the beginning of a multiple
co-existing tool change standard.

The only obvious bad case I can think of here is if someone has left the
directory there, but deleted the include_dir statement; then the file
would be written successfully but never included. Seems like in the
worst case the postgresql.conf parser would just need to flag whether it
found the default directory included or not, to try and flag avoid
obvious foot shooting.

Some of the better received ideas I floated for merging the
recovery.conf file seemed headed this way too. That also all blocked
behind the include directory bit being surprisingly tricky to get
committed. So that's possible to revive usefully now. And as much as I
hate to expand scope by saying it, both changes should probably be
tackled at once. It's important to make sure they're both solved well
by whatever is adopted, they are going to co-exist as committed one day.

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

On Fri, Nov 2, 2012 at 2:19 AM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:

> On 10/31/12 12:17 PM, Magnus Hagander wrote:
>
> The idea at the time was to use the include *directory* functionality,
>> for say a "config.d" directory in pgdata. The builtin one would then
>> use a predictable filename in this directory, so that the DBA who
>> prefers it can drop files both before and after that file into the
>> directory.
>>
>
> That's how I remember things as well. Unfortunately Amit's proposal seems
> like an even more complicated version of ideas that were clearly beaten
> down multiple times over many years now, partly for being too complicated.
>
> The only idea I remember ever crossing the gap between the "edit by hand"
> and "tool config" crowd was based on the include directory concept. The
> bugs in that implementation are finally worked out and the include_dir
> feature committed recently, so now it's possible to consider using it as a
> building block now.
>
> Here is a much simpler proposal for example:
>
> -Add a configuration subdirectory to the default installation. Needs to
> follow the config file location, so things like the Debian relocation of
> postgresql.conf still work. Maybe it has zero files; maybe it has one
> that's named for this purpose, which defaults to the usual:
>

What do you mean by "needs to follow"? In particular, do you mean that it
should be relative to postgresql.conf? I think that would actually be a
*problem* for any system that moves the config file away, like debian,
since you'd then have to grant postgres write permissions on a directory in
/etc/...

> # Don't edit this file by hand! It's overwritten by...
>
> -Have the standard postgresql.conf end by including that directory
> -SQL parameter changes collect up all other active parameter changes,
> rewrite that file, and signal the server. If any change requested requires
> a full server restart. warn the user of that fact.
>
> And that's basically it. Cranky old-timers can remove the include
> directive and/or directory if they don't like it, act as if nothing has
> changed, and move along. Everyone else gets the beginning of a multiple
> co-existing tool change standard.
>
> The only obvious bad case I can think of here is if someone has left the
> directory there, but deleted the include_dir statement; then the file would
> be written successfully but never included. Seems like in the worst case
> the postgresql.conf parser would just need to flag whether it found the
> default directory included or not, to try and flag avoid obvious foot
> shooting.
>

Yes. And we could pretty easily find that - have the function that reloads
the config file actually check the source file and line number to make sure
it matches the one fro mthe auto file, and give a WARNING if it doesn't
(which indicates that either the file isn't included, or something else
"later in the chain" overwrote it)

Some of the better received ideas I floated for merging the recovery.conf
> file seemed headed this way too. That also all blocked behind the include
> directory bit being surprisingly tricky to get committed. So that's
> possible to revive usefully now. And as much as I hate to expand scope by
> saying it, both changes should probably be tackled at once. It's important
> to make sure they're both solved well by whatever is adopted, they are
> going to co-exist as committed one day.
>

Yeah, we don't need the code for both, but we certainly need a "reasonable
design" capable of dealing with both, so we don't paint ourselves into a
corner.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Fri, Nov 2, 2012 at 1:19 AM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
>> The idea at the time was to use the include *directory* functionality,
>> for say a "config.d" directory in pgdata. The builtin one would then
>> use a predictable filename in this directory, so that the DBA who
>> prefers it can drop files both before and after that file into the
>> directory.
>
>
> That's how I remember things as well.

This sounds similar but a bit different from the solution I advocated
for and thought there was widespread support for.

If we changed the default postgresql.conf to be empty except for an
"include postgresql.conf.auto" and had tools to write out
postgresql.conf.auto then things would basically just work.

The main gotcha would have been if people *do* put any settings in
postgresql.conf manually then they would override any auto settings
(if they came after the include) or be overridden by them (if they
come before the include). This might be a bit confusing but I think it
would be fine -- the tools might want to display a warning if the
current source is from a setting in a different file.

--
greg

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> I think it only really becomes viable with the introduction of
> directory includes where you can use one file per value.

+1

--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

> -Add a configuration subdirectory to the default installation. Needs to
> follow the config file location, so things like the Debian relocation of
> postgresql.conf still work. Maybe it has zero files; maybe it has one
> that's named for this purpose, which defaults to the usual:
>
> # Don't edit this file by hand! It's overwritten by...
>
> -Have the standard postgresql.conf end by including that directory
> -SQL parameter changes collect up all other active parameter changes,
> rewrite that file, and signal the server. If any change requested
> requires a full server restart. warn the user of that fact.

+1

Simple, easy to understand, easy to customize.

> The only obvious bad case I can think of here is if someone has left the
> directory there, but deleted the include_dir statement; then the file
> would be written successfully but never included. Seems like in the
> worst case the postgresql.conf parser would just need to flag whether it
> found the default directory included or not, to try and flag avoid
> obvious foot shooting.

Yes, and we can have the comment:

# this includes the default directory for extra configuration files
# do not delete or comment this out; remove any extra configuration
# files you don't want instead

... or similar to warn users. Frankly, if someone removes the
"includedir config/" line, we can presume they know what they are doing.

For that matter, some users might want to move the line to the beginning
of the file, instead of the end.

> Some of the better received ideas I floated for merging the
> recovery.conf file seemed headed this way too. That also all blocked
> behind the include directory bit being surprisingly tricky to get
> committed. So that's possible to revive usefully now. And as much as I
> hate to expand scope by saying it, both changes should probably be
> tackled at once. It's important to make sure they're both solved well
> by whatever is adopted, they are going to co-exist as committed one day.

Yes.

I'll also point out that includedir would help solve the issue of
"postgresql.conf is under Puppet, but I want to change the logging
options ..." more handily than current solutions.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Based on feedback in this mail chain, please find the modified method to have this feature:

Syntax for Command:

1. Have SQL command to change the configuration parameter values:
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'} COMMENT 'value';
2. Have built-in to change the configuration parameter values:
pg_set_config(config_param,value)/pg_change_config(config_param,value)

If there is no objection, I would like to go-ahead with the 2nd approach (built-in) as per suggestion by Magnus.

Implementation approach

1. Add a configuration subdirectory (config.d) to the default installation. This will contain default .auto file.
Default .auto file will be empty. However we can modify it to contain all uncommented parameters of postgresql.conf if required.

2. Have the standard postgresql.conf end by including that directory.
Here user can have at end, begin or in-between, I think it will get handled.
By default we can keep at end which means the parameters in .auto file will be given more priority.

3. SQL parameter changes collect up all other active parameter changes, rewrite that file, and signal the server.
If any change requested requires a full server restart. warn the user of that fact.

I am not able to fully understand the writing of .auto file as suggested in this mail chain. What I understood from the above is that,
when user executes this function, it should collect all changed parameters and rewrite the .auto file. But according to
my understanding it can write incorrect values in .auto file as backend from which this command is getting executed
might have some old values.
The key point is how backend can get the latest config values without reading .auto file or by communicating with other backends?

4. Warning on sighup, to indicate that either the file isn't included, or something else "later in the chain" overwrote it.

5. Unite recovery.conf with postgresql.conf
I think if we use include dir concept for current feature implementation, it can address the basic design level concern for both the features.

Suggestions/Comments?

With Regards,
Amit Kapila.

On Saturday, November 03, 2012 7:09 AM Josh Berkus wrote:
>
> > -Add a configuration subdirectory to the default installation. Needs
> to
> > follow the config file location, so things like the Debian relocation
> of
> > postgresql.conf still work. Maybe it has zero files; maybe it has one
> > that's named for this purpose, which defaults to the usual:
> >
> > # Don't edit this file by hand! It's overwritten by...
> >
> > -Have the standard postgresql.conf end by including that directory
> > -SQL parameter changes collect up all other active parameter changes,
> > rewrite that file, and signal the server. If any change requested
> > requires a full server restart. warn the user of that fact.
>
> +1
>
> Simple, easy to understand, easy to customize.
>
> > The only obvious bad case I can think of here is if someone has left
> the
> > directory there, but deleted the include_dir statement; then the file
> > would be written successfully but never included. Seems like in the
> > worst case the postgresql.conf parser would just need to flag whether
> it
> > found the default directory included or not, to try and flag avoid
> > obvious foot shooting.
>
> Yes, and we can have the comment:
>
> # this includes the default directory for extra configuration files
> # do not delete or comment this out; remove any extra configuration
> # files you don't want instead
>
> ... or similar to warn users. Frankly, if someone removes the
> "includedir config/" line, we can presume they know what they are doing.
>
> For that matter, some users might want to move the line to the beginning
> of the file, instead of the end.
>
> > Some of the better received ideas I floated for merging the
> > recovery.conf file seemed headed this way too. That also all blocked
> > behind the include directory bit being surprisingly tricky to get
> > committed. So that's possible to revive usefully now. And as much as
> I
> > hate to expand scope by saying it, both changes should probably be
> > tackled at once. It's important to make sure they're both solved well
> > by whatever is adopted, they are going to co-exist as committed one
> day.
>
> Yes.
>
> I'll also point out that includedir would help solve the issue of
> "postgresql.conf is under Puppet, but I want to change the logging
> options ..." more handily than current solutions.
>
> --
> Josh Berkus
> PostgreSQL Experts Inc.
> http://pgexperts.com
>

On Wed, Oct 31, 2012 at 8:17 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> I'm not convinced we ever *had* a consensus on this. There were
>> proposals, but I'm not sure a majority ever bought into any one of 'em.
>
> I thought there was a consensus. But given that the one I thought we
> had consensus on was different, I'm not sure we can correctly call it
> consensus.
>
> What we discussed at that time was to have a *function* that changes
> the permanent configuration, and not actually extend the syntax of the
> system. As a starting point.
>
> The idea at the time was to use the include *directory* functionality,
> for say a "config.d" directory in pgdata. The builtin one would then
> use a predictable filename in this directory, so that the DBA who
> prefers it can drop files both before and after that file into the
> directory.

Reading over this thread, it seems that there are at least three
different proposals for how this should work in detail:

1. Have a configuration file that can be rewritten using SQL, and have
postgresql.conf include it by default.
2. Have a configuration directory that gets included in
postgresql.conf by default, and one file in that directory will
contain all the parameters set via SQL.
3. Have a configuration directory that gets included in
postgresql.conf by default, with one file per parameter, and rewrite
just that file when the corresponding parameter is set via SQL.

Also, there are at least three different proposals for what the syntax
should look like:

1. ALTER SYSTEM
2. SET PERSISENT
3. pg_frob_my_configuration()

For all of that, I think there is broad agreement that being able to
set configuration parameters on a server-wide basis via SQL commands
is a useful thing to do. I certainly think it is.

It seems to me that the only reason why we have any of this
information in a text file at all is because there are some parameters
that the server has to know before it can start. After all, ALTER
USER and ALTER DATABASE and ALTER FUNCTION store their values in the
database itself, and no one has said, oh, those values should be
stored in a file so people can edit them with a text editor. Why not?
Surely that's just as defensible as wanting to edit the server-wide
parameters, but you can't.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tuesday, November 06, 2012 11:30 PM Robert Haas wrote:
> On Wed, Oct 31, 2012 at 8:17 AM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> >> I'm not convinced we ever *had* a consensus on this. There were
> >> proposals, but I'm not sure a majority ever bought into any one of
> 'em.
> >
> > I thought there was a consensus. But given that the one I thought we
> > had consensus on was different, I'm not sure we can correctly call it
> > consensus.
> >
> > What we discussed at that time was to have a *function* that changes
> > the permanent configuration, and not actually extend the syntax of the
> > system. As a starting point.
> >
> > The idea at the time was to use the include *directory* functionality,
> > for say a "config.d" directory in pgdata. The builtin one would then
> > use a predictable filename in this directory, so that the DBA who
> > prefers it can drop files both before and after that file into the
> > directory.
>
> Reading over this thread, it seems that there are at least three
> different proposals for how this should work in detail:
>
> 1. Have a configuration file that can be rewritten using SQL, and have
> postgresql.conf include it by default.
> 2. Have a configuration directory that gets included in
> postgresql.conf by default, and one file in that directory will
> contain all the parameters set via SQL.
> 3. Have a configuration directory that gets included in
> postgresql.conf by default, with one file per parameter, and rewrite
> just that file when the corresponding parameter is set via SQL.
>
> Also, there are at least three different proposals for what the syntax
> should look like:
>
> 1. ALTER SYSTEM
> 2. SET PERSISENT
> 3. pg_frob_my_configuration()

This is very good summarization of all discussion in this mail chain.
However there is one more point which I am not able to clearly make out is
how to write into file that contains
all configuration parameters changed by SQL.
What I could understand from Greg and Josh's mail is that they are
suggesting to write a file by collecting active changed parameters from
memory or use pg_settings.
But as mentioned in other mail as per my understanding that this can lead to
have incorrect values in .auto file.
I think I am missing or not able to understand how can it be done without
reading .auto file or by communicating with other backends?
Can you please point me what is wrong in my understanding?

> For all of that, I think there is broad agreement that being able to
> set configuration parameters on a server-wide basis via SQL commands
> is a useful thing to do. I certainly think it is.

> It seems to me that the only reason why we have any of this
> information in a text file at all is because there are some parameters
> that the server has to know before it can start. After all, ALTER
> USER and ALTER DATABASE and ALTER FUNCTION store their values in the
> database itself, and no one has said, oh, those values should be
> stored in a file so people can edit them with a text editor. Why not?
> Surely that's just as defensible as wanting to edit the server-wide
> parameters, but you can't.

On Wed, Nov 7, 2012 at 5:19 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Tuesday, November 06, 2012 11:30 PM Robert Haas wrote:
>> On Wed, Oct 31, 2012 at 8:17 AM, Magnus Hagander <magnus(at)hagander(dot)net>
>> wrote:
>> >> I'm not convinced we ever *had* a consensus on this. There were
>> >> proposals, but I'm not sure a majority ever bought into any one of
>> 'em.
>> >
>> > I thought there was a consensus. But given that the one I thought we
>> > had consensus on was different, I'm not sure we can correctly call it
>> > consensus.
>> >
>> > What we discussed at that time was to have a *function* that changes
>> > the permanent configuration, and not actually extend the syntax of the
>> > system. As a starting point.
>> >
>> > The idea at the time was to use the include *directory* functionality,
>> > for say a "config.d" directory in pgdata. The builtin one would then
>> > use a predictable filename in this directory, so that the DBA who
>> > prefers it can drop files both before and after that file into the
>> > directory.
>>
>> Reading over this thread, it seems that there are at least three
>> different proposals for how this should work in detail:
>>
>> 1. Have a configuration file that can be rewritten using SQL, and have
>> postgresql.conf include it by default.
>> 2. Have a configuration directory that gets included in
>> postgresql.conf by default, and one file in that directory will
>> contain all the parameters set via SQL.
>> 3. Have a configuration directory that gets included in
>> postgresql.conf by default, with one file per parameter, and rewrite
>> just that file when the corresponding parameter is set via SQL.
>>
>> Also, there are at least three different proposals for what the syntax
>> should look like:
>>
>> 1. ALTER SYSTEM
>> 2. SET PERSISENT
>> 3. pg_frob_my_configuration()
>
> This is very good summarization of all discussion in this mail chain.
> However there is one more point which I am not able to clearly make out is
> how to write into file that contains
> all configuration parameters changed by SQL.
> What I could understand from Greg and Josh's mail is that they are
> suggesting to write a file by collecting active changed parameters from
> memory or use pg_settings.
> But as mentioned in other mail as per my understanding that this can lead to
> have incorrect values in .auto file.
> I think I am missing or not able to understand how can it be done without
> reading .auto file or by communicating with other backends?

Perhaps you can look at pg_settings, to see if the current setting is
from the .auto file. If it is, then that's where it came from and it
should be written back there. If it's something else, that's not where
it came from.

That will remove it from the .auto file if someone manually adds an
override later, but I'm not sure we need to support people who do the
same config in two different ways - as long as we document how this
happens.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Wed, Nov 7, 2012 at 5:19 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>> However there is one more point which I am not able to clearly make out is
>> how to write into file that contains
>> all configuration parameters changed by SQL.

> Perhaps you can look at pg_settings, to see if the current setting is
> from the .auto file. If it is, then that's where it came from and it
> should be written back there. If it's something else, that's not where
> it came from.

Note that the whole point of the one-value-per-file approach is to not
have to figure this out.

I'm not sure that the above approach works anyway --- for instance, the
"current setting" might be a SET LOCAL result, in which case you still
don't know anything about what the appropriate thing to put into the
file is. I think there are probably also race conditions with cases
where somebody else just changed some other setting but your session
hasn't absorbed it yet.

regards, tom lane

On Wed, Nov 7, 2012 at 5:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Wed, Nov 7, 2012 at 5:19 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>>> However there is one more point which I am not able to clearly make out is
>>> how to write into file that contains
>>> all configuration parameters changed by SQL.
>
>> Perhaps you can look at pg_settings, to see if the current setting is
>> from the .auto file. If it is, then that's where it came from and it
>> should be written back there. If it's something else, that's not where
>> it came from.
>
> Note that the whole point of the one-value-per-file approach is to not
> have to figure this out.

Yeah - but I don't think that's the approach that Amit was talking
about? I thought that was a single file...

> I'm not sure that the above approach works anyway --- for instance, the
> "current setting" might be a SET LOCAL result, in which case you still
> don't know anything about what the appropriate thing to put into the
> file is. I think there are probably also race conditions with cases
> where somebody else just changed some other setting but your session
> hasn't absorbed it yet.

Well, you don't have to look at pg_settings specifically - since this
is inside the backend. You can look at the underlying structures. We
stack them up so we can RESET them, right? So we could just peek up in
that stack and find the data there.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Wed, Nov 7, 2012 at 5:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm not sure that the above approach works anyway --- for instance, the
>> "current setting" might be a SET LOCAL result, in which case you still
>> don't know anything about what the appropriate thing to put into the
>> file is. I think there are probably also race conditions with cases
>> where somebody else just changed some other setting but your session
>> hasn't absorbed it yet.

> Well, you don't have to look at pg_settings specifically - since this
> is inside the backend. You can look at the underlying structures. We
> stack them up so we can RESET them, right? So we could just peek up in
> that stack and find the data there.

You could dig it out of the stack if it's there, but that doesn't fix
the race-condition aspect. Now a race is inevitable if two sessions try
to set the *same* variable, but I think people will be unhappy if a SET
on one variable makes a recent SET on some other variable disappear.

The one-value-per-file solution neatly bypasses all these problems,
which is why this topic got put on the back burner originally until
we had the include-directory functionality. I don't see why we are
revisiting the bugs in an approach that was already rejected.

regards, tom lane

On Wed, Nov 7, 2012 at 6:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Wed, Nov 7, 2012 at 5:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I'm not sure that the above approach works anyway --- for instance, the
>>> "current setting" might be a SET LOCAL result, in which case you still
>>> don't know anything about what the appropriate thing to put into the
>>> file is. I think there are probably also race conditions with cases
>>> where somebody else just changed some other setting but your session
>>> hasn't absorbed it yet.
>
>> Well, you don't have to look at pg_settings specifically - since this
>> is inside the backend. You can look at the underlying structures. We
>> stack them up so we can RESET them, right? So we could just peek up in
>> that stack and find the data there.
>
> You could dig it out of the stack if it's there, but that doesn't fix
> the race-condition aspect. Now a race is inevitable if two sessions try
> to set the *same* variable, but I think people will be unhappy if a SET
> on one variable makes a recent SET on some other variable disappear.

I think if we require an exclusive lock on a single global lock for
"set permanent", people are quite ok with that, really. Changing
permanent settings concurrently doesn't seem like a veyr likely
scenario.

> The one-value-per-file solution neatly bypasses all these problems,
> which is why this topic got put on the back burner originally until
> we had the include-directory functionality. I don't see why we are
> revisiting the bugs in an approach that was already rejected.

Yeah, agreed - that certainly takes most of it away. And there is
nothing preventing somebody from having both that and another
directory-include somewhere if they'd like to...

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Wed, Nov 7, 2012 at 6:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> You could dig it out of the stack if it's there, but that doesn't fix
>> the race-condition aspect. Now a race is inevitable if two sessions try
>> to set the *same* variable, but I think people will be unhappy if a SET
>> on one variable makes a recent SET on some other variable disappear.

> I think if we require an exclusive lock on a single global lock for
> "set permanent", people are quite ok with that, really.

That doesn't fix it either, at least not without a whole lot of other
changes --- we don't normally read the config file within-commands,
and there are both semantic and implementation problems to overcome
if you want to do so.

regards, tom lane

On Wed, Nov 7, 2012 at 12:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Wed, Nov 7, 2012 at 6:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> You could dig it out of the stack if it's there, but that doesn't fix
>>> the race-condition aspect. Now a race is inevitable if two sessions try
>>> to set the *same* variable, but I think people will be unhappy if a SET
>>> on one variable makes a recent SET on some other variable disappear.
>
>> I think if we require an exclusive lock on a single global lock for
>> "set permanent", people are quite ok with that, really.
>
> That doesn't fix it either, at least not without a whole lot of other
> changes --- we don't normally read the config file within-commands,
> and there are both semantic and implementation problems to overcome
> if you want to do so.

Why would you need to? It seems to me that we ought to be able to
rewrite a machine-generated configuration file without loading those
values into the current session. If we can't, that seems like prima
facie evidence that the format is not sufficiently easy to
machine-parse.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Nov 7, 2012 at 12:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> ... we don't normally read the config file within-commands,
>> and there are both semantic and implementation problems to overcome
>> if you want to do so.

> Why would you need to? It seems to me that we ought to be able to
> rewrite a machine-generated configuration file without loading those
> values into the current session.

Well, Magnus' proposed implementation supposed that the existing values
*have* been loaded into the current session. I agree that with some
locking and yet more code you could implement it without that. But this
still doesn't seem to offer any detectable benefit over value-per-file.

regards, tom lane

> Well, Magnus' proposed implementation supposed that the existing values
> *have* been loaded into the current session. I agree that with some
> locking and yet more code you could implement it without that. But this
> still doesn't seem to offer any detectable benefit over value-per-file.

Well, value-per-file is ugly (imagine you've set 40 different variables
that way) but dodges a lot of complicated issues. And I suppose "ugly"
doesn't matter, because the whole idea of the auto-generated files is
that users aren't supposed to look at them anyway.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wed, Nov 7, 2012 at 2:50 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> Well, Magnus' proposed implementation supposed that the existing values
>> *have* been loaded into the current session. I agree that with some
>> locking and yet more code you could implement it without that. But this
>> still doesn't seem to offer any detectable benefit over value-per-file.
>
> Well, value-per-file is ugly (imagine you've set 40 different variables
> that way) but dodges a lot of complicated issues. And I suppose "ugly"
> doesn't matter, because the whole idea of the auto-generated files is
> that users aren't supposed to look at them anyway.

That's pretty much how I feel about it, too. I think value-per-file
is an ugly wimp-out that shouldn't really be necessary to solve this
problem. It can't be that hard to rewrite a file where every like is
of the form:

key = 'value'

However, as Josh said upthread, +1 for the implementation that will
get committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 07, 2012 at 03:15:07PM -0500, Robert Haas wrote:
> On Wed, Nov 7, 2012 at 2:50 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> >> Well, Magnus' proposed implementation supposed that the existing values
> >> *have* been loaded into the current session. I agree that with some
> >> locking and yet more code you could implement it without that. But this
> >> still doesn't seem to offer any detectable benefit over value-per-file.
> >
> > Well, value-per-file is ugly (imagine you've set 40 different variables
> > that way) but dodges a lot of complicated issues. And I suppose "ugly"
> > doesn't matter, because the whole idea of the auto-generated files is
> > that users aren't supposed to look at them anyway.
>
> That's pretty much how I feel about it, too. I think value-per-file
> is an ugly wimp-out that shouldn't really be necessary to solve this
> problem. It can't be that hard to rewrite a file where every like is
> of the form:
>
> key = 'value'
>
> However, as Josh said upthread, +1 for the implementation that will
> get committed.

Why do you think its that ugly? It seems to me the one-value-per-file
solution has the advantage of being relatively easy to integrate into
other systems that manage postgres' configuration.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/2/12 11:17 AM, Magnus Hagander wrote:
> -Add a configuration subdirectory to the default installation.
> Needs to follow the config file location, so things like the
> Debian relocation of postgresql.conf still work. Maybe it has zero
> files; maybe it has one that's named for this purpose, which
> defaults to the usual:
>
> What do you mean by "needs to follow"? In particular, do you mean that
> it should be relative to postgresql.conf? I think that would actually be
> a *problem* for any system that moves the config file away, like debian,
> since you'd then have to grant postgres write permissions on a directory
> in /etc/...

I should have just said that the rules for the directly location are the
ones implied by the include-dir feature.

My understanding is that Debian Postgres installs already had writable
config files in etc, so that you can modify the postgresql.conf,
pg_hba.conf, etc. Here's a Squeeze server running the stock 8.4 plus
9.1 from backports, and /etc/postgresql/<version>/<cluster> is writable
by the postgres user:

$ ls -ld /etc/postgresql/9.1/main/
drwxr-xr-x postgres postgres /etc/postgresql/9.1/main/

$ ls -ld /etc/postgresql/8.4/main/
drwxr-xr-x postgres postgres /etc/postgresql/8.4/main/

$ ls -ld /etc/postgresql/9.1/main/postgresql.conf
-rw-r--r-- postgres postgres /etc/postgresql/9.1/main/postgresql.conf

$ ls -ld /etc/postgresql/8.4/main/postgresql.conf
-rw-r--r-- postgres postgres /etc/postgresql/8.4/main/postgresql.conf

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

On Thursday, November 08, 2012 1:45 AM Robert Haas wrote:
> On Wed, Nov 7, 2012 at 2:50 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> >> Well, Magnus' proposed implementation supposed that the existing
> values
> >> *have* been loaded into the current session. I agree that with some
> >> locking and yet more code you could implement it without that. But
> this
> >> still doesn't seem to offer any detectable benefit over value-per-
> file.
> >
> > Well, value-per-file is ugly (imagine you've set 40 different
> variables
> > that way) but dodges a lot of complicated issues. And I suppose
> "ugly"
> > doesn't matter, because the whole idea of the auto-generated files is
> > that users aren't supposed to look at them anyway.
>
> That's pretty much how I feel about it, too. I think value-per-file
> is an ugly wimp-out that shouldn't really be necessary to solve this
> problem. It can't be that hard to rewrite a file where every like is
> of the form:
>
> key = 'value'

I also believe that it should be possible to rewrite a file without loading
values into the current session.
One of the solution if we assume that file is of fixed format and each
record (key = 'value') of fixed length can be:

1. While writing .auto file, it will always assume that .auto file contain
all config parameters.
Now as this .auto file is of fixed format and fixed record size, it can
directly write a given record to its particular position.
2. To handle locking issues, we can follow an approach similar to what "GIT"
is doing for editing conf files (using .lock file):
a. copy the latest content of .auto to .auto.lock
b. make all the changes to auto.lock file.
c. at the end of command rename the auto.lock file to .auto file
d. otherwise if SQL COMMAND/function failed in-between we can delete the
".auto.lock" file
3. Two backends trying to write to .auto file
we can use ".auto.lock" as the the lock by trying to create it in
exclusive mode as the first step
of the command. If it already exists then backend needs to wait.

> However, as Josh said upthread, +1 for the implementation that will
> get committed.

With Regards,
Amit Kapila.

Amit Kapila escribió:

> 3. Two backends trying to write to .auto file
> we can use ".auto.lock" as the the lock by trying to create it in
> exclusive mode as the first step
> of the command. If it already exists then backend needs to wait.

So changing .auto settings would be nontransactional? The other way to
define this would be to have a lock that you grab and keep until end of
transaction, and the .auto.lock file is deleted if the transaction is
aborted; so have the .auto.lock -> .auto rename only happen at
transaction commit.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thursday, November 08, 2012 12:28 AM Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, Nov 7, 2012 at 12:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> ... we don't normally read the config file within-commands,
> >> and there are both semantic and implementation problems to overcome
> >> if you want to do so.
>
> > Why would you need to? It seems to me that we ought to be able to
> > rewrite a machine-generated configuration file without loading those
> > values into the current session.
>
> Well, Magnus' proposed implementation supposed that the existing values
> *have* been loaded into the current session. I agree that with some
> locking and yet more code you could implement it without that. But this
> still doesn't seem to offer any detectable benefit over value-per-file.

In value-per-file Approach if 2 sessions trying to update same variable
(trying to write in same file),
then won't there be chances that it can corrupt the file if there is no
locking?

Won't this have any impact on base backup/restore, restart and SIGHUP in
terms of that it needs to open,read,close so many files
instead of one file.

"Oracle" and "Git" which provides mechanism to edit of conf file using a
command doesn't use multiple file concept, which indicates that might be
single file concept is better.
Even if we say that user doesn't need to edit or change anything in config
directory, but still some advanced database users/DBA's generally try to
understand the meaning of each folder/file in database to manage it in a
better way. So when we explain them the contents of this folder and
explanation of same, they might not feel good based on their experience with
Oracle or some other similar database.

As per discussion and different opinions "value-per-file" Approach has
merits over "single-file" in terms of design and implementation and
single-file has merits over "value-per-file" in-terms of ugliness (usability
or maintainence or ...)

IMHO, to conclude it, we can see if it is possible to have some not so
complex solution(design) to handle "single-file" Approach then we can use
it, otherwise we can go for "value-per-file" Approach.

With Regards,
Amit Kapila.

On Thursday, November 08, 2012 5:24 AM Greg Smith wrote:
> On 11/2/12 11:17 AM, Magnus Hagander wrote:
> > -Add a configuration subdirectory to the default installation.
> > Needs to follow the config file location, so things like the
> > Debian relocation of postgresql.conf still work. Maybe it has
> zero
> > files; maybe it has one that's named for this purpose, which
> > defaults to the usual:
> >
> > What do you mean by "needs to follow"? In particular, do you mean that
> > it should be relative to postgresql.conf? I think that would actually
> be
> > a *problem* for any system that moves the config file away, like
> debian,
> > since you'd then have to grant postgres write permissions on a
> directory
> > in /etc/...
>
> I should have just said that the rules for the directly location are the
> ones implied by the include-dir feature.
>
> My understanding is that Debian Postgres installs already had writable
> config files in etc, so that you can modify the postgresql.conf,
> pg_hba.conf, etc. Here's a Squeeze server running the stock 8.4 plus
> 9.1 from backports, and /etc/postgresql/<version>/<cluster> is writable
> by the postgres user:
>
> $ ls -ld /etc/postgresql/9.1/main/
> drwxr-xr-x postgres postgres /etc/postgresql/9.1/main/
>
> $ ls -ld /etc/postgresql/8.4/main/
> drwxr-xr-x postgres postgres /etc/postgresql/8.4/main/
>
> $ ls -ld /etc/postgresql/9.1/main/postgresql.conf
> -rw-r--r-- postgres postgres /etc/postgresql/9.1/main/postgresql.conf
>
> $ ls -ld /etc/postgresql/8.4/main/postgresql.conf
> -rw-r--r-- postgres postgres /etc/postgresql/8.4/main/postgresql.conf

So is it okay if we have absolute path of config directory in
postgresql.conf?

With Regards,
Amit Kapila.

On Thursday, November 08, 2012 8:07 PM Alvaro Herrera wrote:
> Amit Kapila escribió:
>
> > 3. Two backends trying to write to .auto file
> > we can use ".auto.lock" as the the lock by trying to create it
> in
> > exclusive mode as the first step
> > of the command. If it already exists then backend needs to
> wait.
>
> So changing .auto settings would be nontransactional?

No, it should behave the way you explained below.
The points mentioned in above mail are just to explain the basic concept.

>The other way to
> define this would be to have a lock that you grab and keep until end of
> transaction, and the .auto.lock file is deleted if the transaction is
> aborted; so have the .auto.lock -> .auto rename only happen at
> transaction commit.

Is this behavior sane for Transaction block, as in transaction block some
other backend might need to wait
for little longer, if both issued a command to change config parameter?

IMO it is okay, as the usage of command to change config parameters inside a
transaction block would be less.

With Regards,
Amit Kapila.

Amit Kapila escribió:
> On Thursday, November 08, 2012 8:07 PM Alvaro Herrera wrote:

> >The other way to
> > define this would be to have a lock that you grab and keep until end of
> > transaction, and the .auto.lock file is deleted if the transaction is
> > aborted; so have the .auto.lock -> .auto rename only happen at
> > transaction commit.
>
> Is this behavior sane for Transaction block, as in transaction block some
> other backend might need to wait
> for little longer, if both issued a command to change config parameter?

IMO yes, it's sane to make the second backend wait until the first one
commits.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thursday, November 08, 2012 7:56 PM Amit Kapila
On Thursday, November 08, 2012 1:45 AM Robert Haas wrote:
> On Wed, Nov 7, 2012 at 2:50 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> >> Well, Magnus' proposed implementation supposed that the existing
>> values
>> >> *have* been loaded into the current session. I agree that with some
>> >> locking and yet more code you could implement it without that. But
> this
>> >> still doesn't seem to offer any detectable benefit over value-per-
> file.
> >
>> > Well, value-per-file is ugly (imagine you've set 40 different
> variables
>> > that way) but dodges a lot of complicated issues. And I suppose
> "ugly"
> >> doesn't matter, because the whole idea of the auto-generated files is
> > >that users aren't supposed to look at them anyway.
>
>> That's pretty much how I feel about it, too. I think value-per-file
> >is an ugly wimp-out that shouldn't really be necessary to solve this
>> problem. It can't be that hard to rewrite a file where every like is
>> of the form:
>
>> key = 'value'

> I also believe that it should be possible to rewrite a file without loading
> values into the current session.
> One of the solution if we assume that file is of fixed format and each
> record (key = 'value') of fixed length can be:

> 1. While writing .auto file, it will always assume that .auto file contain
> all config parameters.
> Now as this .auto file is of fixed format and fixed record size, it can
> directly write a given record to its particular position.
> 2. To handle locking issues, we can follow an approach similar to what "GIT"
> is doing for editing conf files (using .lock file):
> a. copy the latest content of .auto to .auto.lock
> b. make all the changes to auto.lock file.
> c. at the end of command rename the auto.lock file to .auto file
> d. otherwise if SQL COMMAND/function failed in-between we can delete the
> ".auto.lock" file
>3. Two backends trying to write to .auto file
> we can use ".auto.lock" as the the lock by trying to create it in
>exclusive mode as the first step
> of the command. If it already exists then backend needs to wait.

Please let me know if there are any objections or problems in above method of implementation,
else I can go ahead to prepare the patch for the coming CF.

For initial version I will use the function as syntax to provide this feature.

With Regards,
Amit Kapila.

On 11/9/12 11:59 PM, Amit kapila wrote:

> Please let me know if there are any objections or problems in above method of implementation,
> else I can go ahead to prepare the patch for the coming CF.

It may be the case that the locking scheme Robert described is the best
approach here. It seems kind of heavy to me though. I suspect that
some more thinking about it might come up with something better.

Regardless, exactly how to prevent two concurrent processes from writing
the same file feels like the last step in the small roadmap for what
this feature needs. If you wanted to work on it more, I'd suggest
breaking it into chunks in this order:

1) Change to add scanning a .conf directory in the default configuration
using include-dir. This is a quick fix. I predict most of the
headaches around it will end up being for packagers rather than the core
code to deal with.

You could submit this as a small thing to be evaluated on its own. How
it's done is going to be controversial. Might as well get that fighting
focused against a sample implementation as soon as possible.

2) Get familiar with navigating the GUC data and figuring out what,
exactly, needs to be written out. This should include something that
navigates like things appear after a RESET, ignoring per-user or
per-session changes when figuring out what goes there. It seems
inevitable that some amount of validating against the source
information--what pg_settings labels source, sourcefile, and sourceline
will be needed. An example is the suggestion Magnus made for confirming
that the include-dir is still active before writing something there.

3) Add the function to write a new file out. Work out some test cases
for that to confirm the logic and error checking in the previous step
all works.

I'd next submit what you've got for (2) and (3) to review at this point,
before complicating things further with the locking parts.

4) Make the file write atomic and able to work when multiple users try
it at once. You have to reach here successfully before the trivial
around file locking comes into play. I wouldn't even bother aiming for
that part in a first patch. It's obviously a solvable problem in a
number of ways. You need a rock solid way to figure out what to write
there before that solution is useful though.

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

Greg Smith <greg(at)2ndQuadrant(dot)com> writes:
> Regardless, exactly how to prevent two concurrent processes from writing
> the same file feels like the last step in the small roadmap for what
> this feature needs.

"Write a temp file and use rename(2) to move it into place" is the
standard solution for that.

regards, tom lane

On Monday, November 12, 2012 12:07 PM Greg Smith wrote:
On 11/9/12 11:59 PM, Amit kapila wrote:

>> Please let me know if there are any objections or problems in above method of implementation,
>> else I can go ahead to prepare the patch for the coming CF.

> It may be the case that the locking scheme Robert described is the best
> approach here. It seems kind of heavy to me though. I suspect that
> some more thinking about it might come up with something better.

Yes, we should evaluate multiple options to do this and then choose the best among it.
I am ready to work on evaluating other ways to accomplish this feature.

Is the above opinion about only locking or even on a way to write the changed things in a file as mentioned in point-1 in mail chain upthread.
(Point-1: > 1. While writing .auto file, it will always assume that .auto file contain
> all config parameters.
> Now as this .auto file is of fixed format and fixed record size, it can
> directly write a given record to its particular position.)
What my thinking was that if we can decide that the format and size of each configuration is fixed, it can be directly written without doing anything for it in memory.

> Regardless, exactly how to prevent two concurrent processes from writing
> the same file feels like the last step in the small roadmap for what
> this feature needs. If you wanted to work on it more, I'd suggest
> breaking it into chunks in this order:

> 1) Change to add scanning a .conf directory in the default configuration
> using include-dir. This is a quick fix. I predict most of the
> headaches around it will end up being for packagers rather than the core
> code to deal with.

> You could submit this as a small thing to be evaluated on its own. How
> it's done is going to be controversial. Might as well get that fighting
> focused against a sample implementation as soon as possible.

As per my understanding,
a. during initdb, new conf directory can be created and also create .auto file in it.
b. use include_dir at end of postgresql.conf to include directory created in above step.
c. server start/sighup will take care of above include_dir

> 2) Get familiar with navigating the GUC data and figuring out what,
> exactly, needs to be written out. This should include something that
> navigates like things appear after a RESET, ignoring per-user or
> per-session changes when figuring out what goes there. It seems
> inevitable that some amount of validating against the source
> information--what pg_settings labels source, sourcefile, and sourceline
> will be needed. An example is the suggestion Magnus made for confirming
> that the include-dir is still active before writing something there.

Okay, what I will do for this is that I shall explain in next mail the way to do by navigating GUC's.

> 3) Add the function to write a new file out. Work out some test cases
> for that to confirm the logic and error checking in the previous step
> all works.

> I'd next submit what you've got for (2) and (3) to review at this point,
> before complicating things further with the locking parts.

Okay.

> 4) Make the file write atomic and able to work when multiple users try
> it at once. You have to reach here successfully before the trivial
> around file locking comes into play. I wouldn't even bother aiming for
> that part in a first patch. It's obviously a solvable problem in a
> number of ways. You need a rock solid way to figure out what to write
> there before that solution is useful though.

With Regards,
Amit Kapila.

On 11/12/12 7:59 PM, Amit kapila wrote:
> On Monday, November 12, 2012 12:07 PM Greg Smith wrote:
> On 11/9/12 11:59 PM, Amit kapila wrote:
>
>>> Please let me know if there are any objections or problems in above method of implementation,
>>> else I can go ahead to prepare the patch for the coming CF.
>
>> It may be the case that the locking scheme Robert described is the best
>> approach here. It seems kind of heavy to me though. I suspect that
>> some more thinking about it might come up with something better.

So, here's the problem I'm seeing with having a single .auto file: when
we write settings to a file, are we writing a *single* setting or *all
of a user's current settings*?

I was imagining writing single, specific settings, which inevitably
leads to one-setting-per-file, e.g.:

SET PERSISTENT work_mem = 256MB;

What Amit seems to be talking about is more EXPORT SETTINGS, where you
dump all current settings in the session to a file. This seems likely
to produce accidental changes when the user writes out settings they've
forgotten they changed.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> I was imagining writing single, specific settings, which inevitably
> leads to one-setting-per-file, e.g.:

> SET PERSISTENT work_mem = 256MB;

> What Amit seems to be talking about is more EXPORT SETTINGS, where you
> dump all current settings in the session to a file. This seems likely
> to produce accidental changes when the user writes out settings they've
> forgotten they changed.

Yeah. It also seems to be unnecessarily different from the existing
model of SET. I'd go with one-setting-per-command.

regards, tom lane

On Tue, Nov 13, 2012 at 1:16 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> I was imagining writing single, specific settings, which inevitably
>> leads to one-setting-per-file, e.g.:
>
>> SET PERSISTENT work_mem = 256MB;
>
>> What Amit seems to be talking about is more EXPORT SETTINGS, where you
>> dump all current settings in the session to a file. This seems likely
>> to produce accidental changes when the user writes out settings they've
>> forgotten they changed.
>
> Yeah. It also seems to be unnecessarily different from the existing
> model of SET. I'd go with one-setting-per-command.

+1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> Is the above opinion about only locking or even on a way to write the changed things in a file as mentioned in point-1 in mail chain upthread.
> (Point-1: > 1. While writing .auto file, it will always assume that .auto file contain
>> all config parameters.
>> Now as this .auto file is of fixed format and fixed record size, it can
>> directly write a given record to its particular position.)
> What my thinking was that if we can decide that the format and size of each configuration is fixed, it can be directly written without doing anything for it in memory.

Uh, no, I don't think that's a good idea. IMHO, what we should do is:

1. Read postgresql.conf.auto and remember all the settings we saw. If
we see something funky like an include directive, barf.
2. Forget the value we remembered for the particular setting being
changed. Instead, remember the user-supplied new value for that
parameter.
3. Write a new postgresql.conf.auto based on the information
remembered in steps 1 and 2.

Of course, if we go with one-file-per-setting, then this becomes even
simpler: just clobber the file for the single setting being updated -
creating it if it exists - and ignore all the rest. I don't
personally favor that approach because I think I think it's clunky to
manage, but YMMV.

With either approach, it's worth noting that a RESET variant of this
could be useful - which would either remove the chosen setting from
postgresql.conf.auto, or remove the file containing the
automatically-set value for that setting. I think my personal
favorite syntax is:

ALTER SYSTEM .. SET wunk = 'thunk';
ALTER SYSTEM .. RESET wunk;

But I'm OK with something else if there's consensus. I don't
particularly like SET PERSISTENT because I think this is more like
ALTER DATABASE .. SET than it is like SET LOCAL, but IJWH.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wednesday, November 14, 2012 12:25 AM Robert Haas wrote:
> On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > Is the above opinion about only locking or even on a way to write the
> changed things in a file as mentioned in point-1 in mail chain upthread.
> > (Point-1: > 1. While writing .auto file, it will always assume that
> .auto file contain
> >> all config parameters.
> >> Now as this .auto file is of fixed format and fixed record size, it
> can
> >> directly write a given record to its particular position.)
> > What my thinking was that if we can decide that the format and size of
> each configuration is fixed, it can be directly written without doing
> anything for it in memory.
>
> Uh, no, I don't think that's a good idea. IMHO, what we should do is:
>
> 1. Read postgresql.conf.auto and remember all the settings we saw. If
> we see something funky like an include directive, barf.
> 2. Forget the value we remembered for the particular setting being
> changed. Instead, remember the user-supplied new value for that
> parameter.
> 3. Write a new postgresql.conf.auto based on the information
> remembered in steps 1 and 2.

I am okay with implementing the above way because as per my understanding
this is almost very similar to what I have mentioned in my initial proposal
(Point-5 in Algorithm of Alter System Set ...).
http://archives.postgresql.org/pgsql-hackers/2012-10/msg01509.php

However as now Greg suggested to explore GUC concept as well, so I would
like to check and see the feasibility by that method.

The only reason I have mentioned about fixed format and fixed record size
concept is that during previous discussions for writing the file with GUC,
it came up that is it possible to write file without reading it in current
session.
(-- It seems to me that we ought to be able to rewrite a machine-generated
configuration file without loading those values into the current session.)
Now on second thought it seems to me may be you want to say by above comment
was without loading into session specific GUC.

> Of course, if we go with one-file-per-setting, then this becomes even
> simpler: just clobber the file for the single setting being updated -
> creating it if it exists - and ignore all the rest. I don't
> personally favor that approach because I think I think it's clunky to
> manage, but YMMV.

> With either approach, it's worth noting that a RESET variant of this
> could be useful - which would either remove the chosen setting from
> postgresql.conf.auto, or remove the file containing the
> automatically-set value for that setting. I think my personal
> favorite syntax is:
>
> ALTER SYSTEM .. SET wunk = 'thunk';
> ALTER SYSTEM .. RESET wunk;
>
> But I'm OK with something else if there's consensus. I don't
> particularly like SET PERSISTENT because I think this is more like
> ALTER DATABASE .. SET than it is like SET LOCAL, but IJWH.

I think for this there are multiple ways, one is Alter System .., other is
provide this through built-in function.
For first version may be I will go with built-in function Approach, then if
there is consensus to give it through
Alter System, we can change it.
One advantage, I am seeing in your above suggestion is that a method to
provide RESET will be better with ALTER SYSTEM rather than built-in
function. For the same to achieve through built-in, I think one way to
provide is to give a separate function.

With Regards,
Amit Kapila.

On Tuesday, November 13, 2012 11:43 PM Josh Berkus wrote:
> On 11/12/12 7:59 PM, Amit kapila wrote:
> > On Monday, November 12, 2012 12:07 PM Greg Smith wrote:
> > On 11/9/12 11:59 PM, Amit kapila wrote:
> >
> >>> Please let me know if there are any objections or problems in above
> method of implementation,
> >>> else I can go ahead to prepare the patch for the coming CF.
> >
> >> It may be the case that the locking scheme Robert described is the
> best
> >> approach here. It seems kind of heavy to me though. I suspect that
> >> some more thinking about it might come up with something better.
>
> So, here's the problem I'm seeing with having a single .auto file: when
> we write settings to a file, are we writing a *single* setting or *all
> of a user's current settings*?

Single setting.

> I was imagining writing single, specific settings, which inevitably
> leads to one-setting-per-file, e.g.:
>
> SET PERSISTENT work_mem = 256MB;

Yes, from beginning what I was discussing was setting of single config parameter as in your example.
However, it can be done with one-file for all variables as well.
I have already mentioned 2 ways of doing it, one is fixed format and fixed size file, other is similar to what Robert has detailed
in his mail (http://archives.postgresql.org/pgsql-hackers/2012-11/msg00572.php).

> What Amit seems to be talking about is more EXPORT SETTINGS, where you
> dump all current settings in the session to a file. This seems likely
> to produce accidental changes when the user writes out settings they've
> forgotten they changed.

I think may be I was not clear enough in my previous mails, but for sure whatever I am talking is never related to
"dump all current settings in the session to a file".
In fact both my ideas (fixed format file, initial proposal) was not to touch or check the current session parameters.
There is only one Approach which is to see if from GUC, we can write the file that talks about writing multiple parameters.

With Regards,
Amit Kapila.

On Tuesday, November 13, 2012 9:29 AM Amit kapila wrote:
On Monday, November 12, 2012 12:07 PM Greg Smith wrote:
On 11/9/12 11:59 PM, Amit kapila wrote:

>> 1) Change to add scanning a .conf directory in the default configuration
>> using include-dir. This is a quick fix. I predict most of the
>> headaches around it will end up being for packagers rather than the core
>> code to deal with.

>> You could submit this as a small thing to be evaluated on its own. How
>> it's done is going to be controversial. Might as well get that fighting
>> focused against a sample implementation as soon as possible.

> As per my understanding,
> a. during initdb, new conf directory can be created and also create .auto file in it.
> b. use include_dir at end of postgresql.conf to include directory created in above step.
> c. server start/sighup will take care of above include_dir

WIP patch to address above point is attached.
It needs cleanup w.r.t moving function for absolute path to common place where initdb as well as server code can use it.

>> 2) Get familiar with navigating the GUC data and figuring out what,
>> exactly, needs to be written out. This should include something that
>> navigates like things appear after a RESET, ignoring per-user or
>> per-session changes when figuring out what goes there. It seems
>> inevitable that some amount of validating against the source
>> information--what pg_settings labels source, sourcefile, and sourceline
>> will be needed. An example is the suggestion Magnus made for confirming
>> that the include-dir is still active before writing something there.

> Okay, what I will do for this is that I shall explain in next mail the way to do by navigating GUC's.

One basic idea to do execution of SQL Command with GUC's is described as below:

1. Take Global level lock so that no other session can run the ALTER SYSTEM/built-in Command to change config parameter
2. Navigate through GUC variable's and remember all GUC's (of .auto file ) reset_val.
3. Compare the config parameter to be changed with the parameters stored in step-2 and if it exists, replace its value else add new variable-value to it.
4. Flush the file with all parameters computed in Step-3.
5. Signal all other backends to update this value in their GUC's reset_val, so that all backends always have recent copy.
6. When all backends have updated, change corresponding reset_val in current session as well.
7. Release the Global level lock.

Some problems with the above approach:
a. When the signal is sent in step-5, if other backend is also waiting on global lock, it can cause signal handling little tricky,
may be special handling needs to be done to handle this situation
b. If after step-5 or 6, rollback happened it might be difficult to rollback. In general if this command executes in transaction-block, the same issue can arise.
c. Updation of reset_val for parameters which cannot be reset without restart is wrong. For such kind of parameters, I think we can give warning to users.

I think this is the initial idea to check if I am thinking on lines you have in mind.

Comments/Suggestions?

With Regards,
Amit Kapila.

On Wednesday, November 14, 2012 12:24 AM Robert Haas wrote:
On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:

> Uh, no, I don't think that's a good idea. IMHO, what we should do is:

> 1. Read postgresql.conf.auto and remember all the settings we saw. If we see something funky like an include directive, barf.
> 2. Forget the value we remembered for the particular setting being changed. Instead, remember the user-supplied new value for that parameter.
> 3. Write a new postgresql.conf.auto based on the information remembered in steps 1 and 2.

Attached patch contains implementaion for above concept.
It can be changed to adapt the write file based on GUC variables as described by me yesterday or in some other better way.

Currenty to remember and forget, I have used below concept:
1. Read postgresql.auto.conf in memory.
2. parse the GUC_file for exact loction of changed variable
3. update the changed variable in memory at location found in step-2
4. Write the postgresql.auto.conf

Overall changes:
1. include dir in postgresql.conf at time of initdb
2. new built-in function pg_change_conf to change configuration settings
3. write file as per logic described above.

Some more things left are:
1. Transactional capability to command, so that rename of .lock file to .auto.conf can be done at commit time.

I am planing to upload the attached for this CF.

Suggestions/Comments?

With Regards,
Amit Kapila.

Le jeudi 15 novembre 2012 15:48:14, Amit kapila a écrit :
> On Wednesday, November 14, 2012 12:24 AM Robert Haas wrote:
>
> On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
wrote:
> > Uh, no, I don't think that's a good idea. IMHO, what we should do is:
> >
> > 1. Read postgresql.conf.auto and remember all the settings we saw. If we
> > see something funky like an include directive, barf. 2. Forget the value
> > we remembered for the particular setting being changed. Instead,
> > remember the user-supplied new value for that parameter. 3. Write a new
> > postgresql.conf.auto based on the information remembered in steps 1 and
> > 2.
>
> Attached patch contains implementaion for above concept.
> It can be changed to adapt the write file based on GUC variables as
> described by me yesterday or in some other better way.
>
> Currenty to remember and forget, I have used below concept:
> 1. Read postgresql.auto.conf in memory.
> 2. parse the GUC_file for exact loction of changed variable
> 3. update the changed variable in memory at location found in step-2
> 4. Write the postgresql.auto.conf
>
> Overall changes:
> 1. include dir in postgresql.conf at time of initdb
> 2. new built-in function pg_change_conf to change configuration settings
> 3. write file as per logic described above.
>
> Some more things left are:
> 1. Transactional capability to command, so that rename of .lock file to
> .auto.conf can be done at commit time.
>
> I am planing to upload the attached for this CF.
>
> Suggestions/Comments?

Yes, sorry to jump here so late.
* Why don't we use pg_settings ? (i.e. why not materialize the view and use
it, it should be easier to have something transactional and also serializable
with probably a DEFERRABLE select pg_reload_config() which mv the configuration
file at commit time)
* Can I define automatic parameters to be loaded before and/or after the non-
automatic parameters in a convenient way (without editing files at all)?

--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

On Thursday, November 15, 2012 11:28 PM Cédric Villemain wrote:
> Le jeudi 15 novembre 2012 15:48:14, Amit kapila a écrit :
> > On Wednesday, November 14, 2012 12:24 AM Robert Haas wrote:
> >
> > On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > > Uh, no, I don't think that's a good idea. IMHO, what we should do
> is:
> > >
> > > 1. Read postgresql.conf.auto and remember all the settings we saw.
> > > If we see something funky like an include directive, barf. 2. Forget
> > > the value we remembered for the particular setting being changed.
> > > Instead, remember the user-supplied new value for that parameter. 3.
> > > Write a new postgresql.conf.auto based on the information remembered
> > > in steps 1 and 2.
> >
> > Attached patch contains implementaion for above concept.
> > It can be changed to adapt the write file based on GUC variables as
> > described by me yesterday or in some other better way.
> >
> > Currenty to remember and forget, I have used below concept:
> > 1. Read postgresql.auto.conf in memory.
> > 2. parse the GUC_file for exact loction of changed variable 3. update
> > the changed variable in memory at location found in step-2 4. Write
> > the postgresql.auto.conf
> >
> > Overall changes:
> > 1. include dir in postgresql.conf at time of initdb 2. new built-in
> > function pg_change_conf to change configuration settings 3. write file
> > as per logic described above.
> >
> > Some more things left are:
> > 1. Transactional capability to command, so that rename of .lock file
> > to .auto.conf can be done at commit time.
> >
> > I am planing to upload the attached for this CF.
> >
> > Suggestions/Comments?
>
> Yes, sorry to jump here so late.
> * Why don't we use pg_settings ? (i.e. why not materialize the view and
> use it, it should be easier to have something transactional and also
> serializable with probably a DEFERRABLE select pg_reload_config() which
> mv the configuration file at commit time)

I think some consistency issues can come, because before editing and
flushing, each backend has to have latest copy
else it might override some parameters values.
Can you explain the whole idea in detail, may be it will be easier to verify
if it has any problem.

> * Can I define automatic parameters to be loaded before and/or after the
> non- automatic parameters in a convenient way (without editing files at
> all)?

In the current implementation, there is no way. Do you have any suggestion?

With Regards,
Amit Kapila.

Le vendredi 16 novembre 2012 07:16:09, Amit Kapila a écrit :
> On Thursday, November 15, 2012 11:28 PM Cédric Villemain wrote:
> > Le jeudi 15 novembre 2012 15:48:14, Amit kapila a écrit :
> > > On Wednesday, November 14, 2012 12:24 AM Robert Haas wrote:
> > >
> > > On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
> >
> > wrote:
> > > > Uh, no, I don't think that's a good idea. IMHO, what we should do
> >
> > is:
> > > > 1. Read postgresql.conf.auto and remember all the settings we saw.
> > > > If we see something funky like an include directive, barf. 2. Forget
> > > > the value we remembered for the particular setting being changed.
> > > > Instead, remember the user-supplied new value for that parameter. 3.
> > > > Write a new postgresql.conf.auto based on the information remembered
> > > > in steps 1 and 2.
> > >
> > > Attached patch contains implementaion for above concept.
> > > It can be changed to adapt the write file based on GUC variables as
> > > described by me yesterday or in some other better way.
> > >
> > > Currenty to remember and forget, I have used below concept:
> > > 1. Read postgresql.auto.conf in memory.
> > > 2. parse the GUC_file for exact loction of changed variable 3. update
> > > the changed variable in memory at location found in step-2 4. Write
> > > the postgresql.auto.conf
> > >
> > > Overall changes:
> > > 1. include dir in postgresql.conf at time of initdb 2. new built-in
> > > function pg_change_conf to change configuration settings 3. write file
> > > as per logic described above.
> > >
> > > Some more things left are:
> > > 1. Transactional capability to command, so that rename of .lock file
> > > to .auto.conf can be done at commit time.
> > >
> > > I am planing to upload the attached for this CF.
> > >
> > > Suggestions/Comments?
> >
> > Yes, sorry to jump here so late.
> > * Why don't we use pg_settings ? (i.e. why not materialize the view and
> > use it, it should be easier to have something transactional and also
> > serializable with probably a DEFERRABLE select pg_reload_config() which
> > mv the configuration file at commit time)
>
> I think some consistency issues can come, because before editing and
> flushing, each backend has to have latest copy
> else it might override some parameters values.
> Can you explain the whole idea in detail, may be it will be easier to
> verify if it has any problem.

It looks like a bit similar to what proposed Peter in another thread.
If you use a table to store the values, the action of writing the file is just
flush a table to disk, it might be a deferred trigger for example.
This table can be inserted/updated/deleted in a « BEGIN TRANSACTION ISOLATION
SERIALIZABLE » transaction so there is no issue on who touch what and when.
Either it is commited without serialization error or it is not.
(and we can elaborate with the table content being deleted at commit time, or
not, etc.).

I suppose it can be an extension also.

>
> > * Can I define automatic parameters to be loaded before and/or after the
> > non- automatic parameters in a convenient way (without editing files at
> > all)?
>
> In the current implementation, there is no way. Do you have any suggestion?

not yet.
...thinking some more...
Maybe add a column to define where to write the updated GUC (before everything
else, after everything else, instead of the current content). The trigger
responsible to write that will do.
Currently, nothing prevent 2 users to log in system, edit postgresql.conf and
both issue a reload... So what's the problem ? Only have a single new GUC like
'conf_from_sql=on|off' so it is possible to forbid update of configuration from
SQL (it looks like a requirement for this new feature, anyway); or have it
like an extension, and superuser are free to install or not.

Maybe I am repeating some previous suggestions which have been invalidated. In
such case please excuse that I did not take time on my side to re-read all
relative threads.
--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

> From: Cédric Villemain [mailto:cedric(at)2ndquadrant(dot)com]
> Sent: Friday, November 16, 2012 1:55 PM
> To: pgsql-hackers(at)postgresql(dot)org
> Cc: Amit Kapila; 'Robert Haas'; 'Greg Smith'; 'Josh Berkus'; 'Tom Lane';
> 'Magnus Hagander'; 'Christopher Browne'
> Subject: Re: [HACKERS] Proposal for Allow postgresql.conf values to be
> changed via SQL
On Friday, November 16, 2012 1:55 PM Cédric Villemain wrote:
> Le vendredi 16 novembre 2012 07:16:09, Amit Kapila a écrit :
> > On Thursday, November 15, 2012 11:28 PM Cédric Villemain wrote:
> > > Le jeudi 15 novembre 2012 15:48:14, Amit kapila a écrit :
> > > > On Wednesday, November 14, 2012 12:24 AM Robert Haas wrote:
> > > >
> > > > On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila
> > > > <amit(dot)kapila(at)huawei(dot)com>
> > >
> > > wrote:
> > > > > Uh, no, I don't think that's a good idea. IMHO, what we should
> > > > > do
> > >
> > > is:
> > > > > 1. Read postgresql.conf.auto and remember all the settings we
> saw.
> > > > > If we see something funky like an include directive, barf. 2.
> > > > > Forget the value we remembered for the particular setting being
> changed.
> > > > > Instead, remember the user-supplied new value for that
> parameter. 3.
> > > > > Write a new postgresql.conf.auto based on the information
> > > > > remembered in steps 1 and 2.
> > > >
> > > > Attached patch contains implementaion for above concept.
> > > > It can be changed to adapt the write file based on GUC variables
> > > > as described by me yesterday or in some other better way.
> > > >
> > > > Currenty to remember and forget, I have used below concept:
> > > > 1. Read postgresql.auto.conf in memory.
> > > > 2. parse the GUC_file for exact loction of changed variable 3.
> > > > update the changed variable in memory at location found in step-2
> > > > 4. Write the postgresql.auto.conf
> > > >
> > > > Overall changes:
> > > > 1. include dir in postgresql.conf at time of initdb 2. new
> > > > built-in function pg_change_conf to change configuration settings
> > > > 3. write file as per logic described above.
> > > >
> > > > Some more things left are:
> > > > 1. Transactional capability to command, so that rename of .lock
> > > > file to .auto.conf can be done at commit time.
> > > >
> > > > I am planing to upload the attached for this CF.
> > > >
> > > > Suggestions/Comments?
> > >
> > > Yes, sorry to jump here so late.
> > > * Why don't we use pg_settings ? (i.e. why not materialize the view
> > > and use it, it should be easier to have something transactional and
> > > also serializable with probably a DEFERRABLE select
> > > pg_reload_config() which mv the configuration file at commit time)
> >
> > I think some consistency issues can come, because before editing and
> > flushing, each backend has to have latest copy else it might override
> > some parameters values.
> > Can you explain the whole idea in detail, may be it will be easier to
> > verify if it has any problem.
>
> It looks like a bit similar to what proposed Peter in another thread.
> If you use a table to store the values, the action of writing the file
> is just flush a table to disk, it might be a deferred trigger for
> example.
> This table can be inserted/updated/deleted in a « BEGIN TRANSACTION
> ISOLATION SERIALIZABLE » transaction so there is no issue on who touch
> what and when.
> Either it is commited without serialization error or it is not.
> (and we can elaborate with the table content being deleted at commit
> time, or not, etc.).
>
> I suppose it can be an extension also.
>
> >
> > > * Can I define automatic parameters to be loaded before and/or after
> > > the
> > > non- automatic parameters in a convenient way (without editing files
> > > at all)?
> >
> > In the current implementation, there is no way. Do you have any
> suggestion?
>
> not yet.
> ...thinking some more...
> Maybe add a column to define where to write the updated GUC (before
> everything else, after everything else, instead of the current content).
> The trigger responsible to write that will do.

Currently, it will write all the configuration parameters to be changed by
SQL commands in separate PostgreSQL.auto.conf file and that will get
included at end of postgresql.conf. So I think wherever we write it in .auto
file as it is included at end, always parameters of .auto file will be
loaded later.

With Regards,
Amit Kapila.

On Thursday, November 15, 2012 8:18 PM Amit kapila wrote:
> On Wednesday, November 14, 2012 12:24 AM Robert Haas wrote:
> On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
>
> > Uh, no, I don't think that's a good idea. IMHO, what we should do is:
>
> > 1. Read postgresql.conf.auto and remember all the settings we saw. If
> we see something funky like an include directive, barf.
> > 2. Forget the value we remembered for the particular setting being
> changed. Instead, remember the user-supplied new value for that
> parameter.
> > 3. Write a new postgresql.conf.auto based on the information
> remembered in steps 1 and 2.
>
> Attached patch contains implementaion for above concept.
> It can be changed to adapt the write file based on GUC variables as
> described by me yesterday or in some other better way.
>
> Currenty to remember and forget, I have used below concept:
> 1. Read postgresql.auto.conf in memory.
> 2. parse the GUC_file for exact loction of changed variable 3. update
> the changed variable in memory at location found in step-2 4. Write the
> postgresql.auto.conf
>
> Overall changes:
> 1. include dir in postgresql.conf at time of initdb 2. new built-in
> function pg_change_conf to change configuration settings 3. write file
> as per logic described above.
>
> Some more things left are:
> 1. Transactional capability to command, so that rename of .lock file to
> .auto.conf can be done at commit time.

About transaction capability, I think it will be difficult to implement it
in transaction block,
because during Rollback to savepoint it is difficult to rollback (delete the
file), as there is no track of changes w.r.t
Savepoint.

So to handle, we can do one of the following:
1. Handle it at command level only
2. Don't allow this command in transaction blocks
3. Rollback to Savepoint will have no effect w.r.t this command, only when
top transaction commits/rollback,
It commit/rollback the file.

IMO, option-2 is better.

Suggestions/Comments?

With Regards,
Amit Kapila.

Le vendredi 16 novembre 2012 15:08:30, Amit Kapila a écrit :
> On Thursday, November 15, 2012 8:18 PM Amit kapila wrote:
> > On Wednesday, November 14, 2012 12:24 AM Robert Haas wrote:
> > On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
> >
> > wrote:
> > > Uh, no, I don't think that's a good idea. IMHO, what we should do is:
> > >
> > > 1. Read postgresql.conf.auto and remember all the settings we saw. If
> >
> > we see something funky like an include directive, barf.
> >
> > > 2. Forget the value we remembered for the particular setting being
> >
> > changed. Instead, remember the user-supplied new value for that
> > parameter.
> >
> > > 3. Write a new postgresql.conf.auto based on the information
> >
> > remembered in steps 1 and 2.
> >
> > Attached patch contains implementaion for above concept.
> > It can be changed to adapt the write file based on GUC variables as
> > described by me yesterday or in some other better way.
> >
> > Currenty to remember and forget, I have used below concept:
> > 1. Read postgresql.auto.conf in memory.
> > 2. parse the GUC_file for exact loction of changed variable 3. update
> > the changed variable in memory at location found in step-2 4. Write the
> > postgresql.auto.conf
> >
> > Overall changes:
> > 1. include dir in postgresql.conf at time of initdb 2. new built-in
> > function pg_change_conf to change configuration settings 3. write file
> > as per logic described above.
> >
> > Some more things left are:
> > 1. Transactional capability to command, so that rename of .lock file to
> > .auto.conf can be done at commit time.
>
> About transaction capability, I think it will be difficult to implement it
> in transaction block,
> because during Rollback to savepoint it is difficult to rollback (delete
> the file), as there is no track of changes w.r.t
> Savepoint.

not a problem.
consider that pseudo code:

begin serializable;

update pg_settings; -- or whatever the name of the object (can include
creation of a table, etc...)

savepoint...

update pg_settings;

rollback to savepoint;

commit; <-- here the deferred trigger FOR STATEMENT on pg_settings is fired
and is responsible to write/mv the/a file.

Is there something obvious I'm not seeing ?

--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

On Friday, November 16, 2012 7:52 PM Cédric Villemain wrote:
> Le vendredi 16 novembre 2012 15:08:30, Amit Kapila a écrit :
> > On Thursday, November 15, 2012 8:18 PM Amit kapila wrote:
> > > On Wednesday, November 14, 2012 12:24 AM Robert Haas wrote:
> > > On Mon, Nov 12, 2012 at 10:59 PM, Amit kapila
> > > <amit(dot)kapila(at)huawei(dot)com>
> > >
> > > wrote:
> > > > Uh, no, I don't think that's a good idea. IMHO, what we should do
> is:
> > > >
> > > > 1. Read postgresql.conf.auto and remember all the settings we saw.
> > > > If
> > >
> > > we see something funky like an include directive, barf.
> > >
> > > > 2. Forget the value we remembered for the particular setting being
> > >
> > > changed. Instead, remember the user-supplied new value for that
> > > parameter.
> > >
> > > > 3. Write a new postgresql.conf.auto based on the information
> > >
> > > remembered in steps 1 and 2.
> > >
> > > Attached patch contains implementaion for above concept.
> > > It can be changed to adapt the write file based on GUC variables as
> > > described by me yesterday or in some other better way.
> > >
> > > Currenty to remember and forget, I have used below concept:
> > > 1. Read postgresql.auto.conf in memory.
> > > 2. parse the GUC_file for exact loction of changed variable 3.
> > > update the changed variable in memory at location found in step-2 4.
> > > Write the postgresql.auto.conf
> > >
> > > Overall changes:
> > > 1. include dir in postgresql.conf at time of initdb 2. new built-in
> > > function pg_change_conf to change configuration settings 3. write
> > > file as per logic described above.
> > >
> > > Some more things left are:
> > > 1. Transactional capability to command, so that rename of .lock file
> > > to .auto.conf can be done at commit time.
> >
> > About transaction capability, I think it will be difficult to
> > implement it in transaction block, because during Rollback to
> > savepoint it is difficult to rollback (delete the file), as there is
> > no track of changes w.r.t Savepoint.
>
> not a problem.
> consider that pseudo code:
>
> begin serializable;
>
> update pg_settings; -- or whatever the name of the object (can include
> creation of a table, etc...)
>
> savepoint...
>
> update pg_settings;
>
> rollback to savepoint;
>
> commit; <-- here the deferred trigger FOR STATEMENT on pg_settings is
> fired and is responsible to write/mv the/a file.
>
> Is there something obvious I'm not seeing ?

I think transaction handling is better with the way you are mentioning.

Here is what I am able to think about your idea:

1. have a system table pg_global_system_settings(key,value)
2. On SQL command execution, insert if the value doesn't exist or update if
already existing.
3. On commit, a deffered trigger will read from table and put all the rows
in a .auto flat file
4. In the deffered trigger, may be we need to use lock for writting to file,
so that 2 backends
writting same time may not garbled the file. I am not sure if lock is
required or not?

Advantages of this approach:
1. transaction handling can be better.
2. updation of config value row can be easier

Problem which needs to be thought
Sychronization between flat file .auto.conf and system table
Case-1
a. During commit, we write into file (deffered trigger execution)
"before" marking transaction as commit.
b. after writting to file, any error or system crash, then table and file
will have different contents.
Case-2
a. During commit, we write into file (deffered trigger execution) "after"
marking transaction as commit.
b. any error or system crash before write into file can lead to different
contents in table and flat file.

Resolution
May be during recovery, we can try to make table and file consistent, but it
can be tricky.

Comparison with Approach I have implemented
1. Because it will be done in serializable isolation, 2 users trying to
modify the same value, will get error.
However in current approach, user will not get this error.
2. The lock time will be lesser in system table approach but don't think it
will matter because it is a rarely used
command.

I think, some other people thoughts are also required to see if there is any

deeper design issue which I could not see in this approach and whether it
can clearly score over approach
with which currently it is implemented(directly operate on a file).

Suggestions/Thoughts?

With Regards,
Amit Kapila.

On Sat, Nov 17, 2012 at 10:25 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> 1. have a system table pg_global_system_settings(key,value)

Do we really need to store the settings in a system table?
Since WAL would be generated when storing the settings
in a system table, this approach seems to prevent us from
changing the settings in the standby.

> 2. On SQL command execution, insert if the value doesn't exist or update if
> already existing.

This means that we should implement something like MERGE
command first?

Regards,

--
Fujii Masao

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> Do we really need to store the settings in a system table?
> Since WAL would be generated when storing the settings
> in a system table, this approach seems to prevent us from
> changing the settings in the standby.

That's a really good point: if we try to move all GUCs into a system
table, there's no way for a standby to have different values; and for
some of them different values are *necessary*.

I think that shoots down this line of thought entirely. Can we go
back to the plain "write a file" approach now? I think a "SET
PERSISTENT" command that's disallowed in transaction blocks and just
writes the file immediately is perfectly sensible.

regards, tom lane

Le samedi 17 novembre 2012 22:57:49, Tom Lane a écrit :
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> > Do we really need to store the settings in a system table?
> > Since WAL would be generated when storing the settings
> > in a system table, this approach seems to prevent us from
> > changing the settings in the standby.
>
> That's a really good point: if we try to move all GUCs into a system
> table, there's no way for a standby to have different values; and for
> some of them different values are *necessary*.
>
> I think that shoots down this line of thought entirely. Can we go
> back to the plain "write a file" approach now? I think a "SET
> PERSISTENT" command that's disallowed in transaction blocks and just
> writes the file immediately is perfectly sensible.

I was justifying the usage of a table structure, not to keep it in sync (just
use it to hide the complexity of locks).

Anyway that was just comments.
--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

> On Sunday, November 18, 2012 3:08 AM Fujii Masao wrote:
> On Sat, Nov 17, 2012 at 10:25 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > 1. have a system table pg_global_system_settings(key,value)
>
> Do we really need to store the settings in a system table?
> Since WAL would be generated when storing the settings
> in a system table, this approach seems to prevent us from
> changing the settings in the standby.

Thanks for your feedback.

It is one of the Approach, we were trying to evaluate for this feature.
However this feature can be done by directly operating on file as well.

With Regards,
Amit Kapila.

On Sunday, November 18, 2012 3:28 AM Tom Lane wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> > Do we really need to store the settings in a system table?
> > Since WAL would be generated when storing the settings
> > in a system table, this approach seems to prevent us from
> > changing the settings in the standby.
>
> That's a really good point: if we try to move all GUCs into a system
> table, there's no way for a standby to have different values; and for
> some of them different values are *necessary*.
>
> I think that shoots down this line of thought entirely. Can we go
> back to the plain "write a file" approach now?

Sure.

> I think a "SET
> PERSISTENT" command that's disallowed in transaction blocks and just
> writes the file immediately is perfectly sensible.

I got the point that we can disallow inside transaction blocks.
Just to clarify, that by above do you mean to say that file write (rename
from .conf.lock to .conf) should be done as soon as
command execution ends rather than the transaction end or it should be done
at transaction end?

Still I think we are not able to completely rule out one or other from
syntax perspective.

We have discussion about below 3 different syntaxes for this command

1. ALTER SYSTEM
2. SET PERSISENT
3. pg_change_conf()

I think to conclude, we need to evaluate which syntax has more advantages.
Comparison for above syntax

1. If we want to allow multiple configuration parameters now or in future to
be updated in single command. Example
a. Alter System Set port = 5435, max_connections = 100;
b. Select pg_change_conf('port', 5435),
pg_change_conf('max_connections',100);

I think it might be convenient for user to use Command syntax.

2. If we provide built-in, user can try to use in some complicated syntax
Select 1/0 from tbl where a= pg_change_conf('max_connections',100);
The design and test needs to take care of such usage, so that it doesn't
create any problem.

3. Using with the SET command syntax can create some confusion for user, as
SET SESSION | LOCAL option can work
in transaction blocks, but this feature may not be required to work in
transaction blocks as it will change in
config file which can take effect only on re-start or sighup.

I believe some more thoughts and suggestions are required to conclude.

Thoughts/Suggestions/Comments?

With Regards,
Amit Kapila.

On Sunday, November 18, 2012 3:22 PM Cédric Villemain wrote:
> Le samedi 17 novembre 2012 22:57:49, Tom Lane a écrit :
> > Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> > > Do we really need to store the settings in a system table?
> > > Since WAL would be generated when storing the settings in a system
> > > table, this approach seems to prevent us from changing the settings
> > > in the standby.
> >
> > That's a really good point: if we try to move all GUCs into a system
> > table, there's no way for a standby to have different values; and for
> > some of them different values are *necessary*.
> >
> > I think that shoots down this line of thought entirely. Can we go
> > back to the plain "write a file" approach now? I think a "SET
> > PERSISTENT" command that's disallowed in transaction blocks and just
> > writes the file immediately is perfectly sensible.
>
> I was justifying the usage of a table structure, not to keep it in sync
> (just use it to hide the complexity of locks).
>
> Anyway that was just comments.
Thanks.
You comments are thought provoking. I was able to proceed for table
related approach based on your suggestions.

With Regards,
Amit Kapila.

Amit Kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> We have discussion about below 3 different syntaxes for this command
>
> 1. ALTER SYSTEM
> 2. SET PERSISENT
> 3. pg_change_conf()
>
> I think to conclude, we need to evaluate which syntax has more advantages.
> Comparison for above syntax

I think ALTER SYSTEM should be what Peter Eisentraut proposed in another
thread, using system catalogs and thus not supporting the whole range of
parameters and reset behavior on SIGHUP. That's still very useful, and
seems to me clear enough to document.

Then, I think you could implement a SET PERSISENT command that call the
pg_change_conf() fonction. The problem is that you then can't have the
command unavailable in a transaction block if all it does is calling the
function, because the function call needs to happen in a transaction.

I'd vote for having a lock that serialize any calls to that function. My
understanding of the use cases makes it really ok not be to accept any
concurrency behavior here.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Monday, November 19, 2012 7:53 PM Dimitri Fontaine wrote:
> Amit Kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> > We have discussion about below 3 different syntaxes for this command
> >
> > 1. ALTER SYSTEM
> > 2. SET PERSISENT
> > 3. pg_change_conf()
> >
> > I think to conclude, we need to evaluate which syntax has more
> advantages.
> > Comparison for above syntax
>
> I think ALTER SYSTEM should be what Peter Eisentraut proposed in another
> thread, using system catalogs and thus not supporting the whole range of
> parameters and reset behavior on SIGHUP. That's still very useful, and
> seems to me clear enough to document.

> Then, I think you could implement a SET PERSISENT command that call the
> pg_change_conf() fonction. The problem is that you then can't have the
> command unavailable in a transaction block if all it does is calling the
> function, because the function call needs to happen in a transaction.

If we want to go for SET PERSISTENT, then no need of built-in function call
pg_change_conf(),
the functionality can be implemented in some internal function.
I believe still avoiding start of transaction is un-avoidable, as even parse
of statement is called
after StartTransaction.
The only point I can see against SET PERSISTENT is that other variants of
SET command can be used in
transaction blocks means for them ROLLBACK TO SAVEPOINT functionality works,
but for SET PERSISTENT,
it can't be done.
So to handle that might be we need to mention this point in User Manual, so
that users can be aware of this usage.
If that is okay, then I think SET PERSISTENT is good to go.

With Regards,
Amit Kapila.

Amit Kapila escribió:

> The only point I can see against SET PERSISTENT is that other variants of
> SET command can be used in
> transaction blocks means for them ROLLBACK TO SAVEPOINT functionality works,
> but for SET PERSISTENT,
> it can't be done.
> So to handle that might be we need to mention this point in User Manual, so
> that users can be aware of this usage.
> If that is okay, then I think SET PERSISTENT is good to go.

I think that's okay. There are other commands which have some forms
that can run inside a transaction block and others not. CLUSTER is
one example (maybe the only one? Not sure).

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Monday, November 19, 2012 8:36 PM Alvaro Herrera wrote:
> Amit Kapila escribió:
>
> > The only point I can see against SET PERSISTENT is that other variants
> of
> > SET command can be used in
> > transaction blocks means for them ROLLBACK TO SAVEPOINT functionality
> works,
> > but for SET PERSISTENT,
> > it can't be done.
> > So to handle that might be we need to mention this point in User
> Manual, so
> > that users can be aware of this usage.
> > If that is okay, then I think SET PERSISTENT is good to go.
>
> I think that's okay. There are other commands which have some forms
> that can run inside a transaction block and others not. CLUSTER is
> one example (maybe the only one? Not sure).

In that case, it can have one more advantage that all configuration setting
can be done with one command
and in future we might want to have option like BOTH where the command will
take effect for memory as well
as file.

Can you think of any strong reason why not to have with Alter System
Command?

In any case SET PERSISTENT is fine.

With Regards,
Amit Kapila.

On Monday, November 19, 2012 9:07 PM Amit Kapila wrote:
> On Monday, November 19, 2012 8:36 PM Alvaro Herrera wrote:
> > Amit Kapila escribió:
> >
> > > The only point I can see against SET PERSISTENT is that other
> variants
> > of
> > > SET command can be used in
> > > transaction blocks means for them ROLLBACK TO SAVEPOINT
> functionality
> > works,
> > > but for SET PERSISTENT,
> > > it can't be done.
> > > So to handle that might be we need to mention this point in User
> > Manual, so
> > > that users can be aware of this usage.
> > > If that is okay, then I think SET PERSISTENT is good to go.
> >
> > I think that's okay. There are other commands which have some forms
> > that can run inside a transaction block and others not. CLUSTER is
> > one example (maybe the only one? Not sure).
>
> In that case, it can have one more advantage that all configuration
> setting
> can be done with one command
> and in future we might want to have option like BOTH where the command
> will
> take effect for memory as well
> as file.
>
> Can you think of any strong reason why not to have with Alter System
> Command?
>
> In any case SET PERSISTENT is fine.

If no objections to SET PERSISTENT .. syntax, I shall update the patch for
implementation of same.

With Regards,
Amit Kapila.

On Tuesday, November 20, 2012 7:21 PM Amit Kapila wrote:
On Monday, November 19, 2012 9:07 PM Amit Kapila wrote:
> On Monday, November 19, 2012 8:36 PM Alvaro Herrera wrote:
> > Amit Kapila escribió:
> >
> > > The only point I can see against SET PERSISTENT is that other
> variants
> > of
> > > SET command can be used in
> > > transaction blocks means for them ROLLBACK TO SAVEPOINT
> functionality
> > works,
> > > but for SET PERSISTENT,
> > > it can't be done.
> > > So to handle that might be we need to mention this point in User
> > Manual, so
> > > that users can be aware of this usage.
> > > If that is okay, then I think SET PERSISTENT is good to go.
> >
> > I think that's okay. There are other commands which have some forms
> > that can run inside a transaction block and others not. CLUSTER is
> > one example (maybe the only one? Not sure).
>

> If no objections to SET PERSISTENT .. syntax, I shall update the patch for
> implementation of same.

Patch to implement SET PERSISTENT command is attached with this mail.
Now it can be reviewed.

I have not update docs, as I want feedback about the behaviour of implementation, so that docs can be updated appropriately.

With Regards,
Amit Kapila.

On Thu, Nov 22, 2012 at 9:38 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> Patch to implement SET PERSISTENT command is attached with this mail.
> Now it can be reviewed.

I got the following compile warnings:
xact.c:1847: warning: implicit declaration of function
'AtEOXact_UpdateAutoConfFiles'
guc.c:5134: warning: enumeration value 'PGC_ENUM' not handled in switch

"SET PERSISTENT name TO value" failed, though "SET PERSISTENT name = value"
succeeded.

=# SET PERSISTENT synchronous_commit TO 'local';
ERROR: syntax error at or near "TO"
LINE 1: SET PERSISTENT synchronous_commit TO 'local';
=# SET PERSISTENT synchronous_commit = 'local';
SET

SET PERSISTENT succeeded even if invalid setting value was set.

=# SET PERSISTENT synchronous_commit = 'hoge';
SET

SET PERSISTENT syntax should be explained by "\help SET" psql command.

When I remove postgresql.auto.conf, SET PERSISTENT failed.

=# SET PERSISTENT synchronous_commit = 'local';
ERROR: failed to open "postgresql.auto.conf" file

We should implement "RESET PERSISTENT"? Otherwise, there is no way
to get rid of the parameter setting from postgresql.auto.conf, via SQL. Also
We should implement "SET PERSISTENT name TO DEFAULT"?

Is it helpful to output the notice message like 'Run pg_reload_conf() or
restart the server if you want new settings to take effect' always after
SET PERSISTENT command?

Regards,

--
Fujii Masao

On Thursday, November 22, 2012 10:09 PM Fujii Masao wrote:
> On Thu, Nov 22, 2012 at 9:38 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > Patch to implement SET PERSISTENT command is attached with this mail.
> > Now it can be reviewed.
>
> I got the following compile warnings:
> xact.c:1847: warning: implicit declaration of function
> 'AtEOXact_UpdateAutoConfFiles'
> guc.c:5134: warning: enumeration value 'PGC_ENUM' not handled in switch
>
> "SET PERSISTENT name TO value" failed, though "SET PERSISTENT name =
> value"
> succeeded.
>
> =# SET PERSISTENT synchronous_commit TO 'local';
> ERROR: syntax error at or near "TO"
> LINE 1: SET PERSISTENT synchronous_commit TO 'local';
> =# SET PERSISTENT synchronous_commit = 'local';
> SET
>
> SET PERSISTENT succeeded even if invalid setting value was set.
>
> =# SET PERSISTENT synchronous_commit = 'hoge';
> SET
>
> SET PERSISTENT syntax should be explained by "\help SET" psql command.

Thank you. I shall take care of above in next patch and do more test.
>
> When I remove postgresql.auto.conf, SET PERSISTENT failed.
>
> =# SET PERSISTENT synchronous_commit = 'local';
> ERROR: failed to open "postgresql.auto.conf" file

There can be 2 ways to handle this, either we recreate the
"postgresql.auto.conf" file or give error.
I am not sure if user tries to delete internal files, what should be exact
behavior?
Any suggestion?

> We should implement "RESET PERSISTENT"? Otherwise, there is no way
> to get rid of the parameter setting from postgresql.auto.conf, via SQL.
> Also
> We should implement "SET PERSISTENT name TO DEFAULT"?

Till now, I have not implemented this in patch, thinking that it can be done
as a 2nd part if basic stuff is ready.
However I think you are right without one of "RESET PERSISTENT" or "SET
PERSISTENT name TO DEFAULT", it is difficult for user
to get rid of parameter.
Will "SET PERSISTENT name TO DEFAULT" be sufficient or do you think both are
necessary, because RESET PERSISTENT also internally might need
to behave similarly.

For implementation of "SET PERSISTENT name TO DEFAULT", there can be 2 ways
1) Delete the entry from postgresql.auto.conf
2) Update the entry value in postgresql.auto.conf to default value

Incase we just do update, then user might not be able to modify
postgresql.conf manually once the command is executed.
So I think Delete is better option. Let me know if you think otherwise or
you have some other idea to achieve it.

> Is it helpful to output the notice message like 'Run pg_reload_conf() or
> restart the server if you want new settings to take effect' always after
> SET PERSISTENT command?

Not sure because if someone uses SET PERSISTENT command, he should know the
effect or behavior of same.
I think it's good to put this in UM along with "PERSISTENT" option
explanation.

With Regards,
Amit Kapila.

On Fri, Nov 23, 2012 at 6:56 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>> When I remove postgresql.auto.conf, SET PERSISTENT failed.
>>
>> =# SET PERSISTENT synchronous_commit = 'local';
>> ERROR: failed to open "postgresql.auto.conf" file
>
> There can be 2 ways to handle this, either we recreate the
> "postgresql.auto.conf" file or give error.
> I am not sure if user tries to delete internal files, what should be exact
> behavior?
> Any suggestion?

I prefer to recreate it.

$PGDATA/config_dir is specified in include_dir by default. Users might
create and remove the configuration files in that directory many times.
So I'm not surprised even if a user accidentally removes
postgresql.auto.conf in that directory. Also users might purposely remove
that file to reset all the settings by SET PERSISTENT. So I think that SET
PERSISTENT should handle the case where postgresql.auto.conf doesn't
exist.

We might be able to expect that postgresql.auto.conf is not deleted by
a user if it's in $PGDATA/global or base directory.

>> We should implement "RESET PERSISTENT"? Otherwise, there is no way
>> to get rid of the parameter setting from postgresql.auto.conf, via SQL.
>> Also
>> We should implement "SET PERSISTENT name TO DEFAULT"?
>
> Till now, I have not implemented this in patch, thinking that it can be done
> as a 2nd part if basic stuff is ready.
> However I think you are right without one of "RESET PERSISTENT" or "SET
> PERSISTENT name TO DEFAULT", it is difficult for user
> to get rid of parameter.
> Will "SET PERSISTENT name TO DEFAULT" be sufficient or do you think both are
> necessary, because RESET PERSISTENT also internally might need
> to behave similarly.
>
> For implementation of "SET PERSISTENT name TO DEFAULT", there can be 2 ways
> 1) Delete the entry from postgresql.auto.conf
> 2) Update the entry value in postgresql.auto.conf to default value

Both seems to be useful. I think that "SET ... TO DEFAULT" is suitable for
2), and "RESET PERSISTENT ..." is suitable for 1).

Another comment is:

What happens if the server crashes while SET PERSISTENT is writing the
setting to the file? A partial write occurs and restart of the server would fail
because of corrupted postgresql.auto.conf?

Regards,

--
Fujii Masao

On Friday, November 23, 2012 10:10 PM Fujii Masao wrote:
On Fri, Nov 23, 2012 at 6:56 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>> When I remove postgresql.auto.conf, SET PERSISTENT failed.
>>
>>> =# SET PERSISTENT synchronous_commit = 'local';
>>> ERROR: failed to open "postgresql.auto.conf" file
>
>> There can be 2 ways to handle this, either we recreate the
>> "postgresql.auto.conf" file or give error.
>> I am not sure if user tries to delete internal files, what should be exact
>> behavior?
>> Any suggestion?

> I prefer to recreate it.

>$PGDATA/config_dir is specified in include_dir by default. Users might
>create and remove the configuration files in that directory many times.
>So I'm not surprised even if a user accidentally removes
>postgresql.auto.conf in that directory. Also users might purposely remove
>that file to reset all the settings by SET PERSISTENT.

One of the suggestion in this mail chain was if above happens then at restart, it should display/log message.
However I think if such a situation happens then we should provide some mechanism to users so that the settings through
command should work.

> So I think that SET
>PERSISTENT should handle the case where postgresql.auto.conf doesn't
>exist.

If we directly create a file user might not be aware that his settings done in previous commands will not work.
So how about if we display NOTICE also when internally for SET PERSISTENT new file needs to be created.
Notice should suggest that the settings done by previous commands are lost due to manual deletion of internal file.

>We might be able to expect that postgresql.auto.conf is not deleted by
>a user if it's in $PGDATA/global or base directory.

So do you mean to say that if we don't find file in config_dir, then we should search in $PGDATA/global or base directory?
Can't we mandate to user that even if it is deleted, the file has to be only expected in config_dir.

>>> We should implement "RESET PERSISTENT"? Otherwise, there is no way
>>> to get rid of the parameter setting from postgresql.auto.conf, via SQL.
>>> Also
>>> We should implement "SET PERSISTENT name TO DEFAULT"?
>
>> Till now, I have not implemented this in patch, thinking that it can be done
>> as a 2nd part if basic stuff is ready.
>> However I think you are right without one of "RESET PERSISTENT" or "SET
>> PERSISTENT name TO DEFAULT", it is difficult for user
>> to get rid of parameter.
>> Will "SET PERSISTENT name TO DEFAULT" be sufficient or do you think both are
>> necessary, because RESET PERSISTENT also internally might need
>> to behave similarly.
>
>> For implementation of "SET PERSISTENT name TO DEFAULT", there can be 2 ways
>> 1) Delete the entry from postgresql.auto.conf
>> 2) Update the entry value in postgresql.auto.conf to default value

>Both seems to be useful. I think that "SET ... TO DEFAULT" is suitable for
>2), and "RESET PERSISTENT ..." is suitable for 1).

For other commands behavior for "SET ... TO DEFAULT" and "RESET ..." is same.
In the docs it is mentioned "RESET "is an alternative name for "SET ... TO DEFAULT"

However still the way you are telling can be done.
Others, any objection to it, else I will go ahead with it?

> Another comment is:

> What happens if the server crashes while SET PERSISTENT is writing the
> setting to the file? A partial write occurs and restart of the server would fail
> because of corrupted postgresql.auto.conf?

This situation will not happen as SET PERSISTENT command will first write to ".lock" file and then at commit time,
rename it to ".auto.conf".

With Regards,
Amit Kapila.

Amit kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> On Friday, November 23, 2012 10:10 PM Fujii Masao wrote:
>> What happens if the server crashes while SET PERSISTENT is writing the
>> setting to the file? A partial write occurs and restart of the server would fail
>> because of corrupted postgresql.auto.conf?

> This situation will not happen as SET PERSISTENT command will first write to ".lock" file and then at commit time,
> rename it to ".auto.conf".

Yes, the right way to write the config file is to write under a
temporary name, fsync the file, and then use rename(2) to atomically
move it into place. However, the above is contemplating some extra
complexity that I think is useless and undesirable, namely postponing
the rename until commit time. The point of the suggestion that SET
PERSISTENT not be allowed inside a transaction block is so that you can
write the file immediately rather than have to add commit-time mechanism
to support the feature. Aside from being extra complexity, and some
extra cycles added in *every single commit*, a post-commit write creates
another way to have post-commit failures, which we cannot cope with in
any sane way.

regards, tom lane

On Saturday, November 24, 2012 10:56 PM Tom Lane wrote:
Amit kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> On Friday, November 23, 2012 10:10 PM Fujii Masao wrote:
>>> What happens if the server crashes while SET PERSISTENT is writing the
>>> setting to the file? A partial write occurs and restart of the server would fail
>>> because of corrupted postgresql.auto.conf?

>> This situation will not happen as SET PERSISTENT command will first write to ".lock" file and then at commit time,
>> rename it to ".auto.conf".

>Yes, the right way to write the config file is to write under a
>temporary name, fsync the file, and then use rename(2) to atomically
>move it into place. However, the above is contemplating some extra
>complexity that I think is useless and undesirable, namely postponing
>the rename until commit time. The point of the suggestion that SET
>PERSISTENT not be allowed inside a transaction block is so that you can
>write the file immediately rather than have to add commit-time mechanism
>to support the feature.

Sure, I will update the code such that it should write the file immediately.
However for error cases, the temporary file will be deleted at abort time only rather than by using TRY..CATCH.

With Regards,
Amit Kapila.

On Sunday, November 25, 2012 10:31 AM Amit kapila wrote:
On Saturday, November 24, 2012 10:56 PM Tom Lane wrote:
Amit kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> On Friday, November 23, 2012 10:10 PM Fujii Masao wrote:
>>>> What happens if the server crashes while SET PERSISTENT is writing the
>>>> setting to the file? A partial write occurs and restart of the server would fail
>>>> because of corrupted postgresql.auto.conf?

>>> This situation will not happen as SET PERSISTENT command will first write to ".lock" file and then at commit time,
>>> rename it to ".auto.conf".

>>Yes, the right way to write the config file is to write under a
>>temporary name, fsync the file, and then use rename(2) to atomically
>>move it into place. However, the above is contemplating some extra
>>complexity that I think is useless and undesirable, namely postponing
>>the rename until commit time. The point of the suggestion that SET
>>PERSISTENT not be allowed inside a transaction block is so that you can
>>write the file immediately rather than have to add commit-time mechanism
>>to support the feature.

>Sure, I will update the code such that it should write the file immediately.
>However for error cases, the temporary file will be deleted at abort time only rather than by using TRY..CATCH.

The updated Patch with this mail contains following updation:

1. Fixed all problems reported.
2. Added syntax for the following.
1. Reset persistent config_parameter.
2. Reset persistent all
3. Modified the functionality of set default as get the default value for the configuration and convert into units (sec, MB and etc)
and add/rewrite configuration parameter in the postgresql.auto.conf.
4. Added the tests to the regression suite.
5. PERSISTENT Keyword is added to the reserved keyword list. As it was giving some errors given below while parsing gram.y
15 shift/reduce conflicts .
6. During set persistent command if the postgresql.auto.conf file not exists, then it creates the file and gives
a NOTICE message to user.
7. During restart of internal processes because of any backend crash time, if any .lock file presents, it will be deleted.
8. Gives a warning message to user, if the postgresql.auto.conf file is not present in postgresql.conf file.

With Regards,
Amit Kapila.

On Wed, Nov 28, 2012 at 9:47 AM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> 5. PERSISTENT Keyword is added to the reserved keyword list. As it was giving some errors given below while parsing gram.y
> 15 shift/reduce conflicts .

Allow me to be the first to say that any syntax for this feature that
involves reserving new keywords is a bad syntax.

The cost of an unreserved keyword is that the parser gets a little
bigger and slows down, but previous experimentation shows that the
effect is pretty small. However, adding a new reserved keyword breaks
user applications. It is hardly difficult to imagine that there are a
decent number of users who have columns or PL/pgsql variables called
"persistent". Let's not break them. Instead, since there were
multiple proposed syntaxes for this feature, let's just pick one of
the other ones that doesn't have this problem.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Nov 28, 2012 at 9:47 AM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>> 5. PERSISTENT Keyword is added to the reserved keyword list. As it was giving some errors given below while parsing gram.y
>> 15 shift/reduce conflicts .

> Allow me to be the first to say that any syntax for this feature that
> involves reserving new keywords is a bad syntax.

Let me put that a little more strongly: syntax that requires reserving
words that aren't reserved in the SQL standard is unacceptable.

Even if the new word is reserved according to SQL, we'll frequently
try pretty hard to avoid making it reserved in Postgres, so as not to
break existing applications. But if it's not in the standard then
you're breaking applications that can reasonably expect not to get
broken.

But having said that, it's not apparent to me why inventing SET
PERSISTENT should require reserving PERSISTENT. In the existing
syntaxes SET LOCAL and SET SESSION, there's not been a need to
reserve LOCAL or SESSION. Maybe you're just trying to be a bit
too cute in the grammar productions? Frequently there's more than
one way to do it and not all require the same level of keyword
reservedness.

regards, tom lane

On Saturday, December 01, 2012 1:30 AM Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, Nov 28, 2012 at 9:47 AM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> >> 5. PERSISTENT Keyword is added to the reserved keyword list. As it
> was giving some errors given below while parsing gram.y
> >> 15 shift/reduce conflicts .
>
> > Allow me to be the first to say that any syntax for this feature that
> > involves reserving new keywords is a bad syntax.
>
> Let me put that a little more strongly: syntax that requires reserving
> words that aren't reserved in the SQL standard is unacceptable.
>
> Even if the new word is reserved according to SQL, we'll frequently
> try pretty hard to avoid making it reserved in Postgres, so as not to
> break existing applications. But if it's not in the standard then
> you're breaking applications that can reasonably expect not to get
> broken.
>
> But having said that, it's not apparent to me why inventing SET
> PERSISTENT should require reserving PERSISTENT. In the existing
> syntaxes SET LOCAL and SET SESSION, there's not been a need to
> reserve LOCAL or SESSION. Maybe you're just trying to be a bit
> too cute in the grammar productions? Frequently there's more than
> one way to do it and not all require the same level of keyword
> reservedness.

The problem is due to RESET PERSISTENT configuration_variable Syntax.
I think the reason is that configuration_variable name can also be
persistent, so its not able to resolve.
I have tried quite a few ways. I shall try some more and send you result of
all.

If you have any idea or any hint where similar syntax is used, please point
me I will refer it.

Any other Suggestions?

With Regards,
Amit Kapila.

Amit Kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> On Saturday, December 01, 2012 1:30 AM Tom Lane wrote:
>> But having said that, it's not apparent to me why inventing SET
>> PERSISTENT should require reserving PERSISTENT.

> The problem is due to RESET PERSISTENT configuration_variable Syntax.
> I think the reason is that configuration_variable name can also be
> persistent, so its not able to resolve.

Well, that certainly looks like it should not be very difficult.

The secret to getting bison to do what you want is to not ask it to
make a shift/reduce decision too early. In this case I imagine you
are trying to do something like

RESET opt_persistent var_name

which cannot work if "persistent" could be a var_name, because bison
has to decide whether to reduce opt_persistent to PERSISTENT or empty
before it can see if there's anything after the var_name. So the fix
is to not use an opt_persistent production, but spell out both
alternatives:

RESET var_name

RESET PERSISTENT var_name

Now bison doesn't have to choose what to reduce until it can see the end
of the statement; that is, once it's scanned RESET and PERSISTENT, the
choice of whether to treat PERSISTENT as a var_name can be conditional
on whether the next token is a name or EOL.

But even if we can't make that work, it's not grounds for reserving
PERSISTENT. Instead I'd be inclined to forget about "RESET PERSISTENT"
syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that.
(BTW, I wonder what behavior that syntax has now in your patch.)

regards, tom lane

I wrote:
> But even if we can't make that work, it's not grounds for reserving
> PERSISTENT. Instead I'd be inclined to forget about "RESET PERSISTENT"
> syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that.
> (BTW, I wonder what behavior that syntax has now in your patch.)

In fact, rereading this, I wonder why you think "RESET PERSISTENT"
is a good idea even if there were no bison issues with it. We don't
write RESET LOCAL or RESET SESSION, so it seems asymmetric to have
RESET PERSISTENT.

regards, tom lane

On Saturday, December 01, 2012 10:00 PM Tom Lane wrote:
Amit Kapila <amit(dot)kapila(at)huawei(dot)com> writes :
> On Saturday, December 01, 2012 1:30 AM Tom Lane wrote:
>>> But having said that, it's not apparent to me why inventing SET
>>> PERSISTENT should require reserving PERSISTENT.

>> The problem is due to RESET PERSISTENT configuration_variable Syntax.
>> I think the reason is that configuration_variable name can also be
>> persistent, so its not able to resolve.

> But even if we can't make that work, it's not grounds for reserving
> PERSISTENT. Instead I'd be inclined to forget about "RESET PERSISTENT"
>syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that.
> (BTW, I wonder what behavior that syntax has now in your patch.)

The current behavior is
1. "RESET PERSISTENT ..." will delete the entry from postgresql.auto.conf
2. "SET PERSISTENT... TO DEFAULT" will update the entry value in postgresql.auto.conf to default value

The discussion for this is at below link:
http://archives.postgresql.org/pgsql-hackers/2012-11/msg01276.php

I am not too keen to have RESET Persistent, but the only point of keeping it was that it can give user flexibility that it
can have the values to take effect from postgresql.conf even if user has executed SET Persistent.
Apart from this also user can do that by putting the configuration variable after include_dir in postgresql.conf.

With Regards,
Amit Kapila.

On Saturday, December 01, 2012 10:15 PM Tom Lane wrote:
I wrote:
>> But even if we can't make that work, it's not grounds for reserving
>> PERSISTENT. Instead I'd be inclined to forget about "RESET PERSISTENT"
>> syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that.
>> (BTW, I wonder what behavior that syntax has now in your patch.)

> In fact, rereading this, I wonder why you think "RESET PERSISTENT"
> is a good idea even if there were no bison issues with it. We don't
> write RESET LOCAL or RESET SESSION, so it seems asymmetric to have
> RESET PERSISTENT.

Currently RESET will reset both local and session specific parameter value on commit.
I am not sure if we can change the persistent value with current RESET command.
However as you said if we introduce PERSISTENT it will be asymmetric as per current specification of RESET command,
so we can even let RESET behavior as it is and user will have mechanism to change default value only with
SET PERSISTENT var_name TO DEFAULT.

With Regards,
Amit Kapila.

On Saturday, December 01, 2012 10:00 PM Tom Lane wrote:
> Amit Kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> > On Saturday, December 01, 2012 1:30 AM Tom Lane wrote:
> which cannot work if "persistent" could be a var_name, because bison
> has to decide whether to reduce opt_persistent to PERSISTENT or empty
> before it can see if there's anything after the var_name. So the fix
> is to not use an opt_persistent production, but spell out both
> alternatives:
>
> RESET var_name
>
> RESET PERSISTENT var_name
>
> Now bison doesn't have to choose what to reduce until it can see the end
> of the statement; that is, once it's scanned RESET and PERSISTENT, the
> choice of whether to treat PERSISTENT as a var_name can be conditional
> on whether the next token is a name or EOL.

With the above suggestion also, it's not able to resolve shift/reduce
errors.

However I have tried with below changes in gram.y, it works after below
change.

%left PERSISTENT

VariableResetStmt:
RESET opt_persistent reset_common
{
VariableSetStmt *n = $3;
n->is_persistent = $2;
$$ = (Node *) n;
}
;

opt_persistent: PERSISTENT { $$ = TRUE; }
| /*EMPTY*/ %prec Op { $$ = FALSE; }
;

I am not sure if there are any problems with above change.
Found one difference with the change is, the command "reset persistent"
execution results in different errors with/without change.

without change:
unrecognized configuration parameter "persistent".
with change:
syntax error at or near ";"

As per your yesterday's mail, it seems to me you are disinclined to have
RESET PERSISTENT syntax.
Can we conclude on that point? My only worry is user will have no way to
delete the entry from .auto.conf file.

Suggestions?

With Regards,
Amit Kapila.

On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> But even if we can't make that work, it's not grounds for reserving
>> PERSISTENT. Instead I'd be inclined to forget about "RESET PERSISTENT"
>> syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that.
>> (BTW, I wonder what behavior that syntax has now in your patch.)
>
> In fact, rereading this, I wonder why you think "RESET PERSISTENT"
> is a good idea even if there were no bison issues with it. We don't
> write RESET LOCAL or RESET SESSION, so it seems asymmetric to have
> RESET PERSISTENT.

I think this feature is more analagous to ALTER DATABASE .. SET or
ALTER ROLE .. SET. Which is, incidentally, another reason I don't
like SET PERSISTENT as a proposed syntax. But even if we stick with
that syntax, it feels weird to have an SQL command to put a line into
postgresql.conf.auto and no syntax to take it back out again. We
wouldn't allow a CREATE command without a corresponding DROP
operation...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Dec 3, 2012 at 6:38 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Saturday, December 01, 2012 10:00 PM Tom Lane wrote:
>> Amit Kapila <amit(dot)kapila(at)huawei(dot)com> writes:
>> > On Saturday, December 01, 2012 1:30 AM Tom Lane wrote:
>> which cannot work if "persistent" could be a var_name, because bison
>> has to decide whether to reduce opt_persistent to PERSISTENT or empty
>> before it can see if there's anything after the var_name. So the fix
>> is to not use an opt_persistent production, but spell out both
>> alternatives:
>>
>> RESET var_name
>>
>> RESET PERSISTENT var_name
>>
>> Now bison doesn't have to choose what to reduce until it can see the end
>> of the statement; that is, once it's scanned RESET and PERSISTENT, the
>> choice of whether to treat PERSISTENT as a var_name can be conditional
>> on whether the next token is a name or EOL.
>
> With the above suggestion also, it's not able to resolve shift/reduce
> errors.
>
> However I have tried with below changes in gram.y, it works after below
> change.
>
> %left PERSISTENT
>
> VariableResetStmt:
> RESET opt_persistent reset_common
> {
> VariableSetStmt *n = $3;
> n->is_persistent = $2;
> $$ = (Node *) n;
> }
> ;
>
> opt_persistent: PERSISTENT { $$ = TRUE; }
> | /*EMPTY*/ %prec Op { $$ = FALSE; }
> ;
>
> I am not sure if there are any problems with above change.

We usually try to avoid operator precedence declarations. They
sometimes have unforeseen consequences.

> Found one difference with the change is, the command "reset persistent"
> execution results in different errors with/without change.
>
> without change:
> unrecognized configuration parameter "persistent".
> with change:
> syntax error at or near ";"

...but this in itself doesn't seem like a problem.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> But even if we can't make that work, it's not grounds for reserving
>>> PERSISTENT. Instead I'd be inclined to forget about "RESET PERSISTENT"
>>> syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that.
>>> (BTW, I wonder what behavior that syntax has now in your patch.)
>>
>> In fact, rereading this, I wonder why you think "RESET PERSISTENT"
>> is a good idea even if there were no bison issues with it. We don't
>> write RESET LOCAL or RESET SESSION, so it seems asymmetric to have
>> RESET PERSISTENT.

> I think this feature is more analagous to ALTER DATABASE .. SET or
> ALTER ROLE .. SET. Which is, incidentally, another reason I don't
> like SET PERSISTENT as a proposed syntax. But even if we stick with
> that syntax, it feels weird to have an SQL command to put a line into
> postgresql.conf.auto and no syntax to take it back out again.

Neither of you have responded to the point about what "SET PERSISTENT
var_name TO DEFAULT" will do, and whether it is or should be different
from RESET PERSISTENT, and if not why we should put the latter into
the grammar as well.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Dec 3, 2012 at 6:38 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>> opt_persistent: PERSISTENT { $$ = TRUE; }
>> | /*EMPTY*/ %prec Op { $$ = FALSE; }
>> ;
>>
>> I am not sure if there are any problems with above change.

> We usually try to avoid operator precedence declarations. They
> sometimes have unforeseen consequences.

Yes. This is not an improvement over factoring out opt_persistent as
I recommended previously.

>> Found one difference with the change is, the command "reset persistent"
>> execution results in different errors with/without change.
>>
>> without change:
>> unrecognized configuration parameter "persistent".
>> with change:
>> syntax error at or near ";"

> ...but this in itself doesn't seem like a problem.

Indeed, this demonstrates why kluging the behavior this way isn't a good
solution. If PERSISTENT is an unreserved word, then you *should* get
the former error, because it's a perfectly valid interpretation of the
command. If you get the latter then PERSISTENT is not acting like an
unreserved word.

regards, tom lane

On 12/03/2012 10:32 AM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Dec 3, 2012 at 6:38 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>>> opt_persistent: PERSISTENT { $$ = TRUE; }
>>> | /*EMPTY*/ %prec Op { $$ = FALSE; }
>>> ;
>>>
>>> I am not sure if there are any problems with above change.
>> We usually try to avoid operator precedence declarations. They
>> sometimes have unforeseen consequences.
> Yes. This is not an improvement over factoring out opt_persistent as
> I recommended previously.

This is by no means the first time this has come up. See
<http://wiki.postgresql.org/wiki/Fixing_shift/reduce_conflicts_in_Bison>

cheers

andrew

On Monday, December 03, 2012 8:59 PM Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> But even if we can't make that work, it's not grounds for reserving
> >>> PERSISTENT. Instead I'd be inclined to forget about "RESET
> > I think this feature is more analagous to ALTER DATABASE .. SET or
> > ALTER ROLE .. SET. Which is, incidentally, another reason I don't
> > like SET PERSISTENT as a proposed syntax. But even if we stick with
> > that syntax, it feels weird to have an SQL command to put a line into
> > postgresql.conf.auto and no syntax to take it back out again.
>
> Neither of you have responded to the point about what "SET PERSISTENT
> var_name TO DEFAULT" will do, and whether it is or should be different
> from RESET PERSISTENT, and if not why we should put the latter into
> the grammar as well.

I have mentioned this in my previous mail but may be it has some problem
http://archives.postgresql.org/pgsql-hackers/2012-12/msg00062.php

The current behavior is
1. "RESET PERSISTENT ..." will delete the entry from postgresql.auto.conf
2. "SET PERSISTENT... TO DEFAULT" will update the entry value in
postgresql.auto.conf to default value

However we can even change "SET PERSISTENT... TO DEFAULT" to delete the
entry and then we can avoid "RESET PERSISTENT ..."

Opinions?

With Regards,
Amit Kapila.

On Monday, December 03, 2012 8:50 PM Robert Haas wrote:
> On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > I wrote:
> >> But even if we can't make that work, it's not grounds for reserving
> >> PERSISTENT. Instead I'd be inclined to forget about "RESET
> PERSISTENT"
> >> syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that.
> >> (BTW, I wonder what behavior that syntax has now in your patch.)
> >
> > In fact, rereading this, I wonder why you think "RESET PERSISTENT"
> > is a good idea even if there were no bison issues with it. We don't
> > write RESET LOCAL or RESET SESSION, so it seems asymmetric to have
> > RESET PERSISTENT.
>
> I think this feature is more analagous to ALTER DATABASE .. SET or
> ALTER ROLE .. SET. Which is, incidentally, another reason I don't
> like SET PERSISTENT as a proposed syntax. But even if we stick with
> that syntax, it feels weird to have an SQL command to put a line into
> postgresql.conf.auto and no syntax to take it back out again. We
> wouldn't allow a CREATE command without a corresponding DROP
> operation...

Current implementation of "SET PERSISTENT... TO DEFAULT" will update the
entry value in postgresql.auto.conf to default value.

How about if make the behavior of "SET PERSISTENT... TO DEFAULT" such that
it will delete the entry in postgresql.auto.conf?

With Regards,
Amit Kapila.

On Tuesday, December 04, 2012 8:37 AM Amit Kapila wrote:
> On Monday, December 03, 2012 8:59 PM Tom Lane wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > > On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >>> But even if we can't make that work, it's not grounds for
> reserving
> > >>> PERSISTENT. Instead I'd be inclined to forget about "RESET
> > > I think this feature is more analagous to ALTER DATABASE .. SET or
> > > ALTER ROLE .. SET. Which is, incidentally, another reason I don't
> > > like SET PERSISTENT as a proposed syntax. But even if we stick with
> > > that syntax, it feels weird to have an SQL command to put a line
> into
> > > postgresql.conf.auto and no syntax to take it back out again.
> >
> > Neither of you have responded to the point about what "SET PERSISTENT
> > var_name TO DEFAULT" will do, and whether it is or should be different
> > from RESET PERSISTENT, and if not why we should put the latter into
> > the grammar as well.
>
>
> The current behavior is
> 1. "RESET PERSISTENT ..." will delete the entry from
> postgresql.auto.conf
> 2. "SET PERSISTENT... TO DEFAULT" will update the entry value in
> postgresql.auto.conf to default value
>
> However we can even change "SET PERSISTENT... TO DEFAULT" to delete the
> entry and then we can avoid "RESET PERSISTENT ..."

As per my understanding from the points raised by you, the behavior could be
defined as follows:

1. No need to have "RESET PERSISTENT ..." syntax.
2. It is better if we provide a way to delete entry which could be done for
syntax:
"SET PERSISTENT... TO DEFAULT"

If you don't have any objections, I will update the patch as per above 2
points.

With Regards,
Amit Kapila.

On Fri, Nov 23, 2012 at 4:56 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Thursday, November 22, 2012 10:09 PM Fujii Masao wrote:
>
>> Is it helpful to output the notice message like 'Run pg_reload_conf() or
>> restart the server if you want new settings to take effect' always after
>> SET PERSISTENT command?
>
> Not sure because if someone uses SET PERSISTENT command, he should know the
> effect or behavior of same.
> I think it's good to put this in UM along with "PERSISTENT" option
> explanation.
>

can we at least send the source file in the error message when a
parameter has a wrong value?

suppose you do: SET PERSISTENT shared_preload_libraries TO
'some_nonexisting_lib';
when you restart postgres and that lib doesn't exist you can get
confused when looking at postgresql.conf and find an empty string
there

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157

On Tuesday, December 11, 2012 5:25 AM Jaime Casanova wrote:
> On Fri, Nov 23, 2012 at 4:56 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > On Thursday, November 22, 2012 10:09 PM Fujii Masao wrote:
> >
> >> Is it helpful to output the notice message like 'Run pg_reload_conf()
> or
> >> restart the server if you want new settings to take effect' always
> after
> >> SET PERSISTENT command?
> >
> > Not sure because if someone uses SET PERSISTENT command, he should
> know the
> > effect or behavior of same.
> > I think it's good to put this in UM along with "PERSISTENT" option
> > explanation.
> >
>
> can we at least send the source file in the error message when a
> parameter has a wrong value?
>
> suppose you do: SET PERSISTENT shared_preload_libraries TO
> 'some_nonexisting_lib';
> when you restart postgres and that lib doesn't exist you can get
> confused when looking at postgresql.conf and find an empty string
> there

This can be done but for this we need to change internal functions
internal_load_library
initialize_SSL
postmastermain

The same information needs to be provided for some other parameters also
like 'listen_address', etc.
which are not validated during the configuration parameter set.

How about giving some Warning/Notice message for all postmaster specific
configuration parameters during set persistent as if any problem occurs
during restart please verify the postgresql.auto.conf file?
Also Provide information in User manual as how to recover from such problems
occurs because of postgresql.auto.conf?

With Regards,
Amit Kapila.

Amit Kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> On Tuesday, December 11, 2012 5:25 AM Jaime Casanova wrote:
>> can we at least send the source file in the error message when a
>> parameter has a wrong value?
>>
>> suppose you do: SET PERSISTENT shared_preload_libraries TO
>> 'some_nonexisting_lib';
>> when you restart postgres and that lib doesn't exist you can get
>> confused when looking at postgresql.conf and find an empty string
>> there

> This can be done but for this we need to change internal functions

You'd need to change a whole lot of places to respond to this request.
Frankly it sounds like wishful thinking to me. In many cases it's not
obvious whether a failure might be triggered by a GUC setting, and I
can't see dumping the whole content of postgresql.conf for any startup
failure.

I don't really believe Jaime's assumption anyway, which seems to be that
people will be too dumb to look at the include files when wondering what
value a setting has. We've not had reports of such when using the
existing inclusion mechanism.

regards, tom lane