Thread: DEFAULT of domain ignored in plpgsql (8.4.1)

Hi

It seems that pl/pgsql ignores the DEFAULT value of domains for local
variables. With the following definitions in place

create domain myint as int default 0;
create or replace function myint() returns myint as $body$
declare  v_result myint;
begin  return v_result;
end;
$body$ language plpgsql immutable;

issuing
select myint();
returns NULL, not 0 on postgres 8.4.1

If the line  v_result myint;
is changes to  v_result myint default 0;
than 0 is returned as expected.

I've tried to create a patch, but didn't see how I'd convert the result
from get_typedefault() (A Node*, presumeably the parsetree corresponding
to the default expression?) into a plan that I could store in a
PLpgSQL_expr. I guess I'd need something like SPI_prepare_plan that
takes a parse tree instead of a query string. Or am I on a completely
wrong track there?

While trying to cook up a patch I've also stumbled over what I perceive
as a bug relating to DOMAINS and column DEFAULTs. I'll write that up in
a second E-Mail to avoid confusion.

best regards,
Florian Pflug

On Thu, Nov 19, 2009 at 9:06 PM, Florian G. Pflug <fgp@phlo.org> wrote:
> Hi
>
> It seems that pl/pgsql ignores the DEFAULT value of domains for local
> variables. With the following definitions in place
>
> create domain myint as int default 0;
> create or replace function myint() returns myint as $body$
> declare
>  v_result myint;
> begin
>  return v_result;
> end;
> $body$ language plpgsql immutable;
>
> issuing
> select myint();
> returns NULL, not 0 on postgres 8.4.1
>
> If the line
>  v_result myint;
> is changes to
>  v_result myint default 0;
> than 0 is returned as expected.
>
> I've tried to create a patch, but didn't see how I'd convert the result
> from get_typedefault() (A Node*, presumeably the parsetree corresponding
> to the default expression?) into a plan that I could store in a
> PLpgSQL_expr. I guess I'd need something like SPI_prepare_plan that
> takes a parse tree instead of a query string. Or am I on a completely
> wrong track there?
>
> While trying to cook up a patch I've also stumbled over what I perceive
> as a bug relating to DOMAINS and column DEFAULTs. I'll write that up in
> a second E-Mail to avoid confusion.

I suggest adding this to the open CommitFest (2010-01) at
https://commitfest.postgresql.org/action/commitfest_view/open

...Robert

Robert Haas wrote:
> On Thu, Nov 19, 2009 at 9:06 PM, Florian G. Pflug <fgp@phlo.org> wrote:
>> I've tried to create a patch, but didn't see how I'd convert the result
>> from get_typedefault() (A Node*, presumeably the parsetree corresponding
>> to the default expression?) into a plan that I could store in a
>> PLpgSQL_expr. I guess I'd need something like SPI_prepare_plan that
>> takes a parse tree instead of a query string. Or am I on a completely
>> wrong track there?
>>
>> While trying to cook up a patch I've also stumbled over what I perceive
>> as a bug relating to DOMAINS and column DEFAULTs. I'll write that up in
>> a second E-Mail to avoid confusion.
> 
> I suggest adding this to the open CommitFest (2010-01) at
> https://commitfest.postgresql.org/action/commitfest_view/open

Hm, but I don't (yet) have a patch to add...

best regards,
Florian Pflug

On Fri, Nov 20, 2009 at 12:51 PM, Florian G. Pflug <fgp@phlo.org> wrote:
> Robert Haas wrote:
>>
>> On Thu, Nov 19, 2009 at 9:06 PM, Florian G. Pflug <fgp@phlo.org> wrote:
>>>
>>> I've tried to create a patch, but didn't see how I'd convert the result
>>> from get_typedefault() (A Node*, presumeably the parsetree corresponding
>>> to the default expression?) into a plan that I could store in a
>>> PLpgSQL_expr. I guess I'd need something like SPI_prepare_plan that
>>> takes a parse tree instead of a query string. Or am I on a completely
>>> wrong track there?
>>>
>>> While trying to cook up a patch I've also stumbled over what I perceive
>>> as a bug relating to DOMAINS and column DEFAULTs. I'll write that up in
>>> a second E-Mail to avoid confusion.
>>
>> I suggest adding this to the open CommitFest (2010-01) at
>> https://commitfest.postgresql.org/action/commitfest_view/open
>
> Hm, but I don't (yet) have a patch to add...

Woops, I saw an attachment and thought there was a patch, but it was
just smime.p7s... sorry for the noise.

...Robert

"Florian G. Pflug" <fgp@phlo.org> writes:
> It seems that pl/pgsql ignores the DEFAULT value of domains for local
> variables.

The plpgsql documentation seems entirely clear on this:
The DEFAULT clause, if given, specifies the initial value assigned tothe variable when the block is entered. If the
DEFAULTclause is notgiven then the variable is initialized to the SQL null value.
 
        regards, tom lane

Tom Lane wrote:
> "Florian G. Pflug" <fgp@phlo.org> writes:
>> It seems that pl/pgsql ignores the DEFAULT value of domains for
>> local variables.
> 
> The plpgsql documentation seems entirely clear on this:
> 
> The DEFAULT clause, if given, specifies the initial value assigned to
>  the variable when the block is entered. If the DEFAULT clause is not
>  given then the variable is initialized to the SQL null value.

Hm, must have missed that paragraph :-(. Sorry for that.

Would a patch that changes that have any chance of being accepted? Or is
the gain (not having to repeat the DEFAULT clause, and being able to
maintain it at one place instead of many) considered too small compared
to the risk of breaking existing code?

best regards,
Florian Pflug

> Would a patch that changes that have any chance of being accepted? Or is
> the gain (not having to repeat the DEFAULT clause, and being able to
> maintain it at one place instead of many) considered too small compared
> to the risk of breaking existing code?

I don't think there's a lot of risk of code breakage; few people use
domains, fewer use them with defaults, and you might be the only one
using them as variable types.  And there are going to be more
substantial backwards compat issues with the lexer changes anyway.  As
long as we remember to flag the compatibility issue in the release
notes, I don't see it as a problem.

However, there are some other issues to be resolved:

(1) what should be the interaction of DEFAULT parameters and domains
with defaults?

(2) this change, while very useful, does change what had been a simple
rule ("All variables are NULL unless specifically set otherwise") into a
conditional one ("All variables are NULL unless set otherwise OR unless
they are declared as domain types with defaults").  Do people feel that
the new behavior would be sufficiently intuitive to avoid user confusion?

(3) Last I checked, there were still several places in which domains did
not behave consistently in stored procedures.  I think that Elein had
some unfinished patches in this regard -- you'll want to search the
archives and the TODO list.

--Josh Berkus

On 21 Nov 2009, at 02:56, Josh Berkus wrote:

>
>> Would a patch that changes that have any chance of being accepted? Or is
>> the gain (not having to repeat the DEFAULT clause, and being able to
>> maintain it at one place instead of many) considered too small compared
>> to the risk of breaking existing code?
>
> I don't think there's a lot of risk of code breakage; few people use
> domains, fewer use them with defaults, and you might be the only one
> using them as variable types.  And there are going to be more
> substantial backwards compat issues with the lexer changes anyway.  As
> long as we remember to flag the compatibility issue in the release
> notes, I don't see it as a problem.

we use domains with defaults, a lot. That's one of the purposes of domains, to have certain type, constraint, and
default. 

Josh Berkus <josh@agliodbs.com> writes:
> (2) this change, while very useful, does change what had been a simple
> rule ("All variables are NULL unless specifically set otherwise") into a
> conditional one ("All variables are NULL unless set otherwise OR unless
> they are declared as domain types with defaults").  Do people feel that
> the new behavior would be sufficiently intuitive to avoid user confusion?

I'm inclined to leave it alone.  It complicates the mental model, and
frankly attaching defaults to domains was not one of the SQL committee's
better ideas anyway.  It's *fundamentally* non-orthogonal.
        regards, tom lane

Tom Lane wrote:
> Josh Berkus <josh@agliodbs.com> writes:
>> (2) this change, while very useful, does change what had been a
>> simple rule ("All variables are NULL unless specifically set
>> otherwise") into a conditional one ("All variables are NULL unless
>> set otherwise OR unless they are declared as domain types with
>> defaults").  Do people feel that the new behavior would be
>> sufficiently intuitive to avoid user confusion?
> 
> I'm inclined to leave it alone.  It complicates the mental model, and
>  frankly attaching defaults to domains was not one of the SQL
> committee's better ideas anyway.  It's *fundamentally*
> non-orthogonal.

I've always though of domains as being a kind of subtype of it's base
type. In this picture, DEFAULTs for domains correspond to overriding the
default constructor of the type (thinking C++ now), and seem like a
natural thing to have. But maybe that's more a C++ programmers than a
database designers point of view...

I've just checked how rowtypes behave, and while the "set to null unless
specifically set otherwise" rule kind of holds for them, their NULL
value seems to be special-cased enough to blur the line quite a bit

create or replace function myt() returns t as $body$
declare  r t;
begin  raise notice 'r: %, r is null: %', r, (r is null);  return r;
end;
$body$ language plpgsql immutable;
select myt(),myt() is null;

gives:

NOTICE:  r: (,), r is null: t
NOTICE:  r: (,), r is null: t myt | ?column?
-----+---------- (,) | f

Strange I think... And at least half of an exception to the simple
"always null unless specifically set otherwise" rule

It also seems that while domain DEFAULTs are ignored, the resulting
(null-initialized) variable is still checked against the domain's
constraints, including a potential NOT NULL constraint

create domain myint as int not null;
create or replace function myint() returns myint as $body$
declare  i myint;
begin  return i;
end;
$body$ language plpgsql immutable;

raises

ERROR:  domain myint does not allow null values
CONTEXT:  PL/pgSQL function "myint" line 3 during statement block local
variable initialization

This has the potential to cause some headache I think if you use domains
to prohibit NULL values because they make no semantic sense for your
application, and depend on DEFAULT to fill in some other value (like an
empty string or an empty array).

best regards,
Florian Pflug

Gurjeet Singh wrote:
> On Sat, Nov 21, 2009 at 7:26 AM, Josh Berkus <josh@agliodbs.com 
> <mailto:josh@agliodbs.com>> wrote: However, there are some other
> issues to be resolved:
> 
> (1) what should be the interaction of DEFAULT parameters and domains 
> with defaults?
> 
> The function's DEFAULT parameter should take precedence over the
> default of the domain.

I think Josh was pondering whether

create domain myint as int default 0;
create function f(i myint) ...;

should behave like

create function f(i myint default 0) ...;

and hence call f(0) if you do "select f();", or instead
raise an error because no f with zero parameters is defined (as it does
now).

I'd say no, because "no default" should be treated the same as "default
null", so for consistency we'd then have to also support

create function g(i int) ...;
select g();

And of course throw an error if there was another function defined as
create function g() ...;

This way leads to madness...

If one really wanted to do that, there'd have to be an OPTIONAL clause
for function parameters that works like DEFAULT, but doesn't take a
default value and instead uses the type's default (NULL except for
domains with DEFAULT clause). But I wouldn't got that far, personally...

best regards,
Florian Pflug

On lör, 2009-11-21 at 22:20 +0100, Florian G. Pflug wrote:
> > I'm inclined to leave it alone.  It complicates the mental model, and
> >  frankly attaching defaults to domains was not one of the SQL
> > committee's better ideas anyway.  It's *fundamentally*
> > non-orthogonal.
> 
> I've always though of domains as being a kind of subtype of it's base
> type. In this picture, DEFAULTs for domains correspond to overriding the
> default constructor of the type (thinking C++ now), and seem like a
> natural thing to have. But maybe that's more a C++ programmers than a
> database designers point of view...

There are other things in the SQL standard to do that, like types with
inheritance and types with constructors.  We have already overextended
domains enough beyond what the SQL standards says, mostly because these
other things that are the correct solution are not implemented yet.