Re: Odd(?) RI-trigger behavior

I was just fooling around with replacing the existing plain index on
pg_trigger.tgrelid with a unique index on (tgrelid, tgname). In theory
this should not affect anything --- the code already enforced that two
triggers on the same relation can't have the same name. The index
should merely provide a backup check.

So I was a tad surprised to get a regression test failure:

*** ./expected/foreign_key.out Thu Apr 11 15:13:36 2002
--- ./results/foreign_key.out Thu Apr 18 21:26:20 2002
***************
*** 899,905 ****
ERROR: <unnamed> referential integrity violation - key in pktable still referenced from pktable
-- fails (1,1) is being referenced (twice)
update pktable set base1=3 where base1=1;
! ERROR: <unnamed> referential integrity violation - key in pktable still referenced from pktable
-- this sequence of two deletes will work, since after the first there will be no (2,*) references
delete from pktable where base2=2;
delete from pktable where base1=2;
--- 899,905 ----
ERROR: <unnamed> referential integrity violation - key in pktable still referenced from pktable
-- fails (1,1) is being referenced (twice)
update pktable set base1=3 where base1=1;
! ERROR: <unnamed> referential integrity violation - key referenced from pktable not found in pktable
-- this sequence of two deletes will work, since after the first there will be no (2,*) references
delete from pktable where base2=2;
delete from pktable where base1=2;

======================================================================

This particular test involves a table with a foreign-key reference to
itself, ie, it's both PK and FK. What apparently is happening is that
the two RI triggers are now being fired in a different order than
before. While either of them would have detected an error, we now get
the other error first.

Does this bother anyone? It seems to me that the old code essentially
had no guarantee at all about the order in which the triggers would
fire, and so it was pure luck that the regression test never showed
the other message.

With the modified code, because we load the triggers by scanning
an index on (tgrelid, tgname), it is actually true that triggers are
fired in name order. We've had requests in the past to provide a
well-defined firing order for triggers --- should we document this
behavior and support it, or should we pretend it ain't there?

BTW, the same goes for rules: it would now be pretty easy to guarantee
that rules are fired in name order.

regards, tom lane

On Thu, 18 Apr 2002, Tom Lane wrote:

> This particular test involves a table with a foreign-key reference to
> itself, ie, it's both PK and FK. What apparently is happening is that
> the two RI triggers are now being fired in a different order than
> before. While either of them would have detected an error, we now get
> the other error first.
>
> Does this bother anyone? It seems to me that the old code essentially
> had no guarantee at all about the order in which the triggers would
> fire, and so it was pure luck that the regression test never showed
> the other message.

That's probably a bad thing even if I doubt that it'd ever come up the
other way barring changes to other regression tests in practice. Forcing
an order probably helps with this case anyway.

> With the modified code, because we load the triggers by scanning
> an index on (tgrelid, tgname), it is actually true that triggers are
> fired in name order. We've had requests in the past to provide a
> well-defined firing order for triggers --- should we document this
> behavior and support it, or should we pretend it ain't there?

Didn't someone (Peter?) say that the mandated firing order was based on
creation order/time in SQL99?

Stephan Szabo <sszabo(at)megazone23(dot)bigpanda(dot)com> writes:
> Didn't someone (Peter?) say that the mandated firing order was based on
> creation order/time in SQL99?

It does say that:

The order of execution of a set of triggers is ascending by value
of their timestamp of creation in their descriptors, such that the
oldest trigger executes first. If one or more triggers have the
same timestamp value, then their relative order of execution is
implementation-defined.

However, this strikes me as fairly brain-dead; it's unnecessarily hard
to control the order of trigger execution. You have to drop and
recreate triggers if you want to insert a new one at a desired position.
Worse, if you create several triggers in the same transaction, they'll
have the same timestamp --- leaving you right back in the
implementation-defined case. But if you want to make your rearrangement
atomically with respect to other transactions, you have little choice
but to drop/recreate in one xact. Looks like a catch-22 to me.

ISTM we had discussed this before and concluded that name order was
a more reasonable definition. Nobody had got round to doing anything
about it though. (Indeed my current hack was not intended to provide
a predictable firing order, it just fell out that way...)

regards, tom lane

On Thu, 18 Apr 2002, Tom Lane wrote:

> Stephan Szabo <sszabo(at)megazone23(dot)bigpanda(dot)com> writes:
> > Didn't someone (Peter?) say that the mandated firing order was based on
> > creation order/time in SQL99?
>
> It does say that:
>
> The order of execution of a set of triggers is ascending by value
> of their timestamp of creation in their descriptors, such that the
> oldest trigger executes first. If one or more triggers have the
> same timestamp value, then their relative order of execution is
> implementation-defined.
>
> However, this strikes me as fairly brain-dead; it's unnecessarily hard
> to control the order of trigger execution. You have to drop and
> recreate triggers if you want to insert a new one at a desired position.
> Worse, if you create several triggers in the same transaction, they'll
> have the same timestamp --- leaving you right back in the
> implementation-defined case. But if you want to make your rearrangement
> atomically with respect to other transactions, you have little choice
> but to drop/recreate in one xact. Looks like a catch-22 to me.
>
> ISTM we had discussed this before and concluded that name order was
> a more reasonable definition. Nobody had got round to doing anything
> about it though. (Indeed my current hack was not intended to provide
> a predictable firing order, it just fell out that way...)

I agree that name is better, I wasn't sure if we'd reached a consensus on
it or if the conversation drifted away due to the fact that noone was
looking at it at the time.

En Thu, 18 Apr 2002 20:43:54 -0700 (PDT)
Stephan Szabo <sszabo(at)megazone23(dot)bigpanda(dot)com> escribió:

> I agree that name is better, I wasn't sure if we'd reached a consensus on
> it or if the conversation drifted away due to the fact that noone was
> looking at it at the time.

http://archives.postgresql.org/pgsql-general/2001-09/msg00234.php

Nobody opposed to the idea of name ordering in that thread.

But note that this is on TODO:

* Allow user to control trigger firing order

That probably means that the user should have some reasonable way to
change the name, besides fiddling with system catalogs.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Siempre hay que alimentar a los dioses, aunque la tierra este seca" (Orual)

> But note that this is on TODO:
>
> * Allow user to control trigger firing order
>
> That probably means that the user should have some reasonable way to
> change the name, besides fiddling with system catalogs.

An ALTER TRIGGER command? Of course, it should not allow modification of
constraint triggers...

Chris

Alvaro Herrera <alvherre(at)atentus(dot)com> writes:
> http://archives.postgresql.org/pgsql-general/2001-09/msg00234.php
> Nobody opposed to the idea of name ordering in that thread.

Okay, I've committed the fixes that implement this.

> But note that this is on TODO:
> * Allow user to control trigger firing order
> That probably means that the user should have some reasonable way to
> change the name, besides fiddling with system catalogs.

Yeah. As of CVS tip, to reshuffle the order of existing triggers you
must (a) do a manual UPDATE pg_trigger SET tgname = 'something' ...
then (b) restart your backend(s), because the relcache code does not
notice that you did that, so it'll keep using the trigger data it
already had loaded. This is pretty ugly. An ALTER TRIGGER command
seems called for if we want to call the TODO item really done.
I haven't got time for that at the moment; any volunteers?

regards, tom lane

Tom Lane wrote:
>
> Yeah. As of CVS tip, to reshuffle the order of existing triggers you
> must (a) do a manual UPDATE pg_trigger SET tgname = 'something' ...
> then (b) restart your backend(s), because the relcache code does not
> notice that you did that, so it'll keep using the trigger data it
> already had loaded. This is pretty ugly. An ALTER TRIGGER command
> seems called for if we want to call the TODO item really done.
> I haven't got time for that at the moment; any volunteers?
>

I'll take it.

Joe

Joe Conway wrote:
> Tom Lane wrote:
>
>>
>> Yeah. As of CVS tip, to reshuffle the order of existing triggers you
>> must (a) do a manual UPDATE pg_trigger SET tgname = 'something' ...
>> then (b) restart your backend(s), because the relcache code does not
>> notice that you did that, so it'll keep using the trigger data it
>> already had loaded. This is pretty ugly. An ALTER TRIGGER command
>> seems called for if we want to call the TODO item really done.
>> I haven't got time for that at the moment; any volunteers?
>>
>
> I'll take it.
>

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?

Joe

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.

regards, tom lane

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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)atentus(dot)com> writes:
> > http://archives.postgresql.org/pgsql-general/2001-09/msg00234.php
> > Nobody opposed to the idea of name ordering in that thread.
>
> Okay, I've committed the fixes that implement this.
>
> > But note that this is on TODO:
> > * Allow user to control trigger firing order
> > That probably means that the user should have some reasonable way to
> > change the name, besides fiddling with system catalogs.
>
> Yeah. As of CVS tip, to reshuffle the order of existing triggers you
> must (a) do a manual UPDATE pg_trigger SET tgname = 'something' ...
> then (b) restart your backend(s), because the relcache code does not
> notice that you did that, so it'll keep using the trigger data it
> already had loaded. This is pretty ugly. An ALTER TRIGGER command
> seems called for if we want to call the TODO item really done.
> I haven't got time for that at the moment; any volunteers?

TODO updated with:

* -Allow user to control trigger firing order
* Add ALTER TRIGGER ... RENAME

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

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

Patch applied. Thanks.

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

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