Re: The Contrib Roundup (long)

Josh Berkus <josh(at)agliodbs(dot)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(at)cs(dot)unitn(dot)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(at)cs(dot)unitn(dot)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(at)azuli(dot)co(dot)uk). John, why
> do we have two of these? Otherwise, data_types/.

xml needs to be retired, same as tsearch.

regards, tom lane