Toggle quote (131 lines)
> 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...
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 :-)
