Code review for LIKE INCLUDING CONSTRAINTS patch --- improve comments,
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 11 Oct 2006 16:42:59 +0000 (16:42 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 11 Oct 2006 16:42:59 +0000 (16:42 +0000)
don't cheat on the raw-vs-cooked status of a constraint.

src/backend/commands/tablecmds.c
src/backend/parser/analyze.c
src/include/nodes/parsenodes.h

index 9ad11b6f329f693345808fca60e282b81068aab5..631b02e0fcbf8beb50e2df8790c0132126d9d764 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.204 2006/10/06 17:13:59 petere Exp $
+ *   $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.205 2006/10/11 16:42:58 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -163,6 +163,8 @@ static List *MergeAttributes(List *schema, List *supers, bool istemp,
                List **supOids, List **supconstr, int *supOidCount);
 static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
 static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
+static void add_nonduplicate_constraint(Constraint *cdef,
+                                       ConstrCheck *check, int *ncheck);
 static bool change_varattnos_walker(Node *node, const AttrNumber *newattno);
 static void StoreCatalogInheritance(Oid relationId, List *supers);
 static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
@@ -285,7 +287,6 @@ DefineRelation(CreateStmt *stmt, char relkind)
    List       *rawDefaults;
    Datum       reloptions;
    ListCell   *listptr;
-   int         i;
    AttrNumber  attnum;
 
    /*
@@ -378,49 +379,35 @@ DefineRelation(CreateStmt *stmt, char relkind)
    localHasOids = interpretOidsOption(stmt->options);
    descriptor->tdhasoid = (localHasOids || parentOidCount > 0);
 
-   if (old_constraints != NIL)
+   if (old_constraints || stmt->constraints)
    {
-       ConstrCheck *check = (ConstrCheck *)
-       palloc0(list_length(old_constraints) * sizeof(ConstrCheck));
+       ConstrCheck *check;
        int         ncheck = 0;
 
+       /* make array that's certainly big enough */
+       check = (ConstrCheck *)
+           palloc((list_length(old_constraints) +
+                   list_length(stmt->constraints)) * sizeof(ConstrCheck));
+       /* deal with constraints from MergeAttributes */
        foreach(listptr, old_constraints)
        {
            Constraint *cdef = (Constraint *) lfirst(listptr);
-           bool        dup = false;
 
-           if (cdef->contype != CONSTR_CHECK)
-               continue;
-           Assert(cdef->name != NULL);
-           Assert(cdef->raw_expr == NULL && cdef->cooked_expr != NULL);
+           if (cdef->contype == CONSTR_CHECK)
+               add_nonduplicate_constraint(cdef, check, &ncheck);
+       }
+       /*
+        * analyze.c might have passed some precooked constraints too,
+        * due to LIKE tab INCLUDING CONSTRAINTS
+        */
+       foreach(listptr, stmt->constraints)
+       {
+           Constraint *cdef = (Constraint *) lfirst(listptr);
 
-           /*
-            * In multiple-inheritance situations, it's possible to inherit
-            * the same grandparent constraint through multiple parents.
-            * Hence, discard inherited constraints that match as to both name
-            * and expression.  Otherwise, gripe if the names conflict.
-            */
-           for (i = 0; i < ncheck; i++)
-           {
-               if (strcmp(check[i].ccname, cdef->name) != 0)
-                   continue;
-               if (strcmp(check[i].ccbin, cdef->cooked_expr) == 0)
-               {
-                   dup = true;
-                   break;
-               }
-               ereport(ERROR,
-                       (errcode(ERRCODE_DUPLICATE_OBJECT),
-                        errmsg("duplicate check constraint name \"%s\"",
-                               cdef->name)));
-           }
-           if (!dup)
-           {
-               check[ncheck].ccname = cdef->name;
-               check[ncheck].ccbin = pstrdup(cdef->cooked_expr);
-               ncheck++;
-           }
+           if (cdef->contype == CONSTR_CHECK && cdef->cooked_expr != NULL)
+               add_nonduplicate_constraint(cdef, check, &ncheck);
        }
+       /* if we found any, insert 'em into the descriptor */
        if (ncheck > 0)
        {
            if (descriptor->constr == NULL)
@@ -1118,66 +1105,57 @@ MergeAttributes(List *schema, List *supers, bool istemp,
    return schema;
 }
 
+
 /*
- * Varattnos of pg_constraint.conbin must be rewritten when subclasses inherit
- * constraints from parent classes, since the inherited attributes could
- * be given different column numbers in multiple-inheritance cases.
- *
- * Note that the passed node tree is modified in place!
- *
- * This function is used elsewhere such as in analyze.c
- *
+ * In multiple-inheritance situations, it's possible to inherit
+ * the same grandparent constraint through multiple parents.
+ * Hence, we want to discard inherited constraints that match as to
+ * both name and expression.  Otherwise, gripe if there are conflicting
+ * names.  Nonconflicting constraints are added to the array check[]
+ * of length *ncheck ... caller must ensure there is room!
  */
-
-void
-change_varattnos_of_a_node(Node *node, const AttrNumber *newattno)
+static void
+add_nonduplicate_constraint(Constraint *cdef, ConstrCheck *check, int *ncheck)
 {
-   change_varattnos_walker(node, newattno);
-}
-
-/* Generate a map for change_varattnos_of_a_node from two tupledesc's. */
+   int         i;
 
-AttrNumber *
-varattnos_map(TupleDesc old, TupleDesc new)
-{
-   int         i,
-               j;
-   AttrNumber *attmap = palloc0(sizeof(AttrNumber) * old->natts);
+   /* Should only see precooked constraints here */
+   Assert(cdef->contype == CONSTR_CHECK);
+   Assert(cdef->name != NULL);
+   Assert(cdef->raw_expr == NULL && cdef->cooked_expr != NULL);
 
-   for (i = 1; i <= old->natts; i++)
+   for (i = 0; i < *ncheck; i++)
    {
-       if (old->attrs[i - 1]->attisdropped)
-       {
-           attmap[i - 1] = 0;
+       if (strcmp(check[i].ccname, cdef->name) != 0)
            continue;
-       }
-       for (j = 1; j <= new->natts; j++)
-           if (!strcmp(NameStr(old->attrs[i - 1]->attname), NameStr(new->attrs[j - 1]->attname)))
-               attmap[i - 1] = j;
+       if (strcmp(check[i].ccbin, cdef->cooked_expr) == 0)
+           return;             /* duplicate constraint, so ignore it */
+       ereport(ERROR,
+               (errcode(ERRCODE_DUPLICATE_OBJECT),
+                errmsg("duplicate check constraint name \"%s\"",
+                       cdef->name)));
    }
-   return attmap;
+   /* No match on name, so add it to array */
+   check[*ncheck].ccname = cdef->name;
+   check[*ncheck].ccbin = pstrdup(cdef->cooked_expr);
+   (*ncheck)++;
 }
 
+
 /*
- * Generate a map for change_varattnos_of_a_node from a tupledesc and a list of
- * ColumnDefs
+ * Replace varattno values in an expression tree according to the given
+ * map array, that is, varattno N is replaced by newattno[N-1].  It is
+ * caller's responsibility to ensure that the array is long enough to
+ * define values for all user varattnos present in the tree.  System column
+ * attnos remain unchanged.
+ *
+ * Note that the passed node tree is modified in-place!
  */
-AttrNumber *
-varattnos_map_schema(TupleDesc old, List *schema)
+void
+change_varattnos_of_a_node(Node *node, const AttrNumber *newattno)
 {
-   int         i;
-   AttrNumber *attmap = palloc0(sizeof(AttrNumber) * old->natts);
-
-   for (i = 1; i <= old->natts; i++)
-   {
-       if (old->attrs[i - 1]->attisdropped)
-       {
-           attmap[i - 1] = 0;
-           continue;
-       }
-       attmap[i - 1] = findAttrByName(NameStr(old->attrs[i - 1]->attname), schema);
-   }
-   return attmap;
+   /* no setup needed, so away we go */
+   (void) change_varattnos_walker(node, newattno);
 }
 
 static bool
@@ -1206,6 +1184,59 @@ change_varattnos_walker(Node *node, const AttrNumber *newattno)
                                  (void *) newattno);
 }
 
+/*
+ * Generate a map for change_varattnos_of_a_node from old and new TupleDesc's,
+ * matching according to column name.
+ */
+AttrNumber *
+varattnos_map(TupleDesc old, TupleDesc new)
+{
+   AttrNumber *attmap;
+   int         i,
+               j;
+
+   attmap = (AttrNumber *) palloc0(sizeof(AttrNumber) * old->natts);
+   for (i = 1; i <= old->natts; i++)
+   {
+       if (old->attrs[i - 1]->attisdropped)
+           continue;           /* leave the entry as zero */
+
+       for (j = 1; j <= new->natts; j++)
+       {
+           if (strcmp(NameStr(old->attrs[i - 1]->attname),
+                      NameStr(new->attrs[j - 1]->attname)) == 0)
+           {
+               attmap[i - 1] = j;
+               break;
+           }
+       }
+   }
+   return attmap;
+}
+
+/*
+ * Generate a map for change_varattnos_of_a_node from a TupleDesc and a list
+ * of ColumnDefs
+ */
+AttrNumber *
+varattnos_map_schema(TupleDesc old, List *schema)
+{
+   AttrNumber *attmap;
+   int         i;
+
+   attmap = (AttrNumber *) palloc0(sizeof(AttrNumber) * old->natts);
+   for (i = 1; i <= old->natts; i++)
+   {
+       if (old->attrs[i - 1]->attisdropped)
+           continue;           /* leave the entry as zero */
+
+       attmap[i - 1] = findAttrByName(NameStr(old->attrs[i - 1]->attname),
+                                      schema);
+   }
+   return attmap;
+}
+
+
 /*
  * StoreCatalogInheritance
  *     Updates the system catalogs with proper inheritance information.
index f4e58a3b8d148b72e638669c5314b29531d3911e..0df3bec1694722312612e85d6eca35adf2a07f40 100644 (file)
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.352 2006/10/04 00:29:55 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.353 2006/10/11 16:42:59 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1286,12 +1286,10 @@ transformInhRelation(ParseState *pstate, CreateStmtContext *cxt,
                     InhRelation *inhRelation)
 {
    AttrNumber  parent_attno;
-
    Relation    relation;
    TupleDesc   tupleDesc;
    TupleConstr *constr;
    AclResult   aclresult;
-
    bool        including_defaults = false;
    bool        including_constraints = false;
    bool        including_indexes = false;
@@ -1342,15 +1340,18 @@ transformInhRelation(ParseState *pstate, CreateStmtContext *cxt,
                including_indexes = false;
                break;
            default:
-               elog(ERROR, "unrecognized CREATE TABLE LIKE option: %d", option);
+               elog(ERROR, "unrecognized CREATE TABLE LIKE option: %d",
+                    option);
        }
    }
 
    if (including_indexes)
-       elog(ERROR, "TODO");
+       ereport(ERROR,
+               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                errmsg("LIKE INCLUDING INDEXES is not implemented")));
 
    /*
-    * Insert the inherited attributes into the cxt for the new table
+    * Insert the copied attributes into the cxt for the new table
     * definition.
     */
    for (parent_attno = 1; parent_attno <= tupleDesc->natts;
@@ -1367,7 +1368,7 @@ transformInhRelation(ParseState *pstate, CreateStmtContext *cxt,
            continue;
 
        /*
-        * Create a new inherited column.
+        * Create a new column, which is marked as NOT inherited.
         *
         * For constraints, ONLY the NOT NULL constraint is inherited by the
         * new column definition per SQL99.
@@ -1389,7 +1390,7 @@ transformInhRelation(ParseState *pstate, CreateStmtContext *cxt,
        cxt->columns = lappend(cxt->columns, def);
 
        /*
-        * Copy default if any, and the default has been requested
+        * Copy default, if present and the default has been requested
         */
        if (attribute->atthasdef && including_defaults)
        {
@@ -1419,10 +1420,14 @@ transformInhRelation(ParseState *pstate, CreateStmtContext *cxt,
        }
    }
 
+   /*
+    * Copy CHECK constraints if requested, being careful to adjust
+    * attribute numbers
+    */
    if (including_constraints && tupleDesc->constr)
    {
-       int         ccnum;
        AttrNumber *attmap = varattnos_map_schema(tupleDesc, cxt->columns);
+       int         ccnum;
 
        for (ccnum = 0; ccnum < tupleDesc->constr->num_check; ccnum++)
        {
@@ -1435,8 +1440,8 @@ transformInhRelation(ParseState *pstate, CreateStmtContext *cxt,
 
            n->contype = CONSTR_CHECK;
            n->name = pstrdup(ccname);
-           n->raw_expr = ccbin_node;
-           n->cooked_expr = NULL;
+           n->raw_expr = NULL;
+           n->cooked_expr = nodeToString(ccbin_node);
            n->indexspace = NULL;
            cxt->ckconstraints = lappend(cxt->ckconstraints, (Node *) n);
        }
index 29f68f7fff3d453496fe497b2d6fcaf57d61562e..8afe97cb1e246d497663a82f1228cdd7ed6ce0a8 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/nodes/parsenodes.h,v 1.331 2006/10/04 00:30:09 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/nodes/parsenodes.h,v 1.332 2006/10/11 16:42:59 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -414,9 +414,19 @@ typedef struct InhRelation
 {
    NodeTag     type;
    RangeVar   *relation;
-   List       *options;
+   List       *options;        /* integer List of CreateStmtLikeOption */
 } InhRelation;
 
+typedef enum CreateStmtLikeOption
+{
+   CREATE_TABLE_LIKE_INCLUDING_DEFAULTS,
+   CREATE_TABLE_LIKE_EXCLUDING_DEFAULTS,
+   CREATE_TABLE_LIKE_INCLUDING_CONSTRAINTS,
+   CREATE_TABLE_LIKE_EXCLUDING_CONSTRAINTS,
+   CREATE_TABLE_LIKE_INCLUDING_INDEXES,
+   CREATE_TABLE_LIKE_EXCLUDING_INDEXES
+} CreateStmtLikeOption;
+
 /*
  * IndexElem - index parameters (used in CREATE INDEX)
  *
@@ -1055,16 +1065,6 @@ typedef struct CreateStmt
    char       *tablespacename; /* table space to use, or NULL */
 } CreateStmt;
 
-typedef enum CreateStmtLikeOption
-{
-   CREATE_TABLE_LIKE_INCLUDING_DEFAULTS,
-   CREATE_TABLE_LIKE_EXCLUDING_DEFAULTS,
-   CREATE_TABLE_LIKE_INCLUDING_CONSTRAINTS,
-   CREATE_TABLE_LIKE_EXCLUDING_CONSTRAINTS,
-   CREATE_TABLE_LIKE_INCLUDING_INDEXES,
-   CREATE_TABLE_LIKE_EXCLUDING_INDEXES
-} CreateStmtLikeOption;
-
 /* ----------
  * Definitions for plain (non-FOREIGN KEY) constraints in CreateStmt
  *