Re: YADP - Yet another Dependency Patch - Mailing list pgsql-patches

Rod Taylor <rbt@zort.ca> writes:
> This one has been corrected to fit in with Toms recent changes, as well
> as the changes with command/ restructuring.
> Please accept or reject quickly, else risk conflicts.

This is interesting but certainly far from ready for prime time.

Some random comments, in no particular order:

1. I don't like the code that installs and removes ad-hoc dependencies
from relations to type Oid.  On its own terms it's wrong (if it were
right, then you'd also need to be installing dependencies for Tid, Xid,
and the types of the other system columns), but on a larger scale I
don't see the point of expending cycles, disk space, and code complexity
to record these dependencies.  The system *cannot* work if you drop type
Oid.  So it'd seem to make more sense to wire in a notion that certain
types, tables, etc are "pinned" and may never be dropped; then there's
no need to create explicit dependencies on them.  If you want an
explicit representation of pinning in the pg_depends table, perhaps it
would work to create a row claiming that "table 0 / Oid 0 / subid 0"
depends on a pinned object.

2. Is it really necessary to treat pg_depends as a bootstrapped
relation?  That adds a lot of complexity, as you've evidently already
found, and it does not seem necessary if you're going to load the system
dependencies in a later step of the initdb process.  You can just make
the dependency-adding routines be no-ops in bootstrap mode; then create
pg_depends as an ordinary system catalog; and finally load the entries
post-bootstrap.

3. Isn't there a better way to find the initial dependencies?  That
SELECT is truly ugly, and more to the point is highly likely to break
anytime someone rearranges the catalogs.  I'd like to see it generated
automatically (maybe using a tool like findoidjoins); or perhaps we
could do the discovery of the columns to look at on-the-fly.

4. Do not use the parser's RESTRICT/CASCADE tokens as enumerated type
values.  They change value every time someone tweaks the grammar.
(Yes, I know you copied from extant code; that code is on my hitlist.)
Define your own enum type instead of creating a lot of bogus
dependencies on parser/parser.h.

5. Avoid using heapscans on pg_depend; it's gonna be way too big for
that to give acceptable performance.  Make sure you have indexes
available to match your searches, and use the systable_scan routines.

6. The tests on relation names in dependDelete, getObjectName are (a)
slow and (b) not schema-aware.  Can you make these into OID comparisons
instead?

7. The namespaceIdGetNspname routine you added is unsafe (it would need
to pstrdup the name to be sure it's still valid after releasing the
syscache entry); but more to the point, it duplicates a routine already
present in lsyscache.c (which is where this sort of utility generally
belongs, anyway).

8. Aggregate code seems unaware that aggfinalfn is optional.

I have to leave, but reserve the right to make more comments later ;-)

In general though, this seems like a cool approach and definitely
worth pursuing.  Keep at it!

            regards, tom lane