Re: Requesting external_pid_file with postgres -C when not initialized lead to coredump

Testing that external_pid_file was not null didn't seem necessary to me
because it's initialized to '' and background workers could only start
after the startup piece of code.

If external_pid_file is set to null while the server run, then I prefer to
not hide the problem as it would only be reported at the server shutdown

I tested with ALTER SYSTEM SET but couldn't set external_pid_file to null,
only to ''
This another good reason to use '' instead of null, because it can be set
with ALTER SYSTEM SET even if I see no reason to ;-)

Using strlen(external_pid_file) instead of external_pid_file[0] seems more
readable to me.
I agree that it cost a few cpu cycles more.

I'm also unsure that testing if(A && B) would always be compiled to A being
tested before B so that it won't always protect from testing
external_pid_file[0] with external_pid_file being null.
To ensure that, I would prefer :
if(! external_pid_file){
if(external_pid_file[0])

But, I see it at useless protective code against an impossible situation.
Maybe I'm wrong, but I don't see how it could happen.

About parameters being null, as they're reported as '' in pg_settings and
ALTER SYSTEM SET can only set them to '', I consider this should be avoided.
I used all parameters reported to '' in pg_settings after initdb and found
only external_pid_file to crash postgres -C

Alain

PS : Thanks for pgBackRest ;-)

On 22 June 2016 at 18:51, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> alain radix <alain(dot)radix(at)gmail(dot)com> writes:
> > Another effect of the patch is that it become possible to start a cluster
> > with external_pid_file='' in postgresql.conf
> > It would have that same behavior as many other parameters.
>
> I'd still be inclined to do that with something along the lines of
>
> - if (external_pid_file)
> + if (external_pid_file && external_pid_file[0])
>
> rather than changing the default per se.
>
> regards, tom lane
>

--
Alain Radix