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 radix <alain.radix@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])
