Thread: 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
This should fix the problem.
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?
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
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.186
diff -c -r1.186 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c 30 Jan 2007 18:02:22 -0000 1.186
--- src/pl/plpgsql/src/pl_exec.c 1 Feb 2007 19:10:51 -0000
***************
*** 890,897 ****
--- 890,916 ----
{
if (var->default_val == NULL)
{
+ /* Initially it contains a NULL */
var->value = (Datum) 0;
var->isnull = true;
+ /*
+ * If needed, give the datatype a chance to reject
+ * NULLs, by assigning a NULL to the variable.
+ * We claim the value is of type UNKNOWN, not the
+ * var's datatype, else coercion will be skipped.
+ * (Do this before the notnull check to be
+ * consistent with exec_assign_value.)
+ */
+ if (!var->datatype->typinput.fn_strict)
+ {
+ bool valIsNull = true;
+
+ exec_assign_value(estate,
+ (PLpgSQL_datum *) var,
+ (Datum) 0,
+ UNKNOWNOID,
+ &valIsNull);
+ }
if (var->notnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
Index: src/test/regress/expected/domain.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/domain.out,v
retrieving revision 1.38
diff -c -r1.38 domain.out
*** src/test/regress/expected/domain.out 6 Oct 2006 17:14:01 -0000 1.38
--- src/test/regress/expected/domain.out 1 Feb 2007 19:10:51 -0000
***************
*** 383,388 ****
--- 383,404 ----
create function doubledecrement(p1 pos_int) returns pos_int as $$
declare v pos_int;
begin
+ return p1;
+ end$$ language plpgsql;
+ select doubledecrement(3); -- fail because of implicit null assignment
+ ERROR: domain pos_int does not allow null values
+ CONTEXT: PL/pgSQL function "doubledecrement" line 2 during statement block local variable initialization
+ create or replace function doubledecrement(p1 pos_int) returns pos_int as $$
+ declare v pos_int := 0;
+ begin
+ return p1;
+ end$$ language plpgsql;
+ select doubledecrement(3); -- fail at initialization assignment
+ ERROR: value for domain pos_int violates check constraint "pos_int_check"
+ CONTEXT: PL/pgSQL function "doubledecrement" line 2 during statement block local variable initialization
+ create or replace function doubledecrement(p1 pos_int) returns pos_int as $$
+ declare v pos_int := 1;
+ begin
v := p1 - 1;
return v - 1;
end$$ language plpgsql;
Index: src/test/regress/sql/domain.sql
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/sql/domain.sql,v
retrieving revision 1.21
diff -c -r1.21 domain.sql
*** src/test/regress/sql/domain.sql 5 Apr 2006 22:11:58 -0000 1.21
--- src/test/regress/sql/domain.sql 1 Feb 2007 19:10:51 -0000
***************
*** 310,315 ****
--- 310,331 ----
create function doubledecrement(p1 pos_int) returns pos_int as $$
declare v pos_int;
begin
+ return p1;
+ end$$ language plpgsql;
+
+ select doubledecrement(3); -- fail because of implicit null assignment
+
+ create or replace function doubledecrement(p1 pos_int) returns pos_int as $$
+ declare v pos_int := 0;
+ begin
+ return p1;
+ end$$ language plpgsql;
+
+ select doubledecrement(3); -- fail at initialization assignment
+
+ create or replace function doubledecrement(p1 pos_int) returns pos_int as $$
+ declare v pos_int := 1;
+ begin
v := p1 - 1;
return v - 1;
end$$ language plpgsql;
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.