Code review for ALTER TRIGGER RENAME patch: make better use of index,

don't scribble on tuple returned by table scan.

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 52e9761d759bce2653db47d0db15da0a68d73626..a871c857880b65601892f54182a453b543d256bc 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/commands/trigger.c,v 1.116 2002/04/27 03:45:02 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/commands/trigger.c,v 1.117 2002/04/30 01:24:57 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -514,8 +514,7 @@ RelationRemoveTriggers(Relation rel)
  *                             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
+ *             update row in catalog
  */
 void
 renametrig(Oid relid,
@@ -526,8 +525,7 @@ renametrig(Oid relid,
        Relation        tgrel;
        HeapTuple       tuple;
        SysScanDesc     tgscan;
-       ScanKeyData key;
-       bool            found = FALSE;
+       ScanKeyData key[2];
        Relation        idescs[Num_pg_trigger_indices];
 
        /*
@@ -550,70 +548,69 @@ renametrig(Oid relid,
        /*
         * First pass -- look for name conflict
         */
-       ScanKeyEntryInitialize(&key, 0,
+       ScanKeyEntryInitialize(&key[0], 0,
                                                   Anum_pg_trigger_tgrelid,
                                                   F_OIDEQ,
                                                   ObjectIdGetDatum(relid));
+       ScanKeyEntryInitialize(&key[1], 0,
+                                                  Anum_pg_trigger_tgname,
+                                                  F_NAMEEQ,
+                                                  PointerGetDatum(newname));
        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));
-       }
+                                                               SnapshotNow, 2, key);
+       if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
+               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,
+       ScanKeyEntryInitialize(&key[0], 0,
                                                   Anum_pg_trigger_tgrelid,
                                                   F_OIDEQ,
                                                   ObjectIdGetDatum(relid));
+       ScanKeyEntryInitialize(&key[1], 0,
+                                                  Anum_pg_trigger_tgname,
+                                                  F_NAMEEQ,
+                                                  PointerGetDatum(oldname));
        tgscan = systable_beginscan(tgrel, TriggerRelidNameIndex, true,
-                                                               SnapshotNow, 1, &key);
-       while (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
+                                                               SnapshotNow, 2, key);
+       if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
        {
-               Form_pg_trigger pg_trigger = (Form_pg_trigger) GETSTRUCT(tuple);
+               /*
+                * Update pg_trigger tuple with new tgname.
+                */
+               tuple = heap_copytuple(tuple); /* need a modifiable copy */
 
-               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);
+               namestrcpy(&((Form_pg_trigger) GETSTRUCT(tuple))->tgname, newname);
 
-                       /*
-                        * 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);
+               simple_heap_update(tgrel, &tuple->t_self, tuple);
 
-                       /*
-                        * 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);
+               /*
+                * 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);
 
-                       found = TRUE;
-                       break;
-               }
+               /*
+                * Invalidate relation's relcache entry so that other backends (and
+                * this one too!) are sent SI message to make them rebuild relcache
+                * entries.  (Ideally this should happen automatically...)
+                */
+               CacheInvalidateRelcache(relid);
        }
+       else
+       {
+               elog(ERROR, "renametrig: trigger %s not defined on relation %s",
+                        oldname, RelationGetRelationName(targetrel));
+       }
+
        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!
         */

diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 1ce35eb164806fb9f17481129440510c97a68bbd..646ae45d240375025af62bd9bda6e355d9b533dd 100644 (file)
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: tablecmds.h,v 1.3 2002/04/26 19:29:47 tgl Exp $
+ * $Id: tablecmds.h,v 1.4 2002/04/30 01:24:52 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -15,7 +15,6 @@
 #define TABLECMDS_H
 
 #include "nodes/parsenodes.h"
-#include "utils/inval.h"
 
 extern void AlterTableAddColumn(Oid myrelid, bool inherits,
                                                                ColumnDef *colDef);