Thread: Backend misfeasance for DEFAULT NULL
While poking at the complaint reported here: http://archives.postgresql.org/pgsql-general/2007-10/msg01484.php I realized that there is a related issue for null defaults. Consider create table p (f1 int default 0);create table c (f1 int);alter table c inherit p; At this point, c.f1 has no default, or NULL default if you prefer. However, pg_dump dumps this configuration as create table p (f1 int default 0);create table c (f1 int) inherits (p); so after a reload c.f1 will have default 0 because it'll inherit that from p. I tried to fix this by having pg_dump insert an explicit DEFAULT NULL clause for c.f1, which turned out to be not too hard, but on testing it did nothing at all --- c.f1 still reloaded with default 0! Poking into it, I find that it seems to be another case of the lesson we should have learned some time ago: embedding semantic knowledge in gram.y is usually a Bad Idea. gram.y "knows" that it can throw away DEFAULT NULL --- see the exprIsNullConstant() uses therein. So the clause never makes it to the place in tablecmds.c where we consider whether to adopt inherited defaults or not. ISTM this is a backend bug: if I tell it DEFAULT NULL, by golly I should get DEFAULT NULL. 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. Comments? regards, tom lane
On Sun, 2007-10-28 at 12:44 -0400, Tom Lane wrote: > While poking at the complaint reported here: > http://archives.postgresql.org/pgsql-general/2007-10/msg01484.php > I realized that there is a related issue for null defaults. Consider > > create table p (f1 int default 0); > create table c (f1 int); > alter table c inherit p; Seems more like an unwanted looseness in the meaning of an ALTER TABLE .. INHERIT to me. I'd prefer it if we added some extra clauses to ALTER TABLE: [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES } ] > At this point, c.f1 has no default, or NULL default if you prefer. > However, pg_dump dumps this configuration as > > create table p (f1 int default 0); > create table c (f1 int) inherits (p); > > so after a reload c.f1 will have default 0 because it'll inherit that > from p. > > I tried to fix this by having pg_dump insert an explicit DEFAULT NULL > clause for c.f1, which turned out to be not too hard, but on testing > it did nothing at all --- c.f1 still reloaded with default 0! > > Poking into it, I find that it seems to be another case of the lesson > we should have learned some time ago: embedding semantic knowledge in > gram.y is usually a Bad Idea. gram.y "knows" that it can throw away > DEFAULT NULL --- see the exprIsNullConstant() uses therein. So the > clause never makes it to the place in tablecmds.c where we consider > whether to adopt inherited defaults or not. > > ISTM this is a backend bug: if I tell it DEFAULT NULL, by golly I > should get DEFAULT NULL. Agreed. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
Simon Riggs <simon@2ndquadrant.com> writes:
> Seems more like an unwanted looseness in the meaning of an ALTER
> TABLE .. INHERIT to me. I'd prefer it if we added some extra clauses to
> ALTER TABLE:
> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES } ]
I think you're confusing this with a CREATE TABLE operation.
"Excluding constraints" is not sensible in any case: failing to inherit
check constraints should be disallowed IMHO. (There's already a TODO to
add inheritance info to pg_constraint so that that can be enforced in a
more bulletproof fashion.)
The other two categories of things are explicitly allowed to be
different between a child and a parent, and so I'm not convinced that
ALTER INHERIT has any business touching them.
But even if it's decided that the above is a sensible future feature,
it's certainly not something we can do as a backpatchable bug fix.
regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > ISTM this is a backend bug: if I tell it DEFAULT NULL, by golly I > should get DEFAULT NULL. 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. > > Comments? Well if there's a convenient later place to add the check then sure. Will it mean pg_dump will have to put DEFAULT NULL everywhere though? Or can it detect that it's an inherited table where the default doesn't match? Perhaps it should be even later and we should store the NULL default in the catalog but filter it out when we build the relcache? Then pg_dump wouldn't need any special intelligence to detect when the default doesn't match the parent -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark <stark@enterprisedb.com> writes:
> Well if there's a convenient later place to add the check then sure. Will it
> mean pg_dump will have to put DEFAULT NULL everywhere though? Or can it detect
> that it's an inherited table where the default doesn't match?
The latter --- I already committed that fix.
> Perhaps it should be even later and we should store the NULL default in the
> catalog but filter it out when we build the relcache?
No, I don't think we want to be making useless pg_attrdef entries.
I do want to put the test as late as possible though, maybe even
StoreAttrDefault?
regards, tom lane
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 ----
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > 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? Sure it's a clear-cut bug. The spec (SQL 2003 Section 11.5) clearly says the default of the column overrides the default of the domain: 3) When a site S is set to its default value, Case: a) If the descriptor of S indicates that it represents a column of which some underlying column is an identity columnor a generated column, then S is marked as unassigned. b) If the data descriptor for the site includes a <default option>, then S is set to the value specified by that<default option>. c) If the data descriptor for the site includes a <domain name> that identifies a domain descriptor that includesa <default option>, then S is set to the value specified by that <default option>. d) If the default value is for a column C of a candidate row for insertion into or update of a derived table DT andC has a single counterpart column CC in a leaf generally underlying table of DT, then S is set to the default value of CC, which is obtained by applying the General Rules of this Subclause. e) Otherwise, S is set to the null value. I would tend to be more conservative than we've been in the past with back patching. We keep saying people should be on the most recent point release and people shouldn't be concerned about their application breaking. But if we make behaviour changes, even for things which are definitely bugs, we make those fears justified. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark <stark@enterprisedb.com> writes:
> "Tom Lane" <tgl@sss.pgh.pa.us> writes:
>> Is it OK to change this behavior? Should I
>> back-patch, or not?
> I would tend to be more conservative than we've been in the past with
> back patching. We keep saying people should be on the most recent
> point release and people shouldn't be concerned about their
> application breaking. But if we make behaviour changes, even for
> things which are definitely bugs, we make those fears justified.
You have a point, but on reflection I think the odds of this change
breaking an existing application are low. The reason is that in the old
implementation, "DEFAULT NULL" is effectively not there at all, and so
an update to a newer point-release, or even a dump and reload, wouldn't
change the behavior of an existing database. Somebody creating *new*
tables with DDL that includes such a specification would see the
behavioral change, but if they are specifying it that way they'd
probably want it to work. Also, the lack of a complaint from the field
suggests to me that nobody has really been trying to do this anyway ...
Still, fixing only HEAD would be less work for me, so I'm happy with
that if it's the consensus.
regards, tom lane
Tom Lane wrote: > You have a point, but on reflection I think the odds of this change > breaking an existing application are low. The reason is that in the old > implementation, "DEFAULT NULL" is effectively not there at all, and so > an update to a newer point-release, or even a dump and reload, wouldn't > change the behavior of an existing database. Somebody creating *new* > tables with DDL that includes such a specification would see the > behavioral change, but if they are specifying it that way they'd > probably want it to work. Also, the lack of a complaint from the field > suggests to me that nobody has really been trying to do this anyway ... > > Still, fixing only HEAD would be less work for me, so I'm happy with > that if it's the consensus. > > > I'm in two minds about it. I hate leaving bugs unfixed, however obscure. I suspect domains are one of our least used features, which might account for the lack of complaint. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> I suspect domains are one of our least used features, which might
> account for the lack of complaint.
True, and it would also say that the risk of breaking any existing app
is low ...
regards, tom lane