Re: The Contrib Roundup (long)

Josh Berkus <josh@agliodbs.com> writes:
> I had a lot of time to kill on airplanes recently so I've gone
> digging through /contrib in an effort to sort out what's in
> there and try to apply some consistent rules to it.

Sorry for not responding sooner; I'm catching up on back email.

As already noted, I agree with most of your goals here, though I'm with
Peter that restructuring the directory hierarchy is more trouble than
it's worth; just organizing the docs that way should be sufficient.

Here are some comments about the individual modules (where not stated,
I agree with your evaluation):

> adddepend: is this still needed, or would a proper
> dump-and-reload from 7.2 add the dependancy information anyway?

It is still theoretically useful, but given that the author feels it is
unmaintained and possibly broken, I would agree with removing it (or
maybe better, push to pgfoundry).  Anyone trying to jump straight from
7.2-or-earlier to 8.1 is probably going to have worse issues than lack
of dependencies anyway.

>> dbase

You seem to have missed this one.  I would argue that it should go to
pgfoundry as it is a data conversion tool.

> findoidjoins: again, it's not clear what this module is for. 

We need it to generate the oidjoins regression test.  Possibly it should
move into src/tools; I can't see any reason that ordinary users would
want it.

> intagg: what does this module do which is not already available
> through the built-in array functions and operators?  Maybe I
> don't understand what it does. Unnatributed in the README.  Move
> to pgfoundry?

The aggregate is functionally equivalent to ARRAY(sub-SELECT) but I
think that the aggregate notation is probably easier to use in many
scenarios.  The other function is basically the reverse conversion:
given an array, return a setof integers.  I don't think that we
currently have a built-in equivalent to that.

The functionality is useful but severely limited by the fact that it
only works on one datatype (int4) --- I'd like to see it reimplemented
as polymorphic functions.

> lo: another special data type.   Is its functionality required
> anymore?

The datatype as such is pretty much a waste of time --- you might as
well use OID.  (We could replace the datatype with a domain over OID
and have a compatible one-line implementation...)  The useful part of
this is the "lo_manage" trigger, which essentially supports automatic
dropping of large objects when the (assumed unique) references to them
from the main database go away.  It'd perhaps make sense to migrate
lo_manage into the main backend and lose the rest.

> misc_utils: I believe that all of these utils are obsolesced by
> builtin system commands or easily written userspace functions
> (like max(x,y)).  Also, is under the GPL (see above).  Author
> Massimo Dal Zotto (dz@cs.unitn.it)

I agree with just summarily removing this one.

> noupdate: this is a cool example of a simple C trigger and would
> be lovely to have in a doc somewhere.

As somebody else noted, it's completely broken: it does not do at all
what the documentation claims.  There are much more interesting trigger 
examples under spi/, so I'd agree with removal.

> pg_dumplo:  is this still required for pg large objects?  If
> so, can't we integrate it into the core?  utilities/

Probably drop; this was long ago superseded by functionality in pg_dump.

> pg_upgrade:  what's the status of this, Bruce?   Does it work at
> all?  Shouldn't this be moved to the pgfoundry project of the
> same name until it's stable?

Doesn't work and hasn't worked in a long time.  I'd agree with removal.

> pgbench:  I see repeated complaints on -performance about how
> pgbench results are misleading.   Why are we shipping it with
> PostgreSQL then?

It's handy to have *some* simple concurrent-behavior test included,
even if it's not something we put a lot of stock in.  The parallel
regression tests are a joke as far as exercising concurrent updates
go --- I think pg_bench is an important test tool for that reason.
I'd not vote to remove this without a better replacement.

> reindexdb: now obsolete per the REINDEX {database} command. 
> Remove from contrib.

Per other followups, this isn't obsolete at all.  Possibly the
functionality could be merged into vacuumdb, rather than writing
a whole 'nother program?

> spi: contains TimeTravel functions.  Do these actually still
> work?  The spi stuff is good for documentation purposes anyway
> ... but if the functions aren't working, should be in the docs
> and not /contrib.

Not only do they work, several of them are used in the regression tests.

> string: data_types/  Same problem as Massimo's
> other library; it's GPL.  Also, is it really needed at this
> point?  Massimo (dz@cs.unitn.it).

Actually, I've never looked closely at this before, and now that I have
I've got a serious problem with the proposed mode of use: overwriting
the typoutput functions for standard datatypes is just a guaranteed
recipe for breaking client code left and right.  The functions might be
safe and useful if invoked manually though.

> userlocks: another GPL script, with the problems that entails. 

As already pointed out, we should rewrite this from scratch in the main
backend.

> vacuumlo: is this still required?  If utilities/.

Yes, it is if you aren't using the lo_manage trigger ...

> xml and xml2: both by John Gray (jgray@azuli.co.uk).  John, why
> do we have two of these?  Otherwise, data_types/.

xml needs to be retired, same as tsearch.
        regards, tom lane