Re: ALTER TABLE RENAME fix - Mailing list pgsql-patches

On 10 Nov 2001 at 14:21 (-0500), Tom Lane wrote:
| Brent Verner <brent@rcfile.org> writes:
| > These patches fix the problem where an
| >   ALTER TABLE <table> RENAME <oldcolumn> TO <newcolumn>
| > did not update the RI_ triggers if the oldcolumn was referenced in
| > a RI constraint.
|
| This seems wrong on its face: the code unconditionally updates the
| attname part of a trigger's tgargs without checking whether the trigger
| actually refers to the attribute being renamed.  Don't you need to have
| a comparison against the old attname in there somewhere?

agreed.

| Another problem is that you're assuming that *all* triggers are RI
| triggers.  You need to check the trigger function before assuming that
| the tgargs are of a form you can parse.  There's at least one place in
| the existing code that makes such a check already, but I forget where.

Yes, I thought about this as I was going to sleep. I believe I can just
check that tgisconstraint='t' in the tuple.  I'll make sure this test
is sane.

| Also, I would think that you need to look not only at triggers on the
| relation containing the att being renamed, but also at triggers on the
| rels that are the other side of the FK relationships.  Those triggers
| also contain references to the attname, no?

Either this is already being done, or I do not understand what you mean.
The code currently will inspect all tuples where tgrelid or tgconstrrelid
is the altered relname's oid.  If relname appears as tgargs[1] or tgargs[2],
the attname tgargs[4] or tgargs[5] is updated appropriately.

| A performance issue here is that scanning all of pg_trigger could be
| kind of slow in a database with lots and lots of triggers.  You should
| be doing an indexscan using pg_trigger_tgrelid_index.  Actually, given
| the previous comment, probably more than one indexscan.  My inclination
| would be to accumulate the distinct tgconstrrelid values seen on
| triggers that are found to refer to the renamed att during the first
| indexscan on the original rel's relid, and then do additional indexscans
| looking at those relids (probably there won't be many).

I'll look into this after I get the rest of the code working correctly.

| One stylistic quibble: the direct references to CurrentMemoryContext
| are verbose and unnecessary.  Use palloc() and pstrdup().

gotcha.


Thank you,
  Brent

--
"Develop your talent, man, and leave the world something. Records are
really gifts from people. To think that an artist would love you enough
to share his music with anyone is a beautiful thing."  -- Duane Allman