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

Brent Verner <brent@rcfile.org> writes:
> | 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.

The code I was recalling is in trigger.c, and what it does is to test
the tgfoid to see if it's one of the RI_FKEY functions:

                /*
                 * We are interested in RI_FKEY triggers only.
                 */
                switch (trigger->tgfoid)
                {
                    case F_RI_FKEY_NOACTION_UPD:
                    case F_RI_FKEY_CASCADE_UPD:
                    case F_RI_FKEY_RESTRICT_UPD:
                    case F_RI_FKEY_SETNULL_UPD:
                    case F_RI_FKEY_SETDEFAULT_UPD:
                        is_ri_trigger = true;
                        break;

                    default:
                        is_ri_trigger = false;
                        break;
                }

I'd suggest sticking with that approach.  (It'd probably be a good idea
to break this chunk of knowledge out into a function, along the lines of
bool is_ri_trigger_function(func_oid), rather than duplicating it in two
different modules.)

> | 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.

But your patch has

>   tgrel = heap_openr(TriggerRelationName, AccessShareLock);
>   ScanKeyEntryInitialize(&skey, 0, Anum_pg_trigger_tgrelid,
>                      F_OIDEQ, RelationGetRelid(rel));
>   tgscan = heap_beginscan(tgrel, 0, SnapshotNow, 1, &skey);

The scankey tells the scan subroutines to only return tuples matching
the scan key, ie, only tuples with tgrelid = OID of the relation.
So even though you're plowing through all the tuples of pg_trigger,
only those attached to the target rel will be examined.


Oh, one other thing: patches should be submitted in "diff -c" format.
Plain diff format is considered too risky to apply, since it fails
silently if the line numbers are at all different between what you've
been working with and the file the committer is trying to apply the
patch to.  A context diff provides a little bit of protection.

            regards, tom lane