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

<div class="WordSection1"><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">SYNTAX:</span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">ALTERSYSTEM SET configuration_parameter = value COMMENT
'value';</span><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><p
class="MsoNormal"><spanstyle="font-size:11.0pt;font-family:"Calibri","sans-serif"">DESIGN IDEA: </span><p
class="MsoNormal"><spanstyle="font-size:11.0pt;font-family:"Calibri","sans-serif"">(a) have a postgresql.conf.auto
</span><pclass="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">(b) add a default include
forpostgresql.conf.auto at the beginning of PostgreSQL.conf </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">(c)SQL updates go to postgresql.conf.auto, which consists
onlyof"setting = value #comments" . </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">(d)We document that settings which are changed manually in
postgresql.confwill override postgresql.conf.auto. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">IMPLEMENTATIONIDEA: </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Themain Idea is we create a lock file, it acts as lock to
avoidconcurrent edit into .conf auto file </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">andalso as an intermediate file where we keep all the new
changesuntil we commit the alter system command.         </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">CCREATIONOF  AUTO FILE </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">1.during initdb we create the .auto file and it will be
empty.</span><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">2. .conf file will
haveits first entry as follows </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">#------------------------------------------------------------------------------
</span><pclass="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""># Postgresql.conf.auto
inclusion</span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">#------------------------------------------------------------------------------
</span><pclass="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""># Do not edit
postgresql.conf.autofile or remove the include. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">#You can Edit the settings below in this file which will
overrideauto-generated file.</span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">include= 'postgresql.conf.auto' </span><p
class="MsoNormal"><spanstyle="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">   </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">      </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">ALGORITHMfor ALTER SYSTEM:         </span><p
class="MsoNormal"><spanstyle="font-size:11.0pt;font-family:"Calibri","sans-serif"">        1. check whether the given
key: value is valid. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                --This is done so that next read from .auto
fileshould not throw error. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">        2.get postgresql.conf.auto path. (always the data
directory)</span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                --Since the .auto file in data directory
pg_basebackupwill pick it up. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">        3.Create the postgresql.conf.auto.lock file( with
O_EXCLflag). </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                --This act as a protection from other
backendswho are trying to edit this file. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                --If already exist we wait for some time by
retrying.</span><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">         4. Open
thepostgresql.conf.auto file in read mode.      </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">         5.Write the new (key, value, comment) in to the
postgresql.conf.auto.lockfile by using below steps: </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">           a.read the contents of postgresql.conf.auto in
tomemory buffer line by line.         </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">           b.Scan for key in postgresql.conf.auto file.
</span><pclass="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                  if found
getthe line number in file such that where we have to insert the new (key,value). </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                  elsewe should write the new (key, value)
pairto last line.           </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">           c.add the new (key, value, comment) to memory
bufferto the line as found in step b. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">           d.Write the memory buffer into
postgresql.conf.auto.lockfile.   </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                --here memory buffer  represent the
modifiedstate of the postgresql.conf.auto file. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">           e.Commit the .lock file. </span><p
class="MsoNormal"><spanstyle="font-size:11.0pt;font-family:"Calibri","sans-serif"">                 -- Here rename the
lockfile to auto file. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                 --If auto file is opened by other process
(SIGHUPprocessing) then we retry rename for some time </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                   otherwise alter system command
fails.                                </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">           f.If any error in between rollback lock file
</span><pclass="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                -- here
deletethe lock file. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">                </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">CLARIFICATION</span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">1.Tom, the below is mentioned by you in one of the
discussionsfor this topic. I need small clarification: </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">   "Aboutthe only change I want to make immediately is that
initdbought to shove its settings into postgresql.auto instead of mucking with </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">postgresql.conf."</span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">   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 </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">    parametersbe commented in .conf? </span><p
class="MsoNormal"><spanstyle="font-size:11.0pt;font-family:"Calibri","sans-serif"">2. Do .auto file needs to be
includedby default?</span><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">3. Can
thepath of .auto be fixed as data directory path? </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Note:</span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">1.Only One backend can edit conf file at a time others
wait.</span><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">2. Suppose .auto
haveinvalid entry eg: listening port number mentioned is taken up by other application </span><p
class="MsoNormal"><spanstyle="font-size:11.0pt;font-family:"Calibri","sans-serif"">   then if we try to restart the
postgresit fails. This need manual intervention. </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">3.This command cannot be executed inside the transaction
block.Not sure what to do for this part, whether it needs to be supported </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">   ina block?</span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">4.currently command for reset or invalidation of (key,
value)is not implemented.   </span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Comments/Suggestionsabout the value of this feature and
ImplementationIdea?</span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">WithRegards,</span><p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif"">AmitKapila.</span></div> 

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
tolookup in in postgresql.conf is actually hiding in the .auto file. Assuming only super users/sysadmins would have the
abilityto change things in the config file, wouldn't they be more likely to just do it on the server and edit the .conf
(whichamong 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@agliodbs.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
tolookup in in postgresql.conf is actually hiding in the .auto file. Assuming only super users/sysadmins would have the
abilityto change things in the config file, wouldn't they be more likely to just do it on the server and edit the .conf
(whichamong 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 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.

Christopher Browne escribió:
> On Tue, Oct 30, 2012 at 5:25 PM, Josh Berkus <josh@agliodbs.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
expectto lookup in in postgresql.conf is actually hiding in the .auto file. Assuming only super users/sysadmins would
havethe 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@agliodbs.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: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@sss.pgh.pa.us> wrote:
> Josh Berkus <josh@agliodbs.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 HaganderMe: 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,
fromwhere 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
valueof 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@sss.pgh.pa.us> wrote:
> Josh Berkus <josh@agliodbs.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
configurationvalues 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
notsure 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
similarto 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_configureto display or change server-level settings. To change database-level settings, use ALTER DATABASE.    To
changesettings 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
problemsor 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@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On Fri, Nov 2, 2012 at 1:19 AM, Greg Smith <greg@2ndquadrant.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@2ndquadrant.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,
Ithink 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
anychange 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
theabove 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
isgetting executed  might have some old values.      The key point is how backend can get the latest config values
withoutreading .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,
itcan 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@hagander.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
editthe 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@hagander.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.kapila@huawei.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@hagander.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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Wed, Nov 7, 2012 at 5:19 AM, Amit Kapila <amit.kapila@huawei.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@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Wed, Nov 7, 2012 at 5:19 AM, Amit Kapila <amit.kapila@huawei.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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Wed, Nov 7, 2012 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Wed, Nov 7, 2012 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Wed, Nov 7, 2012 at 6:19 PM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Wed, Nov 7, 2012 at 6:19 PM, Tom Lane <tgl@sss.pgh.pa.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@gmail.com> writes:
> On Wed, Nov 7, 2012 at 12:29 PM, Tom Lane <tgl@sss.pgh.pa.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@agliodbs.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@agliodbs.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@2ndQuadrant.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@agliodbs.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
thechanges to auto.lock file.   c. at the end of command rename the auto.lock file to .auto file   d. otherwise if SQL
COMMAND/functionfailed 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@gmail.com> writes:
> > On Wed, Nov 7, 2012 at 12:29 PM, Tom Lane <tgl@sss.pgh.pa.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@agliodbs.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@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

Greg Smith <greg@2ndQuadrant.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
inmail 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
writtenwithout 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@agliodbs.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@sss.pgh.pa.us> wrote:
> Josh Berkus <josh@agliodbs.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.kapila@huawei.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-1in 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
directlywritten 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.kapila@huawei.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
hasdetailed 
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
elseadd 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
littletricky, 
    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
thinkwe 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.kapila@huawei.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
valuefor 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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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@2ndquadrant.com]
> Sent: Friday, November 16, 2012 1:55 PM
> To: pgsql-hackers@postgresql.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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
triggerexecution) 
"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.kapila@huawei.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.fujii@gmail.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.fujii@gmail.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.kapila@huawei.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
operatingon file as well.
 

With Regards,
Amit Kapila.

On Sunday, November 18, 2012 3:28 AM Tom Lane wrote:
> Fujii Masao <masao.fujii@gmail.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.fujii@gmail.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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@gmail.com> writes:
> On Wed, Nov 28, 2012 at 9:47 AM, Amit kapila <amit.kapila@huawei.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@gmail.com> writes:
> > On Wed, Nov 28, 2012 at 9:47 AM, Amit kapila <amit.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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*/
%precOp        { $$ = 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@sss.pgh.pa.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.kapila@huawei.com> wrote:
> On Saturday, December 01, 2012 10:00 PM Tom Lane wrote:
>> Amit Kapila <amit.kapila@huawei.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@gmail.com> writes:
> On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.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@gmail.com> writes:
> On Mon, Dec 3, 2012 at 6:38 AM, Amit Kapila <amit.kapila@huawei.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@gmail.com> writes:
>> On Mon, Dec 3, 2012 at 6:38 AM, Amit Kapila <amit.kapila@huawei.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@gmail.com> writes:
> > On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.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@gmail.com> writes:
> > > On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.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.kapila@huawei.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.kapila@huawei.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.kapila@huawei.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

P {MARGIN-TOP: 0px; MARGIN-BOTTOM: 0px }

On Thu, 6 Dec 2012 10:12:31 +0530 Amit Kapila wrote:
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:
>> > 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"

Updated patch to handle above 2 points.

 

With Regards,

Amit Kapila.

• Is the patch in context diff format?

Yes. (But with PostgreSQL using GIT, and most developers are
now comfortable with the unified diff format, this question should
not be in the wiki any more...)

• Does it apply cleanly to the current git master?

Not quite cleanly, it gives some offset warnings:

[zozo@localhost postgresql.1]$ cat ../set_persistent_v3.patch | patch -p1
patching file doc/src/sgml/ref/set.sgml
patching file src/backend/nodes/copyfuncs.c
patching file src/backend/nodes/equalfuncs.c
patching file src/backend/parser/gram.y
patching file src/backend/postmaster/postmaster.c
Hunk #1 succeeded at 2552 (offset -4 lines).
patching file src/backend/replication/basebackup.c
Hunk #1 succeeded at 736 (offset 155 lines).
patching file src/backend/utils/misc/guc-file.l
patching file src/backend/utils/misc/guc.c
Hunk #1 succeeded at 3348 (offset -13 lines).
Hunk #2 succeeded at 4125 (offset -13 lines).
Hunk #3 succeeded at 5108 (offset -13 lines).
Hunk #4 succeeded at 6090 (offset -13 lines).
Hunk #5 succeeded at 6657 (offset -13 lines).
Hunk #6 succeeded at 6742 (offset -13 lines).
patching file src/backend/utils/misc/postgresql.conf.sample
patching file src/bin/initdb/initdb.c
patching file src/include/nodes/parsenodes.h
patching file src/include/parser/kwlist.h
patching file src/include/utils/guc.h
patching file src/include/utils/guc_tables.h
patching file src/test/regress/expected/persistent.out
patching file src/test/regress/parallel_schedule
patching file src/test/regress/serial_schedule
patching file src/test/regress/sql/persistent.sql

There are no "fuzz" warnings though, so rebase is not needed.

• Does it include reasonable tests, necessary doc patches, etc?

Yes, it contains documentation, but it will surely need proofreading.
I noticed some typos in it already but the wording seems to be clear.

It also contains an extensive set of regression tests.

One specific comment about the documentation part of the patch:

***************
*** 86,92 **** SET [ SESSION | LOCAL ] TIME ZONE { <replaceable class="PARAMETER">timezone</rep
      <application>PL/pgSQL</application> exception block.  This behavior
      has been changed because it was deemed unintuitive.
     </para>
!   </note>
   </refsect1>
 
   <refsect1>
--- 95,101 ----
      <application>PL/pgSQL</application> exception block.  This behavior
      has been changed because it was deemed unintuitive.
     </para>
!    </note>
   </refsect1>
 
   <refsect1>
***************

• Does the patch actually implement that?

Yes.

• Do we want that?

Yes.

• Do we already have it?

No.

• Does it follow SQL spec, or the community-agreed behavior?

No such SQL spec. It was implemented according to the discussion.

It uses a single file in an extra configuration directory added to
postgresql.conf as:

# This includes the default configuration directory included to support
# SET PERSISTENT statement.
#
# WARNNING: Any configuration parameter values specified after this line
#           will override the values set by SET PERSISTENT statement.
include_dir = 'config_dir'

Note the typo in the above message: WARNNING, there are too many Ns.

The extra configuration file ($PGDATA/config_dir/postgresql.auto.conf)
is loaded automatically. I tested it by setting shared_buffers and work_mem.
Executing "SELECT pg_reload_conf();" changed work_mem and left these
messages in the server log:

LOG:  parameter "shared_buffers" cannot be changed without restarting the server
LOG:  configuration file "/home/zozo/pgc93dev-setpersistent/data/postgresql.conf" contains errors; unaffected changes were applied

Just as if I manually edited postgresql.conf. It works as it should.

According to the discussion, SET PERSISTENT cannot be executed inside
a transaction block, it's implemented and covered by the regression test.

• Does it include pg_dump support (if applicable)?

Not applicable. The $PGDATA/config_dir (and subsequently the
postgresql.auto.conf file) is included in binary dumps via pg_basebackup.

• Are there dangers?

The patch is about modifying the server configuration so there is
some danger in creating a configuration that won't start up the server

on a given machine, but you can do such a thing manually too today.
Also, SET PERSISTENT requires database superuser. If an attacker can

gain superuser privileges, you lost anyway. You can destroy a database

easily with creative uses of COPY TO.

• Have all the bases been covered?

It seems so. The regression test contains a lot of tests
for error cases, but there are no test cases for executing
SET PERSISTENT for an unknown GUC (fails as it should) and
executing SET PERSISTENT in a function inside a transaction
(also fails as it should).

• Does the feature work as advertised?

Yes.

• Are there corner cases the author has failed to consider?

I don't think so, the mentioned corner cases are also handled.

• Are there any assertion failures or crashes?

No. (I compiled the server with --enable-cassert)

• Does the patch slow down simple tests?

No.

• If it claims to improve performance, does it?

It's not a performance patch.

• Does it slow down other things?

No.

• Does it follow the project coding guidelines?

Yes.

• Are there portability issues?

No. I have seen an added chmod() call in setup_config()

in initdb.c but is's used already there so it should be ok.

• Will it work on Windows/BSD etc?

It should.

• Are the comments sufficient and accurate?

I think so.

• Does it do what it says, correctly?

I think so.

• Does it produce compiler warnings?

No.

• Can you make it crash?

No.

• Is everything done in a way that fits together coherently with other features/modules?

Yes.

• Are there interdependencies that can cause problems?

No.

Some more comments:

*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
***************
*** 581,586 **** sendDir(char *path, int basepathlen, bool sizeonly)
--- 581,592 ----
                                        strlen(PG_TEMP_FILE_PREFIX)) == 0)
                        continue;
 
+               /* skip lock files used in postgresql.auto.conf edit */
+               if (strncmp(de->d_name,
+                                       "postgresql.auto.conf.lock",
+                                       strlen("postgresql.auto.conf.lock")) == 0)
+                       continue;
+
                /*
                 * If there's a backup_label file, it belongs to a backup started by
                 * the user with pg_start_backup(). It is *not* correct for this
***************

Since you are using a constant string, it would be a little faster
to use "sizeof(string)-1" as it can be computed at compile-time
and not run the strlen() all the time this code is reached.
Something like this below:

#define PGAUTOCONFLOCK    "postgresql.auto.conf.lock"

+               /* skip lock files used in postgresql.auto.conf edit */
+               if (strncmp(de->d_name,
+                                       PGAUTOCONFLOCK,
+                                       sizeof(PGAUTOCONFLOCK)-1) == 0)

In create_conf_lock_file():

+ static int
+ create_conf_lock_file(const char *LockFileName)
+ {
+       int                     fd;
+       int                     ntries;
+
+       /*
+        * We need a loop here because of race conditions. Retry for 100 times.
+        */
+       for (ntries = 0;; ntries++)
+       {
+               fd = open(LockFileName, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
+               if (fd >= 0)
+                       break;
+
+               /*
+                * Couldn't create the lock file. Probably it already exists. If so
+                * wait for some time
+                */
+               if ((errno != EEXIST && errno != EACCES) || ntries > 100)
+               {
...
+               }
+               else
+               {
+                       pg_usleep(100000);      /* in total wait for 10sec */
+               }
+       }
+
+       return fd;
+ }

Can't we add a new LWLock and use it as a critical section instead
of waiting for 10 seconds? It would be quite surprising to wait
10 seconds  when accidentally executing two SET PERSISTENT
statements in parallel. Something like the attached patch against
set_persistent_v3.patch. The pg_usleep(15000000) in the patch
is intentional (as the comment says) to allow enough time enter
and run two or more SET PERSISTENT statements.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de    http://www.postgresql.at/

<div class="moz-cite-prefix">2013-01-04 18:27 keltezéssel, Boszormenyi Zoltan írta:<br /></div><blockquote
cite="mid:50E71107.9050205@cybertec.at"type="cite"><p>One specific comment about the documentation part of the
patch:<br/><p><br /><p>***************<br /> *** 86,92 **** SET [ SESSION | LOCAL ] TIME ZONE { <replaceable
class="PARAMETER">timezone</rep<br/>       <application>PL/pgSQL</application> exception block.  This
behavior<br/>       has been changed because it was deemed unintuitive.<br />      </para><br /> !  
</note><br/>    </refsect1><br />   <br />    <refsect1><br /> --- 95,101 ----<br />      
<application>PL/pgSQL</application>exception block.  This behavior<br />       has been changed because it
wasdeemed unintuitive.<br />      </para><br /> !    </note><br />    </refsect1><br />   <br />   
<refsect1><br/> ***************<br /><br /></blockquote><br /> I forgot to add the actual comment: this is an
unnecessarywhitespace change.<br /><br /><pre class="moz-signature" cols="90">-- 
 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: <a class="moz-txt-link-freetext" href="http://www.postgresql-support.de">http://www.postgresql-support.de</a>
<aclass="moz-txt-link-freetext" href="http://www.postgresql.at/">http://www.postgresql.at/</a>
 
</pre>

On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
> Hi,

> I am reviewing your patch.

Thank you very much.

> Since you are using a constant string, it would be a little faster
> to use "sizeof(string)-1" as it can be computed at compile-time
> and not run the strlen() all the time this code is reached.

I have reffered the code just above for PG_TEMP_FILE_PREFIX in same function sendDir().
I have that not only that place, but there other places where strlen is used for PG_TEMP_FILE_PREFIX.
I think in this path, such optimization might not help much.
However if you think we should take care of this, then I can find other places where similar change can be done
to make it consistent?

> In create_conf_lock_file():


> Can't we add a new LWLock and use it as a critical section instead
> of waiting for 10 seconds? It would be quite surprising to wait
> 10 seconds  when accidentally executing two SET PERSISTENT
> statements in parallel.

Yes, you are right adding a new LWLock will avoid the use of sleep.
However I am just not sure if for only this purpose we should add a new LWLock?

Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
How about reducing the time of sleep or removing sleep, in that user might get
error and he need to retry to get his command successful?

With Regards,
Amit Kapila.

2013-01-05 05:58 keltezéssel, Amit kapila írta:
> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>> Hi,
>> I am reviewing your patch.
> Thank you very much.
>
>> Since you are using a constant string, it would be a little faster
>> to use "sizeof(string)-1" as it can be computed at compile-time
>> and not run the strlen() all the time this code is reached.
> I have reffered the code just above for PG_TEMP_FILE_PREFIX in same function sendDir().
> I have that not only that place, but there other places where strlen is used for PG_TEMP_FILE_PREFIX.
> I think in this path, such optimization might not help much.
> However if you think we should take care of this, then I can find other places where similar change can be done
> to make it consistent?
>
>> In create_conf_lock_file():
>
>> Can't we add a new LWLock and use it as a critical section instead
>> of waiting for 10 seconds? It would be quite surprising to wait
>> 10 seconds  when accidentally executing two SET PERSISTENT
>> statements in parallel.
> Yes, you are right adding a new LWLock will avoid the use of sleep.
> However I am just not sure if for only this purpose we should add a new LWLock?
>
> Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
> How about reducing the time of sleep or removing sleep, in that user might get
> error and he need to retry to get his command successful?

I think removing the loop entirely is better.

However, the behaviour should be discussed by a wider audience:
- the backends should wait for each other just like other statements   that fight for the same object (in which case
lockingis needed), or 
- throw an error immediately on conflicting parallel work

I also tried to trigger one of the errors by creating the lock file manually.
You need an extra space between the "... retry" and "or ...":

+                               ereport(ERROR,
+ (errcode_for_file_access(),
+                                                errmsg("could not create lock file
\"%s\": %m ", LockFileName),
+                                                errhint("May be too many concurrent edit
into file happening, please wait!! and retry"
+                                                                "or .lock is file
accidently left there please clean the file from config_dir")));

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

2013-01-05 05:58 keltezéssel, Amit kapila írta:
> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>> Hi,
>> I am reviewing your patch.
> Thank you very much.
>
>> Since you are using a constant string, it would be a little faster
>> to use "sizeof(string)-1" as it can be computed at compile-time
>> and not run the strlen() all the time this code is reached.
> I have reffered the code just above for PG_TEMP_FILE_PREFIX in same function sendDir().
> I have that not only that place, but there other places where strlen is used for PG_TEMP_FILE_PREFIX.
> I think in this path, such optimization might not help much.
> However if you think we should take care of this, then I can find other places where similar change can be done
> to make it consistent?

Yes, but it needs a separate cleanup patch.

Also, since the SET PERSISTENT patch seems to create a single lock file,
sizeof(string) (which includes the 0 byte at the end of the string, so it
matches the whole filename) can be used instead of strlen(string) or
sizeof(string)-1 that match the filename as a prefix only.

>
>> In create_conf_lock_file():
>
>> Can't we add a new LWLock and use it as a critical section instead
>> of waiting for 10 seconds? It would be quite surprising to wait
>> 10 seconds  when accidentally executing two SET PERSISTENT
>> statements in parallel.
> Yes, you are right adding a new LWLock will avoid the use of sleep.
> However I am just not sure if for only this purpose we should add a new LWLock?
>
> Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
> How about reducing the time of sleep or removing sleep, in that user might get
> error and he need to retry to get his command successful?
>
> With Regards,
> Amit Kapila.
>


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

On Sat, Jan 05, 2013 at 08:05:11AM +0100, Boszormenyi Zoltan wrote:
> 2013-01-05 05:58 keltez?ssel, Amit kapila ?rta:
>> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>>> In create_conf_lock_file():
>>
>>> Can't we add a new LWLock and use it as a critical section instead
>>> of waiting for 10 seconds? It would be quite surprising to wait
>>> 10 seconds  when accidentally executing two SET PERSISTENT
>>> statements in parallel.
>> Yes, you are right adding a new LWLock will avoid the use of sleep.
>> However I am just not sure if for only this purpose we should add a new LWLock?
>>
>> Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
>> How about reducing the time of sleep or removing sleep, in that user might get
>> error and he need to retry to get his command successful?
>
> I think removing the loop entirely is better.

Agreed.  A new LWLock with a narrow purpose is fine.  CreateLockFile() happens
early, before shared memory is available.  Its approach is not a relevant
precedent for code that runs later.

> However, the behaviour should be discussed by a wider audience:
> - the backends should wait for each other just like other statements
>    that fight for the same object (in which case locking is needed), or
> - throw an error immediately on conflicting parallel work

All other things being equal, the first choice sounds better.  (I don't think
the decision is essential to the utility of this feature.)

On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
2013-01-05 05:58 keltezéssel, Amit kapila írta:
> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>> Hi,
>> I am reviewing your patch.
> Thank you very much.
>
>> Yes, you are right adding a new LWLock will avoid the use of sleep.
>> However I am just not sure if for only this purpose we should add a new LWLock?
>
>> Similar logic is used in CreateLockFile() for postmaster file but it doesn't use sleep.
>> How about reducing the time of sleep or removing sleep, in that user might get
>> error and he need to retry to get his command successful?

> I think removing the loop entirely is better.

> However, the behaviour should be discussed by a wider audience:
> - the backends should wait for each other just like other statements
>    that fight for the same object (in which case locking is needed), or
> - throw an error immediately on conflicting parallel work

Okay, I shall update the patch with first option as suggested by Noah as well.

>I also tried to trigger one of the errors by creating the lock file manually.
> You need an extra space between the "... retry" and "or ...":

> +                               ereport(ERROR,
> + (errcode_for_file_access(),
> +                                                errmsg("could not create lock file
> \"%s\": %m ", LockFileName),
> +                                                errhint("May be too many concurrent edit
> into file happening, please wait!! and retry"
> +                                                                "or .lock is file
> accidently left there please clean the file from config_dir")));

Will fix.


Thank you for review.

With Regards,
Amit Kapila.

On Sunday, January 06, 2013 10:26 AM Boszormenyi Zoltan wrote:
On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
2013-01-05 05:58 keltezéssel, Amit kapila írta:
> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>> Hi,
>> I am reviewing your patch.
> Thank you very much.
>

All comments are addressed as below:

>One specific comment about the documentation part of the patch:
 >
>***************
>*** 86,92 **** SET [ SESSION | LOCAL ] TIME ZONE { <replaceable class="PARAMETER">timezone</rep
>     <application>PL/pgSQL</application> exception block.  This behavior
>      has been changed because it was deemed unintuitive.
>     </para>
>!   </note>
>   </refsect1>
>
>   <refsect1>
>--- 95,101 ----
>     <application>PL/pgSQL</application> exception block.  This behavior
>      has been changed because it was deemed unintuitive.
>     </para>
>!    </note>
>   </refsect1>
>
>   <refsect1>
>***************
Fixed the white space comment.

># This includes the default configuration directory included to support
># SET PERSISTENT statement.
>#
># WARNNING: Any configuration parameter values specified after this line
>#           will override the values set by SET PERSISTENT statement.
>include_dir = 'config_dir'
>
>Note the typo in the above message: WARNNING, there are too many Ns.

Fixed.

>Can't we add a new LWLock and use it as a critical section instead
>of waiting for 10 seconds?

Added LWLock and removed sleep logic. After adding the LWLock the lock file name can be changed
as temp file. but presently the patch contains the name as lock file only. please provide the
suggestions.

>I also tried to trigger one of the errors by creating the lock file manually.
>You need an extra space between the "... retry" and "or ...":

Fixed.

>Also, since the SET PERSISTENT patch seems to create a single lock file,
>sizeof(string) (which includes the 0 byte at the end of the string, so it
>matches the whole filename) can be used instead of strlen(string) or
>sizeof(string)-1 that match the filename as a prefix only.
>#define PGAUTOCONFLOCK    "postgresql.auto.conf.lock"
+               /* skip lock files used in postgresql.auto.conf edit */
+               if (strncmp(de->d_name,
+                                       "postgresql.auto.conf.lock",
+                                       strlen("postgresql.auto.conf.lock")) == 0)

Added a macro and changed the check accordingly.


With Regards,
Amit Kapila.

Hi,

2013-01-09 10:08 keltezéssel, Amit kapila írta:
> On Sunday, January 06, 2013 10:26 AM Boszormenyi Zoltan wrote:
> On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
> 2013-01-05 05:58 keltezéssel, Amit kapila írta:
>> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>>> Hi,
>>> I am reviewing your patch.
>> Thank you very much.
>>
> All comments are addressed as below:
>
>> One specific comment about the documentation part of the patch:
>   >
>> ***************
>> *** 86,92 **** SET [ SESSION | LOCAL ] TIME ZONE { <replaceable class="PARAMETER">timezone</rep
>>      <application>PL/pgSQL</application> exception block.  This behavior
>>       has been changed because it was deemed unintuitive.
>>      </para>
>> !   </note>
>>    </refsect1>
>>
>>    <refsect1>
>> --- 95,101 ----
>>      <application>PL/pgSQL</application> exception block.  This behavior
>>       has been changed because it was deemed unintuitive.
>>      </para>
>> !    </note>
>>    </refsect1>
>>
>>    <refsect1>
>> ***************
> Fixed the white space comment.
>
>> # This includes the default configuration directory included to support
>> # SET PERSISTENT statement.
>> #
>> # WARNNING: Any configuration parameter values specified after this line
>> #           will override the values set by SET PERSISTENT statement.
>> include_dir = 'config_dir'
>>
>> Note the typo in the above message: WARNNING, there are too many Ns.
> Fixed.
>
>> Can't we add a new LWLock and use it as a critical section instead
>> of waiting for 10 seconds?
> Added LWLock and removed sleep logic. After adding the LWLock the lock file name can be changed
> as temp file. but presently the patch contains the name as lock file only. please provide the
> suggestions.

Current state of affairs:

a.) The whole operation is protected by the LWLock so only one backend
can do this any time. Clean operation is ensured.

b.) The code creates the lock file and fails if it the file exists. This protects
against nasties done externally. The current logic to bail out with an error
is OK, I can know that there is a stupid intruder on the system. But then
they can also remove the original .auto.conf file too and anything else and
I lost anyway.

c.) In reaper(), the code cleans up the lock file and since there can
be only one lock file, no need to search for temp files, a straightforward
unlink() call does its job.


This may be changed in two ways to make it more comfortable:

1. Simply unlink() and retry with creat() once.

Comments:
unlink() may fail under Windows if someone keeps this file open.
Then you can either give up with ereport(ERROR) just like now.

POSIX systems simply remove the file <-> inode mapping so unlink()
doesn't fail in this case.

2. Create the tempfile. There will be one less error case, the file creation
may only fail in case you are out of disk space.

Creating the tempfile is tempting. But then reaper() will need a little
more code to remove all the tempfiles.

I just noticed that you are using "%m" in format strings twice. man 3 printf says:
       m      (Glibc extension.)  Print output of strerror(errno). No argument is required.

This doesn't work anywhere else, only on GLIBC systems, it means Linux.
I also like the brevity of this extension but PostgreSQL is a portable system.
You should use %s and strerror(errno) as argument explicitly.

>> I also tried to trigger one of the errors by creating the lock file manually.
>> You need an extra space between the "... retry" and "or ...":
> Fixed.
>
>> Also, since the SET PERSISTENT patch seems to create a single lock file,
>> sizeof(string) (which includes the 0 byte at the end of the string, so it
>> matches the whole filename) can be used instead of strlen(string) or
>> sizeof(string)-1 that match the filename as a prefix only.
>> #define PGAUTOCONFLOCK    "postgresql.auto.conf.lock"
> +               /* skip lock files used in postgresql.auto.conf edit */
> +               if (strncmp(de->d_name,
> +                                       "postgresql.auto.conf.lock",
> +                                       strlen("postgresql.auto.conf.lock")) == 0)
>
> Added a macro and changed the check accordingly.

If the current logic stays or the modification in 1) will be chosen,
the comment needs tweaking:

+               /* skip lock files used in postgresql.auto.conf edit */
                   "skip the lock file ..."

+               if (strncmp(de->d_name,
+                                       PGAUTOCONFLOCK,
+                                       sizeof(PGAUTOCONFLOCK)) == 0)
+                       continue;
+

If the 2nd suggestion is chosen, sizeof()-1 or strlen() must be uses again
to exclude all the possible tempfiles.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

On Friday, January 11, 2013 10:03 PM Boszormenyi Zoltan wrote:
> Hi,

2013-01-09 10:08 keltezéssel, Amit kapila írta:
> On Sunday, January 06, 2013 10:26 AM Boszormenyi Zoltan wrote:
> On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
> 2013-01-05 05:58 keltezéssel, Amit kapila írta:
>> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>>> Hi,
>>> I am reviewing your patch.
>> Thank you very much.
>>
> All comments are addressed as below:
>


>>> Can't we add a new LWLock and use it as a critical section instead
>>> of waiting for 10 seconds?
>> Added LWLock and removed sleep logic. After adding the LWLock the lock file name can be changed
>> as temp file. but presently the patch contains the name as lock file only. please provide the
>> suggestions.

> Current state of affairs:

> a.) The whole operation is protected by the LWLock so only one backend
> can do this any time. Clean operation is ensured.

> b.) The code creates the lock file and fails if it the file exists. This protects
> against nasties done externally. The current logic to bail out with an error
> is OK, I can know that there is a stupid intruder on the system. But then
> they can also remove the original .auto.conf file too and anything else and
> I lost anyway.

> c.) In reaper(), the code cleans up the lock file and since there can
> be only one lock file, no need to search for temp files, a straightforward
> unlink() call does its job.


> This may be changed in two ways to make it more comfortable:

> 1. Simply unlink() and retry with creat() once.

> Comments:
> unlink() may fail under Windows if someone keeps this file open.
> Then you can either give up with ereport(ERROR) just like now.

I think as this is an internal file, user is not supposed to play with file and incase he does so,
then I think current implementation is okay, means during open (O_CREAT) it gives error and the message
is also suggesting the same.



> 2. Create the tempfile. There will be one less error case, the file creation
> may only fail in case you are out of disk space.

> Creating the tempfile is tempting. But then reaper() will need a little
> more code to remove all the tempfiles.

By reaper you mean to say CATCH block?

In that case, I would prefer to keep the current implementation as it is.

Actualy I was thinking of just changing the extension from current .lock to .tmp, so in that case
the same problems can happen with this also.

> I just noticed that you are using "%m" in format strings twice. man 3 printf says:

>        m      (Glibc extension.)  Print output of strerror(errno). No argument is required.

> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
> I also like the brevity of this extension but PostgreSQL is a portable system.
> You should use %s and strerror(errno) as argument explicitly.

%m is used at other places in code as well.

Thank you for feedback.

With Regards,
Amit Kapila.

2013-01-12 06:51 keltezéssel, Amit kapila írta:
> On Friday, January 11, 2013 10:03 PM Boszormenyi Zoltan wrote:
>> Hi,
> 2013-01-09 10:08 keltezéssel, Amit kapila írta:
>> On Sunday, January 06, 2013 10:26 AM Boszormenyi Zoltan wrote:
>> On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
>> 2013-01-05 05:58 keltezéssel, Amit kapila írta:
>>> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>>>> Hi,
>>>> I am reviewing your patch.
>>> Thank you very much.
>>>
>> All comments are addressed as below:
>>
>
>>>> Can't we add a new LWLock and use it as a critical section instead
>>>> of waiting for 10 seconds?
>>> Added LWLock and removed sleep logic. After adding the LWLock the lock file name can be changed
>>> as temp file. but presently the patch contains the name as lock file only. please provide the
>>> suggestions.
>> Current state of affairs:
>> a.) The whole operation is protected by the LWLock so only one backend
>> can do this any time. Clean operation is ensured.
>> b.) The code creates the lock file and fails if it the file exists. This protects
>> against nasties done externally. The current logic to bail out with an error
>> is OK, I can know that there is a stupid intruder on the system. But then
>> they can also remove the original .auto.conf file too and anything else and
>> I lost anyway.
>> c.) In reaper(), the code cleans up the lock file and since there can
>> be only one lock file, no need to search for temp files, a straightforward
>> unlink() call does its job.
>
>> This may be changed in two ways to make it more comfortable:
>> 1. Simply unlink() and retry with creat() once.
>> Comments:
>> unlink() may fail under Windows if someone keeps this file open.
>> Then you can either give up with ereport(ERROR) just like now.
> I think as this is an internal file, user is not supposed to play with file and incase he does so,
> then I think current implementation is okay, means during open (O_CREAT) it gives error and the message
> is also suggesting the same.

That's what I meant in b) above.

>
>
>
>> 2. Create the tempfile. There will be one less error case, the file creation
>> may only fail in case you are out of disk space.
>> Creating the tempfile is tempting. But then reaper() will need a little
>> more code to remove all the tempfiles.
> By reaper you mean to say CATCH block?

No, I mean the reaper(SIGNAL_ARGS) function in
src/backend/postmaster/postmaster.c where your patch has this:

*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 2552,2557 **** reaper(SIGNAL_ARGS)
--- 2552,2569 ----                                continue;                        }

+                       /* Delete the postgresql.auto.conf.lock file if exists */
+                       {
+                               char LockFileName[MAXPGPATH];
+
+                               strlcpy(LockFileName, ConfigFileName, sizeof(LockFileName));
+                               get_parent_directory(LockFileName);
+                               join_path_components(LockFileName, LockFileName,
AutoConfigLockFilename);
+                               canonicalize_path(LockFileName);
+
+                               unlink(LockFileName);
+                       }
+                        /*                         * Startup succeeded, commence normal operations
   */ 


>
> In that case, I would prefer to keep the current implementation as it is.

I also said above the current logic is OK for me.
I just gave food for thought to provoke discussion from others.

>
> Actualy I was thinking of just changing the extension from current .lock to .tmp, so in that case
> the same problems can happen with this also.

Indeed.

>
>> I just noticed that you are using "%m" in format strings twice. man 3 printf says:
>>         m      (Glibc extension.)  Print output of strerror(errno). No argument is required.
>> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
>> I also like the brevity of this extension but PostgreSQL is a portable system.
>> You should use %s and strerror(errno) as argument explicitly.
> %m is used at other places in code as well.
>
> Thank you for feedback.

You're welcome.

>
> With Regards,
> Amit Kapila.
>
>
>


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

Boszormenyi Zoltan <zb@cybertec.at> writes:
> No, I mean the reaper(SIGNAL_ARGS) function in
> src/backend/postmaster/postmaster.c where your patch has this:

> *** a/src/backend/postmaster/postmaster.c
> --- b/src/backend/postmaster/postmaster.c
> ***************
> *** 2552,2557 **** reaper(SIGNAL_ARGS)
> --- 2552,2569 ----
>                                  continue;
>                          }

> +                       /* Delete the postgresql.auto.conf.lock file if exists */
> +                       {
> +                               char LockFileName[MAXPGPATH];
> +
> +                               strlcpy(LockFileName, ConfigFileName, sizeof(LockFileName));
> +                               get_parent_directory(LockFileName);
> +                               join_path_components(LockFileName, LockFileName, 
> AutoConfigLockFilename);
> +                               canonicalize_path(LockFileName);
> +
> +                               unlink(LockFileName);
> +                       }
> +
>                          /*
>                           * Startup succeeded, commence normal operations
>                           */

I have not been following this thread, but I happened to see this bit,
and I have to say that I am utterly dismayed that anything like this is
even on the table.  This screams overdesign.  We do not need a global
lock file, much less postmaster-side cleanup.  All that's needed is a
suitable convention about temp file names that can be written and then
atomically renamed into place.  If we're unlucky enough to crash before
a temp file can be renamed into place, it'll just sit there harmlessly.

>>> I just noticed that you are using "%m" in format strings twice. man 3 printf says:
>>> m      (Glibc extension.)  Print output of strerror(errno). No argument is required.
>>> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
>>> I also like the brevity of this extension but PostgreSQL is a portable system.
>>> You should use %s and strerror(errno) as argument explicitly.
>> %m is used at other places in code as well.

As far as that goes, %m is appropriate in elog/ereport (which contain
special support for it), but not anywhere else.
        regards, tom lane

On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
Boszormenyi Zoltan <zb@cybertec.at> writes:
>> No, I mean the reaper(SIGNAL_ARGS) function in
>> src/backend/postmaster/postmaster.c where your patch has this:

>> *** a/src/backend/postmaster/postmaster.c
>> --- b/src/backend/postmaster/postmaster.c
>> ***************
>> *** 2552,2557 **** reaper(SIGNAL_ARGS)

> I have not been following this thread, but I happened to see this bit,
> and I have to say that I am utterly dismayed that anything like this is
> even on the table.  This screams overdesign.  We do not need a global
> lock file, much less postmaster-side cleanup.  All that's needed is a
> suitable convention about temp file names that can be written and then
> atomically renamed into place.  If we're unlucky enough to crash before
> a temp file can be renamed into place, it'll just sit there harmlessly.

I can think of below ways to generate tmp file
1. Generate session specific tmp file name during operation. This will be similar to what is   currently being done in
OpenTemporaryFileinTablespace()to generate a tempfilename. 
2. Generate global tmp file name. For this we need to maintain global counter.
3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one  session is allowed to
operate.

In any approach, there will be chance that temp files will remain if server crashes during this command execution
which can lead to collision of temp file name next time user tries to use SET persistent command.
An appropriate error will be raised and user manually needs to delete that file.



>>>> I just noticed that you are using "%m" in format strings twice. man 3 printf says:
>>>> m      (Glibc extension.)  Print output of strerror(errno). No argument is required.
>>>> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
>>>> I also like the brevity of this extension but PostgreSQL is a portable system.
>>>> You should use %s and strerror(errno) as argument explicitly.
>>> %m is used at other places in code as well.

> As far as that goes, %m is appropriate in elog/ereport (which contain
> special support for it), but not anywhere else.

In the patch, it's used in ereport, so I assume it is safe and patch doesn't need any change for %m.


With Regards,
Amit Kapila.

2013-01-13 05:49 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>>> No, I mean the reaper(SIGNAL_ARGS) function in
>>> src/backend/postmaster/postmaster.c where your patch has this:
>>> *** a/src/backend/postmaster/postmaster.c
>>> --- b/src/backend/postmaster/postmaster.c
>>> ***************
>>> *** 2552,2557 **** reaper(SIGNAL_ARGS)
>> I have not been following this thread, but I happened to see this bit,
>> and I have to say that I am utterly dismayed that anything like this is
>> even on the table.  This screams overdesign.  We do not need a global
>> lock file, much less postmaster-side cleanup.  All that's needed is a
>> suitable convention about temp file names that can be written and then
>> atomically renamed into place.  If we're unlucky enough to crash before
>> a temp file can be renamed into place, it'll just sit there harmlessly.
> I can think of below ways to generate tmp file
> 1. Generate session specific tmp file name during operation. This will be similar to what is
>     currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
> 2. Generate global tmp file name. For this we need to maintain global counter.
> 3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
>     session is allowed to operate.

What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
that returns   a file descriptor for an already created file with a unique name?

Note that the working directory of PostgreSQL is $PGDATA so "config_dir"
and files under that can be accessed with a relative path.

But a cleanup somewhere is needed to remove the stale temp files.
I think it's enough at postmaster startup. A machine that's crashing
so often that the presence of the stale temp files causes out of disk
errors would surely be noticed much earlier.

>
> In any approach, there will be chance that temp files will remain if server crashes during this command execution
> which can lead to collision of temp file name next time user tries to use SET persistent command.

mkstemp() replaces the "XXXXXX" suffix with a unique hexadecimal suffix
so there will be no collision.

> An appropriate error will be raised and user manually needs to delete that file.
>
>
>
>>>>> I just noticed that you are using "%m" in format strings twice. man 3 printf says:
>>>>> m      (Glibc extension.)  Print output of strerror(errno). No argument is required.
>>>>> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
>>>>> I also like the brevity of this extension but PostgreSQL is a portable system.
>>>>> You should use %s and strerror(errno) as argument explicitly.
>>>> %m is used at other places in code as well.
>> As far as that goes, %m is appropriate in elog/ereport (which contain
>> special support for it), but not anywhere else.
> In the patch, it's used in ereport, so I assume it is safe and patch doesn't need any change for %m.

OK.

>
>
> With Regards,
> Amit Kapila.
>
>


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

On Sunday, January 13, 2013 2:45 PM Boszormenyi Zoltan wrote:
2013-01-13 05:49 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>>>> No, I mean the reaper(SIGNAL_ARGS) function in
>>>> src/backend/postmaster/postmaster.c where your patch has this:
>>>> *** a/src/backend/postmaster/postmaster.c
>>>> --- b/src/backend/postmaster/postmaster.c
>>>> ***************
>>>> *** 2552,2557 **** reaper(SIGNAL_ARGS)
>>> I have not been following this thread, but I happened to see this bit,
>>> and I have to say that I am utterly dismayed that anything like this is
>>> even on the table.  This screams overdesign.  We do not need a global
>>> lock file, much less postmaster-side cleanup.  All that's needed is a
>>> suitable convention about temp file names that can be written and then
>>> atomically renamed into place.  If we're unlucky enough to crash before
>>> a temp file can be renamed into place, it'll just sit there harmlessly.
>> I can think of below ways to generate tmp file
>> 1. Generate session specific tmp file name during operation. This will be similar to what is
>>     currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
>> 2. Generate global tmp file name. For this we need to maintain global counter.
>> 3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
>>     session is allowed to operate.

> What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
> that returns   a file descriptor for an already created file with a unique name?

I think for Windows exactly same behavior API might not exist, we might need to use GetTempFileName.
It will generate some different kind of name, which means cleanup also need to take care of different kind of names.
So if this is right, I think it might not be very appropriate to use it.

Please point me if I am wrong as I have not used it.

> Note that the working directory of PostgreSQL is $PGDATA so "config_dir"
> and files under that can be accessed with a relative path.

> But a cleanup somewhere is needed to remove the stale temp files.
> I think it's enough at postmaster startup. A machine that's crashing
> so often that the presence of the stale temp files causes out of disk
> errors would surely be noticed much earlier.

In PostmasterMain(), we already call RemovePGTempFiles(); So I think we can delete this tmp file at
that place in PostmasterMain().
I shall do this, if nobody raise objection about it.

With Regards,
Amit Kapila.

2013-01-14 07:47 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 2:45 PM Boszormenyi Zoltan wrote:
> 2013-01-13 05:49 keltezéssel, Amit kapila írta:
>> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
>> Boszormenyi Zoltan <zb@cybertec.at> writes:
>>>>> No, I mean the reaper(SIGNAL_ARGS) function in
>>>>> src/backend/postmaster/postmaster.c where your patch has this:
>>>>> *** a/src/backend/postmaster/postmaster.c
>>>>> --- b/src/backend/postmaster/postmaster.c
>>>>> ***************
>>>>> *** 2552,2557 **** reaper(SIGNAL_ARGS)
>>>> I have not been following this thread, but I happened to see this bit,
>>>> and I have to say that I am utterly dismayed that anything like this is
>>>> even on the table.  This screams overdesign.  We do not need a global
>>>> lock file, much less postmaster-side cleanup.  All that's needed is a
>>>> suitable convention about temp file names that can be written and then
>>>> atomically renamed into place.  If we're unlucky enough to crash before
>>>> a temp file can be renamed into place, it'll just sit there harmlessly.
>>> I can think of below ways to generate tmp file
>>> 1. Generate session specific tmp file name during operation. This will be similar to what is
>>>      currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
>>> 2. Generate global tmp file name. For this we need to maintain global counter.
>>> 3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
>>>      session is allowed to operate.
>> What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
>> that returns   a file descriptor for an already created file with a unique name?
> I think for Windows exactly same behavior API might not exist, we might need to use GetTempFileName.
> It will generate some different kind of name, which means cleanup also need to take care of different kind of names.
> So if this is right, I think it might not be very appropriate to use it.

In that case (and also because the LWLock still serialize the whole procedure)
you can use GetTempFileName() on Windows and tempnam(3) for non-Windows.
GetTempFileName() and tempnam() return the constructed temporary file name
out of the directory and the filename prefix components.

The differences are:
1. For GetTempFileName(), you have to provide a buffer and it uses 3 bytes from
the prefix string. Always give you this path: dir\pfx<uuu>.TMP

2. tempnam() gives you a malloc()ed result buffer and uses at most 5 bytes from
the prefix string. According to the NOTES section in the man page, the specified
directory is only considered if $TMPDIR is not set in the environment or the
directory is not usable, like write is not permitted or the directory is missing.
In this case. If TMPDIR not only is present and usable but on a different filesystem
as the target config_dir/postgresql.auto.conf, rename() would return the EXDEV
error:
       EXDEV  The  links  named  by new and old are on different file systems and the
implementation does not support links              between file systems.

Maybe mktemp(3) (not mk*s*temp!) instead of tempnam() is better here
because it always uses the specified template. You have to use you own
buffer for mktemp(). The BUGS section in man 3 mktemp says:

BUGS       Never use mktemp().  Some implementations follow 4.3BSD and replace XXXXXX by the
current process ID  and  a  single       letter,  so that at most 26 different names can be returned. Since on the one
hand 
the names are easy to guess, and       on the other hand there is a race between testing whether the name  exists  and

opening  the  file,  every  use  of       mktemp() is a security risk.  The race is avoided by mkstemp(3).

I think we don't really care about the security risk about the file name
being easy to guess and there is no race condition because of the LWLock.

With GetTempFileName() and mktemp(), you can have a lot of common code,
like pass the same 3-letter prefix and preallocate or statically declare the output buffer.

> Please point me if I am wrong as I have not used it.
>
>> Note that the working directory of PostgreSQL is $PGDATA so "config_dir"
>> and files under that can be accessed with a relative path.
>> But a cleanup somewhere is needed to remove the stale temp files.
>> I think it's enough at postmaster startup. A machine that's crashing
>> so often that the presence of the stale temp files causes out of disk
>> errors would surely be noticed much earlier.
> In PostmasterMain(), we already call RemovePGTempFiles(); So I think we can delete this tmp file at
> that place in PostmasterMain().
> I shall do this, if nobody raise objection about it.

No objection.

>
> With Regards,
> Amit Kapila.
>


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

On Monday, January 14, 2013 6:48 PM Boszormenyi Zoltan wrote:
2013-01-14 07:47 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 2:45 PM Boszormenyi Zoltan wrote:
> 2013-01-13 05:49 keltezéssel, Amit kapila írta:
>> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
>> Boszormenyi Zoltan <zb@cybertec.at> writes:

>>>> I can think of below ways to generate tmp file
>>>> 1. Generate session specific tmp file name during operation. This will be similar to what is
>>>>      currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
>>>> 2. Generate global tmp file name. For this we need to maintain global counter.
>>>> 3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
>>>>      session is allowed to operate.
>>> What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
>>> that returns   a file descriptor for an already created file with a unique name?
>> I think for Windows exactly same behavior API might not exist, we might need to use GetTempFileName.
>> It will generate some different kind of name, which means cleanup also need to take care of different kind of names.
>> So if this is right, I think it might not be very appropriate to use it.

> In that case (and also because the LWLock still serialize the whole procedure)
> you can use GetTempFileName() on Windows and tempnam(3) for non-Windows.
> GetTempFileName() and tempnam() return the constructed temporary file name
> out of the directory and the filename prefix components.

Okay, I shall work on this based on your suggestion.

With Regards,
Amit Kapila.