GNU bug report logs - #30701
[PATCH 0/4] PostgreSQL service changes (add record type, and system test)

Previous Next

Full log

Message #23 received at 30701 <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 30701 <at> debbugs.gnu.org
Subject: Re: [bug#30701] [PATCH 2/4] services: Use a external pid file for
 PostgreSQL.
Date: Mon, 05 Mar 2018 01:32:18 +0100

Christopher Baines <mail <at> 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.

> @@ -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.

>    (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.

>  (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.

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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.