Re: Wrong order of tests in findDependentObjects()

I wrote:
> Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
>> I can't think of any reason you'd want the current behavior.

> But I think fixing it to not recurse to extensions during temp namespace
> cleanup might not be very hard.  I'll take a look.

Here's a draft patch for that.  Rather than sticking yet another special
assumption into deleteWhatDependsOn, I thought it was time to get rid of
that function altogether in favor of selecting its few special behaviors
via flag bits for performDeletion.  So this adds PERFORM_DELETION_QUIETLY
and PERFORM_DELETION_SKIP_ORIGINAL flag bits to do that, plus a
PERFORM_DELETION_SKIP_EXTENSIONS bit that solves the problem at hand.
Treating this as a performDeletion flag bit also lets us disable extension
dropping in autovacuum's forced drop of temp tables, which would otherwise
be a nasty hole in the fix.

I'm not sure if this is a candidate for back-patching or not.  I think
what it's fixing is mostly a convenience thing, since extension scripts
that explicitly drop any temp objects they've created are not at risk,
and that would be good extension authoring practice anyway IMO.
If we do back-patch we'd need to figure out whether we want to preserve
deleteWhatDependsOn as a stub function in the back branches.  (I rather
doubt that any third party code is calling it, since it's so
special-purpose, but you never know ...)

Thoughts?

            regards, tom lane

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index f71d80f..a32cde3 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** static const Oid object_classes[] = {
*** 168,173 ****
--- 168,174 ----


  static void findDependentObjects(const ObjectAddress *object,
+                      int objflags,
                       int flags,
                       ObjectAddressStack *stack,
                       ObjectAddresses *targetObjects,
*************** static void findDependentObjects(const O
*** 175,181 ****
                       Relation *depRel);
  static void reportDependentObjects(const ObjectAddresses *targetObjects,
                         DropBehavior behavior,
!                        int msglevel,
                         const ObjectAddress *origObject);
  static void deleteOneObject(const ObjectAddress *object,
                  Relation *depRel, int32 flags);
--- 176,182 ----
                       Relation *depRel);
  static void reportDependentObjects(const ObjectAddresses *targetObjects,
                         DropBehavior behavior,
!                        int flags,
                         const ObjectAddress *origObject);
  static void deleteOneObject(const ObjectAddress *object,
                  Relation *depRel, int32 flags);
*************** deleteObjectsInList(ObjectAddresses *tar
*** 237,247 ****
      }

      /*
!      * Delete all the objects in the proper order.
       */
      for (i = 0; i < targetObjects->numrefs; i++)
      {
          ObjectAddress *thisobj = targetObjects->refs + i;

          deleteOneObject(thisobj, depRel, flags);
      }
--- 238,254 ----
      }

      /*
!      * Delete all the objects in the proper order, except that if told to, we
!      * should skip the original object(s).
       */
      for (i = 0; i < targetObjects->numrefs; i++)
      {
          ObjectAddress *thisobj = targetObjects->refs + i;
+         ObjectAddressExtra *thisextra = targetObjects->extras + i;
+
+         if ((flags & PERFORM_DELETION_SKIP_ORIGINAL) &&
+             (thisextra->flags & DEPFLAG_ORIGINAL))
+             continue;

          deleteOneObject(thisobj, depRel, flags);
      }
*************** deleteObjectsInList(ObjectAddresses *tar
*** 255,270 ****
   * according to the dependency type.
   *
   * This is the outer control routine for all forms of DROP that drop objects
!  * that can participate in dependencies.  Note that the next two routines
!  * are variants on the same theme; if you change anything here you'll likely
!  * need to fix them too.
   *
!  * flags should include PERFORM_DELETION_INTERNAL when the drop operation is
!  * not the direct result of a user-initiated action.  For example, when a
!  * temporary schema is cleaned out so that a new backend can use it, or when
!  * a column default is dropped as an intermediate step while adding a new one,
!  * that's an internal operation.  On the other hand, when we drop something
!  * because the user issued a DROP statement against it, that's not internal.
   */
  void
  performDeletion(const ObjectAddress *object,
--- 262,293 ----
   * according to the dependency type.
   *
   * This is the outer control routine for all forms of DROP that drop objects
!  * that can participate in dependencies.  Note that performMultipleDeletions
!  * is a variant on the same theme; if you change anything here you'll likely
!  * need to fix that too.
   *
!  * Bits in the flags argument can include:
!  *
!  * PERFORM_DELETION_INTERNAL: indicates that the drop operation is not the
!  * direct result of a user-initiated action.  For example, when a temporary
!  * schema is cleaned out so that a new backend can use it, or when a column
!  * default is dropped as an intermediate step while adding a new one, that's
!  * an internal operation.  On the other hand, when we drop something because
!  * the user issued a DROP statement against it, that's not internal. Currently
!  * this suppresses calling event triggers and making some permissions checks.
!  *
!  * PERFORM_DELETION_CONCURRENTLY: perform the drop concurrently.  This does
!  * not currently work for anything except dropping indexes; don't set it for
!  * other object types or you may get strange results.
!  *
!  * PERFORM_DELETION_QUIETLY: reduce notice messages from NOTICE to DEBUG2.
!  *
!  * PERFORM_DELETION_SKIP_ORIGINAL: do not delete the specified object(s),
!  * but only what depends on it/them.
!  *
!  * PERFORM_DELETION_SKIP_EXTENSIONS: do not delete extensions, even when
!  * deleting objects that are part of an extension.  This should generally
!  * be used only when dropping temporary objects.
   */
  void
  performDeletion(const ObjectAddress *object,
*************** performDeletion(const ObjectAddress *obj
*** 293,298 ****
--- 316,322 ----

      findDependentObjects(object,
                           DEPFLAG_ORIGINAL,
+                          flags,
                           NULL,    /* empty stack */
                           targetObjects,
                           NULL,    /* no pendingObjects */
*************** performDeletion(const ObjectAddress *obj
*** 303,309 ****
       */
      reportDependentObjects(targetObjects,
                             behavior,
!                            NOTICE,
                             object);

      /* do the deed */
--- 327,333 ----
       */
      reportDependentObjects(targetObjects,
                             behavior,
!                            flags,
                             object);

      /* do the deed */
*************** performMultipleDeletions(const ObjectAdd
*** 364,369 ****
--- 388,394 ----

          findDependentObjects(thisobj,
                               DEPFLAG_ORIGINAL,
+                              flags,
                               NULL,        /* empty stack */
                               targetObjects,
                               objects,
*************** performMultipleDeletions(const ObjectAdd
*** 378,384 ****
       */
      reportDependentObjects(targetObjects,
                             behavior,
!                            NOTICE,
                             (objects->numrefs == 1 ? objects->refs : NULL));

      /* do the deed */
--- 403,409 ----
       */
      reportDependentObjects(targetObjects,
                             behavior,
!                            flags,
                             (objects->numrefs == 1 ? objects->refs : NULL));

      /* do the deed */
*************** performMultipleDeletions(const ObjectAdd
*** 391,478 ****
  }

  /*
-  * deleteWhatDependsOn: attempt to drop everything that depends on the
-  * specified object, though not the object itself.  Behavior is always
-  * CASCADE.
-  *
-  * This is currently used only to clean out the contents of a schema
-  * (namespace): the passed object is a namespace.  We normally want this
-  * to be done silently, so there's an option to suppress NOTICE messages.
-  *
-  * Note we don't fire object drop event triggers here; it would be wrong to do
-  * so for the current only use of this function, but if more callers are added
-  * this might need to be reconsidered.
-  */
- void
- deleteWhatDependsOn(const ObjectAddress *object,
-                     bool showNotices)
- {
-     Relation    depRel;
-     ObjectAddresses *targetObjects;
-     int            i;
-
-     /*
-      * We save some cycles by opening pg_depend just once and passing the
-      * Relation pointer down to all the recursive deletion steps.
-      */
-     depRel = heap_open(DependRelationId, RowExclusiveLock);
-
-     /*
-      * Acquire deletion lock on the target object.  (Ideally the caller has
-      * done this already, but many places are sloppy about it.)
-      */
-     AcquireDeletionLock(object, 0);
-
-     /*
-      * Construct a list of objects to delete (ie, the given object plus
-      * everything directly or indirectly dependent on it).
-      */
-     targetObjects = new_object_addresses();
-
-     findDependentObjects(object,
-                          DEPFLAG_ORIGINAL,
-                          NULL,    /* empty stack */
-                          targetObjects,
-                          NULL,    /* no pendingObjects */
-                          &depRel);
-
-     /*
-      * Check if deletion is allowed, and report about cascaded deletes.
-      */
-     reportDependentObjects(targetObjects,
-                            DROP_CASCADE,
-                            showNotices ? NOTICE : DEBUG2,
-                            object);
-
-     /*
-      * Delete all the objects in the proper order, except we skip the original
-      * object.
-      */
-     for (i = 0; i < targetObjects->numrefs; i++)
-     {
-         ObjectAddress *thisobj = targetObjects->refs + i;
-         ObjectAddressExtra *thisextra = targetObjects->extras + i;
-
-         if (thisextra->flags & DEPFLAG_ORIGINAL)
-             continue;
-
-         /*
-          * Since this function is currently only used to clean out temporary
-          * schemas, we pass PERFORM_DELETION_INTERNAL here, indicating that
-          * the operation is an automatic system operation rather than a user
-          * action.  If, in the future, this function is used for other
-          * purposes, we might need to revisit this.
-          */
-         deleteOneObject(thisobj, &depRel, PERFORM_DELETION_INTERNAL);
-     }
-
-     /* And clean up */
-     free_object_addresses(targetObjects);
-
-     heap_close(depRel, RowExclusiveLock);
- }
-
- /*
   * findDependentObjects - find all objects that depend on 'object'
   *
   * For every object that depends on the starting object, acquire a deletion
--- 416,421 ----
*************** deleteWhatDependsOn(const ObjectAddress
*** 492,507 ****
   * its sub-objects too.
   *
   *    object: the object to add to targetObjects and find dependencies on
!  *    flags: flags to be ORed into the object's targetObjects entry
   *    stack: list of objects being visited in current recursion; topmost item
   *            is the object that we recursed from (NULL for external callers)
   *    targetObjects: list of objects that are scheduled to be deleted
   *    pendingObjects: list of other objects slated for destruction, but
   *            not necessarily in targetObjects yet (can be NULL if none)
   *    *depRel: already opened pg_depend relation
   */
  static void
  findDependentObjects(const ObjectAddress *object,
                       int flags,
                       ObjectAddressStack *stack,
                       ObjectAddresses *targetObjects,
--- 435,456 ----
   * its sub-objects too.
   *
   *    object: the object to add to targetObjects and find dependencies on
!  *    objflags: flags to be ORed into the object's targetObjects entry
!  *    flags: PERFORM_DELETION_xxx flags for the deletion operation as a whole
   *    stack: list of objects being visited in current recursion; topmost item
   *            is the object that we recursed from (NULL for external callers)
   *    targetObjects: list of objects that are scheduled to be deleted
   *    pendingObjects: list of other objects slated for destruction, but
   *            not necessarily in targetObjects yet (can be NULL if none)
   *    *depRel: already opened pg_depend relation
+  *
+  * Note: objflags describe the reason for visiting this particular object
+  * at this time, and are not passed down when recursing.  The flags argument
+  * is passed down, since it describes what we're doing overall.
   */
  static void
  findDependentObjects(const ObjectAddress *object,
+                      int objflags,
                       int flags,
                       ObjectAddressStack *stack,
                       ObjectAddresses *targetObjects,
*************** findDependentObjects(const ObjectAddress
*** 532,550 ****
       * auto dependency, too, if we had to.  However there are no known cases
       * where that would be necessary.
       */
!     if (stack_address_present_add_flags(object, flags, stack))
          return;

      /*
       * It's also possible that the target object has already been completely
       * processed and put into targetObjects.  If so, again we just add the
!      * specified flags to its entry and return.
       *
       * (Note: in these early-exit cases we could release the caller-taken
       * lock, since the object is presumably now locked multiple times; but it
       * seems not worth the cycles.)
       */
!     if (object_address_present_add_flags(object, flags, targetObjects))
          return;

      /*
--- 481,499 ----
       * auto dependency, too, if we had to.  However there are no known cases
       * where that would be necessary.
       */
!     if (stack_address_present_add_flags(object, objflags, stack))
          return;

      /*
       * It's also possible that the target object has already been completely
       * processed and put into targetObjects.  If so, again we just add the
!      * specified objflags to its entry and return.
       *
       * (Note: in these early-exit cases we could release the caller-taken
       * lock, since the object is presumably now locked multiple times; but it
       * seems not worth the cycles.)
       */
!     if (object_address_present_add_flags(object, objflags, targetObjects))
          return;

      /*
*************** findDependentObjects(const ObjectAddress
*** 598,603 ****
--- 547,561 ----
              case DEPENDENCY_EXTENSION:

                  /*
+                  * If told to, ignore EXTENSION dependencies altogether.  This
+                  * flag is normally used to prevent dropping extensions during
+                  * temporary-object cleanup, even if a temp object was created
+                  * during an extension script.
+                  */
+                 if (flags & PERFORM_DELETION_SKIP_EXTENSIONS)
+                     break;
+
+                 /*
                   * If the other object is the extension currently being
                   * created/altered, ignore this dependency and continue with
                   * the deletion.  This allows dropping of an extension's
*************** findDependentObjects(const ObjectAddress
*** 699,704 ****
--- 657,663 ----
                   */
                  findDependentObjects(&otherObject,
                                       DEPFLAG_REVERSE,
+                                      flags,
                                       stack,
                                       targetObjects,
                                       pendingObjects,
*************** findDependentObjects(const ObjectAddress
*** 729,735 ****
       * they have to be deleted before the current object.
       */
      mystack.object = object;    /* set up a new stack level */
!     mystack.flags = flags;
      mystack.next = stack;

      ScanKeyInit(&key[0],
--- 688,694 ----
       * they have to be deleted before the current object.
       */
      mystack.object = object;    /* set up a new stack level */
!     mystack.flags = objflags;
      mystack.next = stack;

      ScanKeyInit(&key[0],
*************** findDependentObjects(const ObjectAddress
*** 783,789 ****
              continue;
          }

!         /* Recurse, passing flags indicating the dependency type */
          switch (foundDep->deptype)
          {
              case DEPENDENCY_NORMAL:
--- 742,748 ----
              continue;
          }

!         /* Recurse, passing objflags indicating the dependency type */
          switch (foundDep->deptype)
          {
              case DEPENDENCY_NORMAL:
*************** findDependentObjects(const ObjectAddress
*** 820,825 ****
--- 779,785 ----

          findDependentObjects(&otherObject,
                               subflags,
+                              flags,
                               &mystack,
                               targetObjects,
                               pendingObjects,
*************** findDependentObjects(const ObjectAddress
*** 850,865 ****
   *
   *    targetObjects: list of objects that are scheduled to be deleted
   *    behavior: RESTRICT or CASCADE
!  *    msglevel: elog level for non-error report messages
   *    origObject: base object of deletion, or NULL if not available
   *        (the latter case occurs in DROP OWNED)
   */
  static void
  reportDependentObjects(const ObjectAddresses *targetObjects,
                         DropBehavior behavior,
!                        int msglevel,
                         const ObjectAddress *origObject)
  {
      bool        ok = true;
      StringInfoData clientdetail;
      StringInfoData logdetail;
--- 810,826 ----
   *
   *    targetObjects: list of objects that are scheduled to be deleted
   *    behavior: RESTRICT or CASCADE
!  *    flags: other flags for the deletion operation
   *    origObject: base object of deletion, or NULL if not available
   *        (the latter case occurs in DROP OWNED)
   */
  static void
  reportDependentObjects(const ObjectAddresses *targetObjects,
                         DropBehavior behavior,
!                        int flags,
                         const ObjectAddress *origObject)
  {
+     int            msglevel = (flags & PERFORM_DELETION_QUIETLY) ? DEBUG2 : NOTICE;
      bool        ok = true;
      StringInfoData clientdetail;
      StringInfoData logdetail;
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 8fd4c31..e3cfe22 100644
*** a/src/backend/catalog/namespace.c
--- b/src/backend/catalog/namespace.c
*************** RemoveTempRelations(Oid tempNamespaceId)
*** 3872,3885 ****
      /*
       * We want to get rid of everything in the target namespace, but not the
       * namespace itself (deleting it only to recreate it later would be a
!      * waste of cycles).  We do this by finding everything that has a
!      * dependency on the namespace.
       */
      object.classId = NamespaceRelationId;
      object.objectId = tempNamespaceId;
      object.objectSubId = 0;

!     deleteWhatDependsOn(&object, false);
  }

  /*
--- 3872,3890 ----
      /*
       * We want to get rid of everything in the target namespace, but not the
       * namespace itself (deleting it only to recreate it later would be a
!      * waste of cycles).  Hence, specify SKIP_ORIGINAL.  It's also an INTERNAL
!      * deletion, and we want to not drop any extensions that might happen to
!      * own temp objects.
       */
      object.classId = NamespaceRelationId;
      object.objectId = tempNamespaceId;
      object.objectSubId = 0;

!     performDeletion(&object, DROP_CASCADE,
!                     PERFORM_DELETION_INTERNAL |
!                     PERFORM_DELETION_QUIETLY |
!                     PERFORM_DELETION_SKIP_ORIGINAL |
!                     PERFORM_DELETION_SKIP_EXTENSIONS);
  }

  /*
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 6f4b96b..264298e 100644
*** a/src/backend/postmaster/autovacuum.c
--- b/src/backend/postmaster/autovacuum.c
*************** do_autovacuum(void)
*** 2218,2224 ****
          object.classId = RelationRelationId;
          object.objectId = relid;
          object.objectSubId = 0;
!         performDeletion(&object, DROP_CASCADE, PERFORM_DELETION_INTERNAL);

          /*
           * To commit the deletion, end current transaction and start a new
--- 2218,2227 ----
          object.classId = RelationRelationId;
          object.objectId = relid;
          object.objectSubId = 0;
!         performDeletion(&object, DROP_CASCADE,
!                         PERFORM_DELETION_INTERNAL |
!                         PERFORM_DELETION_QUIETLY |
!                         PERFORM_DELETION_SKIP_EXTENSIONS);

          /*
           * To commit the deletion, end current transaction and start a new
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 09b36c5..4d84a6b 100644
*** a/src/include/catalog/dependency.h
--- b/src/include/catalog/dependency.h
*************** typedef enum ObjectClass
*** 166,176 ****

  #define LAST_OCLASS        OCLASS_TRANSFORM


- /* in dependency.c */

! #define PERFORM_DELETION_INTERNAL            0x0001
! #define PERFORM_DELETION_CONCURRENTLY        0x0002

  extern void performDeletion(const ObjectAddress *object,
                  DropBehavior behavior, int flags);
--- 166,180 ----

  #define LAST_OCLASS        OCLASS_TRANSFORM

+ /* flag bits for performDeletion/performMultipleDeletions: */
+ #define PERFORM_DELETION_INTERNAL            0x0001        /* internal action */
+ #define PERFORM_DELETION_CONCURRENTLY        0x0002        /* concurrent drop */
+ #define PERFORM_DELETION_QUIETLY            0x0004        /* suppress notices */
+ #define PERFORM_DELETION_SKIP_ORIGINAL        0x0008        /* keep original obj */
+ #define PERFORM_DELETION_SKIP_EXTENSIONS    0x0010        /* keep extensions */


! /* in dependency.c */

  extern void performDeletion(const ObjectAddress *object,
                  DropBehavior behavior, int flags);
*************** extern void performDeletion(const Object
*** 178,186 ****
  extern void performMultipleDeletions(const ObjectAddresses *objects,
                           DropBehavior behavior, int flags);

- extern void deleteWhatDependsOn(const ObjectAddress *object,
-                     bool showNotices);
-
  extern void recordDependencyOnExpr(const ObjectAddress *depender,
                         Node *expr, List *rtable,
                         DependencyType behavior);
--- 182,187 ----