plpgsql doesn't coerce boolean expressions to boolean

Tom Lane wrote:

Following up this gripe
http://archives.postgresql.org/pgsql-sql/2003-09/msg00044.php
I've realized that plpgsql just assumes that the test expression
of an IF, WHILE, or EXIT statement is a boolean expression. It
doesn't take any measures to ensure this is the case or convert
the value if it's not the case. This seems pretty bogus to me.

However ... with the code as it stands, for pass-by-reference datatypes
any nonnull value will appear TRUE, while for pass-by-value datatypes
any nonzero value will appear TRUE. I fear that people may actually be
depending on these behaviors, particularly the latter one which is
pretty reasonable if you're accustomed to C. So while I'd like to throw
an error if the argument isn't boolean, I'm afraid of breaking people's
function definitions.

Here are some possible responses, roughly in order of difficulty
to implement:

1. Leave well enough alone (and perhaps document the behavior).

2. Throw an error if the expression doesn't return boolean.

3. Try to convert nonbooleans to boolean using plpgsql's usual method
for cross-type coercion, ie run the type's output proc to get a
string and feed it to bool's input proc. (This seems unlikely to
avoid throwing an error in very many cases, but it'd be the most
consistent with other parts of plpgsql.)

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

Any opinions about what to do?

It won't bite me so maybe I don't have a right to express an opinion :-)

plpgsql is not C - it appears to be in the Algol/Pascal/Ada family,
which do tend to avoid implicit type conversion.

On that basis, option 2 seems like it might be the right answer and also
the one most likely to break lots of existing functions. Maybe the right
thing would be to deprecate relying on implicit conversion to boolean
for one release cycle and then make it an error.

cheers

andrew

Tom Lane wrote:

Here are some possible responses, roughly in order of difficulty
to implement:

1. Leave well enough alone (and perhaps document the behavior).

2. Throw an error if the expression doesn't return boolean.

I'd opt for 2.
It's quite common that newer compilers will detect more bogus coding
than older ones. There might be existing functions that break from this
because they rely on the current "feature", but there are probably
others that will throw an exception, revealing bad coding (and
delivering correct results just by chance, I've seen this more than once...)

Regards,
Andreas

Andreas Pflug <pgadmin@pse-consulting.de> writes:

Tom Lane wrote:

2. Throw an error if the expression doesn't return boolean.

I'd opt for 2.
It's quite common that newer compilers will detect more bogus coding
than older ones. There might be existing functions that break from
this because they rely on the current "feature", but there are
probably others that will throw an exception, revealing bad coding
(and delivering correct results just by chance, I've seen this more
than once...)

I agree, and option 2 also makes sure that "bad" code will fail
cleanly, rather than possibly changing behavior and causing data
loss/corruption.

I agree with another poster that deprecation in 7.4 and removal in
7.5 might make sense.

-Doug

Doug McNaught <doug@mcnaught.org> writes:

I agree with another poster that deprecation in 7.4 and removal in
7.5 might make sense.

How would we "deprecate" it exactly? Throw a NOTICE?

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Doug McNaught <doug@mcnaught.org> writes:

I agree with another poster that deprecation in 7.4 and removal in
7.5 might make sense.

How would we "deprecate" it exactly? Throw a NOTICE?

I was thinking of just a mention in the release notes that we've found
this problem and intend to fix it after giving people a release cycle
to adjust. Not that people read release notes... :(

-Doug

Tom Lane wrote:

Doug McNaught <doug@mcnaught.org> writes:

I agree with another poster that deprecation in 7.4 and removal in
7.5 might make sense.

How would we "deprecate" it exactly? Throw a NOTICE?

Good question. A NOTICE just might be ignored, breaking everything
"surprisingly" in 7.5.

To speak a bit more general, how about some sort of "deprecation
checker" setting, if set to true anything marked as deprecated will
throw an error, if false only a notice will be generated. This would
enable users to check their existing stuff for future compatibility
before it's broken in the next release.

Regards,
Andreas

Tom Lane wrote:

Doug McNaught <doug@mcnaught.org> writes:

I agree with another poster that deprecation in 7.4 and removal in
7.5 might make sense.

How would we "deprecate" it exactly? Throw a NOTICE?

Release notes, I guess. A NOTICE would be fine as long as it didn't
result in a flood of them. If that happened once at parse time that
should be fine, I think.

What's the practice in deprecating other "features"?

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

How would we "deprecate" it exactly? Throw a NOTICE?

Release notes, I guess. A NOTICE would be fine as long as it didn't
result in a flood of them. If that happened once at parse time that
should be fine, I think.

It would be relatively difficult to do that; given the way plpgsql is
structured, a runtime message would be a lot easier.

What's the practice in deprecating other "features"?

We generally don't ;-). Certainly 7.4 contains bigger incompatible
changes than this one, and so have most of our prior releases.

regards, tom lane

I'm using sequences and currval() to retrieve the last inserted row in a table.

If currval() is undefined, as it is when a connection is made, then I know no
rows were inserted in that table and can take a different action. This is
problematic when using a connection pooling library, as the value of currval()
for any given sequence could possibly be set from a previous "connection".

One (theoretical) workaround would be to issue some sort of command to the
back-end database to wipe all values of currval() when a "new" connection is
made. I've done some digging in the system tables and source code, and can't
find an obvious solution. Perhaps one you you gurus can suggest a SQL statement
to do such a thing.

Alternately, if there is a better way to retrieve the last inserted row for any
given table, I'd be very grateful for the tip. It would need to be independent
of the connection history, and undefined if there has not been a row inserted
to the table during a definable interval of time (drop anchor when the
"connection" begins, raise anchor when the "connection" ends), and be
independant of the other connections inserting rows to the same table.

Any idaeas?

CG

__________________________________
Do you Yahoo!?
Yahoo! SiteBuilder - Free, easy-to-use web site design software
http://sitebuilder.yahoo.com

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

What's the practice in deprecating other "features"?

We generally don't ;-). Certainly 7.4 contains bigger incompatible
changes than this one, and so have most of our prior releases.

I thought I had seen discussions along the lines of "we'll give them one
cycle to fix it and then they are shot".

It does seem very late in this cycle to make such changes.

cheers

andrew

Chris Gamache <cgg007@yahoo.com> writes:

One (theoretical) workaround would be to issue some sort of command to the
back-end database to wipe all values of currval() when a "new" connection is
made. I've done some digging in the system tables and source code, and can't
find an obvious solution.

The state involved is in a linked list kept by commands/sequence.c.
Such a command would not be difficult to implement, if you could get
agreement on the syntax to use.

regards, tom lane

Chris Gamache wrote:

I'm using sequences and currval() to retrieve the last inserted row in a table.

If currval() is undefined, as it is when a connection is made, then I know no
rows were inserted in that table and can take a different action. This is
problematic when using a connection pooling library, as the value of currval()
for any given sequence could possibly be set from a previous "connection".

One (theoretical) workaround would be to issue some sort of command to the
back-end database to wipe all values of currval() when a "new" connection is
made. I've done some digging in the system tables and source code, and can't
find an obvious solution. Perhaps one you you gurus can suggest a SQL statement
to do such a thing.

Alternately, if there is a better way to retrieve the last inserted row for any
given table, I'd be very grateful for the tip. It would need to be independent
of the connection history, and undefined if there has not been a row inserted
to the table during a definable interval of time (drop anchor when the
"connection" begins, raise anchor when the "connection" ends), and be
independant of the other connections inserting rows to the same table.

I don't know how you could have an application that doesn't know if it
has issued a nextval() in the current connection. Unless you can explain
that, we have no intention of playing tricks with currval() for
connection pooling.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Mon, 8 Sep 2003, Bruce Momjian wrote:

I don't know how you could have an application that doesn't know if it
has issued a nextval() in the current connection. Unless you can explain
that, we have no intention of playing tricks with currval() for
connection pooling.

Actually, I would think the very act of using connection pooling would
ensure that applications may well not know whether or not a nextval had
been called. In other words, how is an application supposed to know if
the previous bit of code that used this connection issued a nextval() when
you're connection pooling and any piece of code could have run before you.

On the other hand, using currval as a test to see if a value has been used
is probably not the best way of doing things either. I'd imagine some
kind of static or session var would be better suited to that task.

"scott.marlowe" <scott.marlowe@ihs.com> writes:

On Mon, 8 Sep 2003, Bruce Momjian wrote:

I don't know how you could have an application that doesn't know if it
has issued a nextval() in the current connection. Unless you can explain
that, we have no intention of playing tricks with currval() for
connection pooling.

Actually, I would think the very act of using connection pooling would
ensure that applications may well not know whether or not a nextval had
been called.

The point is that it's not very sensible to be using currval except
immediately after a nextval --- usually in the same transaction, I would
think. Certainly, not resetting currval implies that there is
*potential* coupling between different transactions that happen to share
a connection. But ISTM that such coupling would represent a bug in the
application.

Chris said he was using currval being undefined to know that no rows
were inserted, but this seems less than compelling to me (why not look
at the results of the insert commands you used?). I'd support adding a
currval-reset feature if someone can make a more compelling argument why
a connection-pooling application would need it.

There are big chunks of other state in the backend that are not
resettable --- prepared statements being one that I think will have much
more visibility in 7.4. Should we offer something to let all prepared
statements be dropped? Would connection poolers actually find it
useful? (I'd think it much more likely they want to re-use prepared
statements.)

regards, tom lane

I would suggest to throw a error, or at least a warning.

This will FORCE people to program in the correct way.

I also thought that 'IF $1 THEN ...' should work ok but giving it a other
thought it's indeed stuped to write that way (I'm from the C world...)

Ries

-----Oorspronkelijk bericht-----
Van: pgsql-sql-owner@postgresql.org
[mailto:pgsql-sql-owner@postgresql.org]Namens Tom Lane
Verzonden: maandag 8 september 2003 17:41
Aan: pgsql-hackers@postgresql.org; pgsql-sql@postgresql.org
Onderwerp: [SQL] plpgsql doesn't coerce boolean expressions to boolean

Following up this gripe
http://archives.postgresql.org/pgsql-sql/2003-09/msg00044.php
I've realized that plpgsql just assumes that the test expression
of an IF, WHILE, or EXIT statement is a boolean expression. It
doesn't take any measures to ensure this is the case or convert
the value if it's not the case. This seems pretty bogus to me.

However ... with the code as it stands, for pass-by-reference datatypes
any nonnull value will appear TRUE, while for pass-by-value datatypes
any nonzero value will appear TRUE. I fear that people may actually be
depending on these behaviors, particularly the latter one which is
pretty reasonable if you're accustomed to C. So while I'd like to throw
an error if the argument isn't boolean, I'm afraid of breaking people's
function definitions.

Here are some possible responses, roughly in order of difficulty
to implement:

1. Leave well enough alone (and perhaps document the behavior).

2. Throw an error if the expression doesn't return boolean.

3. Try to convert nonbooleans to boolean using plpgsql's usual method
for cross-type coercion, ie run the type's output proc to get a
string and feed it to bool's input proc. (This seems unlikely to
avoid throwing an error in very many cases, but it'd be the most
consistent with other parts of plpgsql.)

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

Any opinions about what to do?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

On Mon, 8 Sep 2003, Chris Gamache wrote:

I'm using sequences and currval() to retrieve the last inserted row in a table.

If currval() is undefined, as it is when a connection is made, then I know no
rows were inserted in that table and can take a different action. This is
problematic when using a connection pooling library, as the value of currval()
for any given sequence could possibly be set from a previous "connection".

One (theoretical) workaround would be to issue some sort of command to the
back-end database to wipe all values of currval() when a "new" connection is
made. I've done some digging in the system tables and source code, and can't
find an obvious solution. Perhaps one you you gurus can suggest a SQL statement
to do such a thing.

Alternately, if there is a better way to retrieve the last inserted row for any
given table, I'd be very grateful for the tip. It would need to be independent
of the connection history, and undefined if there has not been a row inserted
to the table during a definable interval of time (drop anchor when the
"connection" begins, raise anchor when the "connection" ends), and be
independant of the other connections inserting rows to the same table.

Any idaeas?

Are you writing in java?
If yes, then implementing a wrapper around Connection would be a way.

CG

__________________________________
Do you Yahoo!?
Yahoo! SiteBuilder - Free, easy-to-use web site design software
http://sitebuilder.yahoo.com

---------------------------(end of broadcast)---------------------------
TIP 9: the planner will ignore your desire to choose an index scan if your
joining column's datatypes do not match

--
==================================================================
Achilleus Mantzios
S/W Engineer
IT dept
Dynacom Tankers Mngmt
Nikis 4, Glyfada
Athens 16610
Greece
tel: +30-210-8981112
fax: +30-210-8981877
email: achill at matrix dot gatewaynet dot com
mantzios at softlab dot ece dot ntua dot gr

On Mon, 8 Sep 2003, Tom Lane wrote:

"scott.marlowe" <scott.marlowe@ihs.com> writes:

On Mon, 8 Sep 2003, Bruce Momjian wrote:

I don't know how you could have an application that doesn't know if it
has issued a nextval() in the current connection. Unless you can explain
that, we have no intention of playing tricks with currval() for
connection pooling.

Actually, I would think the very act of using connection pooling would
ensure that applications may well not know whether or not a nextval had
been called.

The point is that it's not very sensible to be using currval except
immediately after a nextval --- usually in the same transaction, I would
think.

I'm pretty sure my second paragraph agreed with you on that.

Certainly, not resetting currval implies that there is
*potential* coupling between different transactions that happen to share
a connection. But ISTM that such coupling would represent a bug in the
application.

And that one too.

Chris said he was using currval being undefined to know that no rows
were inserted, but this seems less than compelling to me (why not look
at the results of the insert commands you used?). I'd support adding a
currval-reset feature if someone can make a more compelling argument why
a connection-pooling application would need it.

I'd say that if someone is looking at that, it would be better to have
some kind of reset_connection call that makes a connection look like you
just established it.

Bit I'd never use it.

On Mon, 08 Sep 2003 11:40:32 -0400, Tom Lane <tgl@sss.pgh.pa.us>
wrote:

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

I vote for 4. And - being fully aware of similar proposals having
failed miserably - I propose to proceed as follows:

If the current behaviour is considered a bug, let i=4, else let i=5.

In 7.i: Create a new GUC variable "plpgsql_strict_boolean" (silly
name, I know) in the "VERSION/PLATFORM COMPATIBILITY" section of
postgresql.conf. Make the new behaviour dependent on this variable.
Default plpgsql_strict_boolean to false. Place a warning into the
release notes and maybe into the plpgsql documentation.

In 7.j, j>i: Change the default value of plpgsql_strict_boolean to
true. Issue WARNINGs or NOTICEs as appropriate. Update
documentation.

In 7.k, k>j: Remove old behaviour and GUC variable. Update
documentation.

Servus
Manfred

Define the language! If it breaks code, so be it.

2. Throw an error if the expression doesn't return boolean.
Yes, yes, absolutely.

By definition "an IF, WHILE, or EXIT statement is a boolean expression"
SO
if "some stupid piece of text" THEN
should not compile, there is no BOOLEAN expression.

C's implementation of hat is true and false has always, IMHO, been hideous.
But then again, I am a Pascal kind of thinker.
An integer with a value of 1 is still only an integer,
IF I <> 0 THEN ...
is clear and un-ambiguous.

Tom Lane wrote:

Manfred Koizar <mkoi-pg@aon.at> writes:

On Mon, 08 Sep 2003 11:40:32 -0400, Tom Lane <tgl@sss.pgh.pa.us>
wrote:

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

I vote for 4.

I'm willing to do that.

And - being fully aware of similar proposals having
failed miserably - I propose to proceed as follows:

If the current behaviour is considered a bug, let i=4, else let i=5.

In 7.i: Create a new GUC variable "plpgsql_strict_boolean" (silly
name, I know) in the "VERSION/PLATFORM COMPATIBILITY" section of
postgresql.conf. Make the new behaviour dependent on this variable.
Default plpgsql_strict_boolean to false. Place a warning into the
release notes and maybe into the plpgsql documentation.

In 7.j, j>i: Change the default value of plpgsql_strict_boolean to
true. Issue WARNINGs or NOTICEs as appropriate. Update
documentation.

In 7.k, k>j: Remove old behaviour and GUC variable. Update
documentation.

I'm not willing to do that much work for what is, in the greater scheme
of things, a tiny change. If we did that for every user-visible change,
our rate of forward progress would be a mere fraction of what it is.

regards, tom lane

Tom Lane wrote:

Following up this gripe
http://archives.postgresql.org/pgsql-sql/2003-09/msg00044.php
I've realized that plpgsql just assumes that the test expression
of an IF, WHILE, or EXIT statement is a boolean expression. It
doesn't take any measures to ensure this is the case or convert
the value if it's not the case. This seems pretty bogus to me.

However ... with the code as it stands, for pass-by-reference datatypes
any nonnull value will appear TRUE, while for pass-by-value datatypes
any nonzero value will appear TRUE. I fear that people may actually be
depending on these behaviors, particularly the latter one which is
pretty reasonable if you're accustomed to C. So while I'd like to throw
an error if the argument isn't boolean, I'm afraid of breaking people's
function definitions.

Here are some possible responses, roughly in order of difficulty
to implement:

1. Leave well enough alone (and perhaps document the behavior).

2. Throw an error if the expression doesn't return boolean.

ERROR is the cleanest way, but I'd vote for conversion to boolean to
keep the damage within reason.

Jan

3. Try to convert nonbooleans to boolean using plpgsql's usual method
for cross-type coercion, ie run the type's output proc to get a
string and feed it to bool's input proc. (This seems unlikely to
avoid throwing an error in very many cases, but it'd be the most
consistent with other parts of plpgsql.)

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

Any opinions about what to do?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #

Jan Wieck <JanWieck@Yahoo.com> writes:

ERROR is the cleanest way, but I'd vote for conversion to boolean to
keep the damage within reason.

Which style of conversion did you like? These were the choices:

3. Try to convert nonbooleans to boolean using plpgsql's usual method
for cross-type coercion, ie run the type's output proc to get a
string and feed it to bool's input proc. (This seems unlikely to
avoid throwing an error in very many cases, but it'd be the most
consistent with other parts of plpgsql.)

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

At this point I'm kinda leaning to #4, because (for example) people
could create a cast from integer to boolean to avoid having to fix their
plpgsql functions right away. #3 would not offer any configurability of
behavior.

regards, tom lane

Tom Lane wrote:

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

At this point I'm kinda leaning to #4, because (for example) people
could create a cast from integer to boolean to avoid having to fix their
plpgsql functions right away. #3 would not offer any configurability of
behavior.

Won't people have to analyse their functions to find out what sort of
casts they need to create? If so, why don't they just fix the functions
while they are about it? Surely the fixes in most cases will be quite
trivial, and in all cases backwards compatible.

Does anyone have a take on how many people would be affected? Or how
much they would be affected?

cheers

andrew

Tom Lane wrote:

Manfred Koizar <mkoi-pg@aon.at> writes:

On Mon, 08 Sep 2003 11:40:32 -0400, Tom Lane <tgl@sss.pgh.pa.us>
wrote:

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

I vote for 4.

I'm willing to do that.

OK, what release should we do this?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tom Lane wrote:

Jan Wieck <JanWieck@Yahoo.com> writes:

ERROR is the cleanest way, but I'd vote for conversion to boolean to
keep the damage within reason.

Which style of conversion did you like? These were the choices:

3. Try to convert nonbooleans to boolean using plpgsql's usual method
for cross-type coercion, ie run the type's output proc to get a
string and feed it to bool's input proc. (This seems unlikely to
avoid throwing an error in very many cases, but it'd be the most
consistent with other parts of plpgsql.)

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

At this point I'm kinda leaning to #4, because (for example) people
could create a cast from integer to boolean to avoid having to fix their
plpgsql functions right away. #3 would not offer any configurability of
behavior.

Agreed - #4.

Thinking of the problem about deprication of features and transition
time, it would be nice for this kind of compatibility breaking changes
to have a _per database_ config option that controls old vs. new
behaviour, wouldn't it? Don't know exactly how you'd like that to be.
Maybe with a pg_config catalog that inherits default settings from
template1 but can then be changed in every database. This would even
include the possibility to *switch* one single prod database back to the
old behaviour in case the supposedly cleaned up application isn't as
clean as supposed to.

Jan

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #

Where are we on this --- we all decided on #4. Does this just require
an announcment in the release notes.

(I need to complete the release notes soon.)

---------------------------------------------------------------------------

Tom Lane wrote:

Following up this gripe
http://archives.postgresql.org/pgsql-sql/2003-09/msg00044.php
I've realized that plpgsql just assumes that the test expression
of an IF, WHILE, or EXIT statement is a boolean expression. It
doesn't take any measures to ensure this is the case or convert
the value if it's not the case. This seems pretty bogus to me.

However ... with the code as it stands, for pass-by-reference datatypes
any nonnull value will appear TRUE, while for pass-by-value datatypes
any nonzero value will appear TRUE. I fear that people may actually be
depending on these behaviors, particularly the latter one which is
pretty reasonable if you're accustomed to C. So while I'd like to throw
an error if the argument isn't boolean, I'm afraid of breaking people's
function definitions.

Here are some possible responses, roughly in order of difficulty
to implement:

1. Leave well enough alone (and perhaps document the behavior).

2. Throw an error if the expression doesn't return boolean.

3. Try to convert nonbooleans to boolean using plpgsql's usual method
for cross-type coercion, ie run the type's output proc to get a
string and feed it to bool's input proc. (This seems unlikely to
avoid throwing an error in very many cases, but it'd be the most
consistent with other parts of plpgsql.)

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

Any opinions about what to do?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Where are we on this --- we all decided on #4. Does this just require
an announcment in the release notes.

I haven't done anything about it --- been busy with other stuff, and I
wasn't sure we'd agreed to change it for 7.4 anyway. I'm willing to
make the code change though.

regards, tom lane

Jan Wieck <JanWieck@Yahoo.com> writes:

Tom Lane wrote:

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

Agreed - #4.

My first attempt at doing this failed to pass the regression tests,
because it wasn't prepared for this:

if count(*) = 0 from Room where roomno = new.roomno then
raise exception ''Room % does not exist'', new.roomno;
end if;

Is this really intended to be a feature? It manages to work because
plpgsql simply sticks "SELECT " in front of whatever appears between
IF and THEN, and passes the result to the main SQL engine. But it sure
surprised the heck out of me. The documentation gives no hint that
you're allowed to write anything but a straight boolean expression in IF.
Does Oracle allow that sort of thing?

I would be inclined to think that a more reasonable expression of the
intent would be

if (select count(*) from Room where roomno = new.roomno) = 0 then

Certainly we'd have a big problem supporting the existing coding if we
ever reimplement plpgsql with more awareness of what expressions are.

regards, tom lane

Tom Lane wrote:

Jan Wieck <JanWieck@Yahoo.com> writes:

Tom Lane wrote:

4. Use the parser's coerce_to_boolean procedure, so that nonbooleans
will be accepted in exactly the same cases where they'd be accepted
in a boolean-requiring SQL construct (such as CASE). (By default,
none are, so this isn't really different from #2. But people could
create casts to boolean to override this behavior in a controlled
fashion.)

Agreed - #4.

My first attempt at doing this failed to pass the regression tests,
because it wasn't prepared for this:

if count(*) = 0 from Room where roomno = new.roomno then
raise exception ''Room % does not exist'', new.roomno;
end if;

Is this really intended to be a feature? It manages to work because
plpgsql simply sticks "SELECT " in front of whatever appears between
IF and THEN, and passes the result to the main SQL engine. But it sure
surprised the heck out of me. The documentation gives no hint that
you're allowed to write anything but a straight boolean expression in IF.
Does Oracle allow that sort of thing?

I have to admit it was less an intention than more a side effect of the
actual implementation. It was so easy to simply stick "SELECT " in front
of "everything between IF and THEN" and expect the result to be a boolean.

In the same way you can do

varname := count(*) from Room where roomno = new.roomno;

which is straight forward because it's simply sticking "SELECT " in
front of "everything between := and ;". Well, this does a bit more in
that it tries the typinput(typoutput(result)) casting hack ... I know
that you don't like that one.

I would be inclined to think that a more reasonable expression of the
intent would be

if (select count(*) from Room where roomno = new.roomno) = 0 then

Certainly we'd have a big problem supporting the existing coding if we
ever reimplement plpgsql with more awareness of what expressions are.

Without parsing much, much more, and finally parsing basically the whole
SQL grammar in the PL/pgSQL parser, I don't see how you can do that.

Jan

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #

Jan Wieck <JanWieck@Yahoo.com> writes:

Tom Lane wrote:

if count(*) = 0 from Room where roomno = new.roomno then
raise exception ''Room % does not exist'', new.roomno;
end if;

Is this really intended to be a feature?

I have to admit it was less an intention than more a side effect of the
actual implementation. It was so easy to simply stick "SELECT " in front
of "everything between IF and THEN" and expect the result to be a boolean.

Sure, it was easy given a certain implementation technique. Question
is, do we want to consider it a supported feature even if it makes the
implementation hard?

In the same way you can do
varname := count(*) from Room where roomno = new.roomno;

This actually doesn't bother me; I see it as simply a variant syntax
for SELECT INTO. (Perhaps it should be documented that way.)

If we want to preserve this behavior for IF et al, I don't think there
is any practical way to apply SQL-level type coercion as I had wanted.
We could instead make the code act like it's assigning to a plpgsql
boolean variable --- but it will apply plpgsql's textual conversion
methods, not SQL type coercion.

regards, tom lane

I said:

If we want to preserve this behavior for IF et al, I don't think there
is any practical way to apply SQL-level type coercion as I had wanted.
We could instead make the code act like it's assigning to a plpgsql
boolean variable --- but it will apply plpgsql's textual conversion
methods, not SQL type coercion.

I have now written and tested the patch for this --- it's doing what I
originally called option 3:

3. Try to convert nonbooleans to boolean using plpgsql's usual method
for cross-type coercion, ie run the type's output proc to get a
string and feed it to bool's input proc. (This seems unlikely to
avoid throwing an error in very many cases, but it'd be the most
consistent with other parts of plpgsql.)

Unless someone objects, I'll commit this.

regards, tom lane