Обсуждение: a bunch of potential improvements to the postgresql spec files
Please find below a list of various improvements that we've made to the specfile for PostgreSQL 9.x.
0. globally replace /usr/sbin with %{_sbindir}, /usr/bin with %{_bindir}, and /usr/share/man with %{_mandir}
1. Remove the redundant use of 0%{?rhel} in tests for RHEL-specific values.
Turns:
%if 0%{?rhel} && 0%{rhel} > 6
into:
%if 0%{rhel} > 6
2. add min version to flex dependency:
BuildRequires: flex >= 2.5.31
3. move redundantly specified perl(ExtUtils::MakeMaker) outside of rhel version tests
4. Remove comma (it's not an error, it's just not as aesthetic) from Requires line(s):
-Requires(pre): %{_sbindir}/useradd, %{_sbindir}/groupadd
+Requires(pre): %{_sbindir}/useradd %{_sbindir}/groupadd
5. Remove redundant strip of -ffast-math from CFLAGS
6. replace hard-coded path for PGENGINE variable with %{pgbaseinstdir}:
# prep the setup script, including insertion of some values it needs
sed -e 's|^PGVERSION=.*$|PGVERSION=%{version}|' \
- -e 's|^PGENGINE=.*$|PGENGINE=/usr/pgsql-%{majorversion}/bin|' \
+ -e 's|^PGENGINE=.*$|PGENGINE=%{pgbaseinstdir}/bin|' \
<%{SOURCE17} >postgresql%{packageversion}-setup
7. Remove redundant file list init:
# 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
8. Replace all repetitive blocks for update-alternatives with for loop *AND* use %{mandir} and %{bindir} (per item 0):
%{_sbindir}/update-alternatives --install %{bindir}/psql pgsql-psql %{pgbaseinstdir}/bin/psql %{packageversion}0
%{_sbindir}/update-alternatives --install %{bindir}/clusterdb pgsql-clusterdb %{pgbaseinstdir}/bin/clusterdb %{packageversion}0
...
%{_sbindir}/update-alternatives --install %{bindir}/pg_basebackup pgsql-pg_basebackup %{pgbaseinstdir}/bin/pg_basebackup %{packageversion}0
...
with:
for i in \
pgbench pg_test_timing pg_upgrade pg_xlogdump pg_archivecleanup \
pg_config pg_isready pg_test_fsync pg_receivexlog \
psql clusterdb \
createdb createlang createuser \
dropdb droplang dropuser \
pg_basebackup pg_dump pg_dumpall pg_restore \
reindexdb vacuumdb; do
%{_sbindir}/update-alternatives --install %{bindir}/${i} pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
%{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0
done
9. in %post for -server, don't overwrite the symlink for 'postmaster' which may be supplied by the OS vendor:
# 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 \
initdb pg_controldata pg_ctl pg_resetxlog postgres; do
%{_sbindir}/update-alternatives --install %{bindir}/${i} pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
%{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0
done
# fixup install of postmaster update-alternatives without wrecking
# native OS-install of symlink
if [ "$( readlink /usr/bin/postmaster )" = "/etc/alternatives/pgsql-postmaster" ]; then
%{_sbindir}/update-alternatives --remove pgsql-postmaster %{pgbaseinstdir}/bin/postmaster
%{_sbindir}/update-alternatives --remove pgsql-postmasterman %{pgbaseinstdir}/share/man/man1/postmaster.1
fi
and a similar block in -server's %postun:
# 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 ]
then
for i in \
initdb pg_controldata pg_ctl pg_resetxlog postgres; do
%{_sbindir}/update-alternatives --remove pgsql-${i} %{pgbaseinstdir}/bin/${i}
%{_sbindir}/update-alternatives --remove pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1
done
10. Additional update-alternatives for -contrib and -devel:
%post devel
for i in ecpg; do
%{_sbindir}/update-alternatives --install %{bindir}/${i} pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
%{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0
done
%postun devel
if [ "$1" -eq 0 ]
then
for i in ecpg; do
%{_sbindir}/update-alternatives --remove pgsql-${i} %{pgbaseinstdir}/bin/${i}
%{_sbindir}/update-alternatives --remove pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1
done
fi
%post contrib
for i in oid2name vacuumlo pg_recvlogical pg_standby; do
%{_sbindir}/update-alternatives --install %{bindir}/${i} pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
%{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0
done
%postun contrib
if [ "$1" -eq 0 ]
then
for i in oid2name vacuumlo pg_recvlogical pg_standby; do
%{_sbindir}/update-alternatives --remove pgsql-${i} %{pgbaseinstdir}/bin/${i}
%{_sbindir}/update-alternatives --remove pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1
done
fi
--
Jon Nelson
Dyn / Principal Software Engineer
Re: [pgsql-pkg-yum] a bunch of potential improvements to thepostgresql spec files
От
Devrim Gündüz
Дата:
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: > > 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 values. > 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 >= 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 safe to move? > 4. Remove comma (it's not an error, it's just not as aesthetic) from > Requires line(s): > > -Requires(pre): %{_sbindir}/useradd, %{_sbindir}/groupadd > +Requires(pre): %{_sbindir}/useradd %{_sbindir}/groupadd Ok. > 5. Remove redundant strip of -ffast-math from CFLAGS Wow, ok. > 6. replace hard-coded path for PGENGINE variable with %{pgbaseinstdir}: > # prep the setup script, including insertion of some values it needs > sed -e 's|^PGVERSION=.*$|PGVERSION=%{version}|' \ > - -e 's|^PGENGINE=.*$|PGENGINE=/usr/pgsql-%{majorversion}/bin|' \ > + -e 's|^PGENGINE=.*$|PGENGINE=%{pgbaseinstdir}/bin|' \ > <%{SOURCE17} >postgresql%{packageversion}-setup Ok. > 7. Remove redundant file list init: > > # 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): > > %{_sbindir}/update-alternatives --install %{bindir}/psql pgsql-psql > %{pgbaseinstdir}/bin/psql %{packageversion}0 > %{_sbindir}/update-alternatives --install %{bindir}/clusterdb > pgsql-clusterdb %{pgbaseinstdir}/bin/clusterdb %{packageversion}0 > ... > %{_sbindir}/update-alternatives --install %{bindir}/pg_basebackup > pgsql-pg_basebackup %{pgbaseinstdir}/bin/pg_basebackup %{packageversion}0 > ... > > with: > > for i in \ > pgbench pg_test_timing pg_upgrade pg_xlogdump pg_archivecleanup \ > pg_config pg_isready pg_test_fsync pg_receivexlog \ > psql clusterdb \ > createdb createlang createuser \ > dropdb droplang dropuser \ > pg_basebackup pg_dump pg_dumpall pg_restore \ > reindexdb vacuumdb; do > %{_sbindir}/update-alternatives --install %{bindir}/${i} > pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0 > %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 > pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${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 major releases -- but I liked the approach. Will apply with a few changes. > 9. in %post for -server, don't overwrite the symlink for 'postmaster' which > may be supplied by the OS vendor: > > # 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 \ > initdb pg_controldata pg_ctl pg_resetxlog postgres; do > %{_sbindir}/update-alternatives --install %{bindir}/${i} > pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0 > %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 > pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0 > done > > # fixup install of postmaster update-alternatives without wrecking > # native OS-install of symlink > if [ "$( readlink /usr/bin/postmaster )" = > "/etc/alternatives/pgsql-postmaster" ]; then > %{_sbindir}/update-alternatives --remove pgsql-postmaster > %{pgbaseinstdir}/bin/postmaster > %{_sbindir}/update-alternatives --remove pgsql-postmasterman > %{pgbaseinstdir}/share/man/man1/postmaster.1 > fi > > and a similar block in -server's %postun: > > # 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 ] > then > for i in \ > initdb pg_controldata pg_ctl pg_resetxlog postgres; do > %{_sbindir}/update-alternatives --remove pgsql-${i} > %{pgbaseinstdir}/bin/${i} > %{_sbindir}/update-alternatives --remove pgsql-${i}man > %{pgbaseinstdir}/share/man/man1/${i}.1 > done I personally would ignore this part, because I don't think we are interested in the installations that OS-supplied PG and ours would work together. It would break many more things. > 10. Additional update-alternatives for -contrib and -devel: > > %post devel > for i in ecpg; do > %{_sbindir}/update-alternatives --install %{bindir}/${i} > pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0 > %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 > pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0 > done > > %postun devel > if [ "$1" -eq 0 ] > then > for i in ecpg; do > %{_sbindir}/update-alternatives --remove pgsql-${i} > %{pgbaseinstdir}/bin/${i} > %{_sbindir}/update-alternatives --remove pgsql-${i}man > %{pgbaseinstdir}/share/man/man1/${i}.1 > done > fi > > %post contrib > for i in oid2name vacuumlo pg_recvlogical pg_standby; do > %{_sbindir}/update-alternatives --install %{bindir}/${i} > pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0 > %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 > pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0 > done > > %postun contrib > if [ "$1" -eq 0 ] > then > for i in oid2name vacuumlo pg_recvlogical pg_standby; do > %{_sbindir}/update-alternatives --remove pgsql-${i} > %{pgbaseinstdir}/bin/${i} > %{_sbindir}/update-alternatives --remove pgsql-${i}man > %{pgbaseinstdir}/share/man/man1/${i}.1 > done > fi > Same as 8. I will try to push these changes in next minor release set. Regards, -- Devrim GÜNDÜZ EnterpriseDB: http://www.enterprisedb.com PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer Twitter: @DevrimGunduz , @DevrimGunduzTR
Вложения
Re: [pgsql-pkg-yum] a bunch of potential improvements to thepostgresql spec files
От
Devrim Gündüz
Дата:
Hi Jonathon, Thanks for the report. Committed parts of this to spec file: https://git.postgresql.org/gitweb/?p=pgrpms.git;a=commit;h=b7811829fdbf46b0248f8f0f1c3ca471616b7e04 Regards, Devrim On Fri, 2016-12-02 at 14:06 -0600, Jonathon Nelson wrote: > Please find below a list of various improvements that we've made to the > specfile for PostgreSQL 9.x. > > > > 0. globally replace /usr/sbin with %{_sbindir}, /usr/bin with %{_bindir}, > and /usr/share/man with %{_mandir} > > 1. Remove the redundant use of 0%{?rhel} in tests for RHEL-specific values. > Turns: > %if 0%{?rhel} && 0%{rhel} > 6 > into: > %if 0%{rhel} > 6 > > 2. add min version to flex dependency: > BuildRequires: flex >= 2.5.31 > > 3. move redundantly specified perl(ExtUtils::MakeMaker) outside of rhel > version tests > > 4. Remove comma (it's not an error, it's just not as aesthetic) from > Requires line(s): > > -Requires(pre): %{_sbindir}/useradd, %{_sbindir}/groupadd > +Requires(pre): %{_sbindir}/useradd %{_sbindir}/groupadd > > 5. Remove redundant strip of -ffast-math from CFLAGS > > 6. replace hard-coded path for PGENGINE variable with %{pgbaseinstdir}: > > # prep the setup script, including insertion of some values it needs > sed -e 's|^PGVERSION=.*$|PGVERSION=%{version}|' \ > - -e 's|^PGENGINE=.*$|PGENGINE=/usr/pgsql-%{majorversion}/bin|' \ > + -e 's|^PGENGINE=.*$|PGENGINE=%{pgbaseinstdir}/bin|' \ > <%{SOURCE17} >postgresql%{packageversion}-setup > > 7. Remove redundant file list init: > > # 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 > > 8. Replace all repetitive blocks for update-alternatives with for loop > *AND* use %{mandir} and %{bindir} (per item 0): > > %{_sbindir}/update-alternatives --install %{bindir}/psql pgsql-psql > %{pgbaseinstdir}/bin/psql %{packageversion}0 > %{_sbindir}/update-alternatives --install %{bindir}/clusterdb > pgsql-clusterdb %{pgbaseinstdir}/bin/clusterdb %{packageversion}0 > ... > %{_sbindir}/update-alternatives --install %{bindir}/pg_basebackup > pgsql-pg_basebackup %{pgbaseinstdir}/bin/pg_basebackup %{packageversion}0 > ... > > with: > > for i in \ > pgbench pg_test_timing pg_upgrade pg_xlogdump pg_archivecleanup \ > pg_config pg_isready pg_test_fsync pg_receivexlog \ > psql clusterdb \ > createdb createlang createuser \ > dropdb droplang dropuser \ > pg_basebackup pg_dump pg_dumpall pg_restore \ > reindexdb vacuumdb; do > %{_sbindir}/update-alternatives --install %{bindir}/${i} > pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0 > %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 > pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0 > done > > > 9. in %post for -server, don't overwrite the symlink for 'postmaster' which > may be supplied by the OS vendor: > > # 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 \ > initdb pg_controldata pg_ctl pg_resetxlog postgres; do > %{_sbindir}/update-alternatives --install %{bindir}/${i} > pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0 > %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 > pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0 > done > > # fixup install of postmaster update-alternatives without wrecking > # native OS-install of symlink > if [ "$( readlink /usr/bin/postmaster )" = > "/etc/alternatives/pgsql-postmaster" ]; then > %{_sbindir}/update-alternatives --remove pgsql-postmaster > %{pgbaseinstdir}/bin/postmaster > %{_sbindir}/update-alternatives --remove pgsql-postmasterman > %{pgbaseinstdir}/share/man/man1/postmaster.1 > fi > > and a similar block in -server's %postun: > > # 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 ] > then > for i in \ > initdb pg_controldata pg_ctl pg_resetxlog postgres; do > %{_sbindir}/update-alternatives --remove pgsql-${i} > %{pgbaseinstdir}/bin/${i} > %{_sbindir}/update-alternatives --remove pgsql-${i}man > %{pgbaseinstdir}/share/man/man1/${i}.1 > done > > 10. Additional update-alternatives for -contrib and -devel: > > %post devel > for i in ecpg; do > %{_sbindir}/update-alternatives --install %{bindir}/${i} > pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0 > %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 > pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0 > done > > %postun devel > if [ "$1" -eq 0 ] > then > for i in ecpg; do > %{_sbindir}/update-alternatives --remove pgsql-${i} > %{pgbaseinstdir}/bin/${i} > %{_sbindir}/update-alternatives --remove pgsql-${i}man > %{pgbaseinstdir}/share/man/man1/${i}.1 > done > fi > > %post contrib > for i in oid2name vacuumlo pg_recvlogical pg_standby; do > %{_sbindir}/update-alternatives --install %{bindir}/${i} > pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0 > %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1 > pgsql-${i}man %{pgbaseinstdir}/share/man/man1/${i}.1 %{packageversion}0 > done > > %postun contrib > if [ "$1" -eq 0 ] > then > for i in oid2name vacuumlo pg_recvlogical pg_standby; do > %{_sbindir}/update-alternatives --remove pgsql-${i} > %{pgbaseinstdir}/bin/${i} > %{_sbindir}/update-alternatives --remove pgsql-${i}man > %{pgbaseinstdir}/share/man/man1/${i}.1 > done > fi > > > > -- > Jon Nelson > Dyn / Principal Software Engineer -- Devrim GÜNDÜZ EnterpriseDB: http://www.enterprisedb.com PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer Twitter: @DevrimGunduz , @DevrimGunduzTR
Вложения
Re: [pgsql-pkg-yum] a bunch of potential improvements to thepostgresql spec files
От
Jonathon Nelson
Дата:
On Fri, Dec 9, 2016 at 12:41 PM, Devrim Gündüz <devrim@gunduz.org> wrote:
...
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:
>
> 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 values.
> Turns:
> %if 0%{?rhel} && 0%{rhel} > 6
> into:
> %if 0%{rhel} > 6
If we remove that one, then Fedora builds should fail.
Ah! I think I meant to suggest this:
%if 0%{rhel} > 6
%if 0%{rhel} > 6
(note the addition of the question mark). That should be logically equivalent to
%if 0%{?rhel} && 0%{rhel} > 6
%if 0%{?rhel} && 0%{rhel} > 6
...
> 8. Replace all repetitive blocks for update-alternatives with for loop
> *AND* use %{mandir} and %{bindir} (per item 0):
>
> %{_sbindir}/update-alternatives --install %{bindir}/psql pgsql-psql
> %{pgbaseinstdir}/bin/psql %{packageversion}0
> %{_sbindir}/update-alternatives --install %{bindir}/clusterdb
> pgsql-clusterdb %{pgbaseinstdir}/bin/clusterdb %{packageversion}0
> ...
> %{_sbindir}/update-alternatives --install %{bindir}/pg_basebackup
> pgsql-pg_basebackup %{pgbaseinstdir}/bin/pg_ basebackup %{packageversion}0
> ...
>
> with:
>
> for i in \
> pgbench pg_test_timing pg_upgrade pg_xlogdump pg_archivecleanup \
> pg_config pg_isready pg_test_fsync pg_receivexlog \
> psql clusterdb \
> createdb createlang createuser \
> dropdb droplang dropuser \
> pg_basebackup pg_dump pg_dumpall pg_restore \
> reindexdb vacuumdb; do
> %{_sbindir}/update-alternatives --install %{bindir}/${i}
> pgsql-${i} %{pgbaseinstdir}/bin/${i} %{packageversion}0
> %{_sbindir}/update-alternatives --install %{mandir}/man1/${i}.1
> pgsql-${i}man %{pgbaseinstdir}/share/man/man1/ ${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 major
releases -- but I liked the approach. Will apply with a few changes.
Thanks! I found that not having ready access to the latest versions of many of those binaries (without having to specify the full path) was a pain. Plus, isn't that why alternatives exists?
I will try to push these changes in next minor release set.