Remove the hack in the grammar that "optimized away" DEFAULT NULL clauses.

Instead put in a test to drop a NULL default at the last moment before
storing the catalog entry.  This changes the behavior in a couple of ways:
* Specifying DEFAULT NULL when creating an inheritance child table will
  successfully suppress inheritance of any default expression from the
  parent's column, where formerly it failed to do so.
* Specifying DEFAULT NULL for a column of a domain type will correctly
  override any default belonging to the domain; likewise for a sub-domain.
The latter change happens because by the time the clause is checked,
it won't be a simple null Const but a CoerceToDomain expression.

Personally I think this should be back-patched, but there doesn't seem to
be consensus for that on pgsql-hackers, so refraining.

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index ea985a635ddcca93f00231fe3e4b47e50c253380..d22bb77a50c60389144b61f40961a28649ab37f0 100644 (file)
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/catalog/heap.c,v 1.324 2007/10/12 18:55:11 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/catalog/heap.c,v 1.325 2007/10/29 19:40:39 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -1722,6 +1722,21 @@ AddRelationRawConstraints(Relation rel,
                           atp->atttypid, atp->atttypmod,
                           NameStr(atp->attname));
 
+       /*
+        * If the expression is just a NULL constant, we do not bother
+        * to make an explicit pg_attrdef entry, since the default behavior
+        * is equivalent.
+        *
+        * Note a nonobvious property of this test: if the column is of a
+        * domain type, what we'll get is not a bare null Const but a
+        * CoerceToDomain expr, so we will not discard the default.  This is
+        * critical because the column default needs to be retained to
+        * override any default that the domain might have.
+        */
+       if (expr == NULL ||
+           (IsA(expr, Const) && ((Const *) expr)->constisnull))
+           continue;
+
        StoreAttrDefault(rel, colDef->attnum, nodeToString(expr));
 
        cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));

diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 75a8b7530e8d687070e754f96e3c9e79a2bad1b3..1f58d989f266221a3090ca80bf29b692e43c6fc0 100644 (file)
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/commands/typecmds.c,v 1.108 2007/09/29 17:18:58 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/commands/typecmds.c,v 1.109 2007/10/29 19:40:39 tgl Exp $
  *
  * DESCRIPTION
  *   The "DefineFoo" routines take the parse tree and pick out the
@@ -765,20 +765,40 @@ DefineDomain(CreateDomainStmt *stmt)
                                              domainName);
 
                    /*
-                    * Expression must be stored as a nodeToString result, but
-                    * we also require a valid textual representation (mainly
-                    * to make life easier for pg_dump).
+                    * If the expression is just a NULL constant, we treat
+                    * it like not having a default.
+                    *
+                    * Note that if the basetype is another domain, we'll see
+                    * a CoerceToDomain expr here and not discard the default.
+                    * This is critical because the domain default needs to be
+                    * retained to override any default that the base domain
+                    * might have.
                     */
-                   defaultValue =
-                       deparse_expression(defaultExpr,
-                                          deparse_context_for(domainName,
-                                                              InvalidOid),
-                                          false, false);
-                   defaultValueBin = nodeToString(defaultExpr);
+                   if (defaultExpr == NULL ||
+                       (IsA(defaultExpr, Const) &&
+                        ((Const *) defaultExpr)->constisnull))
+                   {
+                       defaultValue = NULL;
+                       defaultValueBin = NULL;
+                   }
+                   else
+                   {
+                       /*
+                        * Expression must be stored as a nodeToString result,
+                        * but we also require a valid textual representation
+                        * (mainly to make life easier for pg_dump).
+                        */
+                       defaultValue =
+                           deparse_expression(defaultExpr,
+                                              deparse_context_for(domainName,
+                                                                  InvalidOid),
+                                              false, false);
+                       defaultValueBin = nodeToString(defaultExpr);
+                   }
                }
                else
                {
-                   /* DEFAULT NULL is same as not having a default */
+                   /* No default (can this still happen?) */
                    defaultValue = NULL;
                    defaultValueBin = NULL;
                }
@@ -1443,7 +1463,7 @@ AlterDomainDefault(List *names, Node *defaultRaw)
    MemSet(new_record_nulls, ' ', sizeof(new_record_nulls));
    MemSet(new_record_repl, ' ', sizeof(new_record_repl));
 
-   /* Store the new default, if null then skip this step */
+   /* Store the new default into the tuple */
    if (defaultRaw)
    {
        /* Create a dummy ParseState for transformExpr */
@@ -1459,30 +1479,46 @@ AlterDomainDefault(List *names, Node *defaultRaw)
                                  NameStr(typTup->typname));
 
        /*
-        * Expression must be stored as a nodeToString result, but we also
-        * require a valid textual representation (mainly to make life easier
-        * for pg_dump).
+        * If the expression is just a NULL constant, we treat the command
+        * like ALTER ... DROP DEFAULT.  (But see note for same test in
+        * DefineDomain.)
         */
-       defaultValue = deparse_expression(defaultExpr,
+       if (defaultExpr == NULL ||
+           (IsA(defaultExpr, Const) && ((Const *) defaultExpr)->constisnull))
+       {
+           /* Default is NULL, drop it */
+           new_record_nulls[Anum_pg_type_typdefaultbin - 1] = 'n';
+           new_record_repl[Anum_pg_type_typdefaultbin - 1] = 'r';
+           new_record_nulls[Anum_pg_type_typdefault - 1] = 'n';
+           new_record_repl[Anum_pg_type_typdefault - 1] = 'r';
+       }
+       else
+       {
+           /*
+            * Expression must be stored as a nodeToString result, but we also
+            * require a valid textual representation (mainly to make life
+            * easier for pg_dump).
+            */
+           defaultValue = deparse_expression(defaultExpr,
                                deparse_context_for(NameStr(typTup->typname),
                                                    InvalidOid),
                                          false, false);
 
-       /*
-        * Form an updated tuple with the new default and write it back.
-        */
-       new_record[Anum_pg_type_typdefaultbin - 1] = DirectFunctionCall1(textin,
-                                                            CStringGetDatum(
-                                                nodeToString(defaultExpr)));
+           /*
+            * Form an updated tuple with the new default and write it back.
+            */
+           new_record[Anum_pg_type_typdefaultbin - 1] = DirectFunctionCall1(textin,
+                               CStringGetDatum(nodeToString(defaultExpr)));
 
-       new_record_repl[Anum_pg_type_typdefaultbin - 1] = 'r';
-       new_record[Anum_pg_type_typdefault - 1] = DirectFunctionCall1(textin,
+           new_record_repl[Anum_pg_type_typdefaultbin - 1] = 'r';
+           new_record[Anum_pg_type_typdefault - 1] = DirectFunctionCall1(textin,
                                              CStringGetDatum(defaultValue));
-       new_record_repl[Anum_pg_type_typdefault - 1] = 'r';
+           new_record_repl[Anum_pg_type_typdefault - 1] = 'r';
+       }
    }
    else
-       /* Default is NULL, drop it */
    {
+       /* ALTER ... DROP DEFAULT */
        new_record_nulls[Anum_pg_type_typdefaultbin - 1] = 'n';
        new_record_repl[Anum_pg_type_typdefaultbin - 1] = 'r';
        new_record_nulls[Anum_pg_type_typdefault - 1] = 'n';

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 1452fe7ff2b3859bc2622b17148ec7f0431a2374..f9f7a49a312bf99193f90cfa89a2c0ba293a44bb 100644 (file)
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11,7 +11,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/parser/gram.y,v 2.603 2007/09/24 01:29:28 adunstan Exp $
+ *   $PostgreSQL: pgsql/src/backend/parser/gram.y,v 2.604 2007/10/29 19:40:39 tgl Exp $
  *
  * HISTORY
  *   AUTHOR            DATE            MAJOR EVENT
@@ -1685,14 +1685,7 @@ alter_rel_cmd:
        ;
 
 alter_column_default:
-           SET DEFAULT a_expr
-               {
-                   /* Treat SET DEFAULT NULL the same as DROP DEFAULT */
-                   if (exprIsNullConstant($3))
-                       $$ = NULL;
-                   else
-                       $$ = $3;
-               }
+           SET DEFAULT a_expr          { $$ = $3; }
            | DROP DEFAULT              { $$ = NULL; }
        ;
 
@@ -2080,15 +2073,7 @@ ColConstraintElem:
                    Constraint *n = makeNode(Constraint);
                    n->contype = CONSTR_DEFAULT;
                    n->name = NULL;
-                   if (exprIsNullConstant($2))
-                   {
-                       /* DEFAULT NULL should be reported as empty expr */
-                       n->raw_expr = NULL;
-                   }
-                   else
-                   {
-                       n->raw_expr = $2;
-                   }
+                   n->raw_expr = $2;
                    n->cooked_expr = NULL;
                    n->keys = NULL;
                    n->indexspace = NULL;
@@ -9763,23 +9748,6 @@ parser_init(void)
    QueryIsRule = FALSE;
 }
 
-/* exprIsNullConstant()
- * Test whether an a_expr is a plain NULL constant or not.
- */
-bool
-exprIsNullConstant(Node *arg)
-{
-   if (arg && IsA(arg, A_Const))
-   {
-       A_Const *con = (A_Const *) arg;
-
-       if (con->val.type == T_Null &&
-           con->typename == NULL)
-           return TRUE;
-   }
-   return FALSE;
-}
-
 /* doNegate()
  * Handle negation of a numeric constant.
  *

diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 754e18d687cc4ec78588d7d2c960a9cddd079f7d..86fddc4a7a64ee1922738e16746395248ee021f4 100644 (file)
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.221 2007/06/23 22:12:51 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.222 2007/10/29 19:40:40 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -606,6 +606,21 @@ transformParamRef(ParseState *pstate, ParamRef *pref)
    return (Node *) param;
 }
 
+/* Test whether an a_expr is a plain NULL constant or not */
+static bool
+exprIsNullConstant(Node *arg)
+{
+   if (arg && IsA(arg, A_Const))
+   {
+       A_Const *con = (A_Const *) arg;
+
+       if (con->val.type == T_Null &&
+           con->typename == NULL)
+           return true;
+   }
+   return false;
+}
+
 static Node *
 transformAExprOp(ParseState *pstate, A_Expr *a)
 {

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 54e7dfdb161c27138489cabfb02bace8b2300f9c..287e82ddeec10baecdb72da4e74f8e0b3570ec8e 100644 (file)
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -19,7 +19,7 @@
  * Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/backend/parser/parse_utilcmd.c,v 2.3 2007/08/27 03:36:08 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/parser/parse_utilcmd.c,v 2.4 2007/10/29 19:40:40 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -440,7 +440,6 @@ transformColumnDefinition(ParseState *pstate, CreateStmtContext *cxt,
                            (errcode(ERRCODE_SYNTAX_ERROR),
                             errmsg("multiple default values specified for column \"%s\" of table \"%s\"",
                                  column->colname, cxt->relation->relname)));
-               /* Note: DEFAULT NULL maps to constraint->raw_expr == NULL */
                column->raw_default = constraint->raw_expr;
                Assert(constraint->cooked_expr == NULL);
                saw_default = true;

diff --git a/src/include/parser/gramparse.h b/src/include/parser/gramparse.h
index ee6be87c02f30c0bdbca8846c25d8084ae83ac29..34f47a9bea683386e465696dd2a6606c28bdfbf5 100644 (file)
--- a/src/include/parser/gramparse.h
+++ b/src/include/parser/gramparse.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/parser/gramparse.h,v 1.38 2007/01/05 22:19:56 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/parser/gramparse.h,v 1.39 2007/10/29 19:40:40 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -54,6 +54,5 @@ extern void parser_init(void);
 extern int base_yyparse(void);
 extern List *SystemFuncName(char *name);
 extern TypeName *SystemTypeName(char *name);
-extern bool exprIsNullConstant(Node *arg);
 
 #endif   /* GRAMPARSE_H */

diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index c7d5b50334537ccd1db499bb75b4d3b0eb1a1e6d..b951ce8caa8752e36a0e52a38437ad590267aaa1 100644 (file)
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -205,7 +205,15 @@ create table defaulttest
             , col8 ddef5
             );
 NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "defaulttest_pkey" for table "defaulttest"
-insert into defaulttest default values;
+insert into defaulttest(col4) values(0); -- fails, col5 defaults to null
+ERROR:  null value in column "col5" violates not-null constraint
+alter table defaulttest alter column col5 drop default;
+insert into defaulttest default values; -- succeeds, inserts domain default
+-- We used to treat SET DEFAULT NULL as equivalent to DROP DEFAULT; wrong
+alter table defaulttest alter column col5 set default null;
+insert into defaulttest(col4) values(0); -- fails
+ERROR:  null value in column "col5" violates not-null constraint
+alter table defaulttest alter column col5 drop default;
 insert into defaulttest default values;
 insert into defaulttest default values;
 -- Test defaults with copy

diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index 7da99719911348c134f4fe58c73e711d0f896da6..7a4ec383d20cac750f67d0dc54c3eb22cb7ae782 100644 (file)
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -168,7 +168,13 @@ create table defaulttest
             , col7 ddef4 DEFAULT 8000
             , col8 ddef5
             );
-insert into defaulttest default values;
+insert into defaulttest(col4) values(0); -- fails, col5 defaults to null
+alter table defaulttest alter column col5 drop default;
+insert into defaulttest default values; -- succeeds, inserts domain default
+-- We used to treat SET DEFAULT NULL as equivalent to DROP DEFAULT; wrong
+alter table defaulttest alter column col5 set default null;
+insert into defaulttest(col4) values(0); -- fails
+alter table defaulttest alter column col5 drop default;
 insert into defaulttest default values;
 insert into defaulttest default values;