Skip site navigation (1) Skip section navigation (2)

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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Boszormenyi Zoltan <zb(at)cybertec(dot)at>
Cc: Amit kapila <amit(dot)kapila(at)huawei(dot)com>, "'Robert Haas'" <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]
Date: 2013-01-12 19:11:27
Message-ID: 21303.1358017887@sss.pgh.pa.us (view raw or flat)
Thread:
Lists: pgsql-hackers
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


In response to

Responses

pgsql-hackers by date

Next:From: Kevin GrittnerDate: 2013-01-12 19:14:03
Subject: Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
Previous:From: Andres FreundDate: 2013-01-12 18:42:18
Subject: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group