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

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

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2013-01-18 21:32 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan <zb@cybertec.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/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:

2013-01-18 21:32 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan <zb@cybertec.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@cybertec.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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Jan 21, 2013 3:06 AM, "Craig Ringer" <craig@2ndquadrant.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@2ndquadrant.com
<mailto:craig@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jan 21, 2013 at 9:01 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 01/21/2013 02:17 AM, Magnus Hagander wrote:

On Jan 21, 2013 3:06 AM, "Craig Ringer" <craig@2ndquadrant.com
<mailto:craig@2ndquadrant.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/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@cybertec.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@cybertec.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/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila <amit.kapila@huawei.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@cybertec.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jan 22, 2013 at 11:54 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila <amit.kapila@huawei.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@cybertec.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tuesday, January 22, 2013 8:25 PM Fujii Masao wrote:

On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila <amit.kapila@huawei.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@cybertec.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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tuesday, January 22, 2013 10:14 PM Fujii Masao wrote:

On Tue, Jan 22, 2013 at 11:54 PM, Fujii Masao <masao.fujii@gmail.com>
wrote:

On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila

<amit.kapila@huawei.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@cybertec.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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers