BUG #2948: default null values for not-null domains
The following bug has been logged online:
Bug reference: 2948
Logged by: Sergiy Vyshnevetskiy
Email address: serg@vostok.net
PostgreSQL version: 8.2.1
Operating system: FreeBSD-6 stable
Description: default null values for not-null domains
Details:
create domain "DInt4" as int not null;
CREATE DOMAIN
#psql = serg@[local]:5432 test
create or replace function test() returns "DInt4" immutable strict language
plpgsql as $F$declare t "DInt4"; begin return t; end$F$;
CREATE FUNCTION
#psql = serg@[local]:5432 test
select test() is null;
?column?
----------
t
(1 запись)
#psql = serg@[local]:5432 test
Last select should have risen an exception:
ERROR: variable "t" declared NOT NULL cannot default to NULL
Sergiy Vyshnevetskiy <serg@vostok.net> writes:
This should fix the problem.
No, not at all. Consider if you'd written a domain CHECK constraint
that rejects nulls, instead of the easy case. What we'd really have
to do here is see if domain_in() will accept a NULL.
I'm starting to get the feeling that the entire idea of NOT NULL domains
is broken :-(
regards, tom lane
On Wed, 31 Jan 2007, Tom Lane wrote:
Sergiy Vyshnevetskiy <serg@vostok.net> writes:
This should fix the problem.
No, not at all. Consider if you'd written a domain CHECK constraint
that rejects nulls, instead of the easy case.
I've forgotten that.
What we'd really have to do here is see if domain_in() will accept a
NULL.
Almost correct. We need to set the default value for every variable to
null "the right way".
First, we need a PLpgSQL_expr variable for "null" expression. A static
variable assigned at program startup would be best, but on-the-fly will
work too, if slightly inefficient.
Second, we use a pointer to it instead of NULL in decl_defval in gram.y.
That's all!
What to call to convert a text string "null" into PLpgSQL_expr?
Where's the best place to call it?
I'm starting to get the feeling that the entire idea of NOT NULL domains
is broken :-(
Not at all. What's "broken" is the idea of variable as a simple piece of
memory. It is correct for base types, but not for domains - they may have
non-empty constructors (in C++ terminology).
As such, the so-called "optimized" assignment algorithm for null defaults
("let's flip a bit and pretend we've done it") in exec_stmt_block() may
not work for such domains.
Assign them all and let optimizer sort them out. :)
Sergiy Vyshnevetskiy <serg@vostok.net> writes:
Not at all. What's "broken" is the idea of variable as a simple piece of
memory. It is correct for base types, but not for domains - they may have
non-empty constructors (in C++ terminology).
That may be, but I'm unwilling to pay the overhead for *every* variable
when most of them won't be domains. I'm inclined to extend PLpgSQL_type
to include a domain indicator and only do it the hard way when we have to.
[ looks at code... ] Actually, I think we already have the flag we
need: look to see if the typinput function is strict.
regards, tom lane
Tom Lane wrote:
I'm starting to get the feeling that the entire idea of NOT NULL
domains is broken :-(
How is that so very different from having a default value of 5 with a
domain that rejects 5?
--
Peter Eisentraut
http://developer.postgresql.org/~petere/
On Wed, 31 Jan 2007, Tom Lane wrote:
Sergiy Vyshnevetskiy <serg@vostok.net> writes:
Not at all. What's "broken" is the idea of variable as a simple piece of
memory. It is correct for base types, but not for domains - they may have
non-empty constructors (in C++ terminology).That may be, but I'm unwilling to pay the overhead for *every* variable
when most of them won't be domains. I'm inclined to extend PLpgSQL_type
to include a domain indicator and only do it the hard way when we have to.
Why not add PLPGSQL_TTYPE_DOMAIN and rename PLPGSQL_TTYPE_SCALAR to
PLPGSQL_TTYPE_BASE? We only use PLPGSQL_TTYPE_SCALAR in _3_ places!
[ looks at code... ] Actually, I think we already have the flag we
need: look to see if the typinput function is strict.
All domains have domain_in as input function - it is NOT strict.
Anyway, as we assign a value to a domain variable we must check
constraints - that's the whole point of domains. Even when the value is
"null".
Hack attached. Any reasons not to call it a bugfix?
Peter Eisentraut <peter_e@gmx.net> writes:
Tom Lane wrote:
I'm starting to get the feeling that the entire idea of NOT NULL
domains is broken :-(
How is that so very different from having a default value of 5 with a
domain that rejects 5?
Two words for you: outer joins.
regards, tom lane
On Thu, 1 Feb 2007, Peter Eisentraut wrote:
Tom Lane wrote:
I'm starting to get the feeling that the entire idea of NOT NULL
domains is broken :-(How is that so very different from having a default value of 5 with a
domain that rejects 5?
Because 5 will be rejected as a value for a variable or field of such
domain. This is correct (and useful) behavior.
On the other hand null can make it there under some circumstances, even if
domain explicitly forbids nulls. Which is the bug I'm fighting against.
Actually there are several of them, and I plan to post them all. And,
hopefully, bugfixes too.
Sergiy Vyshnevetskiy <serg@vostok.net> writes:
Why not add PLPGSQL_TTYPE_DOMAIN and rename PLPGSQL_TTYPE_SCALAR to
PLPGSQL_TTYPE_BASE? We only use PLPGSQL_TTYPE_SCALAR in _3_ places!
That was my first thought too, but it's wrong. The right thing is to
look at the strictness of the input function, because that is the API
we have defined to determine whether a datatype might possibly be
interested in rejecting nulls. The fact that domain_in() is the only
example in the core system doesn't make it correct to restrict the
functionality to domains.
regards, tom lane
On Thu, 1 Feb 2007, Tom Lane wrote:
Sergiy Vyshnevetskiy <serg@vostok.net> writes:
Why not add PLPGSQL_TTYPE_DOMAIN and rename PLPGSQL_TTYPE_SCALAR to
PLPGSQL_TTYPE_BASE? We only use PLPGSQL_TTYPE_SCALAR in _3_ places!That was my first thought too, but it's wrong. The right thing is to
look at the strictness of the input function, because that is the API
we have defined to determine whether a datatype might possibly be
interested in rejecting nulls. The fact that domain_in() is the only
example in the core system doesn't make it correct to restrict the
functionality to domains.
...
I have been slow.
If input function IS strict then nulls are ALWAYS accepted.
If input function IS NOT strict then nulls MIGHT be rejectted.
And the patch is much more simple now (attached).
Is that it?
Attachments:
patch-pl_exec.ctext/x-csrc; charset=US-ASCII; name=patch-pl_exec.cDownload+5-0
Sergiy Vyshnevetskiy <serg@vostok.net> writes:
If input function IS strict then nulls are ALWAYS accepted.
If input function IS NOT strict then nulls MIGHT be rejectted.
And the patch is much more simple now (attached).
Is that it?
Almost right. exec_assign_value() thinks its isNull argument is the
null flag for the *source* value (not sure why it's pass by reference).
As you set it up, var->isnull would be aliased by *isNull, which might
manage to break things within that function if the code were ever
rearranged.
Also, some comments are usually a good idea (if the purpose were
obvious, it'd have been right the first time, no?), and you always need
to check the regression tests --- it turns out that the wrong behavior
was actually being exposed by the tests.
Patch as-applied is attached.
regards, tom lane
On Thu, 1 Feb 2007, Tom Lane wrote:
Sergiy Vyshnevetskiy <serg@vostok.net> writes:
If input function IS strict then nulls are ALWAYS accepted.
If input function IS NOT strict then nulls MIGHT be rejectted.
And the patch is much more simple now (attached).
Is that it?Almost right. exec_assign_value() thinks its isNull argument is the
null flag for the *source* value (not sure why it's pass by reference).
Because the value may change during type cast. From null to non-null too.
Or vice-versa. I'll try it later.
As you set it up, var->isnull would be aliased by *isNull, which might
manage to break things within that function if the code were ever
rearranged.Also, some comments are usually a good idea (if the purpose were
obvious, it'd have been right the first time, no?),
I will, when I'm sure what I'm doing. For now it's mostly "mokey see -
monkey do".
and you always need to check the regression tests --- it turns out that
the wrong behavior was actually being exposed by the tests.
Hmm? Oh, yeah, I /heard/ something about them ... I think. :)
Patch as-applied is attached.
Excellent. Thanks.