Message-ID: <1481308873.16175.24.camel@gunduz.org>
Subject: Re: a bunch of potential improvements to the
 postgresql spec files
From: Devrim =?ISO-8859-1?Q?G=FCnd=FCz?= <devrim@gunduz.org>
To: Jonathon Nelson <jdnelson@dyn.com>, pgsql-pkg-yum@postgresql.org
Date: Fri, 09 Dec 2016 21:41:13 +0300
In-Reply-To: 
 <CACJqAM3ZeLM8qqHJxF0wTOtDGEJDSb2DOmOF+RLNH6BNQ6j=Jw@mail.gmail.com>
References: 
	<CACJqAM3ZeLM8qqHJxF0wTOtDGEJDSb2DOmOF+RLNH6BNQ6j=Jw@mail.gmail.com>
Content-Type: multipart/signed; micalg="pgp-sha256";
	protocol="application/pgp-signature"; boundary="=-7JXLKHQ7gseSSAmaDLgS"
Mime-Version: 1.0
Precedence: bulk
Sender: pgsql-pkg-yum-owner@postgresql.org


--=-7JXLKHQ7gseSSAmaDLgS
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable


Hi Jonathon,

First of all, thank your for working on this. Comments inline:

On Fri, 2016-12-02 at 14:06 -0600, Jonathon Nelson wrote:
>=20
> 0. globally replace /usr/sbin with %{_sbindir}, /usr/bin with %{_bindir},
> and /usr/share/man with %{_mandir}

Looks good.

> 1. Remove the redundant use of 0%{?rhel} in tests for RHEL-specific value=
s.
> Turns:
> %if 0%{?rhel} && 0%{rhel} > 6
> into:
> %if 0%{rhel} > 6

If we remove that one, then Fedora builds should fail.

> 2. add min version to flex dependency:
> BuildRequires: flex >=3D 2.5.31

Ok.


> 3. move redundantly specified perl(ExtUtils::MakeMaker) outside of rhel
> version tests

AFAIR, we don't need this on RHEL 7 and Fedora. Are you sure that this is s=
afe
to move?

> 4. Remove comma (it's not an error, it's just not as aesthetic) from
> Requires line(s):
>=20
> -Requires(pre):=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/useradd, %{_sbin=
dir}/groupadd
> +Requires(pre):=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/useradd %{_sbind=
ir}/groupadd

Ok.

> 5. Remove redundant strip of -ffast-math from CFLAGS

Wow, ok.

> 6. replace hard-coded path for PGENGINE variable with %{pgbaseinstdir}:
> =C2=A0# prep the setup script, including insertion of some values it need=
s
> =C2=A0sed -e 's|^PGVERSION=3D.*$|PGVERSION=3D%{version}|' \
> -=C2=A0=C2=A0=C2=A0-e 's|^PGENGINE=3D.*$|PGENGINE=3D/usr/pgsql-%{majorver=
sion}/bin|' \
> +=C2=A0=C2=A0=C2=A0-e 's|^PGENGINE=3D.*$|PGENGINE=3D%{pgbaseinstdir}/bin|=
' \
> =C2=A0=C2=A0=C2=A0=C2=A0<%{SOURCE17} >postgresql%{packageversion}-setup

Ok.

> 7. Remove redundant file list init:
>=20
> # initialize file lists
> %{__cp} /dev/null main.lst
> %{__cp} /dev/null libs.lst
> %{__cp} /dev/null server.lst
> %{__cp} /dev/null devel.lst
> %{__cp} /dev/null plperl.lst
> %{__cp} /dev/null pltcl.lst
> %{__cp} /dev/null plpython.lst

Ok (Funny that noone saw this before

> 8. Replace all repetitive blocks for update-alternatives with for loop
> *AND* use %{mandir} and %{bindir} (per item 0):
>=20
> %{_sbindir}/update-alternatives --install %{bindir}/psql=C2=A0=C2=A0=C2=
=A0pgsql-psql
> %{pgbaseinstdir}/bin/psql %{packageversion}0
> %{_sbindir}/update-alternatives --install %{bindir}/clusterdb
> pgsql-clusterdb=C2=A0=C2=A0%{pgbaseinstdir}/bin/clusterdb %{packageversio=
n}0
> ...
> %{_sbindir}/update-alternatives --install %{bindir}/pg_basebackup
> pgsql-pg_basebackup=C2=A0=C2=A0=C2=A0=C2=A0%{pgbaseinstdir}/bin/pg_baseba=
ckup %{packageversion}0
> ...
>=20
> with:
>=20
> for i in \
> =C2=A0=C2=A0=C2=A0=C2=A0pgbench pg_test_timing pg_upgrade pg_xlogdump pg_=
archivecleanup \
> =C2=A0=C2=A0=C2=A0=C2=A0pg_config pg_isready pg_test_fsync pg_receivexlog=
 \
> =C2=A0=C2=A0=C2=A0=C2=A0psql clusterdb \
> =C2=A0=C2=A0=C2=A0=C2=A0createdb createlang createuser \
> =C2=A0=C2=A0=C2=A0=C2=A0dropdb droplang dropuser \
> =C2=A0=C2=A0=C2=A0=C2=A0pg_basebackup pg_dump pg_dumpall pg_restore \
> =C2=A0=C2=A0=C2=A0=C2=A0reindexdb vacuumdb; do
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --install %{bindi=
r}/${i}
> pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --install %{mandi=
r}/man1/${i}.1
> pgsql-${i}man=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{pgbaseinstdir}/share/man/man=
1/${i}.1 %{packageversion}0
> done

This line creates alternatives for *all* binaries, but we don't do this. We
only create alternatives entries for the binaries that can work across majo=
r
releases -- but I liked the approach. Will apply with a few changes.

> 9. in %post for -server, don't overwrite the symlink for 'postmaster' whi=
ch
> may be supplied by the OS vendor:
>=20
> # N.B. -- manually excluded 'postmaster' from update-alternatives
> # so that it doesn't conflict with the native vendor-supplied package
> # which does sometimes symlink postmaster to postgres.
> #
> for i in \
> =C2=A0=C2=A0=C2=A0=C2=A0initdb pg_controldata pg_ctl pg_resetxlog postgre=
s; do
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --install %{bindi=
r}/${i}
> pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --install %{mandi=
r}/man1/${i}.1
> pgsql-${i}man=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{pgbaseinstdir}/share/man/man=
1/${i}.1 %{packageversion}0
> done
>=20
> # fixup install of postmaster update-alternatives without wrecking
> # native OS-install of symlink
> if [ "$( readlink /usr/bin/postmaster )" =3D
> "/etc/alternatives/pgsql-postmaster" ]; then
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --remove pgsql-po=
stmaster
> %{pgbaseinstdir}/bin/postmaster
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --remove pgsql-po=
stmasterman
> %{pgbaseinstdir}/share/man/man1/postmaster.1
> fi
>=20
> and a similar block in -server's %postun:
>=20
> # N.B. -- manually excluded 'postmaster' from update-alternatives
> # so that it doesn't conflict with the native OS-supplied package
> # which does sometimes symlink postmaster to postgres.
> #
> if [ "$1" -eq 0 ]
> =C2=A0=C2=A0=C2=A0=C2=A0then
> =C2=A0=C2=A0=C2=A0=C2=A0for i in \
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0initdb pg_controldata pg_=
ctl pg_resetxlog postgres; do
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-altern=
atives --remove pgsql-${i}
> %{pgbaseinstdir}/bin/${i}
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-altern=
atives --remove pgsql-${i}man
> %{pgbaseinstdir}/share/man/man1/${i}.1
> =C2=A0=C2=A0=C2=A0=C2=A0done

I personally would ignore this part, because I don't think we are intereste=
d in
the installations that OS-supplied PG and ours would work together. It woul=
d
break many more things.

> 10. Additional update-alternatives for -contrib and -devel:
>=20
> %post devel
> for i in ecpg; do
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --install %{bindi=
r}/${i}
> pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --install %{mandi=
r}/man1/${i}.1
> pgsql-${i}man=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{pgbaseinstdir}/share/man/man=
1/${i}.1 %{packageversion}0
> done
>=20
> %postun devel
> if [ "$1" -eq 0 ]
> =C2=A0=C2=A0=C2=A0=C2=A0then
> =C2=A0=C2=A0=C2=A0=C2=A0for i in ecpg; do
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-altern=
atives --remove=C2=A0=C2=A0pgsql-${i}
> %{pgbaseinstdir}/bin/${i}
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-altern=
atives --remove=C2=A0=C2=A0pgsql-${i}man
> %{pgbaseinstdir}/share/man/man1/${i}.1
> =C2=A0=C2=A0=C2=A0=C2=A0done
> fi
>=20
> %post contrib
> for i in oid2name vacuumlo pg_recvlogical pg_standby; do
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --install %{bindi=
r}/${i}
> pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
> =C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-alternatives --install %{mandi=
r}/man1/${i}.1
> pgsql-${i}man=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{pgbaseinstdir}/share/man/man=
1/${i}.1 %{packageversion}0
> done
>=20
> %postun contrib
> if [ "$1" -eq 0 ]
> =C2=A0=C2=A0=C2=A0=C2=A0then
> =C2=A0=C2=A0=C2=A0=C2=A0for i in oid2name vacuumlo pg_recvlogical pg_stan=
dby; do
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-altern=
atives --remove pgsql-${i}
> %{pgbaseinstdir}/bin/${i}
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0%{_sbindir}/update-altern=
atives --remove pgsql-${i}man
> %{pgbaseinstdir}/share/man/man1/${i}.1
> =C2=A0=C2=A0=C2=A0=C2=A0done
> fi
>=20

Same as 8.

I will try to push these changes in next minor release set.

Regards,
--=20
Devrim G=C3=9CND=C3=9CZ
EnterpriseDB: http://www.enterprisedb.com
PostgreSQL Dan=C4=B1=C5=9Fman=C4=B1/Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

--=-7JXLKHQ7gseSSAmaDLgS
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAABCAAGBQJYSvrJAAoJENjDPf6Sz261FngP/A07fT3DVLeAtvJU6Ywj7n82
XITcXBXXaO0j9BPn/uI/49evwHUkno7ji5NIEnDAfYNW/vj5CjxvlU0CTi3eN5Zg
n4gT3n6IX7lHHejANOraO/mTY5srHpZ9OiALBhYu8g1WXXL73fCYBncGBjTlkz/U
7bWQHU52pyALQw37R8z0bH6csBk1y2MM22ygiZKhZuMdHaH6LDhqalutI0jXD4Ld
5XOgYbqbjwJ4nGF6WFu+UL0xcUrjpvljVL/2fPVh8CJfUBUwqzH0riMaPJsm9B3m
N+C1WcdRGvQzPX3QTdWA2HuZSzhNXHMClq4flXkYaUxphB1eq0cWjlcoHLf6pF02
nKjgC+128EkLgmw1r4PQcKdPGDHfmHInn73hTPPDq/K4Xvq1S15qq8wYh0Fl6LvK
AQ+PSrK0a2Hrey5wfSPo3QsRgqnzAI7eiTrpiVAUYXlSMKjqXsgZIu9XHVxnnAfC
jyRiEPG5kCVsJ0/Yt0h37AGiQPEspbuGypFOlVRZirIlp/QHeJEqnhUIs2Oc7MCq
5S8yd+bJTYfzXX+v4nTBLpW18LxsVgUNDh9/0rtAtFTJdklQtJxoVYOczphgAf1o
J0drfdnRrthGyqzCNaaJcU5ZW3an57zEpPNJTGznHjqWocwBzbMK6bPTrjCadiPE
PA6OJ3m/a95PSnrjlrkX
=0/iS
-----END PGP SIGNATURE-----

--=-7JXLKHQ7gseSSAmaDLgS--