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

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

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

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

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

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

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