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 <t...@sss.pgh.pa.us> wrote:
> alain radix <alain.ra...@gmail.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
