Re: [HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread Tom Lane
Andrew Dunstan <[EMAIL PROTECTED]> 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

---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate


Re: [HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread Andrew Dunstan



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

---(end of broadcast)---
TIP 4: Have you searched our list archives?

  http://archives.postgresql.org


Re: [HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread Tom Lane
Gregory Stark <[EMAIL PROTECTED]> writes:
> "Tom Lane" <[EMAIL PROTECTED]> 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

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq


Re: [HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread Gregory Stark
"Tom Lane" <[EMAIL PROTECTED]> 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 column or a generated column, then S is
   marked as unassigned. 

b) If the data descriptor for the site includes a , then S 
is
   set to the value specified by that .

c) If the data descriptor for the site includes a  that
   identifies a domain descriptor that includes a , then S 
is
   set to the value specified by that .

d) If the default value is for a column C of a candidate row for insertion
   into or update of a derived table DT and C 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

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster


Re: [HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread 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 -  1.324
--- src/backend/catalog/heap.c  28 Oct 2007 21:04:59 -
***
*** 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 -  1.108
--- src/backend/commands/typecmds.c 28 Oct 2007 21:04:59 -
***
*** 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))
!   {
!   

Re: [HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread Tom Lane
Gregory Stark <[EMAIL PROTECTED]> 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

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster


Re: [HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread Gregory Stark

"Tom Lane" <[EMAIL PROTECTED]> 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

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


Re: [HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread Tom Lane
Simon Riggs <[EMAIL PROTECTED]> 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

---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate


Re: [HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread Simon Riggs
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


---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly


[HACKERS] Backend misfeasance for DEFAULT NULL

2007-10-28 Thread Tom Lane
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

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly