Bug 244536 - Review Request: postgresql-table_log - Log data changes in a PostgreSQL table
Review Request: postgresql-table_log - Log data changes in a PostgreSQL table
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ruben Kerkhof
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-16 16:54 EDT by Devrim GUNDUZ
Modified: 2007-11-30 17:12 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-23 18:22:02 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ruben: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Devrim GUNDUZ 2007-06-16 16:54:32 EDT
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 03:32:14 EDT
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 06:31:49 EDT
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 07:19:55 EDT
> 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 08:36:54 EDT
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 11:14:09 EDT
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 15:00:08 EDT
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 16:17:58 EDT
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 16:51:45 EDT
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 01:11:40 EDT
cvs done.
Comment 10 Ruben Kerkhof 2007-06-23 17:29:35 EDT
Devrim, don't forget to close this ticket when you're done.
Comment 11 Devrim GUNDUZ 2007-06-23 18:22:02 EDT
Oh, packages are already built and submitted. Closing this. Thanks for the reminder.

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