Re: RENAME TRIGGER patch (was [HACKERS] Odd(?) RI-trigger

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Joe Conway wrote:
> Tom Lane wrote:
> > Joe Conway <mail(at)joeconway(dot)com> writes:
> >
> >> There is already a RenameStmt node which is currently only used to
> >> rename tables or table column names. Is there any objection to
> >> modifying it to handle trigger names (and possibly other things in
> >> the future) also?
> >
> >
> > You'd need to add a field so you could distinguish the type of
> > rename, but on the whole that seems a reasonable thing to do;
> > probably better than adding a brand new node type. We're already
> > sharing node types for DROPs, for example, so I see no reason not to
> > do it for RENAMEs. (Cf 'DropPropertyStmt' in current sources)
> >
> > Renaming rules seems like something that should be on the list too, so
> > you're right that there will be more stuff later.
> >
>
> Attached is a patch for ALTER TRIGGER RENAME per the above thread. I
> left a stub for a future "ALTER RULE RENAME" but did not write that one
> yet. Bruce, if you want to add my name for for that I'll take it and do
> it later.
>
> It passes all regression tests on my RH box. Usage is as follows:
>
> test=# create table foo3(f1 int references foo2(f1));
> NOTICE: CREATE TABLE will create implicit trigger(s) for FOREIGN KEY
> check(s)
> CREATE
> test=# \d foo3
> Table "foo3"
> Column | Type | Modifiers
> --------+---------+-----------
> f1 | integer |
> Triggers: RI_ConstraintTrigger_16663
>
> test=# alter trigger "RI_ConstraintTrigger_16663" on foo3 rename to
> "MyOwnConstTriggerName";
> ALTER
> test=# \d foo3
> Table "foo3"
> Column | Type | Modifiers
> --------+---------+-----------
> f1 | integer |
> Triggers: MyOwnConstTriggerName
>
> Obviously there is no build in restriction on altering the name of
> refint triggers -- is this a problem?
>
> I'll follow up with a doc patch this weekend. If there are no
> objections, please apply.
>
> Thanks,
>
> Joe

> diff -cNr pgsql.cvs.orig/src/backend/commands/tablecmds.c pgsql/src/backend/commands/tablecmds.c
> *** pgsql.cvs.orig/src/backend/commands/tablecmds.c Fri Apr 19 10:32:50 2002
> --- pgsql/src/backend/commands/tablecmds.c Fri Apr 19 16:46:11 2002
> ***************
> *** 2851,2856 ****
> --- 2851,2973 ----
> }
>
> /*
> + * renametrig - changes the name of a trigger on a relation
> + *
> + * trigger name is changed in trigger catalog.
> + * No record of the previous name is kept.
> + *
> + * get proper relrelation from relation catalog (if not arg)
> + * scan trigger catalog
> + * for name conflict (within rel)
> + * for original trigger (if not arg)
> + * modify tgname in trigger tuple
> + * insert modified trigger in trigger catalog
> + * delete original trigger from trigger catalog
> + */
> + extern void renametrig(Oid relid,
> + const char *oldname,
> + const char *newname)
> + {
> + Relation targetrel;
> + Relation tgrel;
> + HeapTuple tuple;
> + SysScanDesc tgscan;
> + ScanKeyData key;
> + bool found = FALSE;
> + Relation idescs[Num_pg_trigger_indices];
> +
> + /*
> + * Grab an exclusive lock on the target table, which we will NOT
> + * release until end of transaction.
> + */
> + targetrel = heap_open(relid, AccessExclusiveLock);
> +
> + /*
> + * Scan pg_trigger twice for existing triggers on relation. We do this in
> + * order to ensure a trigger does not exist with newname (The unique index
> + * on tgrelid/tgname would complain anyway) and to ensure a trigger does
> + * exist with oldname.
> + *
> + * NOTE that this is cool only because we have AccessExclusiveLock on the
> + * relation, so the trigger set won't be changing underneath us.
> + */
> + tgrel = heap_openr(TriggerRelationName, RowExclusiveLock);
> +
> + /*
> + * First pass -- look for name conflict
> + */
> + ScanKeyEntryInitialize(&key, 0,
> + Anum_pg_trigger_tgrelid,
> + F_OIDEQ,
> + ObjectIdGetDatum(relid));
> + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndex, true,
> + SnapshotNow, 1, &key);
> + while (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
> + {
> + Form_pg_trigger pg_trigger = (Form_pg_trigger) GETSTRUCT(tuple);
> +
> + if (namestrcmp(&(pg_trigger->tgname), newname) == 0)
> + elog(ERROR, "renametrig: trigger %s already defined on relation %s",
> + newname, RelationGetRelationName(targetrel));
> + }
> + systable_endscan(tgscan);
> +
> + /*
> + * Second pass -- look for trigger existing with oldname and update
> + */
> + ScanKeyEntryInitialize(&key, 0,
> + Anum_pg_trigger_tgrelid,
> + F_OIDEQ,
> + ObjectIdGetDatum(relid));
> + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndex, true,
> + SnapshotNow, 1, &key);
> + while (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
> + {
> + Form_pg_trigger pg_trigger = (Form_pg_trigger) GETSTRUCT(tuple);
> +
> + if (namestrcmp(&(pg_trigger->tgname), oldname) == 0)
> + {
> + /*
> + * Update pg_trigger tuple with new tgname.
> + * (Scribbling on tuple is OK because it's a copy...)
> + */
> + namestrcpy(&(pg_trigger->tgname), newname);
> + simple_heap_update(tgrel, &tuple->t_self, tuple);
> +
> + /*
> + * keep system catalog indices current
> + */
> + CatalogOpenIndices(Num_pg_trigger_indices, Name_pg_trigger_indices, idescs);
> + CatalogIndexInsert(idescs, Num_pg_trigger_indices, tgrel, tuple);
> + CatalogCloseIndices(Num_pg_trigger_indices, idescs);
> +
> + /*
> + * Invalidate relation's relcache entry so that other
> + * backends (and this one too!) are sent SI message to make them
> + * rebuild relcache entries.
> + */
> + CacheInvalidateRelcache(relid);
> +
> + found = TRUE;
> + break;
> + }
> + }
> + systable_endscan(tgscan);
> +
> + heap_close(tgrel, RowExclusiveLock);
> +
> + if (!found)
> + elog(ERROR, "renametrig: trigger %s not defined on relation %s",
> + oldname, RelationGetRelationName(targetrel));
> +
> + /*
> + * Close rel, but keep exclusive lock!
> + */
> + heap_close(targetrel, NoLock);
> + }
> +
> +
> + /*
> * Given a trigger function OID, determine whether it is an RI trigger,
> * and if so whether it is attached to PK or FK relation.
> *
> diff -cNr pgsql.cvs.orig/src/backend/nodes/copyfuncs.c pgsql/src/backend/nodes/copyfuncs.c
> *** pgsql.cvs.orig/src/backend/nodes/copyfuncs.c Fri Apr 19 10:32:51 2002
> --- pgsql/src/backend/nodes/copyfuncs.c Fri Apr 19 13:58:47 2002
> ***************
> *** 2137,2146 ****
> RenameStmt *newnode = makeNode(RenameStmt);
>
> Node_Copy(from, newnode, relation);
> ! if (from->column)
> ! newnode->column = pstrdup(from->column);
> if (from->newname)
> newnode->newname = pstrdup(from->newname);
>
> return newnode;
> }
> --- 2137,2147 ----
> RenameStmt *newnode = makeNode(RenameStmt);
>
> Node_Copy(from, newnode, relation);
> ! if (from->oldname)
> ! newnode->oldname = pstrdup(from->oldname);
> if (from->newname)
> newnode->newname = pstrdup(from->newname);
> + newnode->renameType = from->renameType;
>
> return newnode;
> }
> diff -cNr pgsql.cvs.orig/src/backend/nodes/equalfuncs.c pgsql/src/backend/nodes/equalfuncs.c
> *** pgsql.cvs.orig/src/backend/nodes/equalfuncs.c Fri Apr 19 10:32:51 2002
> --- pgsql/src/backend/nodes/equalfuncs.c Fri Apr 19 13:56:00 2002
> ***************
> *** 983,992 ****
> {
> if (!equal(a->relation, b->relation))
> return false;
> ! if (!equalstr(a->column, b->column))
> return false;
> if (!equalstr(a->newname, b->newname))
> return false;
>
> return true;
> }
> --- 983,994 ----
> {
> if (!equal(a->relation, b->relation))
> return false;
> ! if (!equalstr(a->oldname, b->oldname))
> return false;
> if (!equalstr(a->newname, b->newname))
> return false;
> + if (a->renameType != b->renameType)
> + return false;
>
> return true;
> }
> diff -cNr pgsql.cvs.orig/src/backend/parser/gram.y pgsql/src/backend/parser/gram.y
> *** pgsql.cvs.orig/src/backend/parser/gram.y Fri Apr 19 10:32:51 2002
> --- pgsql/src/backend/parser/gram.y Fri Apr 19 14:07:35 2002
> ***************
> *** 2915,2922 ****
> {
> RenameStmt *n = makeNode(RenameStmt);
> n->relation = $3;
> ! n->column = $6;
> n->newname = $8;
> $$ = (Node *)n;
> }
> ;
> --- 2915,2935 ----
> {
> RenameStmt *n = makeNode(RenameStmt);
> n->relation = $3;
> ! n->oldname = $6;
> n->newname = $8;
> + if ($6 == NULL)
> + n->renameType = RENAME_TABLE;
> + else
> + n->renameType = RENAME_COLUMN;
> + $$ = (Node *)n;
> + }
> + | ALTER TRIGGER name ON relation_expr RENAME TO name
> + {
> + RenameStmt *n = makeNode(RenameStmt);
> + n->relation = $5;
> + n->oldname = $3;
> + n->newname = $8;
> + n->renameType = RENAME_TRIGGER;
> $$ = (Node *)n;
> }
> ;
> diff -cNr pgsql.cvs.orig/src/backend/tcop/utility.c pgsql/src/backend/tcop/utility.c
> *** pgsql.cvs.orig/src/backend/tcop/utility.c Fri Apr 19 10:32:52 2002
> --- pgsql/src/backend/tcop/utility.c Fri Apr 19 15:59:13 2002
> ***************
> *** 377,399 ****
>
> CheckOwnership(stmt->relation, true);
>
> ! if (stmt->column == NULL)
> {
> ! /*
> ! * rename relation
> ! */
> ! renamerel(RangeVarGetRelid(stmt->relation, false),
> ! stmt->newname);
> ! }
> ! else
> ! {
> ! /*
> ! * rename attribute
> ! */
> ! renameatt(RangeVarGetRelid(stmt->relation, false),
> ! stmt->column, /* old att name */
> stmt->newname, /* new att name */
> ! interpretInhOption(stmt->relation->inhOpt)); /* recursive? */
> }
> }
> break;
> --- 377,406 ----
>
> CheckOwnership(stmt->relation, true);
>
> ! switch (stmt->renameType)
> {
> ! case RENAME_TABLE:
> ! renamerel(RangeVarGetRelid(stmt->relation, false),
> ! stmt->newname);
> ! break;
> ! case RENAME_COLUMN:
> ! renameatt(RangeVarGetRelid(stmt->relation, false),
> ! stmt->oldname, /* old att name */
> stmt->newname, /* new att name */
> ! interpretInhOption(stmt->relation->inhOpt)); /* recursive? */
> ! break;
> ! case RENAME_TRIGGER:
> ! renametrig(RangeVarGetRelid(stmt->relation, false),
> ! stmt->oldname, /* old att name */
> ! stmt->newname); /* new att name */
> ! break;
> ! case RENAME_RULE:
> ! elog(ERROR, "ProcessUtility: Invalid target for RENAME: %d",
> ! stmt->renameType);
> ! break;
> ! default:
> ! elog(ERROR, "ProcessUtility: Invalid target for RENAME: %d",
> ! stmt->renameType);
> }
> }
> break;
> diff -cNr pgsql.cvs.orig/src/include/commands/tablecmds.h pgsql/src/include/commands/tablecmds.h
> *** pgsql.cvs.orig/src/include/commands/tablecmds.h Fri Apr 19 10:32:55 2002
> --- pgsql/src/include/commands/tablecmds.h Fri Apr 19 16:06:39 2002
> ***************
> *** 15,20 ****
> --- 15,21 ----
> #define TABLECMDS_H
>
> #include "nodes/parsenodes.h"
> + #include "utils/inval.h"
>
> extern void AlterTableAddColumn(Oid myrelid, bool inherits,
> ColumnDef *colDef);
> ***************
> *** 60,63 ****
> --- 61,68 ----
> extern void renamerel(Oid relid,
> const char *newrelname);
>
> + extern void renametrig(Oid relid,
> + const char *oldname,
> + const char *newname);
> +
> #endif /* TABLECMDS_H */
> diff -cNr pgsql.cvs.orig/src/include/nodes/parsenodes.h pgsql/src/include/nodes/parsenodes.h
> *** pgsql.cvs.orig/src/include/nodes/parsenodes.h Fri Apr 19 10:32:55 2002
> --- pgsql/src/include/nodes/parsenodes.h Fri Apr 19 14:21:21 2002
> ***************
> *** 1205,1221 ****
> } RemoveOperStmt;
>
> /* ----------------------
> ! * Alter Table Rename Statement
> * ----------------------
> */
> typedef struct RenameStmt
> {
> NodeTag type;
> ! RangeVar *relation; /* relation to be altered */
> ! char *column; /* if NULL, rename the relation name to
> ! * the new name. Otherwise, rename this
> ! * column name. */
> char *newname; /* the new name */
> } RenameStmt;
>
> /* ----------------------
> --- 1205,1227 ----
> } RemoveOperStmt;
>
> /* ----------------------
> ! * Alter Object Rename Statement
> * ----------------------
> + * Currently supports renaming tables, table columns, and triggers.
> + * If renaming a table, oldname is ignored.
> */
> + #define RENAME_TABLE 110
> + #define RENAME_COLUMN 111
> + #define RENAME_TRIGGER 112
> + #define RENAME_RULE 113
> +
> typedef struct RenameStmt
> {
> NodeTag type;
> ! RangeVar *relation; /* owning relation */
> ! char *oldname; /* name of rule, trigger, etc */
> char *newname; /* the new name */
> + int renameType; /* RENAME_TABLE, RENAME_COLUMN, etc */
> } RenameStmt;
>
> /* ----------------------

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo(at)postgresql(dot)org)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026