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

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

[Christopher Baines]

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


I was mostly adding the system test, but also ended up reworking the
service so that Shepherd knows about the PID file.


Christopher Baines (4):
  services: Rework the PostgreSQL config file to use a record type.
  services: Use a external pid file for PostgreSQL.
  tests: databases: Add a system test for PostgreSQL.
  services: databases: Add postgresql-configuration record exports.

 gnu/services/databases.scm | 125 ++++++++++++++++++++++++++++++++++++---------
 gnu/tests/databases.scm    |  59 +++++++++++++++++++++
 2 files changed, 161 insertions(+), 23 deletions(-)

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

[bug#30701] [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type.

[Christopher Baines]

For the default config file representation. This makes it possible to more
easily change the configuration file, and have dynamic content. In particular,
I'm looking at adding a pid file location to the config file.

* gnu/services/databases.scm (<postgresql-config-file>): New record type.
  (%default-postgres-config): Remove this, it's been replaced by the
  configuration file.
  (<postgresql-configuration>): Alter the default for the config file field.
  (postgresql-service): Alter the default value for the config-file parameter.
---
 gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 20 deletions(-)

diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
index 3ca8f471f..9ffb6a5e9 100644
--- a/gnu/services/databases.scm
+++ b/gnu/services/databases.scm
@@ -26,11 +26,20 @@
   #:use-module (gnu system shadow)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages databases)
+  #:use-module (guix store)
   #:use-module (guix modules)
   #:use-module (guix records)
   #:use-module (guix gexp)
+  #:use-module (srfi srfi-1)
   #:use-module (ice-9 match)
-  #:export (postgresql-configuration
+  #:export (<postgresql-config-file>
+            postgresql-config-file
+            postgresql-config-file?
+            postgresql-config-file-log-destination
+            postgresql-config-file-hba-file
+            postgresql-config-file-ident-file
+            postgresql-config-file-extra-config
+
             postgresql-configuration?
             postgresql-service
             postgresql-service-type
@@ -68,6 +77,60 @@
 ;;;
 ;;; Code:
 
+(define %default-postgres-hba
+  (plain-file "pg_hba.conf"
+              "
+local	all	all			trust
+host	all	all	127.0.0.1/32 	trust
+host	all	all	::1/128 	trust"))
+
+(define %default-postgres-ident
+  (plain-file "pg_ident.conf"
+              "# MAPNAME       SYSTEM-USERNAME         PG-USERNAME"))
+
+(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 '())))
+
+(define-gexp-compiler (postgresql-config-file-compiler
+                       (file <postgresql-config-file>) system target)
+  (match file
+    (($ <postgresql-config-file> log-destination hba-file
+                                 ident-file extra-config)
+     (define (quote string)
+       (if string
+           (list "'" string "'")
+           (list)))
+
+     (define contents
+       (append-map
+        (match-lambda
+          ((key) (list))
+          ((key . #f) (list))
+          ((key values ...) `(,key " = " ,@values "\n")))
+
+        `(("log_destination" ,@(quote log-destination))
+          ("hba_file" ,@(quote hba-file))
+          ("ident_file" ,@(quote ident-file))
+          ,@extra-config)))
+
+     (gexp->derivation
+      "postgresql.conf"
+      #~(call-with-output-file (ungexp output "out")
+          (lambda (port)
+            (display
+             (string-append #$@contents)
+             port)))
+      #:local-build? #t))))
+
 (define-record-type* <postgresql-configuration>
   postgresql-configuration make-postgresql-configuration
   postgresql-configuration?
@@ -78,27 +141,10 @@
   (locale         postgresql-configuration-locale
                   (default "en_US.utf8"))
   (config-file    postgresql-configuration-file
-                  (default %default-postgres-config))
+                  (default (postgresql-config-file)))
   (data-directory postgresql-configuration-data-directory
                   (default "/var/lib/postgresql/data")))
 
-(define %default-postgres-hba
-  (plain-file "pg_hba.conf"
-              "
-local	all	all			trust
-host	all	all	127.0.0.1/32 	trust
-host	all	all	::1/128 	trust"))
-
-(define %default-postgres-ident
-  (plain-file "pg_ident.conf"
-             "# MAPNAME       SYSTEM-USERNAME         PG-USERNAME"))
-
-(define %default-postgres-config
-  (mixed-text-file "postgresql.conf"
-                   "log_destination = 'syslog'\n"
-                   "hba_file = '" %default-postgres-hba "'\n"
-                   "ident_file = '" %default-postgres-ident "'\n"))
-
 (define %postgresql-accounts
   (list (user-group (name "postgres") (system? #t))
         (user-account
@@ -192,7 +238,7 @@ host	all	all	::1/128 	trust"))
 (define* (postgresql-service #:key (postgresql postgresql)
                              (port 5432)
                              (locale "en_US.utf8")
-                             (config-file %default-postgres-config)
+                             (config-file (postgresql-config-file))
                              (data-directory "/var/lib/postgresql/data"))
   "Return a service that runs @var{postgresql}, the PostgreSQL database server.
 
-- 
2.16.0

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

[Christopher Baines]

* 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(-)

diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
index 9ffb6a5e9..4090277a7 100644
--- a/gnu/services/databases.scm
+++ b/gnu/services/databases.scm
@@ -26,10 +26,12 @@
   #:use-module (gnu system shadow)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages databases)
+  #:use-module (guix packages)
   #:use-module (guix store)
   #:use-module (guix modules)
   #:use-module (guix records)
   #:use-module (guix gexp)
+  #:use-module (guix utils)
   #:use-module (srfi srfi-1)
   #:use-module (ice-9 match)
   #:export (<postgresql-config-file>
@@ -38,6 +40,7 @@
             postgresql-config-file-log-destination
             postgresql-config-file-hba-file
             postgresql-config-file-ident-file
+            postgresql-config-file-external-pid-file
             postgresql-config-file-extra-config
 
             postgresql-configuration?
@@ -88,23 +91,32 @@ host	all	all	::1/128 	trust"))
   (plain-file "pg_ident.conf"
               "# MAPNAME       SYSTEM-USERNAME         PG-USERNAME"))
 
+(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))))
+  (extra-config      postgresql-config-file-extra-config
+                     (default '())))
 
 (define-gexp-compiler (postgresql-config-file-compiler
                        (file <postgresql-config-file>) system target)
   (match file
     (($ <postgresql-config-file> log-destination hba-file
-                                 ident-file extra-config)
+                                 ident-file external-pid-file
+                                 extra-config)
      (define (quote string)
        (if string
            (list "'" string "'")
@@ -120,6 +132,7 @@ host	all	all	::1/128 	trust"))
         `(("log_destination" ,@(quote log-destination))
           ("hba_file" ,@(quote hba-file))
           ("ident_file" ,@(quote ident-file))
+          ("external_pid_file" ,@(quote external-pid-file))
           ,@extra-config)))
 
      (gexp->derivation
@@ -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))))
   (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"))
 
 (define postgresql-activation
   (match-lambda
-    (($ <postgresql-configuration> postgresql port locale config-file data-directory)
+    (($ <postgresql-configuration> postgresql port locale pid-file
+                                   config-file data-directory)
      #~(begin
          (use-modules (guix build utils)
                       (ice-9 match))
@@ -173,6 +190,10 @@ host	all	all	::1/128 	trust"))
            (mkdir-p #$data-directory)
            (chown #$data-directory (passwd:uid user) (passwd:gid user))
 
+           ;; Create a directory for the pid file
+           (mkdir-p #$(dirname pid-file))
+           (chown #$(dirname pid-file) (passwd:uid user) (passwd:gid user))
+
            ;; Drop privileges and init state directory in a new
            ;; process.  Wait for it to finish before proceeding.
            (match (primitive-fork)
@@ -195,7 +216,8 @@ host	all	all	::1/128 	trust"))
 
 (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"))))))))
 
 (define postgresql-service-type
-- 
2.16.0

[bug#30701] [PATCH 3/4] tests: databases: Add a system test for PostgreSQL.

[Christopher Baines]

* gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables.
  (run-postgresql-test): New procedure.
---
 gnu/tests/databases.scm | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm
index e7097690a..0597105da 100644
--- a/gnu/tests/databases.scm
+++ b/gnu/tests/databases.scm
@@ -30,6 +30,7 @@
   #:use-module (guix store)
   #:export (%test-memcached
             %test-mongodb
+            %test-postgresql
             %test-mysql))
 
 (define %memcached-os
@@ -208,6 +209,64 @@
    (value (run-mongodb-test))))
 
 \f
+;;;
+;;; The PostgreSQL service.
+;;;
+
+(define %postgresql-os
+  (simple-operating-system
+   (service postgresql-service-type)))
+
+(define* (run-postgresql-test)
+  "Run tests in %POSTGRESQL-OS."
+  (define os
+    (marionette-operating-system
+     %postgresql-os
+     #:imported-modules '((gnu services herd)
+                          (guix combinators))))
+
+  (define vm
+    (virtual-machine
+     (operating-system os)
+     (memory-size 512)))
+
+  (define test
+    (with-imported-modules '((gnu build marionette))
+      #~(begin
+          (use-modules (srfi srfi-11) (srfi srfi-64)
+                       (gnu build marionette))
+
+          (define marionette
+            (make-marionette (list #$vm)))
+
+          (mkdir #$output)
+          (chdir #$output)
+
+          (test-begin "postgresql")
+
+          (test-assert "service running"
+            (marionette-eval
+             '(begin
+                (use-modules (gnu services herd))
+                (match (start-service 'postgres)
+                  (#f #f)
+                  (('service response-parts ...)
+                   (match (assq-ref response-parts 'running)
+                     ((pid) (number? pid))))))
+             marionette))
+
+          (test-end)
+          (exit (= (test-runner-fail-count (test-runner-current)) 0)))))
+
+  (gexp->derivation "postgresql-test" test))
+
+(define %test-postgresql
+  (system-test
+   (name "postgresql")
+   (description "Start the PostgreSQL service.")
+   (value (run-postgresql-test))))
+
+\f
 ;;;
 ;;; The MySQL service.
 ;;;
-- 
2.16.0

[bug#30701] [PATCH 4/4] services: databases: Add postgresql-configuration record exports.

[Christopher Baines]

* gnu/services/databases.scm: Export the record type, and all the field
  accessors.
---
 gnu/services/databases.scm | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
index 4090277a7..1e410cd39 100644
--- a/gnu/services/databases.scm
+++ b/gnu/services/databases.scm
@@ -43,7 +43,16 @@
             postgresql-config-file-external-pid-file
             postgresql-config-file-extra-config
 
+            <postgresql-configuration>
+            postgresql-configuration
             postgresql-configuration?
+            postgresql-configuration-postgresql
+            postgresql-configuration-port
+            postgresql-configuration-locale
+            postgresql-configuration-pid-file
+            postgresql-configuration-file
+            postgresql-configuration-data-directory
+
             postgresql-service
             postgresql-service-type
 
-- 
2.16.0

[bug#30701] [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type.

[Clément Lassieur]

Hi Christopher,

Christopher Baines <mail@cbaines.net> writes:

> For the default config file representation. This makes it possible to more
> easily change the configuration file, and have dynamic content. In particular,
> I'm looking at adding a pid file location to the config file.
>
> * gnu/services/databases.scm (<postgresql-config-file>): New record type.
>   (%default-postgres-config): Remove this, it's been replaced by the
>   configuration file.
>   (<postgresql-configuration>): Alter the default for the config file field.
>   (postgresql-service): Alter the default value for the config-file parameter.
> ---
>  gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 66 insertions(+), 20 deletions(-)

Thank you for this work!

> +(define-gexp-compiler (postgresql-config-file-compiler
> +                       (file <postgresql-config-file>) system target)
> +  (match file
> +    (($ <postgresql-config-file> log-destination hba-file
> +                                 ident-file extra-config)
> +     (define (quote string)
> +       (if string
> +           (list "'" string "'")
> +           (list)))

I don't think it's a good thing to hide one of the most important lisp
functions :-).

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

[Clément Lassieur]

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.

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

[bug#30701] [PATCH 3/4] tests: databases: Add a system test for PostgreSQL.

[Clément Lassieur]

Christopher Baines <mail@cbaines.net> writes:

> * gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables.
>   (run-postgresql-test): New procedure.
> ---
>  gnu/tests/databases.scm | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
...

> +          (test-assert "service running"
> +            (marionette-eval
> +             '(begin
> +                (use-modules (gnu services herd))
> +                (match (start-service 'postgres)
> +                  (#f #f)
> +                  (('service response-parts ...)
> +                   (match (assq-ref response-parts 'running)
> +                     ((pid) (number? pid))))))
> +             marionette))

I don't understand the point of the PID check here.  pg_ctl will ensure
that the daemon has started (by checking its PID), so I don't think
there is any need to redo its work.  I guess the PID you'll get here is
the one of pg_ctl, which is probably not what you want.

I believe that (start-service 'postgres) returning true means pg_ctl
succeeded in its check that the daemon is running.  So this is probably
enough:

          (test-assert "service running"
            (marionette-eval
             '(begin
                (use-modules (gnu services herd))
                (start-service 'postgres))
             marionette))

Clément

[bug#30701] [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type.

[Christopher Baines]

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


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

> Hi Christopher,
>
> Christopher Baines <mail@cbaines.net> writes:
>
>> For the default config file representation. This makes it possible to more
>> easily change the configuration file, and have dynamic content. In particular,
>> I'm looking at adding a pid file location to the config file.
>>
>> * gnu/services/databases.scm (<postgresql-config-file>): New record type.
>>   (%default-postgres-config): Remove this, it's been replaced by the
>>   configuration file.
>>   (<postgresql-configuration>): Alter the default for the config file field.
>>   (postgresql-service): Alter the default value for the config-file parameter.
>> ---
>>  gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 66 insertions(+), 20 deletions(-)
>
> Thank you for this work!

No problem, I've finally got around to going through some patches I've
had sitting around for a while.

>> +(define-gexp-compiler (postgresql-config-file-compiler
>> +                       (file <postgresql-config-file>) system target)
>> +  (match file
>> +    (($ <postgresql-config-file> log-destination hba-file
>> +                                 ident-file extra-config)
>> +     (define (quote string)
>> +       (if string
>> +           (list "'" string "'")
>> +           (list)))
>
> I don't think it's a good thing to hide one of the most important lisp
> functions :-).

I don't quite follow. I was trying to use '() rather than (list) if that
is what you mean, but I kept getting odd errors from Guile, so I gave
up, and ended up going with this.

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

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

[Christopher Baines]

[-- 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 --]

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

[Christopher Baines]

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


Christopher Baines <mail@cbaines.net> writes:

> Clément Lassieur <clement@lassieur.org> writes:
>
>> 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...

Ah, I've just realised why this is the case, I was misreading the system
test results, it does actually return #t/#f, but as the system test was
expecting a number, it just returns #f to indicate failure.

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

[bug#30701] [PATCH 3/4] tests: databases: Add a system test for PostgreSQL.

[Christopher Baines]

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


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

> Christopher Baines <mail@cbaines.net> writes:
>
>> * gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables.
>>   (run-postgresql-test): New procedure.
>> ---
>>  gnu/tests/databases.scm | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
> ...
>
>> +          (test-assert "service running"
>> +            (marionette-eval
>> +             '(begin
>> +                (use-modules (gnu services herd))
>> +                (match (start-service 'postgres)
>> +                  (#f #f)
>> +                  (('service response-parts ...)
>> +                   (match (assq-ref response-parts 'running)
>> +                     ((pid) (number? pid))))))
>> +             marionette))
>
> I don't understand the point of the PID check here.  pg_ctl will ensure
> that the daemon has started (by checking its PID), so I don't think
> there is any need to redo its work.  I guess the PID you'll get here is
> the one of pg_ctl, which is probably not what you want.

Because of make-forkexec-constructor, it is the main PID as set in the
external pid file created by PostgreSQL.

> I believe that (start-service 'postgres) returning true means pg_ctl
> succeeded in its check that the daemon is running.  So this is probably
> enough:
>
>           (test-assert "service running"
>             (marionette-eval
>              '(begin
>                 (use-modules (gnu services herd))
>                 (start-service 'postgres))
>              marionette))

Sure, I'm happy with this. I'll send some new patches soonish.

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

[bug#30701] [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type.

[Christopher Baines]

For the default config file representation. This makes it possible to more
easily change the configuration file, and have dynamic content. In particular,
I'm looking at adding a pid file location to the config file.

* gnu/services/databases.scm (<postgresql-config-file>): New record type.
  (%default-postgres-config): Remove this, it's been replaced by the
  configuration file.
  (<postgresql-configuration>): Alter the default for the config file field.
  (postgresql-service): Alter the default value for the config-file parameter.
---
 gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 20 deletions(-)

diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
index 3ca8f471f..9ffb6a5e9 100644
--- a/gnu/services/databases.scm
+++ b/gnu/services/databases.scm
@@ -26,11 +26,20 @@
   #:use-module (gnu system shadow)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages databases)
+  #:use-module (guix store)
   #:use-module (guix modules)
   #:use-module (guix records)
   #:use-module (guix gexp)
+  #:use-module (srfi srfi-1)
   #:use-module (ice-9 match)
-  #:export (postgresql-configuration
+  #:export (<postgresql-config-file>
+            postgresql-config-file
+            postgresql-config-file?
+            postgresql-config-file-log-destination
+            postgresql-config-file-hba-file
+            postgresql-config-file-ident-file
+            postgresql-config-file-extra-config
+
             postgresql-configuration?
             postgresql-service
             postgresql-service-type
@@ -68,6 +77,60 @@
 ;;;
 ;;; Code:
 
+(define %default-postgres-hba
+  (plain-file "pg_hba.conf"
+              "
+local	all	all			trust
+host	all	all	127.0.0.1/32 	trust
+host	all	all	::1/128 	trust"))
+
+(define %default-postgres-ident
+  (plain-file "pg_ident.conf"
+              "# MAPNAME       SYSTEM-USERNAME         PG-USERNAME"))
+
+(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 '())))
+
+(define-gexp-compiler (postgresql-config-file-compiler
+                       (file <postgresql-config-file>) system target)
+  (match file
+    (($ <postgresql-config-file> log-destination hba-file
+                                 ident-file extra-config)
+     (define (quote string)
+       (if string
+           (list "'" string "'")
+           (list)))
+
+     (define contents
+       (append-map
+        (match-lambda
+          ((key) (list))
+          ((key . #f) (list))
+          ((key values ...) `(,key " = " ,@values "\n")))
+
+        `(("log_destination" ,@(quote log-destination))
+          ("hba_file" ,@(quote hba-file))
+          ("ident_file" ,@(quote ident-file))
+          ,@extra-config)))
+
+     (gexp->derivation
+      "postgresql.conf"
+      #~(call-with-output-file (ungexp output "out")
+          (lambda (port)
+            (display
+             (string-append #$@contents)
+             port)))
+      #:local-build? #t))))
+
 (define-record-type* <postgresql-configuration>
   postgresql-configuration make-postgresql-configuration
   postgresql-configuration?
@@ -78,27 +141,10 @@
   (locale         postgresql-configuration-locale
                   (default "en_US.utf8"))
   (config-file    postgresql-configuration-file
-                  (default %default-postgres-config))
+                  (default (postgresql-config-file)))
   (data-directory postgresql-configuration-data-directory
                   (default "/var/lib/postgresql/data")))
 
-(define %default-postgres-hba
-  (plain-file "pg_hba.conf"
-              "
-local	all	all			trust
-host	all	all	127.0.0.1/32 	trust
-host	all	all	::1/128 	trust"))
-
-(define %default-postgres-ident
-  (plain-file "pg_ident.conf"
-             "# MAPNAME       SYSTEM-USERNAME         PG-USERNAME"))
-
-(define %default-postgres-config
-  (mixed-text-file "postgresql.conf"
-                   "log_destination = 'syslog'\n"
-                   "hba_file = '" %default-postgres-hba "'\n"
-                   "ident_file = '" %default-postgres-ident "'\n"))
-
 (define %postgresql-accounts
   (list (user-group (name "postgres") (system? #t))
         (user-account
@@ -192,7 +238,7 @@ host	all	all	::1/128 	trust"))
 (define* (postgresql-service #:key (postgresql postgresql)
                              (port 5432)
                              (locale "en_US.utf8")
-                             (config-file %default-postgres-config)
+                             (config-file (postgresql-config-file))
                              (data-directory "/var/lib/postgresql/data"))
   "Return a service that runs @var{postgresql}, the PostgreSQL database server.
 
-- 
2.16.0

[bug#30701] [PATCH 2/3] tests: databases: Add a system test for PostgreSQL.

[Christopher Baines]

* gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables.
  (run-postgresql-test): New procedure.
---
 gnu/tests/databases.scm | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm
index e7097690a..583f484d7 100644
--- a/gnu/tests/databases.scm
+++ b/gnu/tests/databases.scm
@@ -30,6 +30,7 @@
   #:use-module (guix store)
   #:export (%test-memcached
             %test-mongodb
+            %test-postgresql
             %test-mysql))
 
 (define %memcached-os
@@ -208,6 +209,60 @@
    (value (run-mongodb-test))))
 
 \f
+;;;
+;;; The PostgreSQL service.
+;;;
+
+(define %postgresql-os
+  (simple-operating-system
+   (service postgresql-service-type)))
+
+(define* (run-postgresql-test)
+  "Run tests in %POSTGRESQL-OS."
+  (define os
+    (marionette-operating-system
+     %postgresql-os
+     #:imported-modules '((gnu services herd)
+                          (guix combinators))))
+
+  (define vm
+    (virtual-machine
+     (operating-system os)
+     (memory-size 512)))
+
+  (define test
+    (with-imported-modules '((gnu build marionette))
+      #~(begin
+          (use-modules (srfi srfi-11) (srfi srfi-64)
+                       (gnu build marionette))
+
+          (define marionette
+            (make-marionette (list #$vm)))
+
+          (mkdir #$output)
+          (chdir #$output)
+
+          (test-begin "postgresql")
+
+          (test-assert "service running"
+            (marionette-eval
+             '(begin
+                (use-modules (gnu services herd))
+                (start-service 'postgres))
+             marionette))
+
+          (test-end)
+          (exit (= (test-runner-fail-count (test-runner-current)) 0)))))
+
+  (gexp->derivation "postgresql-test" test))
+
+(define %test-postgresql
+  (system-test
+   (name "postgresql")
+   (description "Start the PostgreSQL service.")
+   (value (run-postgresql-test))))
+
+\f
 ;;;
 ;;; The MySQL service.
 ;;;
-- 
2.16.0

[bug#30701] [PATCH 3/3] services: databases: Add postgresql-configuration record exports.

[Christopher Baines]

* gnu/services/databases.scm: Export the record type, and all the field
  accessors.
---
 gnu/services/databases.scm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
index 9ffb6a5e9..b05c0442f 100644
--- a/gnu/services/databases.scm
+++ b/gnu/services/databases.scm
@@ -40,7 +40,15 @@
             postgresql-config-file-ident-file
             postgresql-config-file-extra-config
 
+            <postgresql-configuration>
+            postgresql-configuration
             postgresql-configuration?
+            postgresql-configuration-postgresql
+            postgresql-configuration-port
+            postgresql-configuration-locale
+            postgresql-configuration-file
+            postgresql-configuration-data-directory
+
             postgresql-service
             postgresql-service-type
 
-- 
2.16.0

[bug#30701] [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type.

[Clément Lassieur]

Christopher Baines <mail@cbaines.net> writes:

> Clément Lassieur <clement@lassieur.org> writes:
>
>> Hi Christopher,
>>
>> Christopher Baines <mail@cbaines.net> writes:
>>
>>> For the default config file representation. This makes it possible to more
>>> easily change the configuration file, and have dynamic content. In particular,
>>> I'm looking at adding a pid file location to the config file.
>>>
>>> * gnu/services/databases.scm (<postgresql-config-file>): New record type.
>>>   (%default-postgres-config): Remove this, it's been replaced by the
>>>   configuration file.
>>>   (<postgresql-configuration>): Alter the default for the config file field.
>>>   (postgresql-service): Alter the default value for the config-file parameter.
>>> ---
>>>  gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++-----------
>>>  1 file changed, 66 insertions(+), 20 deletions(-)
>>
>> Thank you for this work!
>
> No problem, I've finally got around to going through some patches I've
> had sitting around for a while.
>
>>> +(define-gexp-compiler (postgresql-config-file-compiler
>>> +                       (file <postgresql-config-file>) system target)
>>> +  (match file
>>> +    (($ <postgresql-config-file> log-destination hba-file
>>> +                                 ident-file extra-config)
>>> +     (define (quote string)
>>> +       (if string
>>> +           (list "'" string "'")
>>> +           (list)))
>>
>> I don't think it's a good thing to hide one of the most important lisp
>> functions :-).
>
> I don't quite follow. I was trying to use '() rather than (list) if that
> is what you mean, but I kept getting odd errors from Guile, so I gave
> up, and ended up going with this.

Sorry for not being clear.  I meant that if you define 'quote', you
won't be able to use the Guile 'quote' anymore.  See
https://www.gnu.org/software/guile/manual/html_node/Expression-Syntax.html.

You could just rename it to 'quote-val' or something else.

[bug#30701] [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type.

[Clément Lassieur]

Christopher Baines <mail@cbaines.net> writes:

> Clément Lassieur <clement@lassieur.org> writes:
>
>> Hi Christopher,
>>
>> Christopher Baines <mail@cbaines.net> writes:
>>
>>> For the default config file representation. This makes it possible to more
>>> easily change the configuration file, and have dynamic content. In particular,
>>> I'm looking at adding a pid file location to the config file.
>>>
>>> * gnu/services/databases.scm (<postgresql-config-file>): New record type.
>>>   (%default-postgres-config): Remove this, it's been replaced by the
>>>   configuration file.
>>>   (<postgresql-configuration>): Alter the default for the config file field.
>>>   (postgresql-service): Alter the default value for the config-file parameter.
>>> ---
>>>  gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++-----------
>>>  1 file changed, 66 insertions(+), 20 deletions(-)
>>
>> Thank you for this work!
>
> No problem, I've finally got around to going through some patches I've
> had sitting around for a while.
>
>>> +(define-gexp-compiler (postgresql-config-file-compiler
>>> +                       (file <postgresql-config-file>) system target)
>>> +  (match file
>>> +    (($ <postgresql-config-file> log-destination hba-file
>>> +                                 ident-file extra-config)
>>> +     (define (quote string)
>>> +       (if string
>>> +           (list "'" string "'")
>>> +           (list)))
>>
>> I don't think it's a good thing to hide one of the most important lisp
>> functions :-).
>
> I don't quite follow. I was trying to use '() rather than (list) if that
> is what you mean, but I kept getting odd errors from Guile, so I gave
> up, and ended up going with this.

You couldn't use '() because '() is the same thing as (quote ()) and you
redefined 'quote'.

[bug#30701] [PATCH 2/3] tests: databases: Add a system test for PostgreSQL.

[Clément Lassieur]

Christopher Baines <mail@cbaines.net> writes:

> * gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables.
>   (run-postgresql-test): New procedure.
> ---
>  gnu/tests/databases.scm | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm
> index e7097690a..583f484d7 100644
> --- a/gnu/tests/databases.scm
> +++ b/gnu/tests/databases.scm
> @@ -30,6 +30,7 @@
>    #:use-module (guix store)
>    #:export (%test-memcached
>              %test-mongodb
> +            %test-postgresql
>              %test-mysql))
>  
>  (define %memcached-os
> @@ -208,6 +209,60 @@
>     (value (run-mongodb-test))))
>  
>  \f
> +;;;
> +;;; The PostgreSQL service.
> +;;;
> +
> +(define %postgresql-os
> +  (simple-operating-system
> +   (service postgresql-service-type)))
> +
> +(define* (run-postgresql-test)

define, instead of define*

> +  "Run tests in %POSTGRESQL-OS."
> +  (define os
> +    (marionette-operating-system
> +     %postgresql-os
> +     #:imported-modules '((gnu services herd)
> +                          (guix combinators))))
> +
> +  (define vm
> +    (virtual-machine
> +     (operating-system os)
> +     (memory-size 512)))
> +
> +  (define test
> +    (with-imported-modules '((gnu build marionette))
> +      #~(begin
> +          (use-modules (srfi srfi-11) (srfi srfi-64)

I think srfi-11 is useless.

> +                       (gnu build marionette))
> +
> +          (define marionette
> +            (make-marionette (list #$vm)))
> +
> +          (mkdir #$output)
> +          (chdir #$output)
> +
> +          (test-begin "postgresql")
> +
> +          (test-assert "service running"
> +            (marionette-eval
> +             '(begin
> +                (use-modules (gnu services herd))
> +                (start-service 'postgres))
> +             marionette))
> +
> +          (test-end)
> +          (exit (= (test-runner-fail-count (test-runner-current)) 0)))))
> +
> +  (gexp->derivation "postgresql-test" test))
> +
> +(define %test-postgresql
> +  (system-test
> +   (name "postgresql")
> +   (description "Start the PostgreSQL service.")
> +   (value (run-postgresql-test))))
> +
> +\f
>  ;;;
>  ;;; The MySQL service.
>  ;;;

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

[Clément Lassieur]

Christopher Baines <mail@cbaines.net> writes:

> 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 :-)

[bug#30701] [PATCH 4/4] services: databases: Add postgresql-configuration record exports.

[Clément Lassieur]

Christopher Baines <mail@cbaines.net> writes:

> * gnu/services/databases.scm: Export the record type, and all the field
>   accessors.
  ^
Small nit-pick: there is no indentation here :-).  You can check "git
log" to see how other people do.

> ---
>  gnu/services/databases.scm | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
> index 4090277a7..1e410cd39 100644
> --- a/gnu/services/databases.scm
> +++ b/gnu/services/databases.scm
> @@ -43,7 +43,16 @@
>              postgresql-config-file-external-pid-file
>              postgresql-config-file-extra-config
>  
> +            <postgresql-configuration>
> +            postgresql-configuration
>              postgresql-configuration?
> +            postgresql-configuration-postgresql
> +            postgresql-configuration-port
> +            postgresql-configuration-locale
> +            postgresql-configuration-pid-file
> +            postgresql-configuration-file
> +            postgresql-configuration-data-directory
> +
>              postgresql-service
>              postgresql-service-type

[bug#30701] [PATCH 2/3] tests: databases: Add a system test for PostgreSQL.

[Christopher Baines]

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


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

> Christopher Baines <mail@cbaines.net> writes:
>
>> * gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables.
>>   (run-postgresql-test): New procedure.
>> ---
>>  gnu/tests/databases.scm | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>
>> diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm
>> index e7097690a..583f484d7 100644
>> --- a/gnu/tests/databases.scm
>> +++ b/gnu/tests/databases.scm
>> @@ -30,6 +30,7 @@
>>    #:use-module (guix store)
>>    #:export (%test-memcached
>>              %test-mongodb
>> +            %test-postgresql
>>              %test-mysql))
>>  
>>  (define %memcached-os
>> @@ -208,6 +209,60 @@
>>     (value (run-mongodb-test))))
>>  
>>  \f
>> +;;;
>> +;;; The PostgreSQL service.
>> +;;;
>> +
>> +(define %postgresql-os
>> +  (simple-operating-system
>> +   (service postgresql-service-type)))
>> +
>> +(define* (run-postgresql-test)
>
> define, instead of define*
>
>> +  "Run tests in %POSTGRESQL-OS."
>> +  (define os
>> +    (marionette-operating-system
>> +     %postgresql-os
>> +     #:imported-modules '((gnu services herd)
>> +                          (guix combinators))))
>> +
>> +  (define vm
>> +    (virtual-machine
>> +     (operating-system os)
>> +     (memory-size 512)))
>> +
>> +  (define test
>> +    (with-imported-modules '((gnu build marionette))
>> +      #~(begin
>> +          (use-modules (srfi srfi-11) (srfi srfi-64)
>
> I think srfi-11 is useless.

Good spot, I'll make these changes and send some new patches.

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

[bug#30701] [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type.

[Christopher Baines]

For the default config file representation. This makes it possible to more
easily change the configuration file, and have dynamic content. In particular,
I'm looking at adding a pid file location to the config file.

* gnu/services/databases.scm (<postgresql-config-file>): New record type.
(%default-postgres-config): Remove this, it's been replaced by the
configuration file.
(<postgresql-configuration>): Alter the default for the config file field.
(postgresql-service): Alter the default value for the config-file parameter.
---
 gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 20 deletions(-)

diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
index 3ca8f471f..f7d5fffd0 100644
--- a/gnu/services/databases.scm
+++ b/gnu/services/databases.scm
@@ -26,11 +26,20 @@
   #:use-module (gnu system shadow)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages databases)
+  #:use-module (guix store)
   #:use-module (guix modules)
   #:use-module (guix records)
   #:use-module (guix gexp)
+  #:use-module (srfi srfi-1)
   #:use-module (ice-9 match)
-  #:export (postgresql-configuration
+  #:export (<postgresql-config-file>
+            postgresql-config-file
+            postgresql-config-file?
+            postgresql-config-file-log-destination
+            postgresql-config-file-hba-file
+            postgresql-config-file-ident-file
+            postgresql-config-file-extra-config
+
             postgresql-configuration?
             postgresql-service
             postgresql-service-type
@@ -68,6 +77,60 @@
 ;;;
 ;;; Code:
 
+(define %default-postgres-hba
+  (plain-file "pg_hba.conf"
+              "
+local	all	all			trust
+host	all	all	127.0.0.1/32 	trust
+host	all	all	::1/128 	trust"))
+
+(define %default-postgres-ident
+  (plain-file "pg_ident.conf"
+              "# MAPNAME       SYSTEM-USERNAME         PG-USERNAME"))
+
+(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 '())))
+
+(define-gexp-compiler (postgresql-config-file-compiler
+                       (file <postgresql-config-file>) system target)
+  (match file
+    (($ <postgresql-config-file> log-destination hba-file
+                                 ident-file extra-config)
+     (define (with-single-quotes string)
+       (if string
+           (list "'" string "'")
+           '()))
+
+     (define contents
+       (append-map
+        (match-lambda
+          ((key) '())
+          ((key . #f) '())
+          ((key values ...) `(,key " = " ,@values "\n")))
+
+        `(("log_destination" ,@(with-single-quotes log-destination))
+          ("hba_file" ,@(with-single-quotes hba-file))
+          ("ident_file" ,@(with-single-quotes ident-file))
+          ,@extra-config)))
+
+     (gexp->derivation
+      "postgresql.conf"
+      #~(call-with-output-file (ungexp output "out")
+          (lambda (port)
+            (display
+             (string-append #$@contents)
+             port)))
+      #:local-build? #t))))
+
 (define-record-type* <postgresql-configuration>
   postgresql-configuration make-postgresql-configuration
   postgresql-configuration?
@@ -78,27 +141,10 @@
   (locale         postgresql-configuration-locale
                   (default "en_US.utf8"))
   (config-file    postgresql-configuration-file
-                  (default %default-postgres-config))
+                  (default (postgresql-config-file)))
   (data-directory postgresql-configuration-data-directory
                   (default "/var/lib/postgresql/data")))
 
-(define %default-postgres-hba
-  (plain-file "pg_hba.conf"
-              "
-local	all	all			trust
-host	all	all	127.0.0.1/32 	trust
-host	all	all	::1/128 	trust"))
-
-(define %default-postgres-ident
-  (plain-file "pg_ident.conf"
-             "# MAPNAME       SYSTEM-USERNAME         PG-USERNAME"))
-
-(define %default-postgres-config
-  (mixed-text-file "postgresql.conf"
-                   "log_destination = 'syslog'\n"
-                   "hba_file = '" %default-postgres-hba "'\n"
-                   "ident_file = '" %default-postgres-ident "'\n"))
-
 (define %postgresql-accounts
   (list (user-group (name "postgres") (system? #t))
         (user-account
@@ -192,7 +238,7 @@ host	all	all	::1/128 	trust"))
 (define* (postgresql-service #:key (postgresql postgresql)
                              (port 5432)
                              (locale "en_US.utf8")
-                             (config-file %default-postgres-config)
+                             (config-file (postgresql-config-file))
                              (data-directory "/var/lib/postgresql/data"))
   "Return a service that runs @var{postgresql}, the PostgreSQL database server.
 
-- 
2.16.0

[bug#30701] [PATCH 2/3] tests: databases: Add a system test for PostgreSQL.

[Christopher Baines]

* gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables.
(run-postgresql-test): New procedure.
---
 gnu/tests/databases.scm | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm
index e7097690a..5c8ca85c1 100644
--- a/gnu/tests/databases.scm
+++ b/gnu/tests/databases.scm
@@ -30,6 +30,7 @@
   #:use-module (guix store)
   #:export (%test-memcached
             %test-mongodb
+            %test-postgresql
             %test-mysql))
 
 (define %memcached-os
@@ -208,6 +209,60 @@
    (value (run-mongodb-test))))
 
 \f
+;;;
+;;; The PostgreSQL service.
+;;;
+
+(define %postgresql-os
+  (simple-operating-system
+   (service postgresql-service-type)))
+
+(define (run-postgresql-test)
+  "Run tests in %POSTGRESQL-OS."
+  (define os
+    (marionette-operating-system
+     %postgresql-os
+     #:imported-modules '((gnu services herd)
+                          (guix combinators))))
+
+  (define vm
+    (virtual-machine
+     (operating-system os)
+     (memory-size 512)))
+
+  (define test
+    (with-imported-modules '((gnu build marionette))
+      #~(begin
+          (use-modules (srfi srfi-64)
+                       (gnu build marionette))
+
+          (define marionette
+            (make-marionette (list #$vm)))
+
+          (mkdir #$output)
+          (chdir #$output)
+
+          (test-begin "postgresql")
+
+          (test-assert "service running"
+            (marionette-eval
+             '(begin
+                (use-modules (gnu services herd))
+                (start-service 'postgres))
+             marionette))
+
+          (test-end)
+          (exit (= (test-runner-fail-count (test-runner-current)) 0)))))
+
+  (gexp->derivation "postgresql-test" test))
+
+(define %test-postgresql
+  (system-test
+   (name "postgresql")
+   (description "Start the PostgreSQL service.")
+   (value (run-postgresql-test))))
+
+\f
 ;;;
 ;;; The MySQL service.
 ;;;
-- 
2.16.0

[bug#30701] [PATCH 3/3] services: databases: Add postgresql-configuration record exports.

[Christopher Baines]

* gnu/services/databases.scm: Export the record type, and all the field
accessors.
---
 gnu/services/databases.scm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
index f7d5fffd0..4c9a50a5a 100644
--- a/gnu/services/databases.scm
+++ b/gnu/services/databases.scm
@@ -40,7 +40,15 @@
             postgresql-config-file-ident-file
             postgresql-config-file-extra-config
 
+            <postgresql-configuration>
+            postgresql-configuration
             postgresql-configuration?
+            postgresql-configuration-postgresql
+            postgresql-configuration-port
+            postgresql-configuration-locale
+            postgresql-configuration-file
+            postgresql-configuration-data-directory
+
             postgresql-service
             postgresql-service-type
 
-- 
2.16.0

[bug#30701] [PATCH 4/4] services: databases: Add postgresql-configuration record exports.

[Christopher Baines]

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


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

> Christopher Baines <mail@cbaines.net> writes:
>
>> * gnu/services/databases.scm: Export the record type, and all the field
>>   accessors.
>   ^
> Small nit-pick: there is no indentation here :-).  You can check "git
> log" to see how other people do.

I've updated the git commit messages, and sent some updated
patches. Thanks for taking a look :)

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

[bug#30701] [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type.

[Clément Lassieur]

Christopher Baines <mail@cbaines.net> writes:

> For the default config file representation. This makes it possible to more
> easily change the configuration file, and have dynamic content. In particular,
> I'm looking at adding a pid file location to the config file.

                             ^
Could you please remove this last sentence (with the pid file)?

> * gnu/services/databases.scm (<postgresql-config-file>): New record type.
> (%default-postgres-config): Remove this, it's been replaced by the
> configuration file.
> (<postgresql-configuration>): Alter the default for the config file field.
> (postgresql-service): Alter the default value for the config-file parameter.
> ---
>  gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 66 insertions(+), 20 deletions(-)
>
> diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
> index 3ca8f471f..f7d5fffd0 100644
> --- a/gnu/services/databases.scm
> +++ b/gnu/services/databases.scm
> @@ -26,11 +26,20 @@
>    #:use-module (gnu system shadow)
>    #:use-module (gnu packages admin)
>    #:use-module (gnu packages databases)
> +  #:use-module (guix store)

I don't think (guix store) is used.  Is it?

>    #:use-module (guix modules)
>    #:use-module (guix records)
>    #:use-module (guix gexp)

...

> +        `(("log_destination" ,@(with-single-quotes log-destination))
> +          ("hba_file" ,@(with-single-quotes hba-file))
> +          ("ident_file" ,@(with-single-quotes ident-file))
                                    ^
Could you please use a shorter name?  Like "enclose", so that we won't
go over 80 columns too easily :-).

Apart from those small things, the three patches LGTM.  Thank you
again!

Clément

bug#30701: [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type.

[Christopher Baines]

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


Sorry, I made some changes, and merged these patches on the weekend, but
I forgot to reply.

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

> Christopher Baines <mail@cbaines.net> writes:
>
>> For the default config file representation. This makes it possible to more
>> easily change the configuration file, and have dynamic content. In particular,
>> I'm looking at adding a pid file location to the config file.
>
>                              ^
> Could you please remove this last sentence (with the pid file)?

Done.

>> * gnu/services/databases.scm (<postgresql-config-file>): New record type.
>> (%default-postgres-config): Remove this, it's been replaced by the
>> configuration file.
>> (<postgresql-configuration>): Alter the default for the config file field.
>> (postgresql-service): Alter the default value for the config-file parameter.
>> ---
>>  gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 66 insertions(+), 20 deletions(-)
>>
>> diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
>> index 3ca8f471f..f7d5fffd0 100644
>> --- a/gnu/services/databases.scm
>> +++ b/gnu/services/databases.scm
>> @@ -26,11 +26,20 @@
>>    #:use-module (gnu system shadow)
>>    #:use-module (gnu packages admin)
>>    #:use-module (gnu packages databases)
>> +  #:use-module (guix store)
>
> I don't think (guix store) is used.  Is it?

It wasn't, I've removed it.

>>    #:use-module (guix modules)
>>    #:use-module (guix records)
>>    #:use-module (guix gexp)
>
> ...
>
>> +        `(("log_destination" ,@(with-single-quotes log-destination))
>> +          ("hba_file" ,@(with-single-quotes hba-file))
>> +          ("ident_file" ,@(with-single-quotes ident-file))
>                                     ^
> Could you please use a shorter name?  Like "enclose", so that we won't
> go over 80 columns too easily :-).

I went with quote' as I think that works well.

> Apart from those small things, the three patches LGTM.  Thank you
> again!

Thanks for taking a look and for your comments :)

Chris

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

[bug#30701] [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type.

[Clément Lassieur]

Christopher Baines <mail@cbaines.net> writes:

> Sorry, I made some changes, and merged these patches on the weekend, but
> I forgot to reply.

[...]

>>> +        `(("log_destination" ,@(with-single-quotes log-destination))
>>> +          ("hba_file" ,@(with-single-quotes hba-file))
>>> +          ("ident_file" ,@(with-single-quotes ident-file))
>>                                     ^
>> Could you please use a shorter name?  Like "enclose", so that we won't
>> go over 80 columns too easily :-).
>
> I went with quote' as I think that works well.

I don't like it because:

  • The extra \' doesn't help describing what the function does.  One
    could believe it's a variant of 'quote', but it's actually very
    different.

  • It doesn't follow our coding style.  See
    https://mumble.net/~campbell/scheme/style.txt.  "Symbolic names are
    written with English words separated by hyphens."  See also the part
    about "Funny Characters".

> Thanks for taking a look and for your comments :)

You're welcome :-)

Clément

[bug#30701] [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type.

[Christopher Baines]

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


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

> Christopher Baines <mail@cbaines.net> writes:
>
>> Sorry, I made some changes, and merged these patches on the weekend, but
>> I forgot to reply.
>
> [...]
>
>>>> +        `(("log_destination" ,@(with-single-quotes log-destination))
>>>> +          ("hba_file" ,@(with-single-quotes hba-file))
>>>> +          ("ident_file" ,@(with-single-quotes ident-file))
>>>                                     ^
>>> Could you please use a shorter name?  Like "enclose", so that we won't
>>> go over 80 columns too easily :-).
>>
>> I went with quote' as I think that works well.
>
> I don't like it because:
>
>   • The extra \' doesn't help describing what the function does.  One
>     could believe it's a variant of 'quote', but it's actually very
>     different.
>
>   • It doesn't follow our coding style.  See
>     https://mumble.net/~campbell/scheme/style.txt.  "Symbolic names are
>     written with English words separated by hyphens."  See also the part
>     about "Funny Characters".

Fair enough, I've changed it to single-quote in
533808383f7fca6563aee1452f5202e0cd1b66b8.

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

[bug#30701] [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type.

[Clément Lassieur]

Christopher Baines <mail@cbaines.net> writes:

> Clément Lassieur <clement@lassieur.org> writes:
>
>> Christopher Baines <mail@cbaines.net> writes:
>>
>>> Sorry, I made some changes, and merged these patches on the weekend, but
>>> I forgot to reply.
>>
>> [...]
>>
>>>>> +        `(("log_destination" ,@(with-single-quotes log-destination))
>>>>> +          ("hba_file" ,@(with-single-quotes hba-file))
>>>>> +          ("ident_file" ,@(with-single-quotes ident-file))
>>>>                                     ^
>>>> Could you please use a shorter name?  Like "enclose", so that we won't
>>>> go over 80 columns too easily :-).
>>>
>>> I went with quote' as I think that works well.
>>
>> I don't like it because:
>>
>>   • The extra \' doesn't help describing what the function does.  One
>>     could believe it's a variant of 'quote', but it's actually very
>>     different.
>>
>>   • It doesn't follow our coding style.  See
>>     https://mumble.net/~campbell/scheme/style.txt.  "Symbolic names are
>>     written with English words separated by hyphens."  See also the part
>>     about "Funny Characters".
>
> Fair enough, I've changed it to single-quote in
> 533808383f7fca6563aee1452f5202e0cd1b66b8.

Thank you Christopher!