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

[Clément Lassieur]

Christopher Baines <address@hidden> writes:

> Clément Lassieur <address@hidden> writes:
>
>> Christopher Baines <address@hidden> 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...

The PID approch only makes sense when the executable run by Shepherd
doesn't return; i.e. it blocks.  Then Shepherd needs to fork a child
process (with make-forkexec-constructor), so that it doesn't block.  And
the only way to know if the daemon has started is to find a "proof",
which is the PID file.  That's the point of the #:pid-file parameter of
make-forkexec-constructor.

But the PostgreSQL package is more elaborate: it provides an 'pg_ctl'
executable that does all the hard work:
  - starting the daemon,
  - checking its PID to make sure the daemon is running,
  - exiting with 0 if the daemon is running.
So Shepherd doesn't need to fork, it just checks the return value of
pg_ctl.

Conclusion: (start-service 'postgres) returning #t means that pg_ctl
exit code is 0, which means that the daemon is running.  I hope it makes
more sense :-)