Bug 244536

Summary: Review Request: postgresql-table_log - Log data changes in a PostgreSQL table
Product: [Fedora] Fedora Reporter: Devrim GUNDUZ <devrim>
Component: Package ReviewAssignee: Ruben Kerkhof <ruben>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: ruben
Target Milestone: ---Flags: ruben: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-06-23 22:22:02 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Devrim GUNDUZ 2007-06-16 20:54:32 UTC
Spec URL: http://developer.postgresql.org/~devrim/rpms/other/table_log/postgresql-table_log.spec
SRPM URL: http://developer.postgresql.org/~devrim/rpms/other/table_log/postgresql-table_log-0.4.4-1.fc7.src.rpm
Description: 
table_log is a set of functions to log changes on a table in PostgreSQL
and to restore the state of the table or a specific row on any time
in the past.

Comment 1 Ruben Kerkhof 2007-06-17 07:32:14 UTC
Hi Devrim,

Some initial comments:
- How about postgresql-tablelog for the package name instead of postgresql-table_log?
- BuildRoot should be one of 
   %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
   %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
   %{_tmppath}/%{name}-%{version}-%{release}-root
- Maybe rename README.table_log to just README

Comment 2 Devrim GUNDUZ 2007-06-17 10:31:49 UTC
Hi Ruben,

(In reply to comment #1)

> - How about postgresql-tablelog for the package name instead of
postgresql-table_log?

The name of the software is table_log, so I did not want to change it...

> - BuildRoot should be one of 
>    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>    %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
>    %{_tmppath}/%{name}-%{version}-%{release}-root

Oops, fixed.

> - Maybe rename README.table_log to just README

Since it will go to contrib directory of PostgreSQL, we want to stick the naming
guidelines there. It is in the form README.nameofthesoftware.

Thanks for the review. I did not bump up the spec file version; but updated spec
and srpm at the website:

Spec URL:
http://developer.postgresql.org/~devrim/rpms/other/table_log/postgresql-table_log.spec
SRPM URL:
http://developer.postgresql.org/~devrim/rpms/other/table_log/postgresql-table_log-0.4.4-1.fc7.src.rpm

Regards, Devrim


Comment 3 Ruben Kerkhof 2007-06-17 11:19:55 UTC
> Since it will go to contrib directory of PostgreSQL, we want to stick the naming
> guidelines there. It is in the form README.nameofthesoftware

But in Fedora it is installed in /usr/share/doc/postgresql-table_log-0.4.4/
You can move it in the specfile. Not a big issue though.

On another note, I tried to get the package working. The functions in table_log_init.sql are written in 
plpgsql, which wasn't installed on my system. Can you add postgresql-plpgsql to the Requires?

Comment 4 Devrim GUNDUZ 2007-06-17 12:36:54 UTC
Hello,

(In reply to comment #3)
 
> But in Fedora it is installed in /usr/share/doc/postgresql-table_log-0.4.4/
> You can move it in the specfile. Not a big issue though.

Ok done :) I also moved .sql files to _datadir/name from contrib. Yeah, it must
not be installed under contrib directory.
 
> On another note, I tried to get the package working. The functions in 
> table_log_init.sql are written in plpgsql, which wasn't installed on my
> system. Can you add postgresql-plpgsql to the Requires?

plpgsql is in core, so I added postgresql-server to Requires. (You will still
need to create plpgsql in your test database with createlang utility. createlang
is provided by postgresql package, but I did not add it to requires, because it
is required by postgresql-server).

Here is the new spec and SRPM:

Spec URL:
http://developer.postgresql.org/~devrim/rpms/other/table_log/postgresql-table_log.spec
SRPM URL:
http://developer.postgresql.org/~devrim/rpms/other/table_log/postgresql-table_log-0.4.4-2.fc7.src.rpm

Regards, Devrim

Comment 5 Ruben Kerkhof 2007-06-17 15:14:09 UTC
Hi again,

> plpgsql is in core

Nope, it's in postgresql-plperl

[ruben@odin review]$ rpm -qi postgresql-plperl
Name        : postgresql-plperl            Relocations: (not relocatable)
Version     : 8.2.4                             Vendor: Red Hat, Inc.
Release     : 1.fc7                         Build Date: Tue 24 Apr 2007 09:27:06 PM CEST
Install Date: Sun 17 Jun 2007 01:08:32 PM CEST      Build Host: ls20-bc1-14.build.redhat.com
Group       : Applications/Databases        Source RPM: postgresql-8.2.4-1.fc7.src.rpm
Size        : 67960                            License: BSD
Signature   : DSA/SHA1, Mon 04 Jun 2007 04:45:23 AM CEST, Key ID b44269d04f2a6fd2
Packager    : Red Hat, Inc. <http://bugzilla.redhat.com/bugzilla>
URL         : http://www.postgresql.org/
Summary     : The Perl procedural language for PostgreSQL.
Description :
PostgreSQL is an advanced Object-Relational database management
system.  The postgresql-plperl package contains the PL/Perl
procedural language for the backend.

[ruben@odin review]$ rpm -ql postgresql-plperl
/usr/lib/pgsql/plperl.so

So you have to require postgresql-plperl (which wil pull in postgresql and postgresql-server)

Comment 6 Devrim GUNDUZ 2007-06-17 19:00:08 UTC
Hello Ruben,

(In reply to comment #5)

> > plpgsql is in core
> 
> Nope, it's in postgresql-plperl

No, it is in postgresql-server:

$ rpm -ql postgresql-server|grep plpgsql
/usr/lib/pgsql/plpgsql.so

and table_log requires plpgsql, not plperl.

Regards, Devrim


Comment 7 Ruben Kerkhof 2007-06-17 20:17:58 UTC
I'm so sorry, you're absolutely right. There's a subtle difference between pl and perl ;-)

Review for release 2:
* RPM name is OK
* Source table_log-0.4.4.tar.gz is the same as upstream
* This is the latest version
* Builds fine in mock
* rpmlint looks OK
* File list looks OK
* Runs ok

This package is approved.

Comment 8 Devrim GUNDUZ 2007-06-17 20:51:45 UTC
New Package CVS Request
=======================
Package Name: postgresql-table_log
Short Description:  Log data changes in a PostgreSQL table
Owners: devrim@commandprompt.com
Branches: EL-4 EL-5 FC-6 F-7

Comment 9 Kevin Fenzi 2007-06-18 05:11:40 UTC
cvs done.

Comment 10 Ruben Kerkhof 2007-06-23 21:29:35 UTC
Devrim, don't forget to close this ticket when you're done.

Comment 11 Devrim GUNDUZ 2007-06-23 22:22:02 UTC
Oh, packages are already built and submitted. Closing this. Thanks for the reminder.