Re: Backend misfeasance for DEFAULT NULL

I wrote:
> ... I propose stripping out gram.y's special
> hack for this, and preserving the efficiency of the case by
> inserting a test very much later to see if the expression is just
> a NULL constant.  Maybe AddRelationRawConstraints is the right place.

I did this (see attached patch) and got an interesting failure in the
domain regression test.  That test has

create domain ddef1 int4 DEFAULT 3;
create table defaulttest
        ...
            , col5 ddef1 NOT NULL DEFAULT NULL
        ...
insert into defaulttest default values;

and the 'expected' result is that this succeeds and inserts 3; meaning
that the domain default overrides the explicit per-column specification.
But with the patch it fails with "null value in column "col5" violates
not-null constraint".

AFAICS this is a flat-out bug too, since the per-column default should
override the domain's default; that's certainly how it works for any
non-null column default value.  But I wasn't expecting any regression
failures with this patch.  Is it OK to change this behavior?  Should I
back-patch, or not?

(BTW, the reason this is exposed is that when a domain is involved, the
apparently plain NULL is really a CoerceToDomain operation, which the
new test sees as not being the same as a plain null constant; correctly
so, if you ask me, since CoerceToDomain might or might not reject a
null.)

            regards, tom lane

Index: src/backend/catalog/heap.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/catalog/heap.c,v
retrieving revision 1.324
diff -c -r1.324 heap.c
*** src/backend/catalog/heap.c    12 Oct 2007 18:55:11 -0000    1.324
--- src/backend/catalog/heap.c    28 Oct 2007 21:04:59 -0000
***************
*** 1722,1727 ****
--- 1722,1736 ----
                             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.
+          */
+         if (expr == NULL ||
+             (IsA(expr, Const) && ((Const *) expr)->constisnull))
+             continue;
+
          StoreAttrDefault(rel, colDef->attnum, nodeToString(expr));

          cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
Index: src/backend/commands/typecmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/typecmds.c,v
retrieving revision 1.108
diff -c -r1.108 typecmds.c
*** src/backend/commands/typecmds.c    29 Sep 2007 17:18:58 -0000    1.108
--- src/backend/commands/typecmds.c    28 Oct 2007 21:04:59 -0000
***************
*** 765,784 ****
                                                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).
                       */
!                     defaultValue =
!                         deparse_expression(defaultExpr,
!                                            deparse_context_for(domainName,
!                                                                InvalidOid),
!                                            false, false);
!                     defaultValueBin = nodeToString(defaultExpr);
                  }
                  else
                  {
!                     /* DEFAULT NULL is same as not having a default */
                      defaultValue = NULL;
                      defaultValueBin = NULL;
                  }
--- 765,798 ----
                                                domainName);

                      /*
!                      * If the expression is just a NULL constant, we treat
!                      * it like not having a default.
                       */
!                     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
                  {
!                     /* No default (can this still happen?) */
                      defaultValue = NULL;
                      defaultValueBin = NULL;
                  }
***************
*** 1443,1449 ****
      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 */
      if (defaultRaw)
      {
          /* Create a dummy ParseState for transformExpr */
--- 1457,1463 ----
      MemSet(new_record_nulls, ' ', sizeof(new_record_nulls));
      MemSet(new_record_repl, ' ', sizeof(new_record_repl));

!     /* Store the new default into the tuple */
      if (defaultRaw)
      {
          /* Create a dummy ParseState for transformExpr */
***************
*** 1459,1488 ****
                                    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).
           */
!         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)));

!         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';
      }
      else
-         /* 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';
--- 1473,1517 ----
                                    NameStr(typTup->typname));

          /*
!          * If the expression is just a NULL constant, we treat the command
!          * like ALTER ... DROP DEFAULT.
           */
!         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)));

!             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';
!         }
      }
      else
      {
+         /* 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';
Index: src/backend/parser/gram.y
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.603
diff -c -r2.603 gram.y
*** src/backend/parser/gram.y    24 Sep 2007 01:29:28 -0000    2.603
--- src/backend/parser/gram.y    28 Oct 2007 21:05:00 -0000
***************
*** 1685,1698 ****
          ;

  alter_column_default:
!             SET DEFAULT a_expr
!                 {
!                     /* Treat SET DEFAULT NULL the same as DROP DEFAULT */
!                     if (exprIsNullConstant($3))
!                         $$ = NULL;
!                     else
!                         $$ = $3;
!                 }
              | DROP DEFAULT                { $$ = NULL; }
          ;

--- 1685,1691 ----
          ;

  alter_column_default:
!             SET DEFAULT a_expr            { $$ = $3; }
              | DROP DEFAULT                { $$ = NULL; }
          ;

***************
*** 2080,2094 ****
                      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->cooked_expr = NULL;
                      n->keys = NULL;
                      n->indexspace = NULL;
--- 2073,2079 ----
                      Constraint *n = makeNode(Constraint);
                      n->contype = CONSTR_DEFAULT;
                      n->name = NULL;
!                     n->raw_expr = $2;
                      n->cooked_expr = NULL;
                      n->keys = NULL;
                      n->indexspace = NULL;
***************
*** 9763,9785 ****
      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.
   *
--- 9748,9753 ----
Index: src/backend/parser/parse_expr.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/parse_expr.c,v
retrieving revision 1.221
diff -c -r1.221 parse_expr.c
*** src/backend/parser/parse_expr.c    23 Jun 2007 22:12:51 -0000    1.221
--- src/backend/parser/parse_expr.c    28 Oct 2007 21:05:00 -0000
***************
*** 606,611 ****
--- 606,626 ----
      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)
  {
Index: src/backend/parser/parse_utilcmd.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/parse_utilcmd.c,v
retrieving revision 2.3
diff -c -r2.3 parse_utilcmd.c
*** src/backend/parser/parse_utilcmd.c    27 Aug 2007 03:36:08 -0000    2.3
--- src/backend/parser/parse_utilcmd.c    28 Oct 2007 21:05:00 -0000
***************
*** 440,446 ****
                              (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;
--- 440,445 ----
Index: src/include/parser/gramparse.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/parser/gramparse.h,v
retrieving revision 1.38
diff -c -r1.38 gramparse.h
*** src/include/parser/gramparse.h    5 Jan 2007 22:19:56 -0000    1.38
--- src/include/parser/gramparse.h    28 Oct 2007 21:05:00 -0000
***************
*** 54,59 ****
  extern int    base_yyparse(void);
  extern List *SystemFuncName(char *name);
  extern TypeName *SystemTypeName(char *name);
- extern bool exprIsNullConstant(Node *arg);

  #endif   /* GRAMPARSE_H */
--- 54,58 ----