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

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(at)cybertec(dot)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.

On using mktemp, linux compilation gives below warning
warning: the use of `mktemp' is dangerous, better use `mkstemp'

So I planned to use mkstemp.

In Windows, there is an api _mktemp_s, followed by open call, behaves in a similar way as mkstemp available in linux.
The code is changed to use of mkstemp functionality to generate a unique temporary file name instead of .lock file.
Please check this part of code, I am not very comfortable with using this API.

Removed the code which is used for deleting the .lock file in case of backend crash or during server startup.

Added a new function RemoveAutoConfTmpFiles used for deleting the temp files left out any during previous
postmaster session in the server startup.

In SendDir(), used sizeof() -1

With Regards,
Amit Kapila.

Hi,

comments below.

2013-01-18 11:05 keltezéssel, Amit kapila írta:
> On using mktemp, linux compilation gives below warning
> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>
> So I planned to use mkstemp.

Good.

> In Windows, there is an api _mktemp_s, followed by open call, behaves in a similar way as mkstemp available in linux.
> The code is changed to use of mkstemp functionality to generate a unique temporary file name instead of .lock file.
> Please check this part of code, I am not very comfortable with using this API.

I read the documentation of _mktemp_s() at

http://msdn.microsoft.com/en-us/library/t8ex5e91%28v=vs.80%29.aspx

The comment says it's only for C++, but I can compile your patch using
the MinGW cross-compiler under Fedora 18. So either the MSDN comment
is false, or MinGW provides this function outside C++. More research is
needed on this.

However, I am not sure whether Cygwin provides the mkstemp() call or not.
Searching... Found bugzilla reports against mkstemp on Cygwin. So, yes, it does
and your code would clash with it. In port/dirmod.c:

----8<--------8<--------8<--------8<--------8<----
#if defined(WIN32) || defined(__CYGWIN__)

...

/*
* pgmkstemp
*/
int
pgmkstemp (char *TmpFileName)
{
int err;

err = _mktemp_s(TmpFileName, strlen(TmpFileName) + 1);
if (err != 0)
return -1;

return open(TmpFileName, O_CREAT | O_RDWR | O_EXCL, S_IRUSR | S_IWUSR);
}

/* We undefined these above; now redefine for possible use below */
#define rename(from, to) pgrename(from, to)
#define unlink(path) pgunlink(path)
#define mkstemp(tempfilename) pgmkstemp(tempfilename)
#endif /* defined(WIN32) || defined(__CYGWIN__) */
----8<--------8<--------8<--------8<--------8<----

pgmkstemp() should be defined in its own block inside

#if defined(WIN32) && !defined(__CYGWIN__)
...
#endif

Same thing applies to src/include/port.h:

----8<--------8<--------8<--------8<--------8<----
#if defined(WIN32) || defined(__CYGWIN__)
/*
* Win32 doesn't have reliable rename/unlink during concurrent access.
*/
extern int pgrename(const char *from, const char *to);
extern int pgunlink(const char *path);
extern int pgmkstemp (char *TmpFileName);

/* Include this first so later includes don't see these defines */
#ifdef WIN32_ONLY_COMPILER
#include <io.h>
#endif

#define rename(from, to) pgrename(from, to)
#define unlink(path) pgunlink(path)
#define mkstemp(tempfilename) pgmkstemp(tempfilename)
#endif /* defined(WIN32) || defined(__CYGWIN__) */
----8<--------8<--------8<--------8<--------8<----

> Removed the code which is used for deleting the .lock file in case of backend crash or during server startup.

Good.

> Added a new function RemoveAutoConfTmpFiles used for deleting the temp files left out any during previous
> postmaster session in the server startup.

Good.

> In SendDir(), used sizeof() -1

Good.

Since you have these defined:

+ #define PG_AUTOCONF_DIR "config_dir"
+ #define PG_AUTOCONF_FILENAME "postgresql.auto.conf"
+ #define PG_AUTOCONF_SAMPLE_TMPFILE "postgresql.auto.conf.XXXXXX"
+ #define PG_AUTOCONF_TMP_FILE "postgresql.auto.conf."

then the hardcoded values can also be changed everywhere. But to kill
them in initdb.c, these #define's must be in pg_config_manual.h instead of
guc.h.

Most notably, postgresql.conf.sample contains:
#include_dir = 'auto.conf.d'
and initdb replaces it with
include_dir = 'config_dir'.
I prefer the auto.conf.d directory name. After killing all hardcoded
"config_dir", changing one #define is all it takes to change it.

There are two blocks of code that Tom commented as being "over-engineered":

+ /* Frame auto conf name and auto conf sample temp file name */
+ snprintf(Filename,sizeof(Filename),"%s/%s", PG_AUTOCONF_DIR, PG_AUTOCONF_FILENAME);
+ StrNCpy(AutoConfFileName, ConfigFileName, sizeof(AutoConfFileName));
+ get_parent_directory(AutoConfFileName);
+ join_path_components(AutoConfFileName, AutoConfFileName, Filename);
+ canonicalize_path(AutoConfFileName);
+
+ snprintf(Filename,sizeof(Filename),"%s/%s", PG_AUTOCONF_DIR,
PG_AUTOCONF_SAMPLE_TMPFILE);
+ StrNCpy(AutoConfTmpFileName, ConfigFileName, sizeof(AutoConfTmpFileName));
+ get_parent_directory(AutoConfTmpFileName);
+ join_path_components(AutoConfTmpFileName, AutoConfTmpFileName, Filename);
+ canonicalize_path(AutoConfTmpFileName);

You can simply do
snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
data_directory,
PG_AUTOCONF_DIR,
PG_AUTOCONF_FILENAME);
snprintf(AutoConfTmpFileName, sizeof(AutoConfTmpFileName),"%s/%s/%s",
data_directory,
PG_AUTOCONF_DIR,
PG_AUTOCONF_SAMPLE_TMPFILE);

Also, mkstemp() and _mktemp_s() modify the file name in place,
so the XXXXXX suffix becomes something else. You can pass the
array pointer to write_auto_conf_file() together with the returned fd
so the ereport() calls can report the actual file name, not the template.

You removed the static keyword from before AbsoluteConfigLocation(),
resulting in this compiler warning:

In file included from guc.c:9274:0:
guc-file.l:382:1: warning: no previous prototype for 'AbsoluteConfigLocation'
[-Wmissing-prototypes]

In validate_conf_option() in guc.c, you had "return NULL;" and "return 0;" mixed.
Since this function returns a pointer, I changed all to "return NULL;".

void-returning function write_auto_conf_file() in guc.c had a "return;"
at the end. It's not needed.

I have fixed these in the attached patch and also changed wording
of the documentation, some comments and the error messages to
be more expressive or more concise. The difference between your
patch and mine is also attached.

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/

Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> 2013-01-18 11:05 keltezssel, Amit kapila rta:
>> On using mktemp, linux compilation gives below warning
>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>>
>> So I planned to use mkstemp.

> Good.

On my HPUX box, the man page disapproves of both, calling them obsolete
(and this man page is old enough to vote, I believe...)

Everywhere else that we need to do something like this, we just use our
own PID to disambiguate, ie
sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
There is no need to deviate from that pattern or introduce portability
issues, since we can reasonably assume that no non-Postgres programs are
creating files in this directory.

regards, tom lane

2013-01-18 21:32 keltezéssel, Tom Lane írta:
> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
>>> On using mktemp, linux compilation gives below warning
>>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>>>
>>> So I planned to use mkstemp.
>> Good.
> On my HPUX box, the man page disapproves of both, calling them obsolete
> (and this man page is old enough to vote, I believe...)
>
> Everywhere else that we need to do something like this, we just use our
> own PID to disambiguate, ie
> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
> There is no need to deviate from that pattern or introduce portability
> issues, since we can reasonably assume that no non-Postgres programs are
> creating files in this directory.

Thanks for the enlightenment, I will post a new version soon.

>
> regards, tom lane
>
>

--
----------------------------------
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-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
>>>> On using mktemp, linux compilation gives below warning
>>>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>>>>
>>>> So I planned to use mkstemp.
>>> Good.
>> On my HPUX box, the man page disapproves of both, calling them obsolete
>> (and this man page is old enough to vote, I believe...)

Then we have to at least consider what this old {p|s}age says... ;-)

>>
>> Everywhere else that we need to do something like this, we just use our
>> own PID to disambiguate, ie
>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
>> There is no need to deviate from that pattern or introduce portability
>> issues, since we can reasonably assume that no non-Postgres programs are
>> creating files in this directory.
>
> Thanks for the enlightenment, I will post a new version soon.

Here it is.

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 Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
>>>>> On using mktemp, linux compilation gives below warning
>>>>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>>>>
>>>>> So I planned to use mkstemp.
>>>> Good.
>>> On my HPUX box, the man page disapproves of both, calling them obsolete
>>> (and this man page is old enough to vote, I believe...)

>Then we have to at least consider what this old {p|s}age says... ;-)

>>
>>> Everywhere else that we need to do something like this, we just use our
>>> own PID to disambiguate, ie
>>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
>>> There is no need to deviate from that pattern or introduce portability
>>> issues, since we can reasonably assume that no non-Postgres programs are
>>> creating files in this directory.
>
>> Thanks for the enlightenment, I will post a new version soon.

> Here it is.

Thanks a ton for providing better modified version.
I shall test it on Monday once and then we can conclude on it.

The only point in modifications, I am not comfortable is that in error messages you have
mentioned (automatic configuration directory). I am not sure if we should use work "automatic"
for config_dir or just use configuration directory.
However I shall keep as it is if no one else has any objection. We can let committer decide about it.

With Regards,
Amit Kapila.

On 01/19/2013 04:08 AM, Boszormenyi Zoltan wrote:
>
> However, I am not sure whether Cygwin provides the mkstemp() call or not.
> Searching... Found bugzilla reports against mkstemp on Cygwin.
Is Cygwin a platform that should be targeted for the server backend
these days?

I can understand making sure that libpq works on Cygwin, but is there
any reason at all to run a Pg server backend on Cygwin rather than as
native Windows binaries?

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

On 01/21/2013 10:03 AM, Craig Ringer wrote:
> On 01/19/2013 04:08 AM, Boszormenyi Zoltan wrote:
>> However, I am not sure whether Cygwin provides the mkstemp() call or not.
>> Searching... Found bugzilla reports against mkstemp on Cygwin.
> Is Cygwin a platform that should be targeted for the server backend
> these days?
>
> I can understand making sure that libpq works on Cygwin, but is there
> any reason at all to run a Pg server backend on Cygwin rather than as
> native Windows binaries?

I'm not suggesting immediately dropping working support, since this is
so trivially worked around. I'm just wondering why anybody cares about
the platform.

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

On Monday, January 21, 2013 7:36 AM Craig Ringer wrote:
> On 01/21/2013 10:03 AM, Craig Ringer wrote:
> > On 01/19/2013 04:08 AM, Boszormenyi Zoltan wrote:
> >> However, I am not sure whether Cygwin provides the mkstemp() call or
> not.
> >> Searching... Found bugzilla reports against mkstemp on Cygwin.
> > Is Cygwin a platform that should be targeted for the server backend
> > these days?
> >
> > I can understand making sure that libpq works on Cygwin, but is there
> > any reason at all to run a Pg server backend on Cygwin rather than as
> > native Windows binaries?
>
> I'm not suggesting immediately dropping working support, since this is
> so trivially worked around. I'm just wondering why anybody cares about
> the platform.

We have avoided the use of mkstemp with small native implementation so now
it won't be problem
for any platform.

With Regards,
Amit Kapila.

On Jan 21, 2013 3:06 AM, "Craig Ringer" <craig(at)2ndquadrant(dot)com> wrote:
>
> On 01/21/2013 10:03 AM, Craig Ringer wrote:
> > On 01/19/2013 04:08 AM, Boszormenyi Zoltan wrote:
> >> However, I am not sure whether Cygwin provides the mkstemp() call or
not.
> >> Searching... Found bugzilla reports against mkstemp on Cygwin.
> > Is Cygwin a platform that should be targeted for the server backend
> > these days?
> >
> > I can understand making sure that libpq works on Cygwin, but is there
> > any reason at all to run a Pg server backend on Cygwin rather than as
> > native Windows binaries?
>
> I'm not suggesting immediately dropping working support, since this is
> so trivially worked around. I'm just wondering why anybody cares about
> the platform.

I have suggested similar before, and been voted down :) iirc Andrew uses
it, no? Either way, the consensus earlier had been that as long as it
doesn't require major surgery or blocks something else, we should try to
keep it working. And as you say this sounds like something that can be
handled trivially, I think now is not the time.

/Magnus

On 01/21/2013 02:17 AM, Magnus Hagander wrote:
>
>
> On Jan 21, 2013 3:06 AM, "Craig Ringer" <craig(at)2ndquadrant(dot)com
> <mailto:craig(at)2ndquadrant(dot)com>> wrote:
> >
> > On 01/21/2013 10:03 AM, Craig Ringer wrote:
> > > On 01/19/2013 04:08 AM, Boszormenyi Zoltan wrote:
> > >> However, I am not sure whether Cygwin provides the mkstemp() call
> or not.
> > >> Searching... Found bugzilla reports against mkstemp on Cygwin.
> > > Is Cygwin a platform that should be targeted for the server backend
> > > these days?
> > >
> > > I can understand making sure that libpq works on Cygwin, but is there
> > > any reason at all to run a Pg server backend on Cygwin rather than as
> > > native Windows binaries?
> >
> > I'm not suggesting immediately dropping working support, since this is
> > so trivially worked around. I'm just wondering why anybody cares about
> > the platform.
>
> I have suggested similar before, and been voted down :) iirc Andrew
> uses it, no? Either way, the consensus earlier had been that as long
> as it doesn't require major surgery or blocks something else, we
> should try to keep it working. And as you say this sounds like
> something that can be handled trivially, I think now is not the time.
>
>

No, I only use the client. But then I support plenty of things I don't use.

cheers

andrew

On Mon, Jan 21, 2013 at 9:01 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/21/2013 02:17 AM, Magnus Hagander wrote:
>
>>
>>
>> On Jan 21, 2013 3:06 AM, "Craig Ringer" <craig(at)2ndquadrant(dot)com
>> <mailto:craig(at)2ndquadrant(dot)com>> wrote:
>> >
>> > On 01/21/2013 10:03 AM, Craig Ringer wrote:
>> > > On 01/19/2013 04:08 AM, Boszormenyi Zoltan wrote:
>> > >> However, I am not sure whether Cygwin provides the mkstemp() call or
>> > >> not.
>> > >> Searching... Found bugzilla reports against mkstemp on Cygwin.
>> > > Is Cygwin a platform that should be targeted for the server backend
>> > > these days?
>> > >
>> > > I can understand making sure that libpq works on Cygwin, but is there
>> > > any reason at all to run a Pg server backend on Cygwin rather than as
>> > > native Windows binaries?
>> >
>> > I'm not suggesting immediately dropping working support, since this is
>> > so trivially worked around. I'm just wondering why anybody cares about
>> > the platform.
>>
>> I have suggested similar before, and been voted down :) iirc Andrew uses
>> it, no? Either way, the consensus earlier had been that as long as it
>> doesn't require major surgery or blocks something else, we should try to
>> keep it working. And as you say this sounds like something that can be
>> handled trivially, I think now is not the time.
>>
>
> No, I only use the client. But then I support plenty of things I don't use.

Oh, I somehow thought you were. And yes, we all support things we
don't use - but it certainly helps if there is *someone* out there who
uses it. Having a buildfarm animal (which we do) only goes so far...

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

On 01/21/2013 07:05 AM, Magnus Hagander wrote:
>> No, I only use the client. But then I support plenty of things I don't use.
> Oh, I somehow thought you were. And yes, we all support things we
> don't use - but it certainly helps if there is *someone* out there who
> uses it. Having a buildfarm animal (which we do) only goes so far...
>

It is being used. I get emails from time to time from people asking
about it.

cheers

andrew

On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
>>>>> On using mktemp, linux compilation gives below warning
>>>>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>>>>

>>
>>> Everywhere else that we need to do something like this, we just use our
>>> own PID to disambiguate, ie
>>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
>>> There is no need to deviate from that pattern or introduce portability
>>> issues, since we can reasonably assume that no non-Postgres programs are
>>> creating files in this directory.
>
>> Thanks for the enlightenment, I will post a new version soon.

> Here it is.

The patch sent by you works fine.
It needs small modification as below:

The "auto.conf.d" directory should follow the postgresql.conf file directory not the data_directory.
The same is validated while parsing the postgresql.conf configuration file.

Patch is changed to use the postgresql.conf file directory as below.

StrNCpy(ConfigFileDir, ConfigFileName, sizeof(ConfigFileDir));
get_parent_directory(ConfigFileDir);
/* Frame auto conf name and auto conf sample temp file name */
snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
ConfigFileDir,
PG_AUTOCONF_DIR,
PG_AUTOCONF_FILENAME);

This closes all comments raised till now for this patch.
Kindly let me know if you feel something is missing?

With Regards,
Amit Kapila.

2013-01-22 13:32 keltezéssel, Amit kapila írta:
> On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
> 2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
>> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
>>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
>>>>>> On using mktemp, linux compilation gives below warning
>>>>>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>
>>>> Everywhere else that we need to do something like this, we just use our
>>>> own PID to disambiguate, ie
>>>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
>>>> There is no need to deviate from that pattern or introduce portability
>>>> issues, since we can reasonably assume that no non-Postgres programs are
>>>> creating files in this directory.
>>> Thanks for the enlightenment, I will post a new version soon.
>> Here it is.
> The patch sent by you works fine.
> It needs small modification as below:
>
> The "auto.conf.d" directory should follow the postgresql.conf file directory not the data_directory.
> The same is validated while parsing the postgresql.conf configuration file.
>
> Patch is changed to use the postgresql.conf file directory as below.
>
> StrNCpy(ConfigFileDir, ConfigFileName, sizeof(ConfigFileDir));
> get_parent_directory(ConfigFileDir);
> /* Frame auto conf name and auto conf sample temp file name */
> snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
> ConfigFileDir,
> PG_AUTOCONF_DIR,
> PG_AUTOCONF_FILENAME);

Maybe it's just me but I prefer to have identical
settings across all replicated servers. But I agree
that there can be use cases with different setups.

All in all, this change makes it necessary to run the
same SET PERSISTENT statements on all slave servers,
too, to make them identical again if the configuration
is separated from the data directory (like on Debian
or Ubuntu using the default packages). This needs to be
documented explicitly.

>
> This closes all comments raised till now for this patch.
> Kindly let me know if you feel something is missing?

I can't think of anything else.

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 Tuesday, January 22, 2013 7:10 PM Zoltán Böszörményi wrote:
> 2013-01-22 13:32 keltezéssel, Amit kapila írta:
> > On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
> > 2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
> >> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
> >>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> >>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
> >>>>>> On using mktemp, linux compilation gives below warning
> >>>>>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
> >
> >>>> Everywhere else that we need to do something like this, we just
> use our
> >>>> own PID to disambiguate, ie
> >>>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
> >>>> There is no need to deviate from that pattern or introduce
> portability
> >>>> issues, since we can reasonably assume that no non-Postgres
> programs are
> >>>> creating files in this directory.
> >>> Thanks for the enlightenment, I will post a new version soon.
> >> Here it is.
> > The patch sent by you works fine.
> > It needs small modification as below:
> >
> > The "auto.conf.d" directory should follow the postgresql.conf file
> directory not the data_directory.
> > The same is validated while parsing the postgresql.conf configuration
> file.
> >
> > Patch is changed to use the postgresql.conf file directory as below.
> >
> > StrNCpy(ConfigFileDir, ConfigFileName, sizeof(ConfigFileDir));
> > get_parent_directory(ConfigFileDir);
> > /* Frame auto conf name and auto conf sample temp file name */
> > snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
> > ConfigFileDir,
> > PG_AUTOCONF_DIR,
> > PG_AUTOCONF_FILENAME);
>
> Maybe it's just me but I prefer to have identical
> settings across all replicated servers. But I agree
> that there can be use cases with different setups.
>
> All in all, this change makes it necessary to run the
> same SET PERSISTENT statements on all slave servers,
> too, to make them identical again if the configuration
> is separated from the data directory (like on Debian
> or Ubuntu using the default packages). This needs to be
> documented explicitly.

SET PERSISTENT command needs to run on all slave servers even if the path we
take w.r.t data directory
unless user takes backup after command.

> >
> > This closes all comments raised till now for this patch.
> > Kindly let me know if you feel something is missing?
>
> I can't think of anything else.

In that case can you mark it as "Ready For Committer"

With Regards,
Amit Kapila.

On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Tuesday, January 22, 2013 7:10 PM Zoltán Böszörményi wrote:
>> 2013-01-22 13:32 keltezéssel, Amit kapila írta:
>> > On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
>> > 2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
>> >> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
>> >>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>> >>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
>> >>>>>> On using mktemp, linux compilation gives below warning
>> >>>>>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>> >
>> >>>> Everywhere else that we need to do something like this, we just
>> use our
>> >>>> own PID to disambiguate, ie
>> >>>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
>> >>>> There is no need to deviate from that pattern or introduce
>> portability
>> >>>> issues, since we can reasonably assume that no non-Postgres
>> programs are
>> >>>> creating files in this directory.
>> >>> Thanks for the enlightenment, I will post a new version soon.
>> >> Here it is.
>> > The patch sent by you works fine.
>> > It needs small modification as below:
>> >
>> > The "auto.conf.d" directory should follow the postgresql.conf file
>> directory not the data_directory.
>> > The same is validated while parsing the postgresql.conf configuration
>> file.
>> >
>> > Patch is changed to use the postgresql.conf file directory as below.
>> >
>> > StrNCpy(ConfigFileDir, ConfigFileName, sizeof(ConfigFileDir));
>> > get_parent_directory(ConfigFileDir);
>> > /* Frame auto conf name and auto conf sample temp file name */
>> > snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
>> > ConfigFileDir,
>> > PG_AUTOCONF_DIR,
>> > PG_AUTOCONF_FILENAME);
>>
>> Maybe it's just me but I prefer to have identical
>> settings across all replicated servers. But I agree
>> that there can be use cases with different setups.
>>
>> All in all, this change makes it necessary to run the
>> same SET PERSISTENT statements on all slave servers,
>> too, to make them identical again if the configuration
>> is separated from the data directory (like on Debian
>> or Ubuntu using the default packages). This needs to be
>> documented explicitly.
>
> SET PERSISTENT command needs to run on all slave servers even if the path we
> take w.r.t data directory
> unless user takes backup after command.

Is it safe to write something in the directory other than data directory
via SQL?

postgres user usually has the write permission for the configuration
directory like /etc/postgresql?

>> > This closes all comments raised till now for this patch.
>> > Kindly let me know if you feel something is missing?
>>
>> I can't think of anything else.

For removing
+ a configuration entry from <filename>postgresql.auto.conf</filename> file,
+ use <command>RESET PERSISTENT</command>. The values will be effective
+ after reload of server configuration (SIGHUP) or server startup.

The description of RESET PERSISTENT is in the document, but it
seems not to be implemented.

Regards,

--
Fujii Masao

On Tue, Jan 22, 2013 at 11:54 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>> On Tuesday, January 22, 2013 7:10 PM Zoltán Böszörményi wrote:
>>> 2013-01-22 13:32 keltezéssel, Amit kapila írta:
>>> > On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
>>> > 2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
>>> >> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
>>> >>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>> >>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
>>> >>>>>> On using mktemp, linux compilation gives below warning
>>> >>>>>> warning: the use of `mktemp' is dangerous, better use `mkstemp'
>>> >
>>> >>>> Everywhere else that we need to do something like this, we just
>>> use our
>>> >>>> own PID to disambiguate, ie
>>> >>>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
>>> >>>> There is no need to deviate from that pattern or introduce
>>> portability
>>> >>>> issues, since we can reasonably assume that no non-Postgres
>>> programs are
>>> >>>> creating files in this directory.
>>> >>> Thanks for the enlightenment, I will post a new version soon.
>>> >> Here it is.
>>> > The patch sent by you works fine.
>>> > It needs small modification as below:
>>> >
>>> > The "auto.conf.d" directory should follow the postgresql.conf file
>>> directory not the data_directory.
>>> > The same is validated while parsing the postgresql.conf configuration
>>> file.
>>> >
>>> > Patch is changed to use the postgresql.conf file directory as below.
>>> >
>>> > StrNCpy(ConfigFileDir, ConfigFileName, sizeof(ConfigFileDir));
>>> > get_parent_directory(ConfigFileDir);
>>> > /* Frame auto conf name and auto conf sample temp file name */
>>> > snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
>>> > ConfigFileDir,
>>> > PG_AUTOCONF_DIR,
>>> > PG_AUTOCONF_FILENAME);
>>>
>>> Maybe it's just me but I prefer to have identical
>>> settings across all replicated servers. But I agree
>>> that there can be use cases with different setups.
>>>
>>> All in all, this change makes it necessary to run the
>>> same SET PERSISTENT statements on all slave servers,
>>> too, to make them identical again if the configuration
>>> is separated from the data directory (like on Debian
>>> or Ubuntu using the default packages). This needs to be
>>> documented explicitly.
>>
>> SET PERSISTENT command needs to run on all slave servers even if the path we
>> take w.r.t data directory
>> unless user takes backup after command.
>
> Is it safe to write something in the directory other than data directory
> via SQL?
>
> postgres user usually has the write permission for the configuration
> directory like /etc/postgresql?
>
>>> > This closes all comments raised till now for this patch.
>>> > Kindly let me know if you feel something is missing?
>>>
>>> I can't think of anything else.
>
> For removing
> + a configuration entry from <filename>postgresql.auto.conf</filename> file,
> + use <command>RESET PERSISTENT</command>. The values will be effective
> + after reload of server configuration (SIGHUP) or server startup.
>
> The description of RESET PERSISTENT is in the document, but it
> seems not to be implemented.

I read only document part in the patch and did simple test.
Here are the comments:

storage.sgml needs to be updated.

Doesn't auto.conf.d need to be explained in config.sgml?

When I created my own configuration file in auto.conf.d and
started the server, I got the following LOG message. This
means that users should not create any their own configuration
file there? Why shouldn't they? I think that it's more useful to
allow users to put also their own configuration files in auto.conf.d.
Then users can include any configuration file without adding
new include derective into postgresql.conf because auto.conf.d
has already been included.

LOG: unexpected file found in automatic configuration directory:
"/data/auto.conf.d/hoge"

When I removed postgresql.auto.conf and restarted the server,
I got the following warning message. This is not correct because
I didn't remove "auto.conf.d" from postgresql.conf. What I removed
is only postgresql.auto.conf.

WARNING: Default "auto.conf.d" is not present in postgresql.conf.
Configuration parameters changed with SET PERSISTENT command will not
be effective.

Regards,

--
Fujii Masao

On Tuesday, January 22, 2013 8:25 PM Fujii Masao wrote:
> On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > On Tuesday, January 22, 2013 7:10 PM Zoltán Böszörményi wrote:
> >> 2013-01-22 13:32 keltezéssel, Amit kapila írta:
> >> > On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
> >> > 2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
> >> >> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
> >> >>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> >> >>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
> >> >>>>>> On using mktemp, linux compilation gives below warning
> >> >>>>>> warning: the use of `mktemp' is dangerous, better use
> `mkstemp'
> >> >
> >> >>>> Everywhere else that we need to do something like this, we just
> >> use our
> >> >>>> own PID to disambiguate, ie
> >> >>>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
> >> >>>> There is no need to deviate from that pattern or introduce
> >> portability
> >> >>>> issues, since we can reasonably assume that no non-Postgres
> >> programs are
> >> >>>> creating files in this directory.
> >> >>> Thanks for the enlightenment, I will post a new version soon.
> >> >> Here it is.
> >> > The patch sent by you works fine.
> >> > It needs small modification as below:
> >> >
> >> > The "auto.conf.d" directory should follow the postgresql.conf file
> >> directory not the data_directory.
> >> > The same is validated while parsing the postgresql.conf
> configuration
> >> file.
> >> >
> >> > Patch is changed to use the postgresql.conf file directory as
> below.
> >> >
> >> > StrNCpy(ConfigFileDir, ConfigFileName, sizeof(ConfigFileDir));
> >> > get_parent_directory(ConfigFileDir);
> >> > /* Frame auto conf name and auto conf sample temp file name */
> >> > snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
> >> > ConfigFileDir,
> >> > PG_AUTOCONF_DIR,
> >> > PG_AUTOCONF_FILENAME);
> >>
> >> Maybe it's just me but I prefer to have identical
> >> settings across all replicated servers. But I agree
> >> that there can be use cases with different setups.
> >>
> >> All in all, this change makes it necessary to run the
> >> same SET PERSISTENT statements on all slave servers,
> >> too, to make them identical again if the configuration
> >> is separated from the data directory (like on Debian
> >> or Ubuntu using the default packages). This needs to be
> >> documented explicitly.
> >
> > SET PERSISTENT command needs to run on all slave servers even if the
> path we
> > take w.r.t data directory
> > unless user takes backup after command.
>
> Is it safe to write something in the directory other than data
> directory
> via SQL?
>
> postgres user usually has the write permission for the configuration
> directory like /etc/postgresql?

As postgresql.conf will also be in same path and if user can change that,
then what's the problem with postgresql.auto.conf

Do you see any security risk?

> >> > This closes all comments raised till now for this patch.
> >> > Kindly let me know if you feel something is missing?
> >>
> >> I can't think of anything else.
>
> For removing
> + a configuration entry from
> <filename>postgresql.auto.conf</filename> file,
> + use <command>RESET PERSISTENT</command>. The values will be
> effective
> + after reload of server configuration (SIGHUP) or server startup.
>
> The description of RESET PERSISTENT is in the document, but it
> seems not to be implemented.

This command support has been removed from patch due to parser issues, so it
should be removed from documentation as well. I shall fix this along with
other issues raised by you.

With Regards,
Amit Kapila.

On Tuesday, January 22, 2013 10:14 PM Fujii Masao wrote:
> On Tue, Jan 22, 2013 at 11:54 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
> wrote:
> > On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila
> <amit(dot)kapila(at)huawei(dot)com> wrote:
> >> On Tuesday, January 22, 2013 7:10 PM Zoltán Böszörményi wrote:
> >>> 2013-01-22 13:32 keltezéssel, Amit kapila írta:
> >>> > On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
> >>> > 2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
> >>> >> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
> >>> >>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> >>> >>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
> >>> >>>>>> On using mktemp, linux compilation gives below warning
> >>> >>>>>> warning: the use of `mktemp' is dangerous, better use
> `mkstemp'
> >>> >
> >>> >>>> Everywhere else that we need to do something like this, we
> just
> >>> use our
> >>> >>>> own PID to disambiguate, ie
> >>> >>>> sprintf(tempfilename, "/path/to/file.%d", (int)
> getpid());
> >>> >>>> There is no need to deviate from that pattern or introduce
> >>> portability
> >>> >>>> issues, since we can reasonably assume that no non-Postgres
> >>> programs are
> >>> >>>> creating files in this directory.
> >>> >>> Thanks for the enlightenment, I will post a new version soon.
> >>> >> Here it is.
> >>> > The patch sent by you works fine.
> >>> > It needs small modification as below:
> >>> >
> >>> > The "auto.conf.d" directory should follow the postgresql.conf
> file
> >>> directory not the data_directory.
> >>> > The same is validated while parsing the postgresql.conf
> configuration
> >>> file.
> >>> >
> >>> > Patch is changed to use the postgresql.conf file directory as
> below.
> >>> >
> >>> > StrNCpy(ConfigFileDir, ConfigFileName, sizeof(ConfigFileDir));
> >>> > get_parent_directory(ConfigFileDir);
> >>> > /* Frame auto conf name and auto conf sample temp file name */
> >>> > snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
> >>> > ConfigFileDir,
> >>> > PG_AUTOCONF_DIR,
> >>> > PG_AUTOCONF_FILENAME);
> >>>
> >>> Maybe it's just me but I prefer to have identical
> >>> settings across all replicated servers. But I agree
> >>> that there can be use cases with different setups.
> >>>
> >>> All in all, this change makes it necessary to run the
> >>> same SET PERSISTENT statements on all slave servers,
> >>> too, to make them identical again if the configuration
> >>> is separated from the data directory (like on Debian
> >>> or Ubuntu using the default packages). This needs to be
> >>> documented explicitly.
> >>
> >> SET PERSISTENT command needs to run on all slave servers even if the
> path we
> >> take w.r.t data directory
> >> unless user takes backup after command.
> >
> > Is it safe to write something in the directory other than data
> directory
> > via SQL?
> >
> > postgres user usually has the write permission for the configuration
> > directory like /etc/postgresql?
> >
> >>> > This closes all comments raised till now for this patch.
> >>> > Kindly let me know if you feel something is missing?
> >>>
> >>> I can't think of anything else.
> >
> > For removing
> > + a configuration entry from
> <filename>postgresql.auto.conf</filename> file,
> > + use <command>RESET PERSISTENT</command>. The values will be
> effective
> > + after reload of server configuration (SIGHUP) or server startup.
> >
> > The description of RESET PERSISTENT is in the document, but it
> > seems not to be implemented.
>
> I read only document part in the patch and did simple test.
> Here are the comments:
>
> storage.sgml needs to be updated.
>
> Doesn't auto.conf.d need to be explained in config.sgml?

I shall update both these files.

> When I created my own configuration file in auto.conf.d and
> started the server, I got the following LOG message. This
> means that users should not create any their own configuration
> file there? Why shouldn't they?

It can be allowed, but for now it has been kept such that automatically
generated conf files will be
present in this directory, that’s what the name 'auto.conf.d' suggests.

> I think that it's more useful to
> allow users to put also their own configuration files in auto.conf.d.
> Then users can include any configuration file without adding
> new include derective into postgresql.conf because auto.conf.d
> has already been included.
>
> LOG: unexpected file found in automatic configuration directory:
> "/data/auto.conf.d/hoge"

Personally I don't have objection in getting other user specific conf files
in this directory.
But I think then we should name this directory also differently as it was
initially (conf_dir) in the Patch.
I would like to see opinion of others also in this matter, so that later I
don't need to
change it back to what currently it is.

> When I removed postgresql.auto.conf and restarted the server,
> I got the following warning message. This is not correct because
> I didn't remove "auto.conf.d" from postgresql.conf. What I removed
> is only postgresql.auto.conf.
>
> WARNING: Default "auto.conf.d" is not present in postgresql.conf.
> Configuration parameters changed with SET PERSISTENT command will not
> be effective.

How about changing it to below message:

WARNING: File 'postgresql.auto.conf' is not accessible, either file
'postgresql.auto.conf' or folder '%s' doesn't exist or default "auto.conf.d"
is not present in postgresql.conf.
Configuration parameters changed with SET PERSISTENT command will not be
effective.

The reason I have kept single error message for all issues related to
'postgresql.auto.conf' is to keep code little simpler, else I need to
distinguish for each particular case.

With Regards,
Amit Kapila.

On Wed, Jan 23, 2013 at 12:24 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Tuesday, January 22, 2013 8:25 PM Fujii Masao wrote:
>> On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
>> wrote:
>> > On Tuesday, January 22, 2013 7:10 PM Zoltán Böszörményi wrote:
>> >> 2013-01-22 13:32 keltezéssel, Amit kapila írta:
>> >> > On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
>> >> > 2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
>> >> >> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
>> >> >>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>> >> >>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
>> >> >>>>>> On using mktemp, linux compilation gives below warning
>> >> >>>>>> warning: the use of `mktemp' is dangerous, better use
>> `mkstemp'
>> >> >
>> >> >>>> Everywhere else that we need to do something like this, we just
>> >> use our
>> >> >>>> own PID to disambiguate, ie
>> >> >>>> sprintf(tempfilename, "/path/to/file.%d", (int) getpid());
>> >> >>>> There is no need to deviate from that pattern or introduce
>> >> portability
>> >> >>>> issues, since we can reasonably assume that no non-Postgres
>> >> programs are
>> >> >>>> creating files in this directory.
>> >> >>> Thanks for the enlightenment, I will post a new version soon.
>> >> >> Here it is.
>> >> > The patch sent by you works fine.
>> >> > It needs small modification as below:
>> >> >
>> >> > The "auto.conf.d" directory should follow the postgresql.conf file
>> >> directory not the data_directory.
>> >> > The same is validated while parsing the postgresql.conf
>> configuration
>> >> file.
>> >> >
>> >> > Patch is changed to use the postgresql.conf file directory as
>> below.
>> >> >
>> >> > StrNCpy(ConfigFileDir, ConfigFileName, sizeof(ConfigFileDir));
>> >> > get_parent_directory(ConfigFileDir);
>> >> > /* Frame auto conf name and auto conf sample temp file name */
>> >> > snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
>> >> > ConfigFileDir,
>> >> > PG_AUTOCONF_DIR,
>> >> > PG_AUTOCONF_FILENAME);
>> >>
>> >> Maybe it's just me but I prefer to have identical
>> >> settings across all replicated servers. But I agree
>> >> that there can be use cases with different setups.
>> >>
>> >> All in all, this change makes it necessary to run the
>> >> same SET PERSISTENT statements on all slave servers,
>> >> too, to make them identical again if the configuration
>> >> is separated from the data directory (like on Debian
>> >> or Ubuntu using the default packages). This needs to be
>> >> documented explicitly.
>> >
>> > SET PERSISTENT command needs to run on all slave servers even if the
>> path we
>> > take w.r.t data directory
>> > unless user takes backup after command.
>>
>> Is it safe to write something in the directory other than data
>> directory
>> via SQL?
>>
>> postgres user usually has the write permission for the configuration
>> directory like /etc/postgresql?
>
> As postgresql.conf will also be in same path and if user can change that,
> then what's the problem with postgresql.auto.conf

If the configuration directory like /etc is owned by root and only root has
a write permission for it, the user running PostgreSQL server would not
be able to update postgresql.auto.conf there. OTOH, even in that case,
a user can switch to root and update the configuration file there. I'm not
sure whether the configuration directory is usually writable by the user
running PostgreSQL server in Debian or Ubuntu, though.

> Do you see any security risk?

I have no idea. I just wondered that because some functions like pg_file_write()
in adminpack are restricted to write something in the directory other
than $PGDATA.

>> >> > This closes all comments raised till now for this patch.
>> >> > Kindly let me know if you feel something is missing?
>> >>
>> >> I can't think of anything else.
>>
>> For removing
>> + a configuration entry from
>> <filename>postgresql.auto.conf</filename> file,
>> + use <command>RESET PERSISTENT</command>. The values will be
>> effective
>> + after reload of server configuration (SIGHUP) or server startup.
>>
>> The description of RESET PERSISTENT is in the document, but it
>> seems not to be implemented.
>
> This command support has been removed from patch due to parser issues, so it
> should be removed from documentation as well. I shall fix this along with
> other issues raised by you.

Okay.

Regards,

--
Fujii Masao

On Wed, Jan 23, 2013 at 6:18 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Tuesday, January 22, 2013 10:14 PM Fujii Masao wrote:
>> When I removed postgresql.auto.conf and restarted the server,
>> I got the following warning message. This is not correct because
>> I didn't remove "auto.conf.d" from postgresql.conf. What I removed
>> is only postgresql.auto.conf.
>>
>> WARNING: Default "auto.conf.d" is not present in postgresql.conf.
>> Configuration parameters changed with SET PERSISTENT command will not
>> be effective.
>
> How about changing it to below message:
>
> WARNING: File 'postgresql.auto.conf' is not accessible, either file
> 'postgresql.auto.conf' or folder '%s' doesn't exist or default "auto.conf.d"
> is not present in postgresql.conf.
> Configuration parameters changed with SET PERSISTENT command will not be
> effective.

Or we should suppress such a warning message in the case where
postgresql.auto.conf doesn't exist? SET PERSISTENT creates that
file automatically if it doesn't exist. So we can expect that configuration
parameters changed with SET PERSISTENT WILL be effective.

This warning message implies that the line "include_dir 'auto.conf.d'"
must not be removed from postgresql.conf? If so, we should warn that
in both document and postgresql.conf.sample? Or we should hard-code
so that something like auto.conf.d is always included even when that
include_dir directive doesn't exist?

Regards,

--
Fujii Masao

Fujii Masao escribió:
> On Wed, Jan 23, 2013 at 12:24 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:

> >> Is it safe to write something in the directory other than data
> >> directory
> >> via SQL?
> >>
> >> postgres user usually has the write permission for the configuration
> >> directory like /etc/postgresql?
> >
> > As postgresql.conf will also be in same path and if user can change that,
> > then what's the problem with postgresql.auto.conf
>
> If the configuration directory like /etc is owned by root and only root has
> a write permission for it, the user running PostgreSQL server would not
> be able to update postgresql.auto.conf there. OTOH, even in that case,
> a user can switch to root and update the configuration file there. I'm not
> sure whether the configuration directory is usually writable by the user
> running PostgreSQL server in Debian or Ubuntu, though.

Yes it is -- the /etc/postgresql/<version>/<cluster> directory (where
postgresql.conf resides) is owned by user postgres.

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

On 2013-01-22 12:32:07 +0000, Amit kapila wrote:
> This closes all comments raised till now for this patch.
> Kindly let me know if you feel something is missing?

I am coming late to this patch, so bear with me if I repeat somethign
said elsewhere.

Review comments of cursory pass through the patch:
* most comments are hard to understand. I know the problem of that
being hard for a non-native speaker by heart, but I think another pass
over them would be good thing.
* The gram.y changes arround set_rest_(more|common) seem pretty confused
to me. E.g. its not possible anymore to set the timezone for a
function. And why is it possible to persistently set the search path,
but not client encoding? Why is FROM CURRENT in set_rest_more?
* set_config_file should elog(ERROR), not return on an unhandled
setstmt->kind
* why are you creating AutoConfFileName if its not stat'able? It seems
better to simply skip parsing the old file in that case
* Writing the temporary file to .$pid seems like a bad idea, better use
one file for that, SET PERSISTENT is protected by an exclusive lock
anyway.
* the write sequence should be:
* fsync(tempfile)
* fsync(directory)
* rename(tempfile, configfile)
* fsync(configfile)
* fsync(directory)
* write_auto_conf_file should probably escape quoted values?
* coding style should be adhered to more closesly, there are many
if (pointer) which should be if (pointer != NULL), single-line blocks
enclosed in curlies which shouldn't, etc.
* replace_auto_config_file and surrounding functions need more comments
in the header
* the check that prevents persistent SETs in a transaction should rather
be in utility.c and use PreventTransactionChain like most of the
others that need to do that (c.f. T_CreatedbStmt).

I think this patch is a good bit away of being ready for committer...

Greetings,

Andres Freund

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

On Wednesday, January 23, 2013 9:51 PM Fujii Masao wrote:
> On Wed, Jan 23, 2013 at 6:18 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > On Tuesday, January 22, 2013 10:14 PM Fujii Masao wrote:
> >> When I removed postgresql.auto.conf and restarted the server,
> >> I got the following warning message. This is not correct because
> >> I didn't remove "auto.conf.d" from postgresql.conf. What I removed
> >> is only postgresql.auto.conf.
> >>
> >> WARNING: Default "auto.conf.d" is not present in postgresql.conf.
> >> Configuration parameters changed with SET PERSISTENT command will
> not
> >> be effective.
> >
> > How about changing it to below message:
> >
> > WARNING: File 'postgresql.auto.conf' is not accessible, either file
> > 'postgresql.auto.conf' or folder '%s' doesn't exist or default
> "auto.conf.d"
> > is not present in postgresql.conf.
> > Configuration parameters changed with SET PERSISTENT command will not
> be
> > effective.
>
> Or we should suppress such a warning message in the case where
> postgresql.auto.conf doesn't exist? SET PERSISTENT creates that
> file automatically if it doesn't exist. So we can expect that
> configuration
> parameters changed with SET PERSISTENT WILL be effective.

This Warning (message) can come during startup or reload, so if at that time
postgresql.auto.conf doesn't exist the parameters set by SET PERSISTENT
previous to startup or reload will not be effective.
So IMO, we can think to change part of this message as below:

"Configuration parameters changed before start of server with SET PERSISTENT
command will not be effective."
Any other idea for change in message?

> This warning message implies that the line "include_dir 'auto.conf.d'"
> must not be removed from postgresql.conf? If so, we should warn that
> in both document and postgresql.conf.sample?

I shall fix this as below:

Change current WARNING in postgresql.conf.sample as below:

# This includes the default configuration directory included to support
# SET PERSISTENT statement.
#
# WARNING: User should not remove below include_dir or directory
auto.conf.d,
as both are required to make SET PERSISTENT command work.
Any configuration parameter values specified after this line
# will override the values set by SET PERSISTENT statement.

Also Update this in runtime.sgml under heading "Starting the Database
Server"

> Or we should hard-code
> so that something like auto.conf.d is always included even when that
> include_dir directive doesn't exist?

I think this can create problem for user who intentionally wants to remove
this directory inclusion.

With Regards,
Amit Kapila.

On Wednesday, January 23, 2013 11:51 PM Alvaro Herrera wrote:
> Fujii Masao escribió:
> > On Wed, Jan 23, 2013 at 12:24 PM, Amit Kapila
> <amit(dot)kapila(at)huawei(dot)com> wrote:
>
> > >> Is it safe to write something in the directory other than data
> > >> directory
> > >> via SQL?
> > >>
> > >> postgres user usually has the write permission for the
> configuration
> > >> directory like /etc/postgresql?
> > >
> > > As postgresql.conf will also be in same path and if user can change
> that,
> > > then what's the problem with postgresql.auto.conf
> >
> > If the configuration directory like /etc is owned by root and only
> root has
> > a write permission for it, the user running PostgreSQL server would
> not
> > be able to update postgresql.auto.conf there. OTOH, even in that
> case,
> > a user can switch to root and update the configuration file there.
> I'm not
> > sure whether the configuration directory is usually writable by the
> user
> > running PostgreSQL server in Debian or Ubuntu, though.
>
> Yes it is -- the /etc/postgresql/<version>/<cluster> directory (where
> postgresql.conf resides) is owned by user postgres.

So in that case we can consider postgresql.auto.conf to be in path w.r.t
postgresql.conf instead of data directory.

With Regards,
Amit Kapila.

On Thursday, January 24, 2013 1:56 AM Andres Freund wrote:
> On 2013-01-22 12:32:07 +0000, Amit kapila wrote:
> > This closes all comments raised till now for this patch.
> > Kindly let me know if you feel something is missing?
>
> I am coming late to this patch, so bear with me if I repeat somethign
> said elsewhere.

Thanks for looking at patch.

> Review comments of cursory pass through the patch:
> * most comments are hard to understand. I know the problem of that
> being hard for a non-native speaker by heart, but I think another
> pass
> over them would be good thing.

I shall give another pass to improve the comments in code.

> * The gram.y changes arround set_rest_(more|common) seem pretty
> confused
> to me.

>E.g. its not possible anymore to set the timezone for a function.

What do you exactly mean by this part of comment.

> And why is it possible to persistently set the search path,
> but not client encoding? Why is FROM CURRENT in set_rest_more?

I think the main reason was some of the commands can work only in
transaction
and as SET Persistent cannot work inside transaction block, so I had kept
some seggregation.
I shall reply you the exact reason in separate mail.

> * set_config_file should elog(ERROR), not return on an unhandled
> setstmt->kind

Shall fix it.

> * why are you creating AutoConfFileName if its not stat'able? It seems
> better to simply skip parsing the old file in that case

Sure, it's better to handle as you are suggesting.

> * Writing the temporary file to .$pid seems like a bad idea, better use
> one file for that, SET PERSISTENT is protected by an exclusive lock
> anyway.

I think we can use one temporary file, infact that was one of the ways I
have asked in one of the previous mails.
However Tom and Zoltan felt this is better way to do it.
I can think of one reason where it will be better to have .$pid, and that
is if some one has opened the
file manually, then all other sessions can fail (in WINDOWS). Infact this
is one of test Zoltan had performed.

> * the write sequence should be:
> * fsync(tempfile)
> * fsync(directory)
> * rename(tempfile, configfile)
> * fsync(configfile)
> * fsync(directory)

Why do we need fsync(directory) and fsync(configfile)?

> * write_auto_conf_file should probably escape quoted values?
Can you please elaborate more, I am not able to understand your point?

> * coding style should be adhered to more closesly, there are many
> if (pointer) which should be if (pointer != NULL),

Are you pointing in function validate_conf_option(const char *name, char
*value)
for below usage:
+ if (value)

> single-line blocks
> enclosed in curlies which shouldn't, etc.
Shall fix.

> * replace_auto_config_file and surrounding functions need more comments
> in the header

Sure, shall add more comments in function header of following functions:
validate_conf_option()
RemoveAutoConfTmpFiles()
write_auto_conf_file()
replace_auto_config_value()

> * the check that prevents persistent SETs in a transaction should
> rather
> be in utility.c and use PreventTransactionChain like most of the
> others that need to do that (c.f. T_CreatedbStmt).

We can move the check in utility.c, but if we use PreventTransactionChain,
then it will be disallowed
in functions as well. But the original idea was to not support in
transaction blocks only.
So I feel use of current function IsTransactionBlock() should be okay.

With Regards,
Amit Kapila.

On 2013-01-24 16:45:42 +0530, Amit Kapila wrote:
> > * The gram.y changes arround set_rest_(more|common) seem pretty
> > confused
> > to me.
>
> >E.g. its not possible anymore to set the timezone for a function.
>
> What do you exactly mean by this part of comment.

The set_rest_more production is used in FunctionSetResetClause and youve
removed capabilities from it.

> > And why is it possible to persistently set the search path,
> > but not client encoding? Why is FROM CURRENT in set_rest_more?
>
> I think the main reason was some of the commands can work only in
> transaction and as SET Persistent cannot work inside transaction block, so I had kept
> some seggregation.

Yes, I can see reasons for doing this, I just think the split isn't
correct as youve done it.

> > * Writing the temporary file to .$pid seems like a bad idea, better use
> > one file for that, SET PERSISTENT is protected by an exclusive lock
> > anyway.
>
> I think we can use one temporary file, infact that was one of the ways I
> have asked in one of the previous mails.
> However Tom and Zoltan felt this is better way to do it.

The have? I didn't read it like that. The file can only ever written by
a running postmaster and we already have code that ensures that. There's
absolutely no need for the tempfile to have a nondeterministic
name. That makes cleanup way easier as well.

> I can think of one reason where it will be better to have .$pid, and that
> is if some one has opened the
> file manually, then all other sessions can fail (in WINDOWS). Infact this
> is one of test Zoltan had performed.

Why on earth should somebody have the tempfile open? There's an
exclusive lock around writing the file in your code and if anybody
interferes like that with postgres' temporary data you have far bigger
problems than SET PERSISTENT erroring out.

> > * the write sequence should be:
> > * fsync(tempfile)
> > * fsync(directory)
> > * rename(tempfile, configfile)
> > * fsync(configfile)
> > * fsync(directory)
>
> Why do we need fsync(directory) and fsync(configfile)?

So they don't vanish /get corrupted after a crash? The above is the
canonically safe way to safely write a file without an invalid file ever
being visible.

> > * write_auto_conf_file should probably escape quoted values?
> Can you please elaborate more, I am not able to understand your
> point?

You do something like (don't have the code right here)
if (quoted)
appendStringInfo("= \"%s\"", value).
what happens if value contains a "?

> > * coding style should be adhered to more closesly, there are many
> > if (pointer) which should be if (pointer != NULL),
>
> Are you pointing in function validate_conf_option(const char *name, char *value)
> for below usage:
> + if (value)

For example, yes.

> > * the check that prevents persistent SETs in a transaction should
> > rather
> > be in utility.c and use PreventTransactionChain like most of the
> > others that need to do that (c.f. T_CreatedbStmt).
>
> We can move the check in utility.c, but if we use PreventTransactionChain,
> then it will be disallowed in functions as well. But the original idea was to not support in
> transaction blocks only.
> So I feel use of current function IsTransactionBlock() should be okay.

I don't think its even remotely safe to allow doing this from a
function. Doing it there explicitly allows doing it in a transaction.

Should we find out its safe, fine, relaxing restrictions lateron is no
problem, imposing ones after introducing a feature is.

Greetings,

Andres Freund

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

On Thursday, January 24, 2013 5:25 PM Andres Freund wrote:
> On 2013-01-24 16:45:42 +0530, Amit Kapila wrote:
> > > * The gram.y changes arround set_rest_(more|common) seem pretty
> > > confused
> > > to me.
> >
> > >E.g. its not possible anymore to set the timezone for a function.
> >
> > What do you exactly mean by this part of comment.
>
> The set_rest_more production is used in FunctionSetResetClause and
> youve
> removed capabilities from it.

set_rest_more has set_reset_common, so there should be no problem.

set_rest_more: /* Generic SET syntaxes: */
set_rest_common
| var_name FROM CURRENT_P
{
VariableSetStmt *n =
makeNode(VariableSetStmt);
n->kind = VAR_SET_CURRENT;
n->name = $1;
$$ = n;
}

> > > And why is it possible to persistently set the search path,
> > > but not client encoding? Why is FROM CURRENT in set_rest_more?
> >
> > I think the main reason was some of the commands can work only in
> > transaction and as SET Persistent cannot work inside transaction
> block, so I had kept
> > some seggregation.
>
> Yes, I can see reasons for doing this, I just think the split isn't
> correct as youve done it.
>
> > > * Writing the temporary file to .$pid seems like a bad idea, better
> use
> > > one file for that, SET PERSISTENT is protected by an exclusive
> lock
> > > anyway.
> >
> > I think we can use one temporary file, infact that was one of the
> ways I
> > have asked in one of the previous mails.
> > However Tom and Zoltan felt this is better way to do it.

> The have? I didn't read it like that. The file can only ever written by
> a running postmaster and we already have code that ensures that.
> There's
> absolutely no need for the tempfile to have a nondeterministic
> name. That makes cleanup way easier as well.

Sure, I understand that cleaning will be easier. So IIUC you are suggesting,
we should just keep
name as postgresql.auto.conf.tmp

In general, please read the below mail where it has been suggested to use
.$pid
http://www.postgresql.org/message-id/28379.1358541135@sss.pgh.pa.us

> > I can think of one reason where it will be better to have .$pid,
> and that
> > is if some one has opened the
> > file manually, then all other sessions can fail (in WINDOWS).
> Infact this
> > is one of test Zoltan had performed.
>
> Why on earth should somebody have the tempfile open? There's an
> exclusive lock around writing the file in your code and if anybody
> interferes like that with postgres' temporary data you have far bigger
> problems than SET PERSISTENT erroring out.

I am also not sure, but may be some people do for test purpose.

> > > * the write sequence should be:
> > > * fsync(tempfile)
> > > * fsync(directory)
> > > * rename(tempfile, configfile)
> > > * fsync(configfile)
> > > * fsync(directory)
> >
> > Why do we need fsync(directory) and fsync(configfile)?
>
> So they don't vanish /get corrupted after a crash? The above is the
> canonically safe way to safely write a file without an invalid file
> ever
> being visible.

Do you think there will be problem if we just use as below:
* fsync(tempfile)
* rename(tempfile, configfile)

I have seen such kind of use elsewhere also in code (writeTimeLineHistory())

> > > * write_auto_conf_file should probably escape quoted values?
> > Can you please elaborate more, I am not able to understand your
> > point?
>
> You do something like (don't have the code right here)
> if (quoted)
> appendStringInfo("= \"%s\"", value).
> what happens if value contains a "?

I think this case is missed. I shall take care to handle the case when value
contains such value.

> > > * coding style should be adhered to more closesly, there are many
> > > if (pointer) which should be if (pointer != NULL),
> >
> > Are you pointing in function validate_conf_option(const char *name,
> char *value)
> > for below usage:
> > + if (value)
>
> For example, yes.

Okay, I shall fix these, but I have seen such code at other places
(set_config_option), that's why I have kept it like how it is currently.

>
> > > * the check that prevents persistent SETs in a transaction should
> > > rather
> > > be in utility.c and use PreventTransactionChain like most of the
> > > others that need to do that (c.f. T_CreatedbStmt).
> >
> > We can move the check in utility.c, but if we use
> PreventTransactionChain,
> > then it will be disallowed in functions as well. But the original
> idea was to not support in
> > transaction blocks only.
> > So I feel use of current function IsTransactionBlock() should be
> okay.
>
> I don't think its even remotely safe to allow doing this from a
> function. Doing it there explicitly allows doing it in a transaction.

As SET command is allowed inside a function, so I don't think without any
reason we should disallow SET PERSISTENT in function.
The reason why it's not allowed in transaction is that we have to delete the
temporary file in transaction end or at rollback which could have made the
logic much more complex.

> Should we find out its safe, fine, relaxing restrictions lateron is no
> problem, imposing ones after introducing a feature is.

I have discussed most of these restrictions in this thread. But there can be
few which I could have missed.

With Regards,
Amit Kapila.

On 2013-01-24 18:37:29 +0530, Amit Kapila wrote:
> On Thursday, January 24, 2013 5:25 PM Andres Freund wrote:
> > On 2013-01-24 16:45:42 +0530, Amit Kapila wrote:
> > > > * The gram.y changes arround set_rest_(more|common) seem pretty
> > > > confused
> > > > to me.
> > >
> > > >E.g. its not possible anymore to set the timezone for a function.
> > >
> > > What do you exactly mean by this part of comment.
> >
> > The set_rest_more production is used in FunctionSetResetClause and
> > youve
> > removed capabilities from it.
>
> set_rest_more has set_reset_common, so there should be no problem.
>
> set_rest_more: /* Generic SET syntaxes: */
> set_rest_common
> | var_name FROM CURRENT_P
> {
> VariableSetStmt *n = makeNode(VariableSetStmt);
> n->kind = VAR_SET_CURRENT;
> n->name = $1;
> $$ = n;
> }

True. I still think that the split youve made isn't right as-is.

> > > > * Writing the temporary file to .$pid seems like a bad idea, better use
> > > > one file for that, SET PERSISTENT is protected by an exclusive lock
> > > > anyway.
> > >
> > > I think we can use one temporary file, infact that was one of the ways I
> > > have asked in one of the previous mails.
> > > However Tom and Zoltan felt this is better way to do it.

> > The have? I didn't read it like that. The file can only ever written by
> > a running postmaster and we already have code that ensures that.
> > There's
> > absolutely no need for the tempfile to have a nondeterministic
> > name. That makes cleanup way easier as well.
>
> Sure, I understand that cleaning will be easier. So IIUC you are suggesting,
> we should just keep
> name as postgresql.auto.conf.tmp
>
> In general, please read the below mail where it has been suggested to use
> .$pid
> http://www.postgresql.org/message-id/28379.1358541135@sss.pgh.pa.us

That was in the context of your use of mkstemp() as far as I read
it. We use constantly named temp files in other locations as well.

> > Why on earth should somebody have the tempfile open? There's an
> > exclusive lock around writing the file in your code and if anybody
> > interferes like that with postgres' temporary data you have far bigger
> > problems than SET PERSISTENT erroring out.
>
> I am also not sure, but may be some people do for test purpose.

That seems like an completely pointless reasoning.

> > > > * the write sequence should be:
> > > > * fsync(tempfile)
> > > > * fsync(directory)
> > > > * rename(tempfile, configfile)
> > > > * fsync(configfile)
> > > > * fsync(directory)
> > >
> > > Why do we need fsync(directory) and fsync(configfile)?
> >
> > So they don't vanish /get corrupted after a crash? The above is the
> > canonically safe way to safely write a file without an invalid file
> > ever
> > being visible.
>
> Do you think there will be problem if we just use as below:
> * fsync(tempfile)
> * rename(tempfile, configfile)
>
> I have seen such kind of use elsewhere also in code (writeTimeLineHistory())

Yea, there are some places where the code isn't entirely safe. No reason
to introduce more of those.
> > > > * the check that prevents persistent SETs in a transaction should
> > > > rather
> > > > be in utility.c and use PreventTransactionChain like most of the
> > > > others that need to do that (c.f. T_CreatedbStmt).
> > >
> > > We can move the check in utility.c, but if we use
> > PreventTransactionChain,
> > > then it will be disallowed in functions as well. But the original
> > idea was to not support in
> > > transaction blocks only.
> > > So I feel use of current function IsTransactionBlock() should be
> > okay.
> >
> > I don't think its even remotely safe to allow doing this from a
> > function. Doing it there explicitly allows doing it in a transaction.
>
> As SET command is allowed inside a function, so I don't think without any
> reason we should disallow SET PERSISTENT in function.
> The reason why it's not allowed in transaction is that we have to delete the
> temporary file in transaction end or at rollback which could have made the
> logic much more complex.

Yea, and allowing use in functions doesn't help you with that at all:

Consider the following plpgsql function:

$$
BEGIN
SET PERSISTENT foo = 'bar';
RAISE ERROR 'blub';
END;
$$

Now you have a SET PERSISTENT that succeeded although the transaction
failed.

Greetings,

Andres Freund

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

Amit Kapila escribió:
> On Wednesday, January 23, 2013 11:51 PM Alvaro Herrera wrote:

> > Yes it is -- the /etc/postgresql/<version>/<cluster> directory (where
> > postgresql.conf resides) is owned by user postgres.
>
> So in that case we can consider postgresql.auto.conf to be in path w.r.t
> postgresql.conf instead of data directory.

That seems sensible to me.

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

On Thursday, January 24, 2013 6:51 PM Andres Freund wrote:
> On 2013-01-24 18:37:29 +0530, Amit Kapila wrote:
> > On Thursday, January 24, 2013 5:25 PM Andres Freund wrote:
> > > On 2013-01-24 16:45:42 +0530, Amit Kapila wrote:
> > > > > * The gram.y changes arround set_rest_(more|common) seem pretty
> > > > > confused
> > > > > to me.
> > > >
> > > > >E.g. its not possible anymore to set the timezone for a
> function.
> > > >
> > > > What do you exactly mean by this part of comment.
> > >
> > > The set_rest_more production is used in FunctionSetResetClause and
> > > youve
> > > removed capabilities from it.
> >
> > set_rest_more has set_reset_common, so there should be no problem.
> >
> > set_rest_more: /* Generic SET syntaxes: */
> > set_rest_common
> > | var_name FROM CURRENT_P
> > {
> > VariableSetStmt *n =
> makeNode(VariableSetStmt);
> > n->kind = VAR_SET_CURRENT;
> > n->name = $1;
> > $$ = n;
> > }
>
> True. I still think that the split youve made isn't right as-is.

I am working on to provide the exact reasoning of split and if some variable
is not appropriate,
I shall do the appropriate movement.

> > > > > * Writing the temporary file to .$pid seems like a bad idea,
> better use
> > > > > one file for that, SET PERSISTENT is protected by an
> exclusive lock
> > > > > anyway.
> > > >
> > > > I think we can use one temporary file, infact that was one of
> the ways I
> > > > have asked in one of the previous mails.
> > > > However Tom and Zoltan felt this is better way to do it.
>
> > > The have? I didn't read it like that. The file can only ever
> written by
> > > a running postmaster and we already have code that ensures that.
> > > There's
> > > absolutely no need for the tempfile to have a nondeterministic
> > > name. That makes cleanup way easier as well.
> >
> > Sure, I understand that cleaning will be easier. So IIUC you are
> suggesting,
> > we should just keep
> > name as postgresql.auto.conf.tmp
> >
> > In general, please read the below mail where it has been suggested to
> use
> > .$pid
> > http://www.postgresql.org/message-id/28379.1358541135@sss.pgh.pa.us
>
> That was in the context of your use of mkstemp() as far as I read
> it. We use constantly named temp files in other locations as well.

Okay, as I told you that I have also proposed initially, so as now you have
pointed it specifically,
I shall do it that way unless somebody will have another strong point of not
doing it this way.

> > > Why on earth should somebody have the tempfile open? There's an
> > > exclusive lock around writing the file in your code and if anybody
> > > interferes like that with postgres' temporary data you have far
> bigger
> > > problems than SET PERSISTENT erroring out.
> >
> > I am also not sure, but may be some people do for test purpose.
>
> That seems like an completely pointless reasoning.

I have no specific reason, so I shall do it the way you have suggested.

> > > > > * the write sequence should be:
> > > > > * fsync(tempfile)
> > > > > * fsync(directory)
> > > > > * rename(tempfile, configfile)
> > > > > * fsync(configfile)
> > > > > * fsync(directory)
> > > >
> > > > Why do we need fsync(directory) and fsync(configfile)?
> > >
> > > So they don't vanish /get corrupted after a crash? The above is the
> > > canonically safe way to safely write a file without an invalid file
> > > ever
> > > being visible.
> >
> > Do you think there will be problem if we just use as below:
> > * fsync(tempfile)
> > * rename(tempfile, configfile)
> >
> > I have seen such kind of use elsewhere also in code
> (writeTimeLineHistory())
>
> Yea, there are some places where the code isn't entirely safe. No
> reason
> to introduce more of those.

Okay, I will check this and let you know if I have any doubts.

> > > > > * the check that prevents persistent SETs in a transaction
> should
> > > > > rather
> > > > > be in utility.c and use PreventTransactionChain like most of
> the
> > > > > others that need to do that (c.f. T_CreatedbStmt).
> > > >
> > > > We can move the check in utility.c, but if we use
> > > PreventTransactionChain,
> > > > then it will be disallowed in functions as well. But the
> original
> > > idea was to not support in
> > > > transaction blocks only.
> > > > So I feel use of current function IsTransactionBlock() should
> be
> > > okay.
> > >
> > > I don't think its even remotely safe to allow doing this from a
> > > function. Doing it there explicitly allows doing it in a
> transaction.
> >
> > As SET command is allowed inside a function, so I don't think without
> any
> > reason we should disallow SET PERSISTENT in function.
> > The reason why it's not allowed in transaction is that we have to
> delete the
> > temporary file in transaction end or at rollback which could have
> made the
> > logic much more complex.
>
> Yea, and allowing use in functions doesn't help you with that at all:
>
> Consider the following plpgsql function:
>
> $$
> BEGIN
> SET PERSISTENT foo = 'bar';
> RAISE ERROR 'blub';
> END;
> $$
>
> Now you have a SET PERSISTENT that succeeded although the transaction
> failed.

I think the core point of this command behavior is that it is not related to
transaction boundary.
So now the question is in such case if we don't allow this command inside
transaction block, why to allow inside
Function?
When user starts transaction and runs this command and then few other
commands and now if he rollback, this command cannot be rolledback.
OTOH in function there is no explicit way to issue rollback, only an error
can cause rollback which is same as when running outside block.

With Regards,
Amit Kapila.

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-01-24 16:45:42 +0530, Amit Kapila wrote:
>>> * Writing the temporary file to .$pid seems like a bad idea, better use
>>> one file for that, SET PERSISTENT is protected by an exclusive lock
>>> anyway.

>> I think we can use one temporary file, infact that was one of the ways I
>> have asked in one of the previous mails.
>> However Tom and Zoltan felt this is better way to do it.

> The have? I didn't read it like that. The file can only ever written by
> a running postmaster and we already have code that ensures that. There's
> absolutely no need for the tempfile to have a nondeterministic
> name. That makes cleanup way easier as well.

Say again? Surely the temp file is being written by whichever backend
is executing SET PERSISTENT, and there could be more than one.

Or, if it isn't and you really do mean that this patch is getting the
postmaster process involved in executing SET PERSISTENT, that's going to
be sufficient grounds for rejection right there. The only way that we
have to keep the postmaster reliable is to not have it doing more than
it absolutely must.

While I have not been paying very much attention to this patch, I have
to say that every time I've looked into the thread I've gotten an
overwhelming impression of overdesign and overcomplication. This should
not be a very large or subtle patch. Write a setting into a temp file,
rename it into place, done.

regards, tom lane

On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-01-24 16:45:42 +0530, Amit Kapila wrote:
> >>> * Writing the temporary file to .$pid seems like a bad idea, better use
> >>> one file for that, SET PERSISTENT is protected by an exclusive lock
> >>> anyway.
>
> >> I think we can use one temporary file, infact that was one of the ways I
> >> have asked in one of the previous mails.
> >> However Tom and Zoltan felt this is better way to do it.
>
> > The have? I didn't read it like that. The file can only ever written by
> > a running postmaster and we already have code that ensures that. There's
> > absolutely no need for the tempfile to have a nondeterministic
> > name. That makes cleanup way easier as well.
>
> Say again? Surely the temp file is being written by whichever backend
> is executing SET PERSISTENT, and there could be more than one.

Sure, but the patch acquires SetPersistentLock exlusively beforehand
which seems fine to me.

Any opinion whether its acceptable to allow SET PERSISTENT in functions?
It seems absurd to me to allow it, but Amit seems to be of another
opinion.

Greetings,

Andres Freund

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
>> Say again? Surely the temp file is being written by whichever backend
>> is executing SET PERSISTENT, and there could be more than one.

> Sure, but the patch acquires SetPersistentLock exlusively beforehand
> which seems fine to me.

Why should we have such a lock? Seems like that will probably introduce
as many problems as it fixes. Deadlock risk, blockages, etc. It is not
necessary for atomicity, since rename() would be atomic already.

> Any opinion whether its acceptable to allow SET PERSISTENT in functions?
> It seems absurd to me to allow it, but Amit seems to be of another
> opinion.

Well, it's really a definitional question I think: do you expect that
subsequent failure of the transaction should cause such a SET to roll
back?

I think it would be entirely self-consistent to define SET PERSISTENT as
a nontransactional operation. Then the implementation would just be to
write the file immediately when the command is executed, and there's no
particular reason why it can't be allowed inside a transaction block.

If you want it to be transactional, then the point of disallowing it in
transaction blocks (or functions) would be to not have a very large
window between writing the file and committing. But it's still possible
that the transaction would fail somewhere in there, leading to the
inconsistent outcome that the transaction reports failing but we applied
the SET anyway. I do agree that it would be nonsensical to allow SET
PERSISTENT in functions but not transaction blocks.

Another approach is to remember the requested setting until somewhere in
the pre-commit sequence, and then try to do the file write at that time.
I'm not terribly thrilled with that approach, though, because (a) it
only narrows the window for an inconsistent outcome, it doesn't remove
it entirely; (b) there are already too many things that want to be the
"last thing done before commit"; and (c) it adds complexity and overhead
that I'd just as soon this patch not add. But if you want S.P. to be
transactional and allowed inside functions, I think this would be the
only acceptable implementation.

regards, tom lane

On 2013-01-24 12:02:59 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
> >> Say again? Surely the temp file is being written by whichever backend
> >> is executing SET PERSISTENT, and there could be more than one.
>
> > Sure, but the patch acquires SetPersistentLock exlusively beforehand
> > which seems fine to me.
>
> Why should we have such a lock? Seems like that will probably introduce
> as many problems as it fixes. Deadlock risk, blockages, etc. It is not
> necessary for atomicity, since rename() would be atomic already.

Well, the patch acquires it as-is, so I made the judgement based on
that.
But I think that lock isn't neccesarily a bad idea. While the
replacement of the values clearly is atomic due to the rename I think
there's another confusing behaviour lurking:

Backend A: starts
Backend B: starts
Backend A: reads the config
Backend B: reads the config
Backend A: does SET PERSISTENT foobar =..;
Backend B: does SET PERSISTENT foobar =..;

Now B overwrites the config change A has made as they are all stored in
the same file.

So the only safe way to do this seems to be:

LWLockAcquire(SetPersistentLock, LW_EXCLUSIVE);
ProcessConfigFile();
validate_option();
write_rename();
LWLockRelease();

We can decide not to care about the above case but the window isn't that
small if no reload is implied and so this seems to be overly grotty.

> > Any opinion whether its acceptable to allow SET PERSISTENT in functions?
> > It seems absurd to me to allow it, but Amit seems to be of another
> > opinion.
>
> Well, it's really a definitional question I think: do you expect that
> subsequent failure of the transaction should cause such a SET to roll
> back?
>
> I think it would be entirely self-consistent to define SET PERSISTENT as
> a nontransactional operation. Then the implementation would just be to
> write the file immediately when the command is executed, and there's no
> particular reason why it can't be allowed inside a transaction block.

Thats how its implemented atm except for not allowing it in transactions.

I think the reason I am weary of allowing it inside transaction is that
I think the config file needs to be reloaded before writing the new one
and it seems dangerous to me to reload the config in all the possible
situations a function can be called.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> Backend A: does SET PERSISTENT foobar =..;
> Backend B: does SET PERSISTENT foobar =..;

> Now B overwrites the config change A has made as they are all stored in
> the same file.

Say what? I thought the plan was one setting per file, so that we don't
get involved in having to parse-and-edit the file contents. What was
all that argument about a new directory, if we're only using one file?

If we are using just one file, then I agree a lock would be needed to
synchronize updates. But that seems to add a lot of complication
elsewhere.

regards, tom lane

On 2013-01-24 12:30:02 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > Backend A: does SET PERSISTENT foobar =..;
> > Backend B: does SET PERSISTENT foobar =..;
>
> > Now B overwrites the config change A has made as they are all stored in
> > the same file.
>
> Say what? I thought the plan was one setting per file, so that we don't
> get involved in having to parse-and-edit the file contents. What was
> all that argument about a new directory, if we're only using one file?

I initially thought that as well (and voted for it) but after reskimming
the thread and reading the patch that doesn't seem to be the case unless
its implemented in a way I don't understand:

+#define PG_AUTOCONF_DIR "auto.conf.d"
+#define PG_AUTOCONF_FILENAME "postgresql.auto.conf"

+ /* Frame auto conf name and auto conf sample temp file name */
+ snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
+ ConfigFileDir,
+ PG_AUTOCONF_DIR,
+ PG_AUTOCONF_FILENAME);
+ snprintf(AutoConfTmpFileName, sizeof(AutoConfTmpFileName),"%s/%s/%s.%d",
+ ConfigFileDir,
+ PG_AUTOCONF_DIR,
+ PG_AUTOCONF_FILENAME,
+ (int) getpid());
+

I don't understand what the conf.d is all about either if only one file
is going to be used.

> If we are using just one file, then I agree a lock would be needed to
> synchronize updates. But that seems to add a lot of complication
> elsewhere.

More people seem to have voted for the single file approach but I still
haven't understood why...

Greetings,

Andres Freund

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

On Thursday, January 24, 2013 10:33 PM Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
> >> Say again? Surely the temp file is being written by whichever
> backend
> >> is executing SET PERSISTENT, and there could be more than one.
>
> > Sure, but the patch acquires SetPersistentLock exlusively beforehand
> > which seems fine to me.
>
> Why should we have such a lock? Seems like that will probably
> introduce
> as many problems as it fixes. Deadlock risk, blockages, etc. It is
> not
> necessary for atomicity, since rename() would be atomic already.
>
> > Any opinion whether its acceptable to allow SET PERSISTENT in
> functions?
> > It seems absurd to me to allow it, but Amit seems to be of another
> > opinion.
>
> Well, it's really a definitional question I think: do you expect that
> subsequent failure of the transaction should cause such a SET to roll
> back?
>
> I think it would be entirely self-consistent to define SET PERSISTENT
> as
> a nontransactional operation. Then the implementation would just be to
> write the file immediately when the command is executed, and there's no
> particular reason why it can't be allowed inside a transaction block.
>
> If you want it to be transactional, then the point of disallowing it in
> transaction blocks (or functions) would be to not have a very large
> window between writing the file and committing. But it's still
> possible
> that the transaction would fail somewhere in there, leading to the
> inconsistent outcome that the transaction reports failing but we
> applied
> the SET anyway. I do agree that it would be nonsensical to allow SET
> PERSISTENT in functions but not transaction blocks.

As we have agreed on before that this command has to be non-transactional,
so
lets stick on that and about allowing it in functions and not in transaction
blocks,
I had some reasoning behind it as below, but as it is not making much sense
we can disallow it for both transaction blocks as well as functions:

Reasoning for disallowing in transaction blocks and allowing in functions:
----------------------------------------------------------------------------
--
" I think the core point of this command behavior is that it is not related
to transaction boundary.
So now the question is in such case if we don't allow this command inside
transaction block, why to allow inside Function?
When user starts transaction and runs this command and then few other
commands and now if he rollback, this command cannot be rolledback.
OTOH in function there is no explicit way to issue rollback, only an error
can cause rollback which is same as when running outside block."

> Say what? I thought the plan was one setting per file, so that we don't
get involved > in having to parse-and-edit the file contents.
> What was all that argument about a new directory, if we're only using one
file?

A new directory (named as auto.conf.d, initially in the patch named as
config_dir) is for having automatically generated configuration files.
Also it was suggested previously that it is better to have this file in
directory instead of standalone file.

> If we are using just one file, then I agree a lock would be needed to
synchronize
> updates. But that seems to add a lot of complication elsewhere.

There were 2 ways to protect the file write,
a. one is by having lock SetPersistentLock
b. another is by opening file in X mode and if open fails wait and retry for
sometime
and then give error.
As suggested in this thread I have implemented as Approach-1.
What is the complication it can add, can we resolve that?

With Regards,
Amit Kapila.

On Thursday, January 24, 2013 6:51 PM Andres Freund wrote:
> On 2013-01-24 18:37:29 +0530, Amit Kapila wrote:
> > On Thursday, January 24, 2013 5:25 PM Andres Freund wrote:
> > > On 2013-01-24 16:45:42 +0530, Amit Kapila wrote:
> > > > > * The gram.y changes arround set_rest_(more|common) seem pretty
> > > > > confused
> > > > > to me.
> > > >
> > > > >E.g. its not possible anymore to set the timezone for a
> function.
> > > >
> > > > What do you exactly mean by this part of comment.
> > >
> > > The set_rest_more production is used in FunctionSetResetClause and
> > > youve
> > > removed capabilities from it.
> >
> > set_rest_more has set_reset_common, so there should be no problem.
> >
> > set_rest_more: /* Generic SET syntaxes: */
> > set_rest_common
> > | var_name FROM CURRENT_P
> > {
> > VariableSetStmt *n =
> makeNode(VariableSetStmt);
> > n->kind = VAR_SET_CURRENT;
> > n->name = $1;
> > $$ = n;
> > }
>
> True. I still think that the split youve made isn't right as-is.

We can move FROM CURRENT and client_encoding to common part.
Other syntax (ROLE, SESSION AUTHORIZATION, TRANSACTION SNAPSHOT) doesn't
exist in postgresql.conf and requires a transaction to set and during reload
we don't have transaction, so we should not allow them.

With Regards,
Amit Kapila.

Thanks to both (Andres and Fujii Masao) of you for review.
Please find the patch attached with this mail which contains fixes for the
comments raised by you.
Kindly confirm if the fixes are okay or if I have missed any point raised by
you.

On Thursday, January 24, 2013 1:56 AM Andres Freund wrote:
> On 2013-01-22 12:32:07 +0000, Amit kapila wrote:
> > This closes all comments raised till now for this patch.
> > Kindly let me know if you feel something is missing?
>
> I am coming late to this patch, so bear with me if I repeat somethign
> said elsewhere.
>
> Review comments of cursory pass through the patch:
> * most comments are hard to understand. I know the problem of that
> being hard for a non-native speaker by heart, but I think another
> pass
> over them would be good thing.
I have changed the comments in most parts of code.

> * The gram.y changes arround set_rest_(more|common) seem pretty
> confused
> to me. E.g. its not possible anymore to set the timezone for a
> function. And why is it possible to persistently set the search path,
> but not client encoding? Why is FROM CURRENT in set_rest_more?

client encoding and FROM CURRENT syntax will be allowed in SET PERSISTENT.
Others cannot be moved as they need transaction to set them and they are
even not present in postgresql.conf file

> * set_config_file should elog(ERROR), not return on an unhandled
> setstmt->kind

Fixed.

> * why are you creating AutoConfFileName if its not stat'able? It seems
> better to simply skip parsing the old file in that case

Fixed.

> * Writing the temporary file to .$pid seems like a bad idea, better use
> one file for that, SET PERSISTENT is protected by an exclusive lock
> anyway.

now only one file (postgresql.auto.conf.temp) will be created

> * the write sequence should be:
> * fsync(tempfile)
> * fsync(directory)
> * rename(tempfile, configfile)
> * fsync(configfile)
> * fsync(directory)
to do fsync of directory we need to open the directory and then do the
fsync.
rename is an atomic operation, so it will always point to either new or
old file.
I am just not sure whether to modify the code to handle this scenario.
So for now I have added the comment at that place.

> * write_auto_conf_file should probably escape quoted values?

Fixed. Changed write_auto_conf_file() to handle it.

> * coding style should be adhered to more closesly, there are many
> if (pointer) which should be if (pointer != NULL), single-line blocks
> enclosed in curlies which shouldn't, etc.

Fixed.

> * replace_auto_config_file and surrounding functions need more comments
> in the header

Added more comments.

> * the check that prevents persistent SETs in a transaction should
> rather
> be in utility.c and use PreventTransactionChain like most of the
> others that need to do that (c.f. T_CreatedbStmt).

Fixed. Now this command will not work for transaction blocks or Functions.

Fujii Masao Comments
---------------------
> When I removed postgresql.auto.conf and restarted the server, I got
> the following warning message. This is not correct because I didn't
> remove "auto.conf.d" from postgresql.conf. What I removed is only
> postgresql.auto.conf.
>
> WARNING: Default "auto.conf.d" is not present in postgresql.conf.
> Configuration parameters changed with SET PERSISTENT command will not
> be effective.
>

Warning message is changed as per discussion in another mail.

> Is it safe to write something in the directory other than data
> directory via SQL?

As discussed, it is okay to write in config directory, so I have not changed
anything for this.

> The description of RESET PERSISTENT is in the document, but it seems
> not to be implemented.

RESET PERSISTENT description is removed from patch.

> storage.sgml needs to be updated.
> Doesn't auto.conf.d need to be explained in config.sgml?

Updated both storage.sgml and config.sgml

> This warning message implies that the line "include_dir 'auto.conf.d'"
> must not be removed from postgresql.conf? If so, we should warn that in
both document and postgresql.conf.sample?

I have modified the WARNING in postgresql.conf.sample, I have thought of
modifying runtime.sgml but not sure if that is the right place, can you
please suggest which document would be better to have this warning?

With Regards,
Amit Kapila.

On Thu, Jan 24, 2013 at 12:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> More people seem to have voted for the single file approach but I still
> haven't understood why...

Me neither. Having an include directory seems good, but I can't think
why we'd want to clutter it up with a bajillion automatically
generated files. One .auto file that gets overwritten at need seems
way nicer.

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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> More people seem to have voted for the single file approach but I still
>> haven't understood why...
>
> Me neither.  Having an include directory seems good, but I can't think
> why we'd want to clutter it up with a bajillion automatically
> generated files.  One .auto file that gets overwritten at need seems
> way nicer.

The single-value per file approach punts the concurrency issues and
ordering issues to the OS, which might significantly simplify the
patch.  I'm not sure that the niceness of the all-in-one-file
approach justifies the extra code.  If you ever want to view them
all in one file as of some moment in time, there is always:

  cat * | less

-Kevin

On 2013-01-26 07:46:50 -0500, Robert Haas wrote:
> On Thu, Jan 24, 2013 at 12:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > More people seem to have voted for the single file approach but I still
> > haven't understood why...
>
> Me neither. Having an include directory seems good, but I can't think
> why we'd want to clutter it up with a bajillion automatically
> generated files. One .auto file that gets overwritten at need seems
> way nicer.

I unfortunately think you misunderstood what I meant. I don't really see
that many arguments for putting all the variables in a single file. That
is I think the one-value-per-file is preferrable.

The primary reason is that the multiple values per file is just
considerably harder to implement without POLA. In order to not loose
values set in this or other backends you basically need to do something
like:

1) exlusive lock
2) reload config file (to update in memory structures)
3) check new variable
4) write config file (needs to be done atomically)
5) optionally reload config file
6) reload lock

Where as you can get acceptable behaviour in the single-value-per-file
approach by:
1) check new config variable
2) write new single-variable file
3) optionally reload config file

Not requiring a global lock while reading the configuration seems to
make stuff noticeably simpler.

The other advantages for a single value approach I know of are:
* way easier to integrate into other systems, no need to parse the whole
file for them
* easier to get yourself out of problems if you screwed up and set a bad
value (shared_buffers=16TB instead of GB ;))

Whereas the only real argument I can see for the all-in-one-file
approach I can see is that it will perform better.

Greetings,

Andres Freund

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jan 24, 2013 at 12:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> More people seem to have voted for the single file approach but I still
>> haven't understood why...

> Me neither. Having an include directory seems good, but I can't think
> why we'd want to clutter it up with a bajillion automatically
> generated files. One .auto file that gets overwritten at need seems
> way nicer.

IMO an include directory containing just one file is silly. If we're
going with the single-file approach, let's lose the directory altogether
and just store the file at $PGDATA/postgresql.conf.auto.

regards, tom lane

On 01/27/2013 01:01 AM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Jan 24, 2013 at 12:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> More people seem to have voted for the single file approach but I still
>>> haven't understood why...
>> Me neither. Having an include directory seems good, but I can't think
>> why we'd want to clutter it up with a bajillion automatically
>> generated files. One .auto file that gets overwritten at need seems
>> way nicer.
> IMO an include directory containing just one file is silly. If we're
> going with the single-file approach, let's lose the directory altogether
> and just store the file at $PGDATA/postgresql.conf.auto.
Wasn't part of the reason for having the config dir to make package
managers' lives easier and make it easier to script updates to
postgresql.conf? For the use of things like pg_wrapper?

I think the config dir has value even if a single .auto file is used, so
that packages can drop their own config snippets into it. For example,
if I installed a packaged extension that had its own postgresql.conf
changes or new GUCs, I'd want it to be able to drop that into the
configdir, not have to script changes to my postgresql.conf.

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

On Sat, Jan 26, 2013 at 6:47 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 01/27/2013 01:01 AM, Tom Lane wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Thu, Jan 24, 2013 at 12:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>>> More people seem to have voted for the single file approach but I still
>>>> haven't understood why...
>>> Me neither. Having an include directory seems good, but I can't think
>>> why we'd want to clutter it up with a bajillion automatically
>>> generated files. One .auto file that gets overwritten at need seems
>>> way nicer.
>> IMO an include directory containing just one file is silly. If we're
>> going with the single-file approach, let's lose the directory altogether
>> and just store the file at $PGDATA/postgresql.conf.auto.
> Wasn't part of the reason for having the config dir to make package
> managers' lives easier and make it easier to script updates to
> postgresql.conf? For the use of things like pg_wrapper?
>
> I think the config dir has value even if a single .auto file is used, so
> that packages can drop their own config snippets into it. For example,
> if I installed a packaged extension that had its own postgresql.conf
> changes or new GUCs, I'd want it to be able to drop that into the
> configdir, not have to script changes to my postgresql.conf.

That was my understanding. But I just work here.

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

On Sunday, January 27, 2013 10:07 AM Robert Haas wrote:
On Sat, Jan 26, 2013 at 6:47 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 01/27/2013 01:01 AM, Tom Lane wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Thu, Jan 24, 2013 at 12:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>>>> More people seem to have voted for the single file approach but I still
>>>>> haven't understood why...
>>>> Me neither. Having an include directory seems good, but I can't think
>>>> why we'd want to clutter it up with a bajillion automatically
>>>> generated files. One .auto file that gets overwritten at need seems
>>>> way nicer.
>>> IMO an include directory containing just one file is silly. If we're
>>> going with the single-file approach, let's lose the directory altogether
>>> and just store the file at $PGDATA/postgresql.conf.auto.
>> Wasn't part of the reason for having the config dir to make package
>> managers' lives easier and make it easier to script updates to
>> postgresql.conf? For the use of things like pg_wrapper?
>
>> I think the config dir has value even if a single .auto file is used, so
>> that packages can drop their own config snippets into it. For example,
>> if I installed a packaged extension that had its own postgresql.conf
>> changes or new GUCs, I'd want it to be able to drop that into the
>> configdir, not have to script changes to my postgresql.conf.

> That was my understanding. But I just work here.

Current implementation of patch is to have directory and single file.
However if user puts other files, it gives LOG: unexpected file found in automatic configuration directory .
As now Craig has explained the usecase for having other files and previously Fujii Masao has also reported similar
usecase, I think it is better to remove the LOG message from code.
Currently the directory is named as auto.conf.d, so if we allow users to place other files, is this name okay
or shall we change it to something like config_dir?

With Regards,
Amit Kapila.

2013-01-24 18:02 keltezéssel, Tom Lane írta:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>> On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
>>> Say again? Surely the temp file is being written by whichever backend
>>> is executing SET PERSISTENT, and there could be more than one.
>> Sure, but the patch acquires SetPersistentLock exlusively beforehand
>> which seems fine to me.
> Why should we have such a lock? Seems like that will probably introduce
> as many problems as it fixes. Deadlock risk, blockages, etc. It is not
> necessary for atomicity, since rename() would be atomic already.

There is a problem when running SET PERSISTENT for different GUCs
in parallel. All happen to read the same original file, and only one
setting ends up in the result if you rely only on the rename() being atomic.
The LWLock provides the serialization for that problem.

>
>> Any opinion whether its acceptable to allow SET PERSISTENT in functions?
>> It seems absurd to me to allow it, but Amit seems to be of another
>> opinion.
> Well, it's really a definitional question I think: do you expect that
> subsequent failure of the transaction should cause such a SET to roll
> back?
>
> I think it would be entirely self-consistent to define SET PERSISTENT as
> a nontransactional operation. Then the implementation would just be to
> write the file immediately when the command is executed, and there's no
> particular reason why it can't be allowed inside a transaction block.
>
> If you want it to be transactional, then the point of disallowing it in
> transaction blocks (or functions) would be to not have a very large
> window between writing the file and committing. But it's still possible
> that the transaction would fail somewhere in there, leading to the
> inconsistent outcome that the transaction reports failing but we applied
> the SET anyway. I do agree that it would be nonsensical to allow SET
> PERSISTENT in functions but not transaction blocks.
>
> Another approach is to remember the requested setting until somewhere in
> the pre-commit sequence, and then try to do the file write at that time.
> I'm not terribly thrilled with that approach, though, because (a) it
> only narrows the window for an inconsistent outcome, it doesn't remove
> it entirely; (b) there are already too many things that want to be the
> "last thing done before commit"; and (c) it adds complexity and overhead
> that I'd just as soon this patch not add. But if you want S.P. to be
> transactional and allowed inside functions, I think this would be the
> only acceptable implementation.
>
> regards, tom lane
>
>

--
----------------------------------
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 2013-02-11 15:21:13 +0100, Boszormenyi Zoltan wrote:
> 2013-01-24 18:02 keltezéssel, Tom Lane írta:
> >Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >>On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
> >>>Say again? Surely the temp file is being written by whichever backend
> >>>is executing SET PERSISTENT, and there could be more than one.
> >>Sure, but the patch acquires SetPersistentLock exlusively beforehand
> >>which seems fine to me.
> >Why should we have such a lock? Seems like that will probably introduce
> >as many problems as it fixes. Deadlock risk, blockages, etc. It is not
> >necessary for atomicity, since rename() would be atomic already.
>
> There is a problem when running SET PERSISTENT for different GUCs
> in parallel. All happen to read the same original file, and only one
> setting ends up in the result if you rely only on the rename() being atomic.
> The LWLock provides the serialization for that problem.

Tom was voting for one-setting-per-file, in that case the problem
doesn't exist.

Greetings,

Andres Freund

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

2013-01-26 13:46 keltezéssel, Robert Haas írta:
> On Thu, Jan 24, 2013 at 12:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> More people seem to have voted for the single file approach but I still
>> haven't understood why...
> Me neither. Having an include directory seems good, but I can't think
> why we'd want to clutter it up with a bajillion automatically
> generated files. One .auto file that gets overwritten at need seems
> way nicer.
>

Sarcasm at work. Nice. :-)

--
----------------------------------
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-02-11 15:25 keltezéssel, Andres Freund írta:
> On 2013-02-11 15:21:13 +0100, Boszormenyi Zoltan wrote:
>> 2013-01-24 18:02 keltezéssel, Tom Lane írta:
>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>>> On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
>>>>> Say again? Surely the temp file is being written by whichever backend
>>>>> is executing SET PERSISTENT, and there could be more than one.
>>>> Sure, but the patch acquires SetPersistentLock exlusively beforehand
>>>> which seems fine to me.
>>> Why should we have such a lock? Seems like that will probably introduce
>>> as many problems as it fixes. Deadlock risk, blockages, etc. It is not
>>> necessary for atomicity, since rename() would be atomic already.
>> There is a problem when running SET PERSISTENT for different GUCs
>> in parallel. All happen to read the same original file, and only one
>> setting ends up in the result if you rely only on the rename() being atomic.
>> The LWLock provides the serialization for that problem.
> Tom was voting for one-setting-per-file, in that case the problem
> doesn't exist.

I voted for the one-file approach and was arguing from the POV
of the current implementation.

--
----------------------------------
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 02/11/2013 06:33 AM, Boszormenyi Zoltan wrote:
> 2013-02-11 15:25 keltezéssel, Andres Freund írta:
>> On 2013-02-11 15:21:13 +0100, Boszormenyi Zoltan wrote:
>>> 2013-01-24 18:02 keltezéssel, Tom Lane írta:
>>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>>>> On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
>>>>>> Say again? Surely the temp file is being written by whichever
>>>>>> backend
>>>>>> is executing SET PERSISTENT, and there could be more than one.
>>>>> Sure, but the patch acquires SetPersistentLock exlusively beforehand
>>>>> which seems fine to me.
>>>> Why should we have such a lock? Seems like that will probably
>>>> introduce
>>>> as many problems as it fixes. Deadlock risk, blockages, etc. It is
>>>> not
>>>> necessary for atomicity, since rename() would be atomic already.
>>> There is a problem when running SET PERSISTENT for different GUCs
>>> in parallel. All happen to read the same original file, and only one
>>> setting ends up in the result if you rely only on the rename() being
>>> atomic.
>>> The LWLock provides the serialization for that problem.
>> Tom was voting for one-setting-per-file, in that case the problem
>> doesn't exist.
>
> I voted for the one-file approach and was arguing from the POV
> of the current implementation.

I thought we discussed this ad naseum, and decided to go with the
one-single-file approach for the first round, since we already had an
implementation for that. I still think that's the right approach to take
with this patch; if it doesn't work out, we can go do
one-file-per-setting in 9.4.

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

On 2013-02-11 11:17:16 -0800, Josh Berkus wrote:
> On 02/11/2013 06:33 AM, Boszormenyi Zoltan wrote:
> > 2013-02-11 15:25 keltezéssel, Andres Freund írta:
> >> On 2013-02-11 15:21:13 +0100, Boszormenyi Zoltan wrote:
> >>> 2013-01-24 18:02 keltezéssel, Tom Lane írta:
> >>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >>>>> On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
> >>>>>> Say again? Surely the temp file is being written by whichever
> >>>>>> backend
> >>>>>> is executing SET PERSISTENT, and there could be more than one.
> >>>>> Sure, but the patch acquires SetPersistentLock exlusively beforehand
> >>>>> which seems fine to me.
> >>>> Why should we have such a lock? Seems like that will probably
> >>>> introduce
> >>>> as many problems as it fixes. Deadlock risk, blockages, etc. It is
> >>>> not
> >>>> necessary for atomicity, since rename() would be atomic already.
> >>> There is a problem when running SET PERSISTENT for different GUCs
> >>> in parallel. All happen to read the same original file, and only one
> >>> setting ends up in the result if you rely only on the rename() being
> >>> atomic.
> >>> The LWLock provides the serialization for that problem.
> >> Tom was voting for one-setting-per-file, in that case the problem
> >> doesn't exist.
> >
> > I voted for the one-file approach and was arguing from the POV
> > of the current implementation.
>
> I thought we discussed this ad naseum, and decided to go with the
> one-single-file approach for the first round, since we already had an
> implementation for that. I still think that's the right approach to take
> with this patch; if it doesn't work out, we can go do
> one-file-per-setting in 9.4.

Well, several people (at least Tom, I, and I think Zoltan as well) think
that the one-file approach is considerably more complex.

Check
http://www.postgresql.org/message-id/20130126162728.GA5482@awork2.anarazel.de
and related messages for some of the problems. Most of which are
unhandled in the current patch, i.e. currently you *will* loose changes
made in concurrent sessions.

Greetings,

Andres Freund

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

On 02/12/2013 03:24 AM, Andres Freund wrote:
> Well, several people (at least Tom, I, and I think Zoltan as well)
> think that the one-file approach is considerably more complex. Check
> http://www.postgresql.org/message-id/20130126162728.GA5482@awork2.anarazel.de
> and related messages for some of the problems. Most of which are
> unhandled in the current patch, i.e. currently you *will* loose
> changes made in concurrent sessions.
We need to read all files in the directory anyway to make the configdir
approach useful for system package managers that want to create config
files for extensions, akin to how /etc/yum.repos.ds/ ,
/etc/apt/sources.list.d/ , /etc/apache2/conf.d/ and so on work.

OTOH, it'll be easier for a sysadmin to understand what's going on if
Pg's auto-written config is a single file, so any system-installed
settings aren't lost in a giant clutter of Pg-created files.

It seems that SQL-level configuration and the config dir feature are
actually more separate than it might've appeared.

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

On Tuesday, February 12, 2013 12:54 AM Andres Freund wrote:
> On 2013-02-11 11:17:16 -0800, Josh Berkus wrote:
> > On 02/11/2013 06:33 AM, Boszormenyi Zoltan wrote:
> > > 2013-02-11 15:25 keltezéssel, Andres Freund írta:
> > >> On 2013-02-11 15:21:13 +0100, Boszormenyi Zoltan wrote:
> > >>> 2013-01-24 18:02 keltezéssel, Tom Lane írta:
> > >>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > >>>>> On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
> > >>>>>> Say again? Surely the temp file is being written by whichever
> > >>>>>> backend
> > >>>>>> is executing SET PERSISTENT, and there could be more than one.
> > >>>>> Sure, but the patch acquires SetPersistentLock exlusively
> beforehand
> > >>>>> which seems fine to me.
> > >>>> Why should we have such a lock? Seems like that will probably
> > >>>> introduce
> > >>>> as many problems as it fixes. Deadlock risk, blockages, etc.
> It is
> > >>>> not
> > >>>> necessary for atomicity, since rename() would be atomic already.
> > >>> There is a problem when running SET PERSISTENT for different GUCs
> > >>> in parallel. All happen to read the same original file, and only
> one
> > >>> setting ends up in the result if you rely only on the rename()
> being
> > >>> atomic.
> > >>> The LWLock provides the serialization for that problem.
> > >> Tom was voting for one-setting-per-file, in that case the problem
> > >> doesn't exist.
> > >
> > > I voted for the one-file approach and was arguing from the POV
> > > of the current implementation.
> >
> > I thought we discussed this ad naseum, and decided to go with the
> > one-single-file approach for the first round, since we already had an
> > implementation for that. I still think that's the right approach to
> take
> > with this patch; if it doesn't work out, we can go do
> > one-file-per-setting in 9.4.
>
> Well, several people (at least Tom, I, and I think Zoltan as well)
> think
> that the one-file approach is considerably more complex.

Zoltan has reviewed this patch very thoroughly, I have never seen a comment
from him that the current
Patch (one-file-all-settings) approach is not as good as having
one-file-per-setting approach.
Zoltan, please correct me, If I am wrong.

What I could understand from mails is Tom has initially suggested to have
one-file-per-setting but for the current patch
(one-file-all-settings) he was telling that if we wanted to go for single
file approach, then there
is no need for separate directory, but for that CRAIG has given a scenario
where separate directory is useful.

Also Robert has suggested from the beginning that (one-file-all-settings) is
better approach.

It took months of discussion to reach the consensus of this level, if we
again want to change approach,
then I think it will be tough to touch this feature again.

I think it would be better if we could try to see if there are any problems
in existing patch which cannot be handled because of it's current design,
then it will make more sense to conclude on changing approach.

> Check
> http://www.postgresql.org/message-
> id/20130126162728(dot)GA5482(at)awork2(dot)anarazel(dot)de
> and related messages for some of the problems. Most of which are
> unhandled in the current patch, i.e. currently you *will* loose changes
> made in concurrent sessions.

How will somebody loose changes in concurrent sessions?
Each session currently waits with LWLock if some other session is operating
on file. Also after having lock they don't acquire any other lock, so there
should be no chances of any other problem.

With Regards,
Amit Kapila.

2013-02-12 04:54 keltezéssel, Amit Kapila írta:
> On Tuesday, February 12, 2013 12:54 AM Andres Freund wrote:
>> On 2013-02-11 11:17:16 -0800, Josh Berkus wrote:
>>> On 02/11/2013 06:33 AM, Boszormenyi Zoltan wrote:
>>>> 2013-02-11 15:25 keltezéssel, Andres Freund írta:
>>>>> On 2013-02-11 15:21:13 +0100, Boszormenyi Zoltan wrote:
>>>>>> 2013-01-24 18:02 keltezéssel, Tom Lane írta:
>>>>>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>>>>>>> On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
>>>>>>>>> Say again? Surely the temp file is being written by whichever
>>>>>>>>> backend
>>>>>>>>> is executing SET PERSISTENT, and there could be more than one.
>>>>>>>> Sure, but the patch acquires SetPersistentLock exlusively
>> beforehand
>>>>>>>> which seems fine to me.
>>>>>>> Why should we have such a lock? Seems like that will probably
>>>>>>> introduce
>>>>>>> as many problems as it fixes. Deadlock risk, blockages, etc.
>> It is
>>>>>>> not
>>>>>>> necessary for atomicity, since rename() would be atomic already.
>>>>>> There is a problem when running SET PERSISTENT for different GUCs
>>>>>> in parallel. All happen to read the same original file, and only
>> one
>>>>>> setting ends up in the result if you rely only on the rename()
>> being
>>>>>> atomic.
>>>>>> The LWLock provides the serialization for that problem.
>>>>> Tom was voting for one-setting-per-file, in that case the problem
>>>>> doesn't exist.
>>>> I voted for the one-file approach and was arguing from the POV
>>>> of the current implementation.
>>> I thought we discussed this ad naseum, and decided to go with the
>>> one-single-file approach for the first round, since we already had an
>>> implementation for that. I still think that's the right approach to
>> take
>>> with this patch; if it doesn't work out, we can go do
>>> one-file-per-setting in 9.4.
>> Well, several people (at least Tom, I, and I think Zoltan as well)
>> think
>> that the one-file approach is considerably more complex.
> Zoltan has reviewed this patch very thoroughly, I have never seen a comment
> from him that the current
> Patch (one-file-all-settings) approach is not as good as having
> one-file-per-setting approach.
> Zoltan, please correct me, If I am wrong.

I cannot recall arguing for the one-file-per-GUC way.
But I haven't re-read my mails in this thread, either.

> What I could understand from mails is Tom has initially suggested to have
> one-file-per-setting but for the current patch
> (one-file-all-settings) he was telling that if we wanted to go for single
> file approach, then there
> is no need for separate directory, but for that CRAIG has given a scenario
> where separate directory is useful.
>
> Also Robert has suggested from the beginning that (one-file-all-settings) is
> better approach.
>
>
> It took months of discussion to reach the consensus of this level, if we
> again want to change approach,
> then I think it will be tough to touch this feature again.
>
> I think it would be better if we could try to see if there are any problems
> in existing patch which cannot be handled because of it's current design,
> then it will make more sense to conclude on changing approach.
>
>> Check
>> http://www.postgresql.org/message-
>> id/20130126162728(dot)GA5482(at)awork2(dot)anarazel(dot)de
>> and related messages for some of the problems. Most of which are
>> unhandled in the current patch, i.e. currently you *will* loose changes
>> made in concurrent sessions.

This mail lists this order for the single file approach:

> 1) exlusive lock
> 2) reload config file (to update in memory structures)
> 3) check new variable
> 4) write config file (needs to be done atomically)
> 5) optionally reload config file
> 6) reload lock

The patch does it this way:
1. check new variable. No problem with that, validation for proper GUC name,
type of the value, etc. can be done outside the lock.
2. grab lock
3. parse the config file
4. write the new config file
5. release lock

Reloading the config file is intentionally not done, it's even documented.
You can do SELECT pg_reload_conf() after SET PERSISTENT if you need it.

> How will somebody loose changes in concurrent sessions?
> Each session currently waits with LWLock if some other session is operating
> on file. Also after having lock they don't acquire any other lock, so there
> should be no chances of any other problem.

Specifically, LWLockAcquire() is called first, then ParseConfigFp()
in a PG_TRY() block, so reading the original postgresql.auto.conf
is serialized. No chance to lose changes done in parallel.

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 Tuesday, February 12, 2013 11:24 AM Boszormenyi Zoltan wrote:
> 2013-02-12 04:54 keltezéssel, Amit Kapila írta:
> > On Tuesday, February 12, 2013 12:54 AM Andres Freund wrote:
> >> On 2013-02-11 11:17:16 -0800, Josh Berkus wrote:
> >>> On 02/11/2013 06:33 AM, Boszormenyi Zoltan wrote:
> >>>> 2013-02-11 15:25 keltezéssel, Andres Freund írta:
> >>>>> On 2013-02-11 15:21:13 +0100, Boszormenyi Zoltan wrote:
> >>>>>> 2013-01-24 18:02 keltezéssel, Tom Lane írta:
> >>>>>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >>>>>>>> On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
> >>>>>>>>> Say again? Surely the temp file is being written by
> whichever
> >>>>>>>>> backend
> >>>>>>>>> is executing SET PERSISTENT, and there could be more than
> one.
> >>>>>>>> Sure, but the patch acquires SetPersistentLock exlusively
> >> beforehand
> >>>>>>>> which seems fine to me.
> >>>>>>> Why should we have such a lock? Seems like that will probably
> >>>>>>> introduce
> >>>>>>> as many problems as it fixes. Deadlock risk, blockages, etc.
> >> It is
> >>>>>>> not
> >>>>>>> necessary for atomicity, since rename() would be atomic
> already.
> >>>>>> There is a problem when running SET PERSISTENT for different
> GUCs
> >>>>>> in parallel. All happen to read the same original file, and only
> >> one
> >>>>>> setting ends up in the result if you rely only on the rename()
> >> being
> >>>>>> atomic.
> >>>>>> The LWLock provides the serialization for that problem.
> >>>>> Tom was voting for one-setting-per-file, in that case the problem
> >>>>> doesn't exist.
> >>>> I voted for the one-file approach and was arguing from the POV
> >>>> of the current implementation.
> >>> I thought we discussed this ad naseum, and decided to go with the
> >>> one-single-file approach for the first round, since we already had
> an
> >>> implementation for that. I still think that's the right approach to
> >> take
> >>> with this patch; if it doesn't work out, we can go do
> >>> one-file-per-setting in 9.4.
> >> Well, several people (at least Tom, I, and I think Zoltan as well)
> >> think
> >> that the one-file approach is considerably more complex.
> > Zoltan has reviewed this patch very thoroughly, I have never seen a
> comment
> > from him that the current
> > Patch (one-file-all-settings) approach is not as good as having
> > one-file-per-setting approach.
> > Zoltan, please correct me, If I am wrong.
>
> I cannot recall arguing for the one-file-per-GUC way.
> But I haven't re-read my mails in this thread, either.

Thanks for confirmation. I also don't think if ever have argument over this
approach.

> > What I could understand from mails is Tom has initially suggested to
> have
> > one-file-per-setting but for the current patch
> > (one-file-all-settings) he was telling that if we wanted to go for
> single
> > file approach, then there
> > is no need for separate directory, but for that CRAIG has given a
> scenario
> > where separate directory is useful.
> >
> > Also Robert has suggested from the beginning that (one-file-all-
> settings) is
> > better approach.
> >
> >
> > It took months of discussion to reach the consensus of this level, if
> we
> > again want to change approach,
> > then I think it will be tough to touch this feature again.
> >
> > I think it would be better if we could try to see if there are any
> problems
> > in existing patch which cannot be handled because of it's current
> design,
> > then it will make more sense to conclude on changing approach.
> >
> >> Check
> >> http://www.postgresql.org/message-
> >> id/20130126162728(dot)GA5482(at)awork2(dot)anarazel(dot)de
> >> and related messages for some of the problems. Most of which are
> >> unhandled in the current patch, i.e. currently you *will* loose
> changes
> >> made in concurrent sessions.
>
> This mail lists this order for the single file approach:
>
> > 1) exlusive lock
> > 2) reload config file (to update in memory structures)
> > 3) check new variable
> > 4) write config file (needs to be done atomically)
> > 5) optionally reload config file
> > 6) reload lock
>
> The patch does it this way:
> 1. check new variable. No problem with that, validation for proper GUC
> name,
> type of the value, etc. can be done outside the lock.
> 2. grab lock
> 3. parse the config file
> 4. write the new config file
> 5. release lock

Before step-3, we open the auto conf file and in step-3 parse will update
in-memory structures.
So I think Andres's Step-2 is same as the missing step and Step-3 mentioned
by you.

> Reloading the config file is intentionally not done, it's even
> documented.
> You can do SELECT pg_reload_conf() after SET PERSISTENT if you need it.
>
> > How will somebody loose changes in concurrent sessions?
> > Each session currently waits with LWLock if some other session is
> operating
> > on file. Also after having lock they don't acquire any other lock, so
> there
> > should be no chances of any other problem.
>
> Specifically, LWLockAcquire() is called first, then ParseConfigFp()
> in a PG_TRY() block, so reading the original postgresql.auto.conf
> is serialized. No chance to lose changes done in parallel.

True.

With Regards,
Amit Kapila.

On 2013-02-12 14:57:51 +0530, Amit Kapila wrote:
> On Tuesday, February 12, 2013 11:24 AM Boszormenyi Zoltan wrote:
> > 2013-02-12 04:54 keltezéssel, Amit Kapila írta:
> > > On Tuesday, February 12, 2013 12:54 AM Andres Freund wrote:
> > > Zoltan has reviewed this patch very thoroughly

Sorry, the patch as referenced in the commitfest app is *far* from being
thoroughly reviewed. Its not even remotely ready for commit. I
absolutely don't want to say *anything* against Zoltans reviews or
such. I have looked over the patch before as well, but I found several
severe issues in a 5min look.

> > > I have never seen a comment
> > > from him that the current
> > > Patch (one-file-all-settings) approach is not as good as having
> > > one-file-per-setting approach.
> > > Zoltan, please correct me, If I am wrong.
> >
> > I cannot recall arguing for the one-file-per-GUC way.
> > But I haven't re-read my mails in this thread, either.

Zoltan, mis(read|remembered) something, sorry.

> Thanks for confirmation. I also don't think if ever have argument over this
> approach.

No idea what youre trying to say here though?

> > >> Check
> > >> http://www.postgresql.org/message-
> > >> id/20130126162728(dot)GA5482(at)awork2(dot)anarazel(dot)de
> > >> and related messages for some of the problems. Most of which are
> > >> unhandled in the current patch, i.e. currently you *will* loose
> > changes
> > >> made in concurrent sessions.
> >
> > This mail lists this order for the single file approach:
> >
> > > 1) exlusive lock
> > > 2) reload config file (to update in memory structures)
> > > 3) check new variable
> > > 4) write config file (needs to be done atomically)
> > > 5) optionally reload config file
> > > 6) reload lock
> >
> > The patch does it this way:
> > 1. check new variable. No problem with that, validation for proper GUC
> > name,
> > type of the value, etc. can be done outside the lock.
> > 2. grab lock
> > 3. parse the config file
> > 4. write the new config file
> > 5. release lock

1) You need to grab the lock before the value is checked since some
variables are interdependent (e.g. log_statement_stats, wal_level,
archive_mode) and thus the check needs to be made after preventing
concurrent changes.
2) You need to apply the current config file for exactly the same
reason before checking the new value. Also
validate_conf_option/find_option doesn't work appropriately without
an up2date config file. E.g. CURRENT does absurd things without but I
think there are other cases as well.

I am not saying its impossible to do the one-file approach correctly, I
just think its harder while not being more useful.

> > Reloading the config file is intentionally not done, it's even
> > documented. You can do SELECT pg_reload_conf() after SET PERSISTENT
> > if you need it.

Not being done and it being documented as not doing so doesn't make it
correct :P
I think a SET having no immediate results is confusing. Especially if I
am right and we do need to apply previous config changes before doing
the next SET. But I don't have *that* strong feelings about it.

> > Specifically, LWLockAcquire() is called first, then ParseConfigFp()
> > in a PG_TRY() block, so reading the original postgresql.auto.conf
> > is serialized. No chance to lose changes done in parallel.

Not a fundamental issue, but I just noticed LWLockRelease isn't called
in the PG_CATCH branch in set_config_file. There's also an ereport()
which needs to be moved into the PG_TRY to avoid exiting with a held
lock.

I think you also forgot to adjust copyfuncs.c et al for your
VariableSetStmt change (addition of is_persistent).

You should also check for GUC_DISALLOW_IN_FILE in validate_conf_option,
its not the same as PGC_INTERNAL.

What do you mean by "framing" a variable? Surrounding it by ""?

It might be worth adding a check that setting and especially resetting a
user-defined variable works correctly. I.e. persistently setting foo.bar
= 'blub' and then resetting it again.

You cannot use the name passed in via the VariableSetStmt in
set_config_file, you should use the one in 'record' as returned by
find_option (via validate_conf_option) as it handles the correct
name. Otherwise you will get into problems with stuff like TimeZone and
similar.

I think validate_conf_option duplicates far too much code. You need to
unify parts of set_config_option with validate_conf_option.

Greetings,

Andres Freund

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

On Tuesday, February 12, 2013 4:55 PM Andres Freund wrote:
> On 2013-02-12 14:57:51 +0530, Amit Kapila wrote:
> > On Tuesday, February 12, 2013 11:24 AM Boszormenyi Zoltan wrote:
> > > 2013-02-12 04:54 keltezéssel, Amit Kapila írta:
> > > > On Tuesday, February 12, 2013 12:54 AM Andres Freund wrote:
> > > > Zoltan has reviewed this patch very thoroughly
>
> Sorry, the patch as referenced in the commitfest app is *far* from
> being
> thoroughly reviewed. Its not even remotely ready for commit. I
> absolutely don't want to say *anything* against Zoltans reviews or
> such. I have looked over the patch before as well, but I found several
> severe issues in a 5min look.

> > > > I have never seen a comment
> > > > from him that the current
> > > > Patch (one-file-all-settings) approach is not as good as having
> > > > one-file-per-setting approach.
> > > > Zoltan, please correct me, If I am wrong.
> > >
> > > I cannot recall arguing for the one-file-per-GUC way.
> > > But I haven't re-read my mails in this thread, either.
>
> Zoltan, mis(read|remembered) something, sorry.
>
> > Thanks for confirmation. I also don't think if ever have argument
> over this
> > approach.
>
> No idea what youre trying to say here though?

Forget it, this is not important.
Just wanted to say, there is no confusion about approach between 2 of us.

> > > >> Check
> > > >> http://www.postgresql.org/message-
> > > >> id/20130126162728(dot)GA5482(at)awork2(dot)anarazel(dot)de
> > > >> and related messages for some of the problems. Most of which are
> > > >> unhandled in the current patch, i.e. currently you *will* loose
> > > changes
> > > >> made in concurrent sessions.
> > >
> > > This mail lists this order for the single file approach:
> > >
> > > > 1) exlusive lock
> > > > 2) reload config file (to update in memory structures)
> > > > 3) check new variable
> > > > 4) write config file (needs to be done atomically)
> > > > 5) optionally reload config file
> > > > 6) reload lock
> > >
> > > The patch does it this way:
> > > 1. check new variable. No problem with that, validation for proper
> GUC
> > > name,
> > > type of the value, etc. can be done outside the lock.
> > > 2. grab lock
> > > 3. parse the config file
> > > 4. write the new config file
> > > 5. release lock
>
> 1) You need to grab the lock before the value is checked since some
> variables are interdependent (e.g. log_statement_stats, wal_level,
> archive_mode) and thus the check needs to be made after preventing
> concurrent changes.

This can happen if we do any SIGHUP after the command, otherwise it will
have old value only.

> 2) You need to apply the current config file for exactly the same
> reason before checking the new value. Also
> validate_conf_option/find_option doesn't work appropriately without
> an up2date config file. E.g. CURRENT does absurd things without but
> I
> think there are other cases as well.

At this moment, I am not able to think through this, could you explain by
small example.

I am thinking that shall we remove check hook function and do other
validation only, as this will be done at time
of reload, similar to what will get done when user manually edits the
postgresql.conf file.

> I am not saying its impossible to do the one-file approach correctly, I
> just think its harder while not being more useful.
>
> > > Reloading the config file is intentionally not done, it's even
> > > documented. You can do SELECT pg_reload_conf() after SET
> PERSISTENT
> > > if you need it.
>
> Not being done and it being documented as not doing so doesn't make it
> correct :P
> I think a SET having no immediate results is confusing. Especially if I
> am right and we do need to apply previous config changes before doing
> the next SET. But I don't have *that* strong feelings about it.

I don't think any such expectation should be there, as with this feature
(SET PERSISTENT),
we will allow user to change the settings in file with command instead of
manually editing the file.

> > > Specifically, LWLockAcquire() is called first, then ParseConfigFp()
> > > in a PG_TRY() block, so reading the original postgresql.auto.conf
> > > is serialized. No chance to lose changes done in parallel.
>
> Not a fundamental issue, but I just noticed LWLockRelease isn't called
> in the PG_CATCH branch in set_config_file. There's also an ereport()
> which needs to be moved into the PG_TRY to avoid exiting with a held
> lock.

I think rollback/abort transaction due to error will handle release of
locks.

> I think you also forgot to adjust copyfuncs.c et al for your
> VariableSetStmt change (addition of is_persistent).

It is there in _copyVariableSetStmt() function.

> You should also check for GUC_DISALLOW_IN_FILE in validate_conf_option,
> its not the same as PGC_INTERNAL.

I shall update the patch to address this issue.

> What do you mean by "framing" a variable? Surrounding it by ""?

Sorry, I am not able to find "framing" in quotes.

> It might be worth adding a check that setting and especially resetting
> a
> user-defined variable works correctly. I.e. persistently setting
> foo.bar
> = 'blub' and then resetting it again.

I shall update the patch to address this issue.

> You cannot use the name passed in via the VariableSetStmt in
> set_config_file, you should use the one in 'record' as returned by
> find_option (via validate_conf_option) as it handles the correct
> name. Otherwise you will get into problems with stuff like TimeZone and
> similar.

I shall check.

> I think validate_conf_option duplicates far too much code. You need to
> unify parts of set_config_option with validate_conf_option.

I had thought of doing this initially, but found it might be little tricky
as duplicate part is not one single chunk.
I shall check, if it can be done in a reasonable way.

Thank you for feedback.

With Regards,
Amit Kapila.

On 2013-02-12 20:19:43 +0530, Amit Kapila wrote:
> On Tuesday, February 12, 2013 4:55 PM Andres Freund wrote:
> > On 2013-02-12 14:57:51 +0530, Amit Kapila wrote:
> > > On Tuesday, February 12, 2013 11:24 AM Boszormenyi Zoltan wrote:
> > > > This mail lists this order for the single file approach:
> > > >
> > > > > 1) exlusive lock
> > > > > 2) reload config file (to update in memory structures)
> > > > > 3) check new variable
> > > > > 4) write config file (needs to be done atomically)
> > > > > 5) optionally reload config file
> > > > > 6) reload lock
> > > >
> > > > The patch does it this way:
> > > > 1. check new variable. No problem with that, validation for proper
> > GUC
> > > > name,
> > > > type of the value, etc. can be done outside the lock.
> > > > 2. grab lock
> > > > 3. parse the config file
> > > > 4. write the new config file
> > > > 5. release lock
> >
> > 1) You need to grab the lock before the value is checked since some
> > variables are interdependent (e.g. log_statement_stats, wal_level,
> > archive_mode) and thus the check needs to be made after preventing
> > concurrent changes.
>
> This can happen if we do any SIGHUP after the command, otherwise it will
> have old value only.

Yes, and thats a problem imo.

SET PERSISTENT log_statement_stats = true;
restart;
SET PERSISTENT log_statement_stats = false;
SET PERSISTENT log_parser_stats = true;
ERROR...

thats why I think the config file needs to be processed.

> > 2) You need to apply the current config file for exactly the same
> > reason before checking the new value. Also
> > validate_conf_option/find_option doesn't work appropriately without
> > an up2date config file. E.g. CURRENT does absurd things without but
> > I
> > think there are other cases as well.
>
> At this moment, I am not able to think through this, could you explain by
> small example.

The most trivial one I can think of is:

Transaction A: SET PERSISTENT blub = 'bar';
Transaction B: SET PERSISTENT blub FROM CURRENT;

> I am thinking that shall we remove check hook function and do other
> validation only, as this will be done at time
> of reload, similar to what will get done when user manually edits the
> postgresql.conf file.

Why? The user isn't editing the file by hand for a reason.

> > I am not saying its impossible to do the one-file approach correctly, I
> > just think its harder while not being more useful.
> >
> > > > Reloading the config file is intentionally not done, it's even
> > > > documented. You can do SELECT pg_reload_conf() after SET
> > PERSISTENT
> > > > if you need it.
> >
> > Not being done and it being documented as not doing so doesn't make it
> > correct :P
> > I think a SET having no immediate results is confusing. Especially if I
> > am right and we do need to apply previous config changes before doing
> > the next SET. But I don't have *that* strong feelings about it.
>
> I don't think any such expectation should be there, as with this feature
> (SET PERSISTENT),
> we will allow user to change the settings in file with command instead of
> manually editing the file.

I don't see why that follows. The users *do* want something different,
otherwise they would hand-edit the file.

> > > > Specifically, LWLockAcquire() is called first, then ParseConfigFp()
> > > > in a PG_TRY() block, so reading the original postgresql.auto.conf
> > > > is serialized. No chance to lose changes done in parallel.
> >
> > Not a fundamental issue, but I just noticed LWLockRelease isn't called
> > in the PG_CATCH branch in set_config_file. There's also an ereport()
> > which needs to be moved into the PG_TRY to avoid exiting with a held
> > lock.
>
> I think rollback/abort transaction due to error will handle release of
> locks.

Yes, in a proper transaction abort this is done but for a utility
command it might be possible to get there without a StartTransaction
being done. I don't immediately see how, but I wouldn't want to rely on
it, especially as doing it is simple.

> > I think you also forgot to adjust copyfuncs.c et al for your
> > VariableSetStmt change (addition of is_persistent).
>
> It is there in _copyVariableSetStmt() function.

Oh, sorry, skipped over it somehow.

> > What do you mean by "framing" a variable? Surrounding it by ""?
>
> Sorry, I am not able to find "framing" in quotes.

The quotes were just there to quote the word ;). I was referring to the
following comment:

+ /*
+ * The "auto.conf.d" directory should follow the postgresql.conf file
+ * directory not the data_directory. The same is validated while parsing
+ * the postgresql.conf configuration file. So while framing the
+ * postgresql.auto.conf and it's temp file we need to follow the
+ * postgresql.conf file directory.
+ */

> > I think validate_conf_option duplicates far too much code. You need to
> > unify parts of set_config_option with validate_conf_option.
>
> I had thought of doing this initially, but found it might be little tricky
> as duplicate part is not one single chunk.
> I shall check, if it can be done in a reasonable way.

I think calling validate_conf_option() from set_config_option and
removing the now redundant validation from there should do the trick.

Greetings,

Andres Freund

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-02-12 20:19:43 +0530, Amit Kapila wrote:
>> On Tuesday, February 12, 2013 4:55 PM Andres Freund wrote:
>>> 1) You need to grab the lock before the value is checked since some
>>> variables are interdependent (e.g. log_statement_stats, wal_level,
>>> archive_mode) and thus the check needs to be made after preventing
>>> concurrent changes.

>> This can happen if we do any SIGHUP after the command, otherwise it will
>> have old value only.

> Yes, and thats a problem imo.

That sounds to me like an entirely unreasonable requirement to put on
the patch. There is no way to positively guarantee that a value with
interdependencies will load successfully into other sessions, so why
try to enforce it at all? I note also that trying to make the value
active in the current session doesn't necessarily result in a meaningful
configuration --- what if there's an active session-level SET, for
instance? You can't just override that.

(I've said this before, but this discussion smells of overdesign every
time I look into the thread. I can't help thinking this is a big patch
with a small patch struggling to get out.)

regards, tom lane

On 2013-02-12 10:45:28 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-02-12 20:19:43 +0530, Amit Kapila wrote:
> >> On Tuesday, February 12, 2013 4:55 PM Andres Freund wrote:
> >>> 1) You need to grab the lock before the value is checked since some
> >>> variables are interdependent (e.g. log_statement_stats, wal_level,
> >>> archive_mode) and thus the check needs to be made after preventing
> >>> concurrent changes.
>
> >> This can happen if we do any SIGHUP after the command, otherwise it will
> >> have old value only.
>
> > Yes, and thats a problem imo.
>
> That sounds to me like an entirely unreasonable requirement to put on
> the patch. There is no way to positively guarantee that a value with
> interdependencies will load successfully into other sessions, so why
> try to enforce it at all? I note also that trying to make the value
> active in the current session doesn't necessarily result in a meaningful
> configuration --- what if there's an active session-level SET, for
> instance? You can't just override that.

Fair point. The reason I think its reasonable to put more effort into
validation is that I am pretty sure that users will expect that its hard
to get into a state where the server doesn't restart anymore. And
getting 80% of that is better than 50%.
And it seems required to reload the configuration anyway to make FROM
CURRENT work properly. Obviously we can also just document that it
throws away previously set values.

There's also the point that I find it suprising behaviour that SET
PERSISTENT doesn't change the active value unless a manual
pg_reload_conf() is issued.

If we decide that those are not important enough warts to care, yes,
there is no reason for any complication here.

> (I've said this before, but this discussion smells of overdesign every
> time I look into the thread. I can't help thinking this is a big patch
> with a small patch struggling to get out.)

To be honest, I don't really think thats fair to the patch. I don't see
much that can be made smaller as long as the "one file for all
persistent settings" dogma is upheld which more people seem to vote for
(exluding me).

There's the duplication around validate_conf_option and some comment
cleanup, but otherwise there really isn't that much code that looks
superflous for a simple version?

Greetings,

Andres Freund

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

On Tuesday, February 12, 2013 9:02 PM Andres Freund wrote:
> On 2013-02-12 20:19:43 +0530, Amit Kapila wrote:
> > On Tuesday, February 12, 2013 4:55 PM Andres Freund wrote:
> > > On 2013-02-12 14:57:51 +0530, Amit Kapila wrote:
> > > > On Tuesday, February 12, 2013 11:24 AM Boszormenyi Zoltan wrote:
> > > > > This mail lists this order for the single file approach:
> > > > >
> > > > > > 1) exlusive lock
> > > > > > 2) reload config file (to update in memory structures)
> > > > > > 3) check new variable
> > > > > > 4) write config file (needs to be done atomically)
> > > > > > 5) optionally reload config file
> > > > > > 6) reload lock
> > > > >
> > > > > The patch does it this way:
> > > > > 1. check new variable. No problem with that, validation for
> proper
> > > GUC
> > > > > name,
> > > > > type of the value, etc. can be done outside the lock.
> > > > > 2. grab lock
> > > > > 3. parse the config file
> > > > > 4. write the new config file
> > > > > 5. release lock
> > >
> > > 1) You need to grab the lock before the value is checked since some
> > > variables are interdependent (e.g. log_statement_stats,
> wal_level,
> > > archive_mode) and thus the check needs to be made after
> preventing
> > > concurrent changes.
> >
> > This can happen if we do any SIGHUP after the command, otherwise it
> will
> > have old value only.
>
> Yes, and thats a problem imo.
>
> SET PERSISTENT log_statement_stats = true;
> restart;
> SET PERSISTENT log_statement_stats = false;
> SET PERSISTENT log_parser_stats = true;
> ERROR...
>
> thats why I think the config file needs to be processed.
>
> > > 2) You need to apply the current config file for exactly the same
> > > reason before checking the new value. Also
> > > validate_conf_option/find_option doesn't work appropriately
> without
> > > an up2date config file. E.g. CURRENT does absurd things without
> but
> > > I
> > > think there are other cases as well.
> >
> > At this moment, I am not able to think through this, could you
> explain by
> > small example.
>
> The most trivial one I can think of is:
>
> Transaction A: SET PERSISTENT blub = 'bar';
> Transaction B: SET PERSISTENT blub FROM CURRENT;
>
> > I am thinking that shall we remove check hook function and do other
> > validation only, as this will be done at time
> > of reload, similar to what will get done when user manually edits the
> > postgresql.conf file.
>
> Why? The user isn't editing the file by hand for a reason.
>
> > > I am not saying its impossible to do the one-file approach
> correctly, I
> > > just think its harder while not being more useful.
> > >
> > > > > Reloading the config file is intentionally not done, it's even
> > > > > documented. You can do SELECT pg_reload_conf() after SET
> > > PERSISTENT
> > > > > if you need it.
> > >
> > > Not being done and it being documented as not doing so doesn't make
> it
> > > correct :P
> > > I think a SET having no immediate results is confusing. Especially
> if I
> > > am right and we do need to apply previous config changes before
> doing
> > > the next SET. But I don't have *that* strong feelings about it.
> >
> > I don't think any such expectation should be there, as with this
> feature
> > (SET PERSISTENT),
> > we will allow user to change the settings in file with command
> instead of
> > manually editing the file.
>
> I don't see why that follows. The users *do* want something different,
> otherwise they would hand-edit the file.
>
> > > > > Specifically, LWLockAcquire() is called first, then
> ParseConfigFp()
> > > > > in a PG_TRY() block, so reading the original
> postgresql.auto.conf
> > > > > is serialized. No chance to lose changes done in parallel.
> > >
> > > Not a fundamental issue, but I just noticed LWLockRelease isn't
> called
> > > in the PG_CATCH branch in set_config_file. There's also an
> ereport()
> > > which needs to be moved into the PG_TRY to avoid exiting with a
> held
> > > lock.
> >
> > I think rollback/abort transaction due to error will handle release
> of
> > locks.
>
> Yes, in a proper transaction abort this is done but for a utility
> command it might be possible to get there without a StartTransaction
> being done. I don't immediately see how, but I wouldn't want to rely on
> it, especially as doing it is simple.

True, doing it is simple but without reason can raise questions again like
why is it released when it
will be done as part of abort.
I will think more on it to see if there can be any way it can reach without
StartTransaction.

> > > I think you also forgot to adjust copyfuncs.c et al for your
> > > VariableSetStmt change (addition of is_persistent).
> >
> > It is there in _copyVariableSetStmt() function.
>
> Oh, sorry, skipped over it somehow.
>
> > > What do you mean by "framing" a variable? Surrounding it by ""?
> >
> > Sorry, I am not able to find "framing" in quotes.
>
> The quotes were just there to quote the word ;). I was referring to the
> following comment:
>
> + /*
> + * The "auto.conf.d" directory should follow the postgresql.conf
> file
> + * directory not the data_directory. The same is validated while
> parsing
> + * the postgresql.conf configuration file. So while framing the
> + * postgresql.auto.conf and it's temp file we need to follow the
> + * postgresql.conf file directory.
> + */

Shall we change it as one of below:
a. "framing the path for postgresql.auto.conf ... "
b. "framing the name for postgresql.auto.conf ... "
c. "generating the path for postgresql.auto.conf ..."
d. any other suggestion?

> > > I think validate_conf_option duplicates far too much code. You need
> to
> > > unify parts of set_config_option with validate_conf_option.
> >
> > I had thought of doing this initially, but found it might be little
> tricky
> > as duplicate part is not one single chunk.
> > I shall check, if it can be done in a reasonable way.
>
> I think calling validate_conf_option() from set_config_option and
> removing the now redundant validation from there should do the trick.

Okay.

I have intentionally not replied some parts of message as I want to wait if
there can be conclusion
for the points you have mentioned in another mail.

With Regards,
Amit Kapila.

> To be honest, I don't really think thats fair to the patch. I don't see
> much that can be made smaller as long as the "one file for all
> persistent settings" dogma is upheld which more people seem to vote for
> (exluding me).

I'm not a fan of "one file to rule them all" either. Hovever, I'm
strongly in favor of anything that can get into 9.3. So if we can make
a 9.3 patch out of this by doing "file-per-setting", then I'm all for it.

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

On Tuesday, February 12, 2013 9:02 PM Andres Freund wrote:
> On 2013-02-12 20:19:43 +0530, Amit Kapila wrote:
> > On Tuesday, February 12, 2013 4:55 PM Andres Freund wrote:
> > > On 2013-02-12 14:57:51 +0530, Amit Kapila wrote:
> > > > On Tuesday, February 12, 2013 11:24 AM Boszormenyi Zoltan wrote:
> > > > > This mail lists this order for the single file approach:
> > > > >
> > > > > > 1) exlusive lock
> > > > > > 2) reload config file (to update in memory structures)
> > > > > > 3) check new variable
> > > > > > 4) write config file (needs to be done atomically)
> > > > > > 5) optionally reload config file
> > > > > > 6) reload lock
> > > > >
> > > > > The patch does it this way:
> > > > > 1. check new variable. No problem with that, validation for
> proper
> > > GUC
> > > > > name,
> > > > > type of the value, etc. can be done outside the lock.
> > > > > 2. grab lock
> > > > > 3. parse the config file
> > > > > 4. write the new config file
> > > > > 5. release lock
> > >
> > > 1) You need to grab the lock before the value is checked since some
> > > variables are interdependent (e.g. log_statement_stats,
> wal_level,
> > > archive_mode) and thus the check needs to be made after
> preventing
> > > concurrent changes.
> >
> > This can happen if we do any SIGHUP after the command, otherwise it
> will
> > have old value only.
>
> Yes, and thats a problem imo.
>
> SET PERSISTENT log_statement_stats = true;
> restart;
> SET PERSISTENT log_statement_stats = false;
> SET PERSISTENT log_parser_stats = true;
> ERROR...
>
> thats why I think the config file needs to be processed.
>
> > > 2) You need to apply the current config file for exactly the same
> > > reason before checking the new value. Also
> > > validate_conf_option/find_option doesn't work appropriately
> without
> > > an up2date config file. E.g. CURRENT does absurd things without
> but
> > > I
> > > think there are other cases as well.
> >
> > At this moment, I am not able to think through this, could you
> explain by
> > small example.
>
> The most trivial one I can think of is:
>
> Transaction A: SET PERSISTENT blub = 'bar';
> Transaction B: SET PERSISTENT blub FROM CURRENT;
>
> > I am thinking that shall we remove check hook function and do other
> > validation only, as this will be done at time
> > of reload, similar to what will get done when user manually edits the
> > postgresql.conf file.
>
> Why? The user isn't editing the file by hand for a reason.
>
> > > I am not saying its impossible to do the one-file approach
> correctly, I
> > > just think its harder while not being more useful.
> > >
> > > > > Reloading the config file is intentionally not done, it's even
> > > > > documented. You can do SELECT pg_reload_conf() after SET
> > > PERSISTENT
> > > > > if you need it.
> > >
> > > Not being done and it being documented as not doing so doesn't make
> it
> > > correct :P
> > > I think a SET having no immediate results is confusing. Especially
> if I
> > > am right and we do need to apply previous config changes before
> doing
> > > the next SET. But I don't have *that* strong feelings about it.
> >
> > I don't think any such expectation should be there, as with this
> feature
> > (SET PERSISTENT),
> > we will allow user to change the settings in file with command
> instead of
> > manually editing the file.
>
> I don't see why that follows. The users *do* want something different,
> otherwise they would hand-edit the file.
>
> > > > > Specifically, LWLockAcquire() is called first, then
> ParseConfigFp()
> > > > > in a PG_TRY() block, so reading the original
> postgresql.auto.conf
> > > > > is serialized. No chance to lose changes done in parallel.
> > >
> > > Not a fundamental issue, but I just noticed LWLockRelease isn't
> called
> > > in the PG_CATCH branch in set_config_file. There's also an
> ereport()
> > > which needs to be moved into the PG_TRY to avoid exiting with a
> held
> > > lock.
> >
> > I think rollback/abort transaction due to error will handle release
> of
> > locks.
>
> Yes, in a proper transaction abort this is done but for a utility
> command it might be possible to get there without a StartTransaction
> being done. I don't immediately see how, but I wouldn't want to rely on
> it, especially as doing it is simple.

We cannot directly call LWLockRelease in catch block as it will lead to
Assert failure.
The reason is errfinish will set InterruptHoldOffCount to 0, and now in
LWLockRelease,
when it will call RESUME_INTERRUPTS it will check InterruptHoldOffCount
which leads to Assertion failure.

To handle it, we need to call HOLD_INTERRUPTS before it similar to
LWLockReleaseAll().
So I am not sure if we are not sure of scenario, we should add such calls in
Catch block.

> > > I think you also forgot to adjust copyfuncs.c et al for your
> > > VariableSetStmt change (addition of is_persistent).
> >
> > It is there in _copyVariableSetStmt() function.
>
> Oh, sorry, skipped over it somehow.
>
> > > What do you mean by "framing" a variable? Surrounding it by ""?
> >
> > Sorry, I am not able to find "framing" in quotes.
>
> The quotes were just there to quote the word ;). I was referring to the
> following comment:
>
> + /*
> + * The "auto.conf.d" directory should follow the postgresql.conf
> file
> + * directory not the data_directory. The same is validated while
> parsing
> + * the postgresql.conf configuration file. So while framing the
> + * postgresql.auto.conf and it's temp file we need to follow the
> + * postgresql.conf file directory.
> + */
>
> > > I think validate_conf_option duplicates far too much code. You need
> to
> > > unify parts of set_config_option with validate_conf_option.
> >
> > I had thought of doing this initially, but found it might be little
> tricky
> > as duplicate part is not one single chunk.
> > I shall check, if it can be done in a reasonable way.
>
> I think calling validate_conf_option() from set_config_option and
> removing the now redundant validation from there should do the trick.

I have tried it, it will not be straight forward, please see some problems
which we need to handle:
1. few checks like GUC_DISALLOW_IN_FILE will be different for persistent
function
- we can address this by passing ispersistent flag
2. Cannot call validate_config_option directly, because the below part of
code is
tightly coupled.
a. if (value)
..
else if()
..
else

Now if the value part is already checked in validate_config_option,
adjusting this loop will be tricky.
b. some of the variable like newextra are required outside function, so
extra parameter
needs to be passed.
c. memory free issues, as newextra will be required in function
set_config_option, however set_config_file
doesn't need it, so it would be better to free that memory inside
validate_config_option

3. Extra parameter elevel would be required in validate_conf_option.

4. Function set_config_option is used from many places, so impact can be
higher if we
do too many adjustments.

Considering all above, I think it might not be advisable to remove common
code of validate_conf_option

> It might be worth adding a check that setting and especially resetting a
user-defined variable works correctly. I.e.
> persistently setting foo.bar = 'blub' and then resetting it again.

What exactly you want in this handling?

With Regards,
Amit Kapila.

On Tuesday, February 12, 2013 4:55 PM Andres Freund wrote:
> On 2013-02-12 14:57:51 +0530, Amit Kapila wrote:
> > On Tuesday, February 12, 2013 11:24 AM Boszormenyi Zoltan wrote:
> > > 2013-02-12 04:54 keltezéssel, Amit Kapila írta:
> > > > On Tuesday, February 12, 2013 12:54 AM Andres Freund wrote:
> > > > Zoltan has reviewed this patch very thoroughly
>

> > >
> > > This mail lists this order for the single file approach:
> > >
> > > > 1) exlusive lock
> > > > 2) reload config file (to update in memory structures)
> > > > 3) check new variable
> > > > 4) write config file (needs to be done atomically)
> > > > 5) optionally reload config file
> > > > 6) reload lock
> > >
> > > The patch does it this way:
> > > 1. check new variable. No problem with that, validation for proper
> GUC
> > > name,
> > > type of the value, etc. can be done outside the lock.
> > > 2. grab lock
> > > 3. parse the config file
> > > 4. write the new config file
> > > 5. release lock
>
> 1) You need to grab the lock before the value is checked since some
> variables are interdependent (e.g. log_statement_stats, wal_level,
> archive_mode) and thus the check needs to be made after preventing
> concurrent changes.

> SET PERSISTENT log_statement_stats = true; restart; SET PERSISTENT
log_statement_stats = false; SET PERSISTENT log_parser_stats = true; ERROR

As we are not loading configuration at end of each command, so this will be
a valid case.

> 2) You need to apply the current config file for exactly the same
> reason before checking the new value. Also
> validate_conf_option/find_option doesn't work appropriately without
> an up2date config file. E.g. CURRENT does absurd things without but
> I
> think there are other cases as well.

> Transaction A: SET PERSISTENT blub = 'bar'; Transaction B: SET PERSISTENT
blub FROM CURRENT;

This behavior is not only for CURRENT, but any other setting will have same
behavior.
It just retains the last set value for any parameter before restart.

> I am not saying its impossible to do the one-file approach correctly, I
> just think its harder while not being more useful.

IMHO, let's try to see if there are any fundamental problems with this
Approach, then it
will make much more sense to change the Approach.

> > > Reloading the config file is intentionally not done, it's even
> > > documented. You can do SELECT pg_reload_conf() after SET
> PERSISTENT
> > > if you need it.
>
> Not being done and it being documented as not doing so doesn't make it
> correct :P
> I think a SET having no immediate results is confusing. Especially if I
> am right and we do need to apply previous config changes before doing
> the next SET. But I don't have *that* strong feelings about it.
>
> > > Specifically, LWLockAcquire() is called first, then ParseConfigFp()
> > > in a PG_TRY() block, so reading the original postgresql.auto.conf
> > > is serialized. No chance to lose changes done in parallel.
>
> Not a fundamental issue, but I just noticed LWLockRelease isn't called
> in the PG_CATCH branch in set_config_file. There's also an ereport()
> which needs to be moved into the PG_TRY to avoid exiting with a held
> lock.

We cannot directly call LWLockRelease in catch block as it will lead to
Assert failure.
The reason is errfinish will set InterruptHoldOffCount to 0, and now in
LWLockRelease, when it will call RESUME_INTERRUPTS it will check
InterruptHoldOffCount which leads to Assertion failure.

To handle it, we need to call HOLD_INTERRUPTS before it similar to
LWLockReleaseAll().
So I am not sure if we should add such calls in Catch block.

If you are very sure that such scenario exist where it can reach without
transaction then
we can handle it in above way.

> I think you also forgot to adjust copyfuncs.c et al for your
> VariableSetStmt change (addition of is_persistent).

> You should also check for GUC_DISALLOW_IN_FILE in validate_conf_option,
> its not the same as PGC_INTERNAL.

Fixed.

> What do you mean by "framing" a variable? Surrounding it by ""?

Changed the line to "faraming the name for".
Here framing means constructing the variable with different inputs.

> It might be worth adding a check that setting and especially resetting
> a
> user-defined variable works correctly. I.e. persistently setting
> foo.bar
> = 'blub' and then resetting it again.

Can you please elaborate a little bit more on this as I am not able to
clearly
Understand.

> You cannot use the name passed in via the VariableSetStmt in
> set_config_file, you should use the one in 'record' as returned by
> find_option (via validate_conf_option) as it handles the correct
> name. Otherwise you will get into problems with stuff like TimeZone and
> similar.

The name passed in via the VariableSetStmt always is lowercase, which will
be same
same as per current postgresql.conf.
Doing it is very simple, but I am not able to understand what is the exact
problem you are
seeing in the current way.

> I think validate_conf_option duplicates far too much code. You need to
> unify parts of set_config_option with validate_conf_option.

I have changed few things in validate_conf_option:
a. comments on top of function
b. memory was not getting freed for one variable.

I have tried to unify, it doesn't seem to be straight forward, please see
some problems which we need to handle:
1. few checks like GUC_DISALLOW_IN_FILE will be different for persistent
function
- we can address this by passing ispersistent flag 2. Cannot call
validate_config_option directly, because the below part of code is
tightly coupled.
a. if (value)
..
else if()
..
else

Now if the value part is already checked in validate_config_option,
adjusting this loop will be tricky.
b. some of the variable like newextra are required outside function, so
extra parameter
needs to be passed.
c. memory free issues, as newextra will be required in function
set_config_option, however set_config_file
doesn't need it, so it would be better to free that memory inside
validate_config_option

3. Extra parameter elevel would be required in validate_conf_option.

4. Function set_config_option is used from many places, so impact can be
higher if we
do too many adjustments.

Considering all above, I think it might not be advisable to remove common
code of validate_conf_option.

With Regards,
Amit Kapila.

This submission didn't have any listed reviewers in the CF app. I added
Zoltan and Andres since both of you went through the usual review steps
and have given lots of feedback.

There are two main sets of issues that keep popping up with this feature:

-The implementation seems too long, at around 2189 lines of diff. I
have a few ideas for how things might be trimmed below. I do appreciate
that a good part of the length is a long set of regression tests,
relative to what a feature like this normally gets.

-It might be possible to get a simpler implementation with a file per
setting.

I can make a pass over cleaning up the wording in your comments and
documentation. There are enough coding issues that I think that should
wait until another rev of the patch first.

= Directory vs. single file =

The main reason I've advocated a configuration file directory is to try
and suggest a standard for tool generated config files to co-exist in.
This particular feature doesn't *need* that. But in the long term I was
hoping to have more tools that can write out .conf files without having
to read and understand every existing file that's in there. It doesn't
make that big of a difference whether this particular one lives in
$PGDATA/postgresql.auto.conf or $PGDATA/postgresql.auto.conf. The main
reason for the directory is to make the second such tool not further
clutter the main $PGDATA directory.

I would like to see the name of the directory be conf.d instead of
auto.conf.d though. What's "auto" about it? Using that word just adds
a source of confusion. The same problem exists with the file name
itself. I was hoping for conf.d/persistent.conf here, and no use of the
word "auto" in the code itself.

What I don't see a lot of value in is the config file per setting idea.
I was hoping SET PERSISTENT put all of its changes into once place.
It should be obvious where they came from, and that people can't edit
that file manually without breaking the feature. To me something like
persistent.conf gives that impression, while postgresql.auto.conf sounds
like the output from some auto-tuning tool.

= Length reduction =

I'm not sure why you are opening the old auto config file with
ParseConfigFp. Can't you just navigate the existing GUCs in memory and
directly write the new one out? If someone is going to manually edit
this file and use SET PERSISTENT, they're going to end up in trouble
regardless. I don't think it's really worth the extra complexity needed
to try and handle that case.

It sounds like you've thought a good bit about how to eliminate all the
code in the validate_conf_option function. I think that idea may take
some more thinking to try and be creative about it. Have you looked at
whether there's any way to refactor this to be smaller by modifying
guc.c to break out parts of its implementation needed here?

Is it possible to modify your changes to src/backend/parser/gram.y to
produce a smaller diff? There looks to be a good amount of code (~25
lines) that aren't really changed that I can see. I suspect they might
have been hit with an editor whitespace change.

= General code issues =

There is some trivial bit rot on your v10 here:

1 out of 8 hunks FAILED -- saving rejects to file
src/backend/parser/gram.y.rej
1 out of 3 hunks FAILED -- saving rejects to file
src/bin/initdb/initdb.c.rej

Your change to gram.y was broken by PROGRAM being added. I'm not sure
why the initdb.c one doesn't apply; whitespace issue maybe? As written
right now, even with fixing those two, I'm getting this compiler error:

gram.y:12694.27-36: warning: type clash on default action: <keyword> != <>
gram.y:1306.31-40: symbol PERSISTENT is used, but is not defined as a
token and has no rules

This combination in src/test/regress/sql/persistent.sql a few times
makes me nervous:

+ select pg_reload_conf();
+ select pg_sleep(1);

Why the pause? There are some regression tests with sleeps in them,
such as the stats collector and the timezone tests. I would expect
pg_reload_conf would not return until its configuration is stable, so
this is a bit of a surprise.

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

> = Directory vs. single file =
>
> The main reason I've advocated a configuration file directory is to try
> and suggest a standard for tool generated config files to co-exist in.
> This particular feature doesn't *need* that. But in the long term I was
> hoping to have more tools that can write out .conf files without having
> to read and understand every existing file that's in there. It doesn't
> make that big of a difference whether this particular one lives in
> $PGDATA/postgresql.auto.conf or $PGDATA/postgresql.auto.conf. The main
> reason for the directory is to make the second such tool not further
> clutter the main $PGDATA directory.
>
> I would like to see the name of the directory be conf.d instead of
> auto.conf.d though. What's "auto" about it? Using that word just adds
> a source of confusion. The same problem exists with the file name
> itself. I was hoping for conf.d/persistent.conf here, and no use of the
> word "auto" in the code itself.

my $0.03: I agree with Greg that using the directory is a good idea, and
that his naming is an improvement.

> What I don't see a lot of value in is the config file per setting idea.
> I was hoping SET PERSISTENT put all of its changes into once place. It
> should be obvious where they came from, and that people can't edit that
> file manually without breaking the feature. To me something like
> persistent.conf gives that impression, while postgresql.auto.conf sounds
> like the output from some auto-tuning tool.

I think most users would prefer a single file. The reason
file-per-setting is being discussed is that it avoids a lot of locking
issues and code complexity.

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

Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> I would like to see the name of the directory be conf.d instead of
>> auto.conf.d though. What's "auto" about it? Using that word just adds
>> a source of confusion. The same problem exists with the file name
>> itself. I was hoping for conf.d/persistent.conf here, and no use of the
>> word "auto" in the code itself.

> my $0.03: I agree with Greg that using the directory is a good idea, and
> that his naming is an improvement.

Neither of those names is consistent with any other PGDATA subdirectory
name we use. It should just be config, or perhaps pg_config, though the
latter risks confusion with the tool of the same name.

FWIW, I do think that having "auto" or some such in the file name(s)
would be a good idea, to help warn people off editing them manually.

regards, tom lane

On 3/3/13 8:34 PM, Tom Lane wrote:
> Neither of those names is consistent with any other PGDATA subdirectory
> name we use. It should just be config, or perhaps pg_config, though the
> latter risks confusion with the tool of the same name.

I'd be just as happy with config/ as the directory name. I have a bias
toward wanting this to look like Apache that I've been scolded for
before, I forgot to avoid that this time.

> FWIW, I do think that having "auto" or some such in the file name(s)
> would be a good idea, to help warn people off editing them manually.

I can see that for the file name itself, as long as it's not in the
directory name. I don't like giving the idea that everything in there
is automatically generated. config/persistent-auto.conf or
config/persist-auto.conf maybe?

The way files are sorted by name means that using
config/auto-persist[ent].conf would make the file more likely to be
processed before other things in that directory. I could live with that
order too. I think it is important to name the file such that it
suggests being connected to the SET PERSISTENT feature though.

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

On 03/04/2013 09:07 AM, Greg Smith wrote:
> I'm not sure why you are opening the old auto config file with
> ParseConfigFp. Can't you just navigate the existing GUCs in memory
> and directly write the new one out? If someone is going to manually
> edit this file and use SET PERSISTENT, they're going to end up in
> trouble regardless. I don't think it's really worth the extra
> complexity needed to try and handle that case.
Additionally, if you want to avoid silently overwriting user changes,
you could store a timestamp for when we last updated the persistent
config and compare it to the on-disk timestamp before writing. If they
don't match a warning would be issued and the config would be
overwritten anyway. There's a race, of course, but since the worst case
is that we fail to issue a warning it's a pretty harmless one.

As for the per-file vs single-file issue and concerns about locking
complexity: Can't we just use a global lock in shm to enforce that
exactly one backend at a time may be modifying the global configuration?
I don't see this ever becoming a realistic concern for concurrency and
performance, and the shm cost would be tiny.

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

On 03/04/2013 09:34 AM, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>>> I would like to see the name of the directory be conf.d instead of
>>> auto.conf.d though. What's "auto" about it? Using that word just adds
>>> a source of confusion. The same problem exists with the file name
>>> itself. I was hoping for conf.d/persistent.conf here, and no use of the
>>> word "auto" in the code itself.
>> my $0.03: I agree with Greg that using the directory is a good idea, and
>> that his naming is an improvement.
> Neither of those names is consistent with any other PGDATA subdirectory
> name we use. It should just be config, or perhaps pg_config, though the
> latter risks confusion with the tool of the same name.
>
> FWIW, I do think that having "auto" or some such in the file name(s)
> would be a good idea, to help warn people off editing them manually.
Yep; auto in the dir name seems unwise given that we want to allow tools
to drop config snippets in there, but auto in the file name would IMO
help people realise it shouldn't be edited directly.

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

On Monday, March 04, 2013 7:26 AM Greg Smith wrote:
> On 3/3/13 8:34 PM, Tom Lane wrote:
> > Neither of those names is consistent with any other PGDATA
> subdirectory
> > name we use. It should just be config, or perhaps pg_config, though
> the
> > latter risks confusion with the tool of the same name.
>
> I'd be just as happy with config/ as the directory name. I have a bias
> toward wanting this to look like Apache that I've been scolded for
> before, I forgot to avoid that this time.
>
> > FWIW, I do think that having "auto" or some such in the file name(s)
> > would be a good idea, to help warn people off editing them manually.
>
> I can see that for the file name itself, as long as it's not in the
> directory name. I don't like giving the idea that everything in there
> is automatically generated. config/persistent-auto.conf or
> config/persist-auto.conf maybe?
>
> The way files are sorted by name means that using
> config/auto-persist[ent].conf would make the file more likely to be
> processed before other things in that directory. I could live with
> that
> order too. I think it is important to name the file such that it
> suggests being connected to the SET PERSISTENT feature though.

If we want to link it with SET PERSISTENT feature, we can choose name as you
suggest or
config/auto.persistent.conf?

If we see the current files postgresql.conf, recovery.conf etc, it seems
most are related
to the functionality it provides. For example recovery.conf suggests that it
contains settings for doing recovery.

The current name postgresql.auto.conf has been chosen to make it's
connection with user editable
postgresql.conf. I thought it will give impression to user that this
contains settings similar to postgresql.conf
but this file is auto generated, so it can't be editable by user.

With Regards,
Amit Kapila.

On Monday, March 04, 2013 9:41 AM Craig Ringer wrote:
> On 03/04/2013 09:07 AM, Greg Smith wrote:
> > I'm not sure why you are opening the old auto config file with
> > ParseConfigFp. Can't you just navigate the existing GUCs in memory
> > and directly write the new one out? If someone is going to manually
> > edit this file and use SET PERSISTENT, they're going to end up in
> > trouble regardless. I don't think it's really worth the extra
> > complexity needed to try and handle that case.
> Additionally, if you want to avoid silently overwriting user changes,
> you could store a timestamp for when we last updated the persistent
> config and compare it to the on-disk timestamp before writing. If they
> don't match a warning would be issued and the config would be
> overwritten anyway. There's a race, of course, but since the worst case
> is that we fail to issue a warning it's a pretty harmless one.

> As for the per-file vs single-file issue and concerns about locking
> complexity: Can't we just use a global lock in shm to enforce that
> exactly one backend at a time may be modifying the global
> configuration?

Yes, this is currently used.

> I don't see this ever becoming a realistic concern for concurrency and
> performance, and the shm cost would be tiny.

With Regards,
Amit Kapila.

On Monday, March 04, 2013 6:38 AM Greg Smith wrote:
> This submission didn't have any listed reviewers in the CF app. I
> added
> Zoltan and Andres since both of you went through the usual review steps
> and have given lots of feedback.

Thank you for review.

> There are two main sets of issues that keep popping up with this
> feature:
>
> -The implementation seems too long, at around 2189 lines of diff. I
> have a few ideas for how things might be trimmed below. I do
> appreciate
> that a good part of the length is a long set of regression tests,
> relative to what a feature like this normally gets.
>
> -It might be possible to get a simpler implementation with a file per
> setting.
>
> I can make a pass over cleaning up the wording in your comments and
> documentation. There are enough coding issues that I think that should
> wait until another rev of the patch first.

I have tried to completely handle all the comments, but need 1 more day.

> = Directory vs. single file =
>
> The main reason I've advocated a configuration file directory is to try
> and suggest a standard for tool generated config files to co-exist in.
> This particular feature doesn't *need* that. But in the long term I
> was
> hoping to have more tools that can write out .conf files without having
> to read and understand every existing file that's in there. It doesn't
> make that big of a difference whether this particular one lives in
> $PGDATA/postgresql.auto.conf or $PGDATA/postgresql.auto.conf. The main
> reason for the directory is to make the second such tool not further
> clutter the main $PGDATA directory.
>
> I would like to see the name of the directory be conf.d instead of
> auto.conf.d though. What's "auto" about it? Using that word just adds
> a source of confusion. The same problem exists with the file name
> itself. I was hoping for conf.d/persistent.conf here, and no use of
> the
> word "auto" in the code itself.

We can name the directory as config.
For file, I am awaiting your reponse to select from below options
a. persistent.auto.conf
b. persist.auto.conf
c. auto.persist.conf
d. postgresql.auto.conf

> What I don't see a lot of value in is the config file per setting idea.
> I was hoping SET PERSISTENT put all of its changes into once place.
> It should be obvious where they came from, and that people can't edit
> that file manually without breaking the feature. To me something like
> persistent.conf gives that impression, while postgresql.auto.conf
> sounds
> like the output from some auto-tuning tool.
>
> = Length reduction =
>
> I'm not sure why you are opening the old auto config file with
> ParseConfigFp. Can't you just navigate the existing GUCs in memory and
> directly write the new one out? If someone is going to manually edit
> this file and use SET PERSISTENT, they're going to end up in trouble
> regardless. I don't think it's really worth the extra complexity
> needed
> to try and handle that case.

This is for logic to see if variable already exist just replace it's value
at the same
location in .auto file, else append at end.
IIUC what you are suggesting is set first in memory then while writing just
check the memory, whatever
Is new value just write it.
if we just traverse in memory how will we know where this exist in file.
This might have complication what if write to memory is success, but to file
it fails.

> It sounds like you've thought a good bit about how to eliminate all the
> code in the validate_conf_option function. I think that idea may take
> some more thinking to try and be creative about it. Have you looked at
> whether there's any way to refactor this to be smaller by modifying
> guc.c to break out parts of its implementation needed here?

Today I tried again by having single common function for both places, but
It is getting clumsy. So I can to write multiple functions for each
datatype?

> Is it possible to modify your changes to src/backend/parser/gram.y to
> produce a smaller diff? There looks to be a good amount of code (~25
> lines) that aren't really changed that I can see. I suspect they might
> have been hit with an editor whitespace change.

I will do pgindent.
Can you point what are extra lines that can be removed.

> = General code issues =
>
> There is some trivial bit rot on your v10 here:
>
> 1 out of 8 hunks FAILED -- saving rejects to file
> src/backend/parser/gram.y.rej
> 1 out of 3 hunks FAILED -- saving rejects to file
> src/bin/initdb/initdb.c.rej
>
> Your change to gram.y was broken by PROGRAM being added. I'm not sure
> why the initdb.c one doesn't apply; whitespace issue maybe? As written
> right now, even with fixing those two, I'm getting this compiler error:
>
> gram.y:12694.27-36: warning: type clash on default action: <keyword> !=
> <>
> gram.y:1306.31-40: symbol PERSISTENT is used, but is not defined as a
> token and has no rules

I shall take care of these, minor code related issues due to recent changes
in HEAD.

> This combination in src/test/regress/sql/persistent.sql a few times
> makes me nervous:
>
> + select pg_reload_conf();
> + select pg_sleep(1);
>
> Why the pause? There are some regression tests with sleeps in them,
> such as the stats collector and the timezone tests. I would expect
> pg_reload_conf would not return until its configuration is stable, so
> this is a bit of a surprise.

pg_reload_conf(), doesn't ensure that configuration has been propagated to
memory.
It just sends the SIGHUP signal to postmaster.
If I remove, it will fail the tests.

With Regards,
Amit Kapila.

On Monday, March 04, 2013 8:46 PM Amit Kapila wrote:
> On Monday, March 04, 2013 6:38 AM Greg Smith wrote:
> > This submission didn't have any listed reviewers in the CF app. I
> > added
> > Zoltan and Andres since both of you went through the usual review
> steps
> > and have given lots of feedback.
>
> Thank you for review.

Fujii Masao has also done the review and test of patch multiple times.

> > There are two main sets of issues that keep popping up with this
> > feature:
> >
> > -The implementation seems too long, at around 2189 lines of diff. I
> > have a few ideas for how things might be trimmed below. I do
> > appreciate
> > that a good part of the length is a long set of regression tests,
> > relative to what a feature like this normally gets.
> >
> > -It might be possible to get a simpler implementation with a file per
> > setting.
> >
> > I can make a pass over cleaning up the wording in your comments and
> > documentation. There are enough coding issues that I think that
> should
> > wait until another rev of the patch first.
>
> I have tried to completely handle all the comments, but need 1 more
> day.
>
> > = Directory vs. single file =
> >
> > The main reason I've advocated a configuration file directory is to
> try
> > and suggest a standard for tool generated config files to co-exist
> in.
> > This particular feature doesn't *need* that. But in the long term I
> > was
> > hoping to have more tools that can write out .conf files without
> having
> > to read and understand every existing file that's in there. It
> doesn't
> > make that big of a difference whether this particular one lives in
> > $PGDATA/postgresql.auto.conf or $PGDATA/postgresql.auto.conf. The
> main
> > reason for the directory is to make the second such tool not further
> > clutter the main $PGDATA directory.
> >
> > I would like to see the name of the directory be conf.d instead of
> > auto.conf.d though. What's "auto" about it? Using that word just
> adds
> > a source of confusion. The same problem exists with the file name
> > itself. I was hoping for conf.d/persistent.conf here, and no use of
> > the
> > word "auto" in the code itself.
>
> We can name the directory as config.
> For file, I am awaiting your reponse to select from below options
> a. persistent.auto.conf
> b. persist.auto.conf
> c. auto.persist.conf
> d. postgresql.auto.conf

In v11 patch, I have changed name of directory to config.
For file name, currently I have not changed, but if you feel it needs to be
changed, kindly suggest any one of above or if any other better you have in
mind.

> > What I don't see a lot of value in is the config file per setting
> idea.
> > I was hoping SET PERSISTENT put all of its changes into once place.
> > It should be obvious where they came from, and that people can't edit
> > that file manually without breaking the feature. To me something
> like
> > persistent.conf gives that impression, while postgresql.auto.conf
> > sounds
> > like the output from some auto-tuning tool.
> >
> > = Length reduction =
> >
> > I'm not sure why you are opening the old auto config file with
> > ParseConfigFp. Can't you just navigate the existing GUCs in memory
> and
> > directly write the new one out? If someone is going to manually edit
> > this file and use SET PERSISTENT, they're going to end up in trouble
> > regardless. I don't think it's really worth the extra complexity
> > needed
> > to try and handle that case.
>
> This is for logic to see if variable already exist just replace it's
> value
> at the same
> location in .auto file, else append at end.
> IIUC what you are suggesting is set first in memory then while writing
> just
> check the memory, whatever
> Is new value just write it.
> if we just traverse in memory how will we know where this exist in
> file.
> This might have complication what if write to memory is success, but to
> file
> it fails.
>
> > It sounds like you've thought a good bit about how to eliminate all
> the
> > code in the validate_conf_option function. I think that idea may
> take
> > some more thinking to try and be creative about it. Have you looked
> at
> > whether there's any way to refactor this to be smaller by modifying
> > guc.c to break out parts of its implementation needed here?
>
> Today I tried again by having single common function for both places,
> but
> It is getting clumsy. So I can to write multiple functions for each
> datatype?

Finally, I could come up with unified function with common parts together
based on
your suggestions.

> > Is it possible to modify your changes to src/backend/parser/gram.y to
> > produce a smaller diff? There looks to be a good amount of code (~25
> > lines) that aren't really changed that I can see. I suspect they
> might
> > have been hit with an editor whitespace change.
>
> I will do pgindent.
> Can you point what are extra lines that can be removed.

I think it is mainly due to the changes done in gram.y.

> > = General code issues =
> >
> > There is some trivial bit rot on your v10 here:
> >
> > 1 out of 8 hunks FAILED -- saving rejects to file
> > src/backend/parser/gram.y.rej
> > 1 out of 3 hunks FAILED -- saving rejects to file
> > src/bin/initdb/initdb.c.rej
> >
> > Your change to gram.y was broken by PROGRAM being added. I'm not
> sure
> > why the initdb.c one doesn't apply; whitespace issue maybe? As
> written
> > right now, even with fixing those two, I'm getting this compiler
> error:
> >
> > gram.y:12694.27-36: warning: type clash on default action: <keyword>
> !=
> > <>
> > gram.y:1306.31-40: symbol PERSISTENT is used, but is not defined as a
> > token and has no rules
>
> I shall take care of these, minor code related issues due to recent
> changes
> in HEAD.

Gram.y - PERSISTENT word is missed from one of the list
Initdb.c - structure subdirs, doesn't contain new directory.
Fixed both of the above.

> > This combination in src/test/regress/sql/persistent.sql a few times
> > makes me nervous:
> >
> > + select pg_reload_conf();
> > + select pg_sleep(1);
> >
> > Why the pause? There are some regression tests with sleeps in them,
> > such as the stats collector and the timezone tests. I would expect
> > pg_reload_conf would not return until its configuration is stable, so
> > this is a bit of a surprise.
>
> pg_reload_conf(), doesn't ensure that configuration has been propagated
> to
> memory.
> It just sends the SIGHUP signal to postmaster.
> If I remove, it will fail the tests.

I have kept this intact.

With Regards,
Amit Kapila.

On 3/5/13 9:07 AM, Amit Kapila wrote:
> In v11 patch, I have changed name of directory to config.
> For file name, currently I have not changed, but if you feel it needs to be
> changed, kindly suggest any one of above or if any other better you have in
> mind.

This seems fine for now. Hashing out exactly which name is best is not
going to be the thing that determines whether this can be committed or not.

Your v11 applies cleanly for me too, and the code does look a lot nicer.
The code diff itself is down to 1300 lines and not nearly as
disruptive, the rest is docs or the many regression tests you've added.
That's still not the sort of small change Tom was hoping this was
possible, but it's closer to the right range. It may not really be
possible to make this much smaller.

I'm out of time for today, but I'll try to do some functional review
work on this tomorrow if no one beats me to it. I would find it very
useful to me personally if this feature were committed, and we know
there's plenty of user demand for it too.

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

On Thursday, March 07, 2013 10:54 AM Greg Smith wrote:
> On 3/5/13 9:07 AM, Amit Kapila wrote:
> > In v11 patch, I have changed name of directory to config.
> > For file name, currently I have not changed, but if you feel it needs
> to be
> > changed, kindly suggest any one of above or if any other better you
> have in
> > mind.
>
> This seems fine for now. Hashing out exactly which name is best is not
> going to be the thing that determines whether this can be committed or
> not.
>
> Your v11 applies cleanly for me too, and the code does look a lot
> nicer.
> The code diff itself is down to 1300 lines and not nearly as
> disruptive, the rest is docs or the many regression tests you've added.

I also think the tests added for regression may be more than required.
Currently it is organized as below:

1. 3 type of parameters to use with set persistent.
a. parameters for which new connection is required (log_connections)
b. parameters for which server restart is required
c. parameters for which reload is required

I think in third category "c", there are many parameters, I think may be
3-4 parameters should be sufficient.
The initial intention to keep more parameters is to have test it for
various type(Boolean, integer, float, string)

2. Set some of the parameters to default, then try reload and reconnect.
3. Again set those parameters to some value and re-verify by reload and
reconnect.
4. Test for set persistent command inside functions and transaction.
5. Set all parameters value to default.

I think tests for step-3 can be removed and tests for step-5 can be merged
to step-2.

If you think above optimization's to reduce number of tests are okay, then I
will
update the patch.

> That's still not the sort of small change Tom was hoping this was
> possible, but it's closer to the right range. It may not really be
> possible to make this much smaller.
>
> I'm out of time for today, but I'll try to do some functional review
> work on this tomorrow if no one beats me to it. I would find it very
> useful to me personally if this feature were committed, and we know
> there's plenty of user demand for it too.

With Regards,
Amit Kapila.

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

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

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

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

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

= Functional changes =

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

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

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

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

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

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

Something like this would be appropriate for SET PERSISTENT:

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

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

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

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

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

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

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

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

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

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

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

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

It needs to look like this:

work_mem = '2 MB'

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

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

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

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

= Tested features that work fine =

Entries added here are tracked as you'd expect:

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

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

= Concurrency and performance slowdown =

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

random-join-limit.sql:

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

random-segments.sql:

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

I then fired off a bunch of these in parallel:

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

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

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

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

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

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

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

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

real 0m0.807s

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

= Loss of in-memory changes =

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

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

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

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

= Error messages =

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

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

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

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

Deleting the whole config directory gives this:

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

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

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

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

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

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

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

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

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

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

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

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

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