[PATCH] Add some properties to install and configure Postgresql

[Sean Whitton]

Hello David,

On Sat 18 Dec 2021 at 11:27PM -04, David Bremner wrote:

> In order to do common tasks like adding users in an idempotent way requires
> some non-obvious incantations, so it is worth providing properties for these
> tasks.

Nice, sounds useful.  I should note I haven't used postgres for ages, so
relying on you to test etc.

> diff --git a/src/property/postgres.lisp b/src/property/postgres.lisp
> new file mode 100644
> index 0000000..ed9fe04
> --- /dev/null
> +++ b/src/property/postgres.lisp
> +(named-readtables:in-readtable :interpol-syntax)

You don't need this, as the :consfigurator named readtable includes it.

> +
> +;; Currently all of properties in this package require Debian because they use
> +;; `runuser' and because of the current implementation of the following
> +;; function.
> +(defproplist installed :posix ()
> +  "Ensure that postgresql and associated utilities are installed."
> +  (:desc "postgresql and associated utilities installed")
> +  (os:etypecase
> +    (debianlike (apt:installed "postgresql" "postgresql-client"))))

I think that comment should be a three-semicolon file-level comment, and
I'd be inclined to drop remarking on INSTALLED -- we have lots of
INSTALLED properties like that, and the use of OS:ETYPECASE is fairly
self-documenting, I think?

However:

> +(defun superuser ()
> +  "Return Postgres superuser."
> +  (or (get-hostattrs-car 'postgres-superuser) "postgres"))
> +
> +(defprop %run-sql :posix (sql)
> +  "Run given sql as the Postgres superuser against the `postgres' database."
> +  (:apply
> +   (mrun "runuser" "-u" (superuser) "--" "psql" "postgres" :input sql)))

How about we do this with AS such that you can drop all the OS:REQUIRED?

(defproplist %run-sql :posix (sql)
  (as (or (get-hostattrs-car 'postgres-superuser) "postgres")
    (cmd:single :input sql "psql" "postgres")))

This also avoids one level of shelling out in the case of a
Lisp-connection, as Consfigurator will setuid directly.

> +(defproplist has-database :posix (db-name &key (owner nil))

Keyword parameter default values default to nil, so please drop the
sublist.

> +  "Ensure Postgres DATABASE exists, optionally with owner OWNER"
> +  (:desc
> +   (strcat #?"Postgres database ${db-name} exists"
> +           (if owner #?" with owner ${owner}" "")))
> +  (:hostattrs
> +   (os:required 'os:debianlike))
> +  (installed)
> +  ;; this has a potential race condition between test and creation
> +  (%run-sql
> +   (strcat
> +    #?"SELECT 'CREATE DATABASE ${db-name}"
> +    (if owner #?" WITH OWNER ${owner}" "")
> +    "' WHERE NOT EXISTS "
> +    #?"(SELECT FROM pg_database WHERE datname = '${db-name}')\\gexec")))

If I later supply an OWNER parameter, will the database ownership be
updated?  If not, could you extend the SQL to do that?  Makes the
property a lot easier to use, I think.

-- 
Sean Whitton