Bug 229321 - Review Request :postgresql-pgpool-II : Connection pooling/replication server for PostgreSQL
Summary: Review Request :postgresql-pgpool-II : Connection pooling/replication server ...
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: Gwyn Ciesla
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
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:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-11 08:17:46 UTC
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 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.