From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1esp2F-0000d6-CT for guix-patches@gnu.org; Mon, 05 Mar 2018 07:16:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1esp2A-0000bY-Ns for guix-patches@gnu.org; Mon, 05 Mar 2018 07:16:07 -0500 Received: from debbugs.gnu.org ([208.118.235.43]:37300) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1esp2A-0000bJ-J7 for guix-patches@gnu.org; Mon, 05 Mar 2018 07:16:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1esp2A-0003s2-70 for guix-patches@gnu.org; Mon, 05 Mar 2018 07:16:02 -0500 Subject: [bug#30701] [PATCH 2/4] services: Use a external pid file for PostgreSQL. Resent-Message-ID: References: <20180304191633.20262-1-mail@cbaines.net> <20180304191633.20262-2-mail@cbaines.net> <87k1urz7jh.fsf@lassieur.org> <87k1uroru8.fsf@cbaines.net> From: =?UTF-8?Q?Cl=C3=A9ment?= Lassieur In-reply-to: <87k1uroru8.fsf@cbaines.net> Date: Mon, 05 Mar 2018 13:15:36 +0100 Message-ID: <87bmg2zpjr.fsf@lassieur.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Christopher Baines Cc: 30701@debbugs.gnu.org Christopher Baines writes: > Clément Lassieur writes: > >> Christopher Baines writes: >> >>> * gnu/services/databases.scm (postgresql-pid-file-for-version): New procedure. >>> (): 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 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 port locale config-file data-directory) >>> + (($ 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 :-)