Re: Proposal for Allow postgresql.conf values to be changed via SQL [review] - Mailing list pgsql-hackers

From Tom Lane
Subject Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]
Date
Msg-id 21303.1358017887@sss.pgh.pa.us
Whole thread Raw
In response to Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]  (Boszormenyi Zoltan <zb@cybertec.at>)
Responses Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Next
From: "Kevin Grittner"
Date:
Subject: Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL