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

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

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

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

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

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

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

regards, tom lane