Subject: systemd service files vs. postgresql9x-setup

From: Bernd Helmle <[email-masked]>
Date: Wed, 12 Aug 2015 16:18:06 +0200

Hi,

Looks like postgresql9[45]-setup is careless when someone tries to use them
in customized
systemd environments (e.g. via drop-in configurations[1]). Currently we do
this:

# this parsing technique fails for PGDATA pathnames containing spaces,
# but there's not much I can do about it given systemctl's output format...
PGDATA=`systemctl show -p Environment "${SERVICE_NAME}.service" |
                sed 's/^Environment=//' | tr ' ' '\n' |
                sed -n 's/^PGDATA=//p' | tail -n 1`

[...some more code later...]

# Get data directory from the service file
PGDATA=`sed -n 's/Environment=PGDATA=//p' "${SERVICE_FILE}"`

So we obviously overwrite any PGDATA setting returned by 'systemctl show'
earlier. If someone uses service files only, this doesn't heavily matter,
since SERVICE_FILE is tested against multiple locations. However, drop-in
configurations are ignored/overwritten with this method. I don't
understand, why we do the sed approach anyways, since 'systemctl show'
already covers all cases, afaics. So i suggest to get rid of handling the
SERVICE_FILE directly and leave 'systemctl show' alone, patch attached.

Opinions?

[1] <http://www.freedesktop.org/software/systemd/man/systemd.unit.html>

-- 
Thanks

	Bernd

-- 
Sent via pgsql-pkg-yum mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-pkg-yum

---

From: Jeff Frost <[email-masked]>
Date: Wed, 12 Aug 2015 16:54:41 -0700

> 
> On Aug 12, 2015, at 7:18 AM, Bernd Helmle <[email-masked]> wrote:
> 
> Hi,
> 

Unfortunately, I’m not that familiar with systemd yet and Devrim is out for a few weeks, so we might not get to this very quickly.

---

From: Bernd Helmle <[email-masked]>
Date: Fri, 14 Aug 2015 10:28:13 +0200

--On 12. August 2015 16:54:41 -0700 Jeff Frost <[email-masked]> wrote:

> Unfortunately, I’m not that familiar with systemd yet and Devrim is out
> for a few weeks, so we might not get to this very quickly.

No worries.

-- 
Thanks

	Bernd

---

From: Bernd Helmle <[email-masked]>
Date: Thu, 17 Sep 2015 14:49:17 +0200

> for a few weeks, so we might not get to this very quickly.

So i'd like to step in again into this patch.

We're working with a RHEL7 environment where those drop-in configurations
are heavily used to move PGDATA to different places and i'd like to
preserve that behavior.

Recap:

postgresql9[45]-setup happily overwrites any PGDATA setting derived from
drop-in configurations by examining the service file directly and ignoring
the previous retrieved setting from systemctl.

So this looks worth to fix for any setups, even when other ways to setup
PGDATA are used.

-- 
Thanks

	Bernd

---

From: Devrim GÜNDÜZ <[email-masked]>
Date: Fri, 18 Sep 2015 16:48:09 +0300

Hi Bernd,

First of all: Thanks for the patch!

The next set of new minor releases are due in a few weeks. I think we
can apply this patch then.

(We need a issue tracker!)

Regards, Devrim
On Thu, 2015-09-17 at 14:49 +0200, Bernd Helmle wrote:
> > > [1] <
> > > http://www.freedesktop.org/software/systemd/man/systemd.unit.html
> > > >
> > 

-- 
Devrim GÜNDÜZ
Principal Systems Engineer @ EnterpriseDB: http://www.enterprisedb.com
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

---

From: Bernd Helmle <[email-masked]>
Date: Fri, 11 Mar 2016 13:01:54 +0100

> (We need a issue tracker!)

Hmm, i've totally forgotten that. I see that it's not committed yet, so i
just go forward and ask if there's something missing we need to address? 

-- 
Thanks

	Bernd

---

From: Strahinja Kustudić <[email-masked]>
Date: Wed, 18 May 2016 17:06:30 +0200

I propose the same patch with a modification to work with spaces in the
path, since systemctl prints spaces as '\x20', so we just needed one more
sed to replace that with a space.

Would also like to see this fixed in the next minor release, since it makes
it really hard to change PGDATA the correct way, which is by droping a
.conf file into /etc/systemd/system/postgresql-9.x.service.d/ directory.

Also would be really cool if the postgres rpm created the
/etc/systemd/system/postgresql-9.x.service.d/ directory, but it's not that
important as the setup scripts working correctly.

Strahinja Kustudić | Lead System Engineer | Nordeus

> just go forward and ask if there's something missing we need to address?
diff --git a/rpm/redhat/9.4/postgresql/EL-7/postgresql94-setup b/rpm/redhat/9.4/postgresql/EL-7/postgresql94-setup
index 1f0cb0f..44cc090 100644
--- a/rpm/redhat/9.4/postgresql/EL-7/postgresql94-setup
+++ b/rpm/redhat/9.4/postgresql/EL-7/postgresql94-setup
@@ -73,36 +73,20 @@ case "$1" in
         ;;
 esac
 
-# this parsing technique fails for PGDATA pathnames containing spaces,
-# but there's not much I can do about it given systemctl's output format...
 PGDATA=`systemctl show -p Environment "${SERVICE_NAME}.service" |
                 sed 's/^Environment=//' | tr ' ' '\n' |
-                sed -n 's/^PGDATA=//p' | tail -n 1`
+                sed -n 's/^PGDATA=//p' | tail -n 1 |
+		sed 's/\\x20/ /g'` # replace '\x20' with space
 if [ x"$PGDATA" = x ]; then
     echo "failed to find PGDATA setting in ${SERVICE_NAME}.service"
     exit 1
 fi
 
-# Find the unit file for new version.
-if [ -f "/etc/systemd/system/${SERVICE_NAME}.service" ]
-then
-    SERVICE_FILE="/etc/systemd/system/${SERVICE_NAME}.service"
-elif [ -f "/lib/systemd/system/${SERVICE_NAME}.service" ]
-then
-    SERVICE_FILE="/lib/systemd/system/${SERVICE_NAME}.service"
-else
-    echo "Could not find systemd unit file ${SERVICE_NAME}.service"
-    exit 1
-fi
-
 # Log file for pg_upgrade
 PGUPLOG=/var/lib/pgsql/$PGMAJORVERSION/pgupgrade.log
 # Log file for initdb
 PGLOG=/var/lib/pgsql/9.4/initdb.log
 
-# Get data directory from the service file
-PGDATA=`sed -n 's/Environment=PGDATA=//p' "${SERVICE_FILE}"`
-
 export PGDATA
 
 # For SELinux we need to use 'runuser' not 'su'
diff --git a/rpm/redhat/9.5/postgresql/EL-7/postgresql95-setup b/rpm/redhat/9.5/postgresql/EL-7/postgresql95-setup
index 697e0b4..c68a1a3 100644
--- a/rpm/redhat/9.5/postgresql/EL-7/postgresql95-setup
+++ b/rpm/redhat/9.5/postgresql/EL-7/postgresql95-setup
@@ -73,36 +73,20 @@ case "$1" in
         ;;
 esac
 
-# this parsing technique fails for PGDATA pathnames containing spaces,
-# but there's not much I can do about it given systemctl's output format...
 PGDATA=`systemctl show -p Environment "${SERVICE_NAME}.service" |
                 sed 's/^Environment=//' | tr ' ' '\n' |
-                sed -n 's/^PGDATA=//p' | tail -n 1`
+                sed -n 's/^PGDATA=//p' | tail -n 1 |
+		sed 's/\\x20/ /g'` # replace '\x20' with space
 if [ x"$PGDATA" = x ]; then
     echo "failed to find PGDATA setting in ${SERVICE_NAME}.service"
     exit 1
 fi
 
-# Find the unit file for new version.
-if [ -f "/etc/systemd/system/${SERVICE_NAME}.service" ]
-then
-    SERVICE_FILE="/etc/systemd/system/${SERVICE_NAME}.service"
-elif [ -f "/lib/systemd/system/${SERVICE_NAME}.service" ]
-then
-    SERVICE_FILE="/lib/systemd/system/${SERVICE_NAME}.service"
-else
-    echo "Could not find systemd unit file ${SERVICE_NAME}.service"
-    exit 1
-fi
-
 # Log file for pg_upgrade
 PGUPLOG=/var/lib/pgsql/$PGMAJORVERSION/pgupgrade.log
 # Log file for initdb
 PGLOG=/var/lib/pgsql/9.5/initdb.log
 
-# Get data directory from the service file
-PGDATA=`sed -n 's/Environment=PGDATA=//p' "${SERVICE_FILE}"`
-
 export PGDATA
 
 # For SELinux we need to use 'runuser' not 'su'