Bug 229321 - Review Request :postgresql-pgpool-II : Connection pooling/replication server for PostgreSQL
Review Request :postgresql-pgpool-II : Connection pooling/replication server ...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jon Ciesla
Fedora Package Reviews List
:
Depends On:
Blocks: 229322 229323
  Show dependency treegraph
 
Reported: 2007-02-20 05:05 EST by Devrim GUNDUZ
Modified: 2008-08-11 04:17 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-11 04:17:46 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)
patch to fix 4. from comment #13 (2.00 KB, patch)
2007-04-12 02:34 EDT, Ralf Corsepius
no flags Details | Diff

  None (edit)
Description Devrim GUNDUZ 2007-02-20 05:05:47 EST
Spec URL: http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/pgpool-II.spec
SRPM URL: http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II-1.0.2-1.src.rpm
Description: 
pgpool-II is a connection pooling/replication server for PostgreSQL.
pgpool-II runs between PostgreSQL's clients(front ends) and servers
(backends). A PostgreSQL client can connect to pgpool-II as if it were
a standard PostgreSQL server.
Comment 1 Jarod Wilson 2007-02-20 14:08:43 EST
Okay, ran through the spec file, and found a fair number of things that I
believe need fixing. I often prefer to make my suggestions in the form of direct
spec file changes that can then be diffed versus the original spec. My altered
spec is here:

http://people.redhat.com/jwilson/packages/postgresql-pgpool-II/postgresql-pgpool-II.spec

Basic summary of things to change:

* Tue Feb 20 2007 Jarod Wilson <jwilson@redhat.com> 1.0.2-2
- Create proper devel package, drop -libs package
- Nuke rpath
- Don't install libtool archive and static lib
- Clean up %%configure line
- Use proper %%_smp_mflags
- Install config files properly, without .sample on the end
- Preserve timestamps on header files
Comment 3 Jarod Wilson 2007-02-22 18:51:52 EST
I'll see if I can make another run through the latest srpm tomorrow.

FYI, the spec you're linking to is a rather outdated one that doesn't match
what's in the srpm...
Comment 4 Devrim GUNDUZ 2007-02-22 18:59:41 EST
Hi,

(In reply to comment #3)
> I'll see if I can make another run through the latest srpm tomorrow.

Ok.
 
> FYI, the spec you're linking to is a rather outdated one that doesn't match
> what's in the srpm...

Ah thanks. It is a copy-paste laziness :)

Here is the real spec:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II.spec

Regards, Devrim
Comment 5 Jon Ciesla 2007-04-11 10:01:51 EDT
At first glance, you need to add BuildRequires: libpqxx-devel.  Also, once
built, rpmlint complains about a lack of docs in -devel, but is otherwise clean.
 TBC. . .
Comment 6 Devrim GUNDUZ 2007-04-11 10:57:46 EDT
Hello,

(In reply to comment #5)
> At first glance, you need to add BuildRequires: libpqxx-devel. 

AFAICS pgpool-II does not depend on libpqxx-devel. Are you sure about that?

> Also, once built, rpmlint complains about a lack of docs in -devel, but is 
> otherwise clean.

Thanks a lot for this review.

Regards, Devrim
Comment 7 Jon Ciesla 2007-04-11 10:59:34 EDT
It build with it, and not without it, on FC6.  I'm testing a mock build without it.
Comment 8 Jon Ciesla 2007-04-11 12:40:25 EDT
Mock confirms the need for that BR.  Otherwise mock build happily, and the
resulting RPMS have only the -devel doc warning, otherwise clean.  
Comment 9 Jon Ciesla 2007-04-11 12:46:09 EDT
Needs naming guidelines.
Good spec name.
Meets Packaging Guidelines, AFAICS.
License is OK.
Comment 10 Jon Ciesla 2007-04-11 13:11:05 EDT
Spec is American English, and legible.
ISSUE:
md5sum from upstream 
adf88e4b7eb7f3347740a6b54aa09e92  pgpool-II-1.0.2.tar.gz
md5sum from SRPM
4abe109bfc3c78d441a9533365ccccd7  /usr/src/redhat/SOURCES/pgpool-II-1.0.2.tar.gz
Comment 11 Jon Ciesla 2007-04-11 14:02:38 EDT
Builds locally and in mock on x86.
BRs, see above.
No locales to handle.
ldconfig correct.
Not relocatable.
Explicitly creates no directories.
ISSUE: %{_mandir}/man8/* will own all man8 man pages.  Must be narrowed down.
No dupes, perms ok.
%clean correct.
Macros OK.
Code, not content.
Docs are reasonably sized and non-critical at runtime.
Dev/static handling correct, no .la.
No pkgconfig.pc.
Devel requires base, versioned.
Not a GUI app.
%install begins correctly.
ALL filenames UTF8.

All MUSTS are OK except for the BR, md5sum, and man8 ownership.
Comment 12 Devrim GUNDUZ 2007-04-12 01:26:01 EDT
Thanks for the review. I still cannot see libpgxx-devel issue. I don't have it
on my box, but I can build pgpool-II. Also I'm pretty sure that there is nothing
in the code that is dependent on libpqxx...

Other than this... I am really not sure about how md5 problem appeared. I fixed
that now. Also, fixed the man ownership.

Spec URL:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/pgpool-II.spec
SRPM URL:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II-1.0.2-2.src.rpm

Regards, Devrim
Comment 13 Ralf Corsepius 2007-04-12 02:19:37 EDT
Several issues:

1. Missing BR's:
...
checking for PQexecPrepared in -lpq... no
configure: error: libpq is not installed or libpq is old
error: Bad exit status from /var/tmp/rpm-tmp.37113 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.37113 (%build)

=> MISSING: BR: postgresql-devel

2. Package supports --disable-static
=> append --disable-static to %configure

3. All of the manual installs in %install are superfluous:

@@ -52,11 +53,6 @@
 make DESTDIR=%{buildroot} install
 mv %{buildroot}%{_sysconfdir}/pgpool.conf.sample
%{buildroot}%{_sysconfdir}/pgpool.conf
 mv %{buildroot}%{_sysconfdir}/pcp.conf.sample %{buildroot}%{_sysconfdir}/pcp.conf
-install -m 644 pgpool.8 %{buildroot}%{_mandir}/man8/
-install -m 644 -p pcp/pcp.h  %{buildroot}%{_includedir}/
-install -m 755 pcp/.libs/libpcp.so.0.* %{buildroot}%{_libdir}/
-cp -a pcp/.libs/libpcp.so.0 %{buildroot}%{_libdir}/
-install -m 644 -p pool_type.h  %{buildroot}%{_includedir}/
 # nuke libtool archive and static lib
 rm -f %{buildroot}%{_libdir}/libpcp.{a,la}

4. Very serious compiler warnings:

if gcc -DHAVE_CONFIG_H -DDEFAULT_CONFIGDIR=\"/etc\" -I. -I. -I.  -Wall
-Wmissing-prototypes -Wmissing-declarations -D_GNU_SOURCE -I /usr/include/pgsql
  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables -MT pool_process_query.o -MD -MP -MF
".deps/pool_process_query.Tpo" -c -o pool_process_query.o pool_process_query.c; \
        then mv -f ".deps/pool_process_query.Tpo" ".deps/pool_process_query.Po";
else rm -f ".deps/pool_process_query.Tpo"; exit 1; fi
pool_process_query.c: In function 'process_reporting':
pool_process_query.c:2453: warning: call to __builtin___strncpy_chk will always
overflow destination buffer
pool_process_query.c:2578: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2580: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2583: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2585: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2588: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2590: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2593: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
pool_process_query.c:2595: warning: call to __builtin___snprintf_chk will always
overflow destination buffer
Comment 14 Ralf Corsepius 2007-04-12 02:34:55 EDT
Created attachment 152359 [details]
patch to fix 4. from comment #13
Comment 15 Ralf Corsepius 2007-04-12 02:37:39 EDT
Why does this BZ depend on 229322 and 229323?

/me thinks these blockers are reversed. Probably this BZ should block 229322 and
229323 - Please correct them.
Comment 16 Devrim GUNDUZ 2007-04-12 03:55:19 EDT
Hi,

(In reply to comment #13)
> 1. Missing BR's:
> ...
> => MISSING: BR: postgresql-devel

:-) I thought I fixed this already. Anyway, thanks.
 
> 2. Package supports --disable-static
> => append --disable-static to %configure

Done.
 
> 3. All of the manual installs in %install are superfluous:

Ok, removed. Thanks.

> 4. Very serious compiler warnings:

Submitted your patch to upstream. I'll apply that patch tomorrow, unless someone
objects. Also added that patch to spec file.

Thanks again.

Regards, Devrim
Comment 17 Jon Ciesla 2007-04-12 08:18:12 EDT
Wow, that'll teach me to sleep. :) Thanks, good catches, Ralf. 
Comment 18 Jon Ciesla 2007-04-16 10:55:23 EDT
Is there an updated spec/SRPM addressing the above yet?
Comment 19 Jon Ciesla 2007-04-17 14:19:23 EDT
Forgot to set flag. . .
Comment 20 Devrim GUNDUZ 2007-04-17 14:33:38 EDT
(In reply to comment #18)
> Is there an updated spec/SRPM addressing the above yet?

Spec file:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II.spec

SRPM:
http://developer.postgresql.org/~devrim/rpms/other/pgpool-II/postgresql-pgpool-II-1.0.2-4.src.rpm

Regards, Devrim
Comment 21 Jon Ciesla 2007-04-18 08:02:04 EDT
BRs, MD5 and man8 issues fixed.  Ralf's issues have been addressed.
APPROVED.
I'll start looking at 229322.
Comment 22 Devrim GUNDUZ 2007-04-18 08:09:38 EDT
New Package CVS Request
=======================
Package Name: pgpool-II
Short Description: Connection pooling/replication server for PostgreSQL
Owners: devrim@CommandPrompt.com
Branches: FC-5 FC-6 EL-4 EL-5
InitialCC: devrim@CommandPrompt.com
Comment 23 Devrim GUNDUZ 2007-04-19 14:24:39 EDT
Opps, fixing package name:

New Package CVS Request
=======================
Package Name: postgresql-pgpool-II
Short Description: Connection pooling/replication server for PostgreSQL
Owners: devrim@CommandPrompt.com
Branches: FC-5 FC-6 EL-4 EL-5
InitialCC: devrim@CommandPrompt.com

Regards, Devrim
Comment 24 Devrim GUNDUZ 2007-06-03 03:49:11 EDT
Package submitted for build.
Comment 25 Michael Schwendt 2007-06-15 13:23:54 EDT
Has this been fixed yet?

postgresql-pgpool - 3.2-1.fc7.i386
  File conflict with: postgresql-pgpool-II - 1.1-1.fc8.i386
     /usr/bin/pgpool

postgresql-pgpool-II - 1.1-1.fc8.i386
  File conflict with: postgresql-pgpool - 3.2-1.fc7.i386
     /usr/bin/pgpool
Comment 26 Jon Ciesla 2007-06-15 14:28:23 EDT
I can't believe I didn't catch this.  

My recommendation would be to rename /u/b/pgbool to /u/b/pgpool-II and patch the
rest to accommodate.
Comment 27 Devrim GUNDUZ 2007-06-15 17:27:07 EDT
Hello,

Good point, but I think we should just obsolete postgresql-pgpool. pgpool-II and
pgpool (-I) should not be run at the same server --pgpool-II can do what pgpool
can do, plus it has some other features.

If adding a Obsoletes: is ok for everyone, I'll go with it.

Regards, Devrim
Comment 28 Jon Ciesla 2007-06-18 07:24:20 EDT
Well, since you're the pgpool maintainer, I can see no problem with that.
Comment 29 Christian Nolte 2007-08-03 05:15:33 EDT
First, a question: Why is it that postgresql-pgpool-II has been pushed into the
fedora repo before this package is approved?

postgresql-pgpool-II needs an init-script to start pgpool as a service.
Comment 30 Jon Ciesla 2007-08-03 06:18:34 EDT
See flag and comment #21.  But you're right, it could use an init script. 
Should this have been a blocker?
Comment 31 Devrim GUNDUZ 2007-08-04 21:30:48 EDT
I pushed init script and /etc/sysconfig/pgpool to -devel. Could you please test?

Regards, Devrim
Comment 32 Jon Ciesla 2007-08-06 14:24:40 EDT
/etc/sysconfig/pgpool is 755, shouldn't it be 644?  Otherwise, looks great.
Comment 33 Jon Ciesla 2007-08-06 14:27:04 EDT
Although I don't see it in the system services menu in the text mode config
tools.   
Comment 34 Devrim GUNDUZ 2007-08-29 01:26:33 EDT
Hi,

(In reply to comment #32)
> /etc/sysconfig/pgpool is 755, shouldn't it be 644?  Otherwise, looks great.

Oops, done.

(In reply to comment #33)
> Although I don't see it in the system services menu in the text mode config
> tools.   

Added:

chkconfig --add pgpool

I am working on these packages nowadays -- Found some problems while using with
the web interface. I will submit it for building ASAP.

Regards, Devrim
Comment 35 Jon Ciesla 2007-10-23 10:38:35 EDT
This can probably be closed, no?
Comment 36 Devrim GUNDUZ 2007-10-23 10:58:16 EDT
No. I'm waiting to push pgpool-ha.

Regards, Devrim
Comment 37 Jon Ciesla 2007-10-23 11:05:11 EDT
Ok, just checking.
Comment 38 Jon Ciesla 2008-06-12 10:15:44 EDT
(In reply to comment #36)
> No. I'm waiting to push pgpool-ha.
> 
> Regards, Devrim

-ha has been pushed, and this package has even been updated a few times. Closable?


Note You need to log in before you can comment on or make changes to this bug.