Re: Column/type dependency recording inconsistencies - Mailing list pgsql-hackers

I wrote:
> ... Also, I find physically removing an entry fairly ugly and
> possibly unsafe, since it's not clear what outer recursion levels might be
> assuming about the contents of the array -- and that certainly won't work
> for the stack case.  What seems better is to invent another flag bit
> to indicate that we no longer need to physically delete this subobject
> because we've discovered the whole object will be deleted.  Then we
> can just skip it during the execution phase.

On further reflection, neither of those approaches is really a good idea.
With either implementation, skipping the DROP COLUMN step would mean that
we're letting the DROP TYPE happen before we drop a table containing a
live column of that type.  While that failed to fail in this particular
example, it sounds like a really bad idea --- any number of things, such
as event triggers, could well blow up when abused like that.  The lack of
prior reports means that this is a very rare case, so rather than trying
to "optimize" it, I think we should just let the drops happen in the
scheduled order.  All that we need is to make sure that the column gets
marked with flags indicating that dropping is allowed.  Accordingly, the
attached patch should do it.

            regards, tom lane

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 256486c..f338acf 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** object_address_present_add_flags(const O
*** 2116,2121 ****
--- 2116,2122 ----
                                   int flags,
                                   ObjectAddresses *addrs)
  {
+     bool        result = false;
      int            i;

      for (i = addrs->numrefs - 1; i >= 0; i--)
*************** object_address_present_add_flags(const O
*** 2130,2151 ****
                  ObjectAddressExtra *thisextra = addrs->extras + i;

                  thisextra->flags |= flags;
!                 return true;
              }
!             if (thisobj->objectSubId == 0)
              {
                  /*
                   * We get here if we find a need to delete a column after
                   * having already decided to drop its whole table.  Obviously
!                  * we no longer need to drop the column.  But don't plaster
!                  * its flags on the table.
                   */
!                 return true;
              }
          }
      }

!     return false;
  }

  /*
--- 2131,2178 ----
                  ObjectAddressExtra *thisextra = addrs->extras + i;

                  thisextra->flags |= flags;
!                 result = true;
              }
!             else if (thisobj->objectSubId == 0)
              {
                  /*
                   * We get here if we find a need to delete a column after
                   * having already decided to drop its whole table.  Obviously
!                  * we no longer need to drop the subobject, so report that we
!                  * found the subobject in the array.  But don't plaster its
!                  * flags on the whole object.
                   */
!                 result = true;
!             }
!             else if (object->objectSubId == 0)
!             {
!                 /*
!                  * We get here if we find a need to delete a whole table after
!                  * having already decided to drop one of its columns.  We
!                  * can't report that the whole object is in the array, but we
!                  * should mark the subobject with the whole object's flags.
!                  *
!                  * It might seem attractive to physically delete the column's
!                  * array entry, or at least mark it as no longer needing
!                  * separate deletion.  But that could lead to, e.g., dropping
!                  * the column's datatype before we drop the table, which does
!                  * not seem like a good idea.  This is a very rare situation
!                  * in practice, so we just take the hit of doing a separate
!                  * DROP COLUMN action even though we know we're gonna delete
!                  * the table later.
!                  *
!                  * Because there could be other subobjects of this object in
!                  * the array, this case means we always have to loop through
!                  * the whole array; we cannot exit early on a match.
!                  */
!                 ObjectAddressExtra *thisextra = addrs->extras + i;
!
!                 thisextra->flags |= flags;
              }
          }
      }

!     return result;
  }

  /*
*************** stack_address_present_add_flags(const Ob
*** 2156,2161 ****
--- 2183,2189 ----
                                  int flags,
                                  ObjectAddressStack *stack)
  {
+     bool        result = false;
      ObjectAddressStack *stackptr;

      for (stackptr = stack; stackptr; stackptr = stackptr->next)
*************** stack_address_present_add_flags(const Ob
*** 2168,2188 ****
              if (object->objectSubId == thisobj->objectSubId)
              {
                  stackptr->flags |= flags;
!                 return true;
              }
-
-             /*
-              * Could visit column with whole table already on stack; this is
-              * the same case noted in object_address_present_add_flags(), and
-              * as in that case, we don't propagate flags for the component to
-              * the whole object.
-              */
-             if (thisobj->objectSubId == 0)
-                 return true;
          }
      }

!     return false;
  }

  /*
--- 2196,2226 ----
              if (object->objectSubId == thisobj->objectSubId)
              {
                  stackptr->flags |= flags;
!                 result = true;
!             }
!             else if (thisobj->objectSubId == 0)
!             {
!                 /*
!                  * We're visiting a column with whole table already on stack.
!                  * As in object_address_present_add_flags(), we can skip
!                  * further processing of the subobject, but we don't want to
!                  * propagate flags for the subobject to the whole object.
!                  */
!                 result = true;
!             }
!             else if (object->objectSubId == 0)
!             {
!                 /*
!                  * We're visiting a table with column already on stack.  As in
!                  * object_address_present_add_flags(), we should propagate
!                  * flags for the whole object to each of its subobjects.
!                  */
!                 stackptr->flags |= flags;
              }
          }
      }

!     return result;
  }

  /*