Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 229321 - Review Request :postgresql-pgpool-II : Connection pooling/replication server for PostgreSQL
Summary: Review Request :postgresql-pgpool-II : Connection pooling/replication server ...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Gwyn Ciesla
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: 229322 229323
TreeView+ depends on / blocked
 
Reported: 2007-02-20 10:05 UTC by Devrim GUNDUZ
Modified: 2008-08-11 08:17 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-08-11 08:17:46 UTC
gwync: fedora-review+
wtogami: fedora-cvs+


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

Description Devrim GUNDUZ 2007-02-20 10:05:47 UTC
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 19:08:43 UTC
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 23:51:52 UTC
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 23:59:41 UTC
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 Gwyn Ciesla 2007-04-11 14:01:51 UTC
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 14:57:46 UTC
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 Gwyn Ciesla 2007-04-11 14:59:34 UTC
It build with it, and not without it, on FC6.  I'm testing a mock build without it.

Comment 8 Gwyn Ciesla 2007-04-11 16:40:25 UTC
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 Gwyn Ciesla 2007-04-11 16:46:09 UTC
Needs naming guidelines.
Good spec name.
Meets Packaging Guidelines, AFAICS.
License is OK.


Comment 10 Gwyn Ciesla 2007-04-11 17:11:05 UTC
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 Gwyn Ciesla 2007-04-11 18:02:38 UTC
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 05:26:01 UTC
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 06:19:37 UTC
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 06:34:55 UTC
Created attachment 152359 [details]
patch to fix 4. from comment #13

Comment 15 Ralf Corsepius 2007-04-12 06:37:39 UTC
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 07:55:19 UTC
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 Gwyn Ciesla 2007-04-12 12:18:12 UTC
Wow, that'll teach me to sleep. :) Thanks, good catches, Ralf. 

Comment 18 Gwyn Ciesla 2007-04-16 14:55:23 UTC
Is there an updated spec/SRPM addressing the above yet?

Comment 19 Gwyn Ciesla 2007-04-17 18:19:23 UTC
Forgot to set flag. . .

Comment 20 Devrim GUNDUZ 2007-04-17 18:33:38 UTC
(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 Gwyn Ciesla 2007-04-18 12:02:04 UTC
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 12:09:38 UTC
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 18:24:39 UTC
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 07:49:11 UTC
Package submitted for build.

Comment 25 Michael Schwendt 2007-06-15 17:23:54 UTC
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 Gwyn Ciesla 2007-06-15 18:28:23 UTC
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 21:27:07 UTC
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 Gwyn Ciesla 2007-06-18 11:24:20 UTC
Well, since you're the pgpool maintainer, I can see no problem with that.

Comment 29 Christian Nolte 2007-08-03 09:15:33 UTC
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 Gwyn Ciesla 2007-08-03 10:18:34 UTC
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-05 01:30:48 UTC
I pushed init script and /etc/sysconfig/pgpool to -devel. Could you please test?

Regards, Devrim

Comment 32 Gwyn Ciesla 2007-08-06 18:24:40 UTC
/etc/sysconfig/pgpool is 755, shouldn't it be 644?  Otherwise, looks great.

Comment 33 Gwyn Ciesla 2007-08-06 18:27:04 UTC
Although I don't see it in the system services menu in the text mode config
tools.   

Comment 34 Devrim GUNDUZ 2007-08-29 05:26:33 UTC
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 Gwyn Ciesla 2007-10-23 14:38:35 UTC
This can probably be closed, no?

Comment 36 Devrim GUNDUZ 2007-10-23 14:58:16 UTC
No. I'm waiting to push pgpool-ha.

Regards, Devrim

Comment 37 Gwyn Ciesla 2007-10-23 15:05:11 UTC
Ok, just checking.

Comment 38 Gwyn Ciesla 2008-06-12 14:15:44 UTC
(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.