[HACKERS] More fun with container types

While poking at the arrays-of-domains TODO item, I found yet another area
in which the existing code fails to handle existing supported cases.
Specifically, although DefineRange will happily take domains or composites
as a range subtype, and we already noticed that domains-over-array-of-
composite are supported, find_composite_type_dependencies is innocent of
the idea that the target type might be embedded in anything but an array
or composite type.  The test cases in the attached patch show cases where
HEAD either fails to notice stored data violating a proposed new domain
constraint, or hits assertion failures or core dumps.

The patch addresses this by generalizing the existing special case for
array-over-composite so that any type that's directly dependent on the
target type (which could be its array type, or a domain or range over
it) is treated as a container type and recursively scanned for.

I believe this coding will work without any further changes for the
case of arrays over domains, but I've not fully tested that yet.

I intend to apply and back-patch this shortly, unless anyone has a
better idea about how to handle such cases.

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1b8d4b3..2afde0a 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** ATTypedTableRecursion(List **wqueue, Rel
*** 4849,4861 ****
  /*
   * find_composite_type_dependencies
   *
!  * Check to see if a composite type is being used as a column in some
!  * other table (possibly nested several levels deep in composite types!).
   * Eventually, we'd like to propagate the check or rewrite operation
!  * into other such tables, but for now, just error out if we find any.
   *
!  * Caller should provide either a table name or a type name (not both) to
!  * report in the error message, if any.
   *
   * We assume that functions and views depending on the type are not reasons
   * to reject the ALTER.  (How safe is this really?)
--- 4849,4866 ----
  /*
   * find_composite_type_dependencies
   *
!  * Check to see if the type "typeOid" is being used as a column in some table
!  * (possibly nested several levels deep in composite types, arrays, etc!).
   * Eventually, we'd like to propagate the check or rewrite operation
!  * into such tables, but for now, just error out if we find any.
   *
!  * Caller should provide either the associated relation of a rowtype,
!  * or a type name (not both) for use in the error message, if any.
!  *
!  * Note that "typeOid" is not necessarily a composite type; it could also be
!  * another container type such as an array or range, or a domain over one of
!  * these things.  The name of this function is therefore somewhat historical,
!  * but it's not worth changing.
   *
   * We assume that functions and views depending on the type are not reasons
   * to reject the ALTER.  (How safe is this really?)
*************** find_composite_type_dependencies(Oid typ
*** 4868,4878 ****
      ScanKeyData key[2];
      SysScanDesc depScan;
      HeapTuple    depTup;
!     Oid            arrayOid;

      /*
!      * We scan pg_depend to find those things that depend on the rowtype. (We
!      * assume we can ignore refobjsubid for a rowtype.)
       */
      depRel = heap_open(DependRelationId, AccessShareLock);

--- 4873,4885 ----
      ScanKeyData key[2];
      SysScanDesc depScan;
      HeapTuple    depTup;
!
!     /* since this function recurses, it could be driven to stack overflow */
!     check_stack_depth();

      /*
!      * We scan pg_depend to find those things that depend on the given type.
!      * (We assume we can ignore refobjsubid for a type.)
       */
      depRel = heap_open(DependRelationId, AccessShareLock);

*************** find_composite_type_dependencies(Oid typ
*** 4894,4901 ****
          Relation    rel;
          Form_pg_attribute att;

!         /* Ignore dependees that aren't user columns of relations */
!         /* (we assume system columns are never of rowtypes) */
          if (pg_depend->classid != RelationRelationId ||
              pg_depend->objsubid <= 0)
              continue;
--- 4901,4922 ----
          Relation    rel;
          Form_pg_attribute att;

!         /* Check for directly dependent types */
!         if (pg_depend->classid == TypeRelationId)
!         {
!             /*
!              * This must be an array, domain, or range containing the given
!              * type, so recursively check for uses of this type.  Note that
!              * any error message will mention the original type not the
!              * container; this is intentional.
!              */
!             find_composite_type_dependencies(pg_depend->objid,
!                                              origRelation, origTypeName);
!             continue;
!         }
!
!         /* Else, ignore dependees that aren't user columns of relations */
!         /* (we assume system columns are never of interesting types) */
          if (pg_depend->classid != RelationRelationId ||
              pg_depend->objsubid <= 0)
              continue;
*************** find_composite_type_dependencies(Oid typ
*** 4952,4965 ****
      systable_endscan(depScan);

      relation_close(depRel, AccessShareLock);
-
-     /*
-      * If there's an array type for the rowtype, must check for uses of it,
-      * too.
-      */
-     arrayOid = get_array_type(typeOid);
-     if (OidIsValid(arrayOid))
-         find_composite_type_dependencies(arrayOid, origRelation, origTypeName);
  }


--- 4973,4978 ----
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index c2fc59d..9bf3086 100644
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
*************** validateDomainConstraint(Oid domainoid,
*** 2787,2796 ****
   * risk by using the weakest suitable lock (ShareLock for most callers).
   *
   * XXX the API for this is not sufficient to support checking domain values
!  * that are inside composite types or arrays.  Currently we just error out
!  * if a composite type containing the target domain is stored anywhere.
!  * There are not currently arrays of domains; if there were, we could take
!  * the same approach, but it'd be nicer to fix it properly.
   *
   * Generally used for retrieving a list of tests when adding
   * new constraints to a domain.
--- 2787,2795 ----
   * risk by using the weakest suitable lock (ShareLock for most callers).
   *
   * XXX the API for this is not sufficient to support checking domain values
!  * that are inside container types, such as composite types, arrays, or
!  * ranges.  Currently we just error out if a container type containing the
!  * target domain is stored anywhere.
   *
   * Generally used for retrieving a list of tests when adding
   * new constraints to a domain.
*************** static List *
*** 2799,2804 ****
--- 2798,2804 ----
  get_rels_with_domain(Oid domainOid, LOCKMODE lockmode)
  {
      List       *result = NIL;
+     char       *domainTypeName = format_type_be(domainOid);
      Relation    depRel;
      ScanKeyData key[2];
      SysScanDesc depScan;
*************** get_rels_with_domain(Oid domainOid, LOCK
*** 2832,2851 ****
          Form_pg_attribute pg_att;
          int            ptr;

!         /* Check for directly dependent types --- must be domains */
          if (pg_depend->classid == TypeRelationId)
          {
!             Assert(get_typtype(pg_depend->objid) == TYPTYPE_DOMAIN);
!
!             /*
!              * Recursively add dependent columns to the output list.  This is
!              * a bit inefficient since we may fail to combine RelToCheck
!              * entries when attributes of the same rel have different derived
!              * domain types, but it's probably not worth improving.
!              */
!             result = list_concat(result,
!                                  get_rels_with_domain(pg_depend->objid,
!                                                       lockmode));
              continue;
          }

--- 2832,2863 ----
          Form_pg_attribute pg_att;
          int            ptr;

!         /* Check for directly dependent types */
          if (pg_depend->classid == TypeRelationId)
          {
!             if (get_typtype(pg_depend->objid) == TYPTYPE_DOMAIN)
!             {
!                 /*
!                  * This is a sub-domain, so recursively add dependent columns
!                  * to the output list.  This is a bit inefficient since we may
!                  * fail to combine RelToCheck entries when attributes of the
!                  * same rel have different derived domain types, but it's
!                  * probably not worth improving.
!                  */
!                 result = list_concat(result,
!                                      get_rels_with_domain(pg_depend->objid,
!                                                           lockmode));
!             }
!             else
!             {
!                 /*
!                  * Otherwise, it is some container type using the domain, so
!                  * fail if there are any columns of this type.
!                  */
!                 find_composite_type_dependencies(pg_depend->objid,
!                                                  NULL,
!                                                  domainTypeName);
!             }
              continue;
          }

*************** get_rels_with_domain(Oid domainOid, LOCK
*** 2882,2888 ****
              if (OidIsValid(rel->rd_rel->reltype))
                  find_composite_type_dependencies(rel->rd_rel->reltype,
                                                   NULL,
!                                                  format_type_be(domainOid));

              /*
               * Otherwise, we can ignore relations except those with both
--- 2894,2900 ----
              if (OidIsValid(rel->rd_rel->reltype))
                  find_composite_type_dependencies(rel->rd_rel->reltype,
                                                   NULL,
!                                                  domainTypeName);

              /*
               * Otherwise, we can ignore relations except those with both
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 5fe999e..3acc696 100644
*** a/src/test/regress/expected/domain.out
--- b/src/test/regress/expected/domain.out
*************** insert into ddtest2 values(row(-1));
*** 661,671 ****
--- 661,688 ----
  alter domain posint add constraint c1 check(value >= 0);
  ERROR:  cannot alter type "posint" because column "ddtest2.f1" uses it
  drop table ddtest2;
+ -- Likewise for domains within arrays of composite
  create table ddtest2(f1 ddtest1[]);
  insert into ddtest2 values('{(-1)}');
  alter domain posint add constraint c1 check(value >= 0);
  ERROR:  cannot alter type "posint" because column "ddtest2.f1" uses it
  drop table ddtest2;
+ -- Likewise for domains within domains over array of composite
+ create domain ddtest1d as ddtest1[];
+ create table ddtest2(f1 ddtest1d);
+ insert into ddtest2 values('{(-1)}');
+ alter domain posint add constraint c1 check(value >= 0);
+ ERROR:  cannot alter type "posint" because column "ddtest2.f1" uses it
+ drop table ddtest2;
+ drop domain ddtest1d;
+ -- Doesn't work for ranges, either
+ create type rposint as range (subtype = posint);
+ create table ddtest2(f1 rposint);
+ insert into ddtest2 values('(-1,3]');
+ alter domain posint add constraint c1 check(value >= 0);
+ ERROR:  cannot alter type "posint" because column "ddtest2.f1" uses it
+ drop table ddtest2;
+ drop type rposint;
  alter domain posint add constraint c1 check(value >= 0);
  create domain posint2 as posint check (value % 2 = 0);
  create table ddtest2(f1 posint2);
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index 5ec128d..0fd383e 100644
*** a/src/test/regress/sql/domain.sql
--- b/src/test/regress/sql/domain.sql
*************** insert into ddtest2 values(row(-1));
*** 451,461 ****
--- 451,478 ----
  alter domain posint add constraint c1 check(value >= 0);
  drop table ddtest2;

+ -- Likewise for domains within arrays of composite
  create table ddtest2(f1 ddtest1[]);
  insert into ddtest2 values('{(-1)}');
  alter domain posint add constraint c1 check(value >= 0);
  drop table ddtest2;

+ -- Likewise for domains within domains over array of composite
+ create domain ddtest1d as ddtest1[];
+ create table ddtest2(f1 ddtest1d);
+ insert into ddtest2 values('{(-1)}');
+ alter domain posint add constraint c1 check(value >= 0);
+ drop table ddtest2;
+ drop domain ddtest1d;
+
+ -- Doesn't work for ranges, either
+ create type rposint as range (subtype = posint);
+ create table ddtest2(f1 rposint);
+ insert into ddtest2 values('(-1,3]');
+ alter domain posint add constraint c1 check(value >= 0);
+ drop table ddtest2;
+ drop type rposint;
+
  alter domain posint add constraint c1 check(value >= 0);

  create domain posint2 as posint check (value % 2 = 0);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers