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