[bug#30701] [PATCH 2/4] services: Use a external pid file for PostgreSQL.

[Christopher Baines <mail@cbaines.net>]

[-- Attachment #1: Type: text/plain, Size: 6142 bytes --]


Clément Lassieur <clement@lassieur.org> writes:

> Christopher Baines <mail@cbaines.net> writes:
>
>> * gnu/services/databases.scm (postgresql-pid-file-for-version): New procedure.
>>   (<postgresql-config-file>): Add a new external-pid-file field.
>>   (postgresql-config-file-compiler): Add support for the external-pid-file.
>>   (postgresql-activation): Create the directory for the pid file.
>>   (postgresql-shepherd-service): Use the pid-file when starting the service.
>> ---
>>  gnu/services/databases.scm | 48 ++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 36 insertions(+), 12 deletions(-)
>
> ...
>
>> +(define (postgresql-pid-file-for-version version)
>> +  (string-append "/var/run/postgresql/"
>> +                 (version-major+minor version)
>> +                 "-main.pid"))
>> +
>>  (define-record-type* <postgresql-config-file>
>>    postgresql-config-file make-postgresql-config-file
>>    postgresql-config-file?
>> -  (log-destination postgresql-config-file-log-destination
>> -                   (default "syslog"))
>> -  (hba-file        postgresql-config-file-hba-file
>> -                   (default %default-postgres-hba))
>> -  (ident-file      postgresql-config-file-ident-file
>> -                   (default %default-postgres-ident))
>> -  (extra-config    postgresql-config-file-extra-config
>> -                   (default '())))
>> +  (log-destination   postgresql-config-file-log-destination
>> +                     (default "syslog"))
>> +  (hba-file          postgresql-config-file-hba-file
>> +                     (default %default-postgres-hba))
>> +  (ident-file        postgresql-config-file-ident-file
>> +                     (default %default-postgres-ident))
>> +  (external-pid-file postgresql-config-file-external-pid-file
>> +                     (default (postgresql-pid-file-for-version
>> +                               (package-version postgresql))))
>
> Why does external-pid-file have a default value.  It's optional, and the
> user already has access to $DATA/postmaster.pid anyway.

I ended up adding this as I was writing the system test. I was coping
previous tests that I have written, in which I've been checking that the
shepherd gets the PID back when it starts the process.

Before I set out in writing this, I didn't realise that PostgreSQL
stores the PID in the data directory by default, or that pg_ctl waits
for actions to complete by default either.

I'm not particularly attached to this patch, I guess it mostly offers
consistency with other services.

>> @@ -140,6 +153,9 @@ host	all	all	::1/128 	trust"))
>>                    (default 5432))
>>    (locale         postgresql-configuration-locale
>>                    (default "en_US.utf8"))
>> +  (pid-file       postgresql-configuration-pid-file
>> +                  (default (postgresql-pid-file-for-version
>> +                               (package-version postgresql))))
>
> The main PID file is chosen by Postgres, and put at
> $DATA/postmaster.pid.  I don't think it's customizable.  This setting
> (pid-file) probably doesn't affect the daemon the way you think it does.

This part of the configuration is just meant to be where the Guix part
of the service expects to find the pid-file (if one is used, and it's
not #f).

>>    (config-file    postgresql-configuration-file
>>                    (default (postgresql-config-file)))
>>    (data-directory postgresql-configuration-data-directory
>> @@ -157,7 +173,8 @@ host	all	all	::1/128 	trust"))
>>
>> +           ;; Create a directory for the pid file
>> +           (mkdir-p #$(dirname pid-file))
>> +           (chown #$(dirname pid-file) (passwd:uid user) (passwd:gid user))
>
> I think it would make more sense to create the directory for the
> external-pid-file.

As far as I understand it, this is what this does.

>>  (define postgresql-shepherd-service
>>    (match-lambda
>> -    (($ <postgresql-configuration> postgresql port locale config-file data-directory)
>> +    (($ <postgresql-configuration> postgresql port locale pid-file
>> +                                   config-file data-directory)
>>       (let* ((pg_ctl-wrapper
>>               ;; Wrapper script that switches to the 'postgres' user before
>>               ;; launching daemon.
>> @@ -221,7 +243,9 @@ host	all	all	::1/128 	trust"))
>>                (provision '(postgres))
>>                (documentation "Run the PostgreSQL daemon.")
>>                (requirement '(user-processes loopback syslogd))
>> -              (start (action "start"))
>> +              (start #~(make-forkexec-constructor
>> +                        '(#$pg_ctl-wrapper "start")
>> +                        #:pid-file #$pid-file))
>>                (stop (action "stop"))))))))
>
> If pid-file != external-pid-file, Sherpherd will wait for a file that
> doesn't exist won't it?  Because Postgresql will never be aware of that
> pid-file.

Yep.

> Plus, there is no reason to use make-forkexec-constructor on pg_ctl
> because pg_ctl returns after it has checked that the daemon is running.
> For the same reason, Shepherd doesn't need to know about Postgres' PID.
> All the hard work is done by pg_ctl.

As the comment I made at the top, I did this when I was writing the
system test. If you remove this patch, when you call (start-service
'postgres), it will return #f if the service starts successfully. If you
tweak the service to make it fail to start (e.g. by changing the "start"
action to something else), you get the same observable behaviour,
start-service returns #f.

The way this works for other services, normally through
make-forkexec-constructor is that calling start-service will return the
PID.

While the system test does still add some value even without checking if
the service has started, doing so would be really good. Even if it's not
using the PID file approach, maybe the exit code of pg_ctl could be
used? I'm not really sure why it isn't working like that already, as
invoke usually returns either #t or #f...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]