Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread tv 
 Hi,
 I was trying to create a trigger with parameters. I've found a potential
 bug
 when the param is boolean.

 Here is code replicating the bug:

 CREATE TABLE x(x TEXT);

 CREATE OR REPLACE FUNCTION trigger_x() RETURNS TRIGGER AS $$
 BEGIN
 RETURN NEW;
 END; $$ LANGUAGE PLPGSQL;

 CREATE TRIGGER trig_x_text BEFORE INSERT ON x FOR EACH ROW EXECUTE
 PROCEDURE
 trigger_x('text');
 CREATE TRIGGER trig_x_int BEFORE INSERT ON x FOR EACH ROW EXECUTE
 PROCEDURE
 trigger_x(10);
 CREATE TRIGGER trig_x_float BEFORE INSERT ON x FOR EACH ROW EXECUTE
 PROCEDURE trigger_x(42.0);
 CREATE TRIGGER trig_x_bool BEFORE INSERT ON x FOR EACH ROW EXECUTE
 PROCEDURE
 trigger_x(true);

 ERROR:  syntax error at or near true
 LINE 1: ... INSERT ON x FOR EACH ROW EXECUTE PROCEDURE trigger_x(true);

The docs clearly state what the valid values are and the literal 'true' is
not one of them (TRUE is). See this:

http://www.postgresql.org/docs/9.0/interactive/datatype-boolean.html

regards
Tomas


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Szymon Guz 
On 11 May 2011 10:56, t...@fuzzy.cz wrote:

  Hi,
  I was trying to create a trigger with parameters. I've found a potential
  bug
  when the param is boolean.
 
  Here is code replicating the bug:
 
  CREATE TABLE x(x TEXT);
 
  CREATE OR REPLACE FUNCTION trigger_x() RETURNS TRIGGER AS $$
  BEGIN
  RETURN NEW;
  END; $$ LANGUAGE PLPGSQL;
 
  CREATE TRIGGER trig_x_text BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE
  trigger_x('text');
  CREATE TRIGGER trig_x_int BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE
  trigger_x(10);
  CREATE TRIGGER trig_x_float BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE trigger_x(42.0);
  CREATE TRIGGER trig_x_bool BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE
  trigger_x(true);
 
  ERROR:  syntax error at or near true
  LINE 1: ... INSERT ON x FOR EACH ROW EXECUTE PROCEDURE trigger_x(true);

 The docs clearly state what the valid values are and the literal 'true' is
 not one of them (TRUE is). See this:

 http://www.postgresql.org/docs/9.0/interactive/datatype-boolean.html

 regards
 Tomas


Well... no.

In the link you've provided there is something different:


Valid literal values for the true state are:

TRUE't''true''y''yes''on''1'

so I could use 'true'... and this doesn't work.

And SQL is not case sensitive... but I will check it for you anyway:

CREATE TRIGGER trig_x_2 BEFORE INSERT ON x FOR EACH ROW EXECUTE PROCEDURE
trigger_x(TRUE);

ERROR:  syntax error at or near TRUE
LINE 1: ... INSERT ON x FOR EACH ROW EXECUTE PROCEDURE trigger_x(TRUE);

regards
Szymon

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Andreas Joseph Krogh 
På onsdag 11. mai 2011 kl 10:56:19 skrev t...@fuzzy.cz:
  Hi,
  I was trying to create a trigger with parameters. I've found a potential
  bug
  when the param is boolean.
 
  Here is code replicating the bug:
 
  CREATE TABLE x(x TEXT);
 
  CREATE OR REPLACE FUNCTION trigger_x() RETURNS TRIGGER AS $$
  BEGIN
  RETURN NEW;
  END; $$ LANGUAGE PLPGSQL;
 
  CREATE TRIGGER trig_x_text BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE
  trigger_x('text');
  CREATE TRIGGER trig_x_int BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE
  trigger_x(10);
  CREATE TRIGGER trig_x_float BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE trigger_x(42.0);
  CREATE TRIGGER trig_x_bool BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE
  trigger_x(true);
 
  ERROR:  syntax error at or near true
  LINE 1: ... INSERT ON x FOR EACH ROW EXECUTE PROCEDURE trigger_x(true);
 
 The docs clearly state what the valid values are and the literal 'true' is
 not one of them (TRUE is). See this:
 
 http://www.postgresql.org/docs/9.0/interactive/datatype-boolean.html

What are you trying to accomplish? CREATE OR REPLACE FUNCTION trigger_x() 
does not declare any formal-parameters, so calling it with arguments doesn't 
make sense. I'm surprised creating the other triggers didn't produce an error 
stating No function defined with the name trigger_ix and the given 
argument-type.

--
Andreas Joseph Krogh andr...@officenet.no
Senior Software Developer / CTO
Public key: http://home.officenet.no/~andreak/public_key.asc
+-+
OfficeNet AS| The most difficult thing in the world is to |
Rosenholmveien 25   | know how to do a thing and to watch |
1414 Trollåsen  | somebody else doing it wrong, without   |
NORWAY  | comment.|
Org.nr: NO 981 479 076  | |
| |
Tlf:+47 24 15 38 90 | |
Fax:+47 24 15 38 91 | |
Mobile: +47 909  56 963 | |
+-+

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Andres Freund 
On Wednesday, May 11, 2011 11:01:56 AM Andreas Joseph Krogh wrote:
 På onsdag 11. mai 2011 kl 10:56:19 skrev t...@fuzzy.cz:
   CREATE TRIGGER trig_x_bool BEFORE INSERT ON x FOR EACH ROW EXECUTE
   PROCEDURE
   trigger_x(true);
  The docs clearly state what the valid values are and the literal 'true'
  is not one of them (TRUE is). See this:
  
  http://www.postgresql.org/docs/9.0/interactive/datatype-boolean.html
 
 What are you trying to accomplish? CREATE OR REPLACE FUNCTION trigger_x()
 does not declare any formal-parameters, so calling it with arguments
 doesn't make sense. I'm surprised creating the other triggers didn't
 produce an error stating No function defined with the name trigger_ix and
 the given argument-type.
Read the docs. Parameters for triggers are not passed as normal function 
parameters. Thats why you access them via via TG_ARGV in plpgsql.

The grammar accepts only a very limited amount of parameters there:

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Andres Freund 
On Wednesday, May 11, 2011 11:21:34 AM Andres Freund wrote:
 On Wednesday, May 11, 2011 11:01:56 AM Andreas Joseph Krogh wrote:
  På onsdag 11. mai 2011 kl 10:56:19 skrev t...@fuzzy.cz:
CREATE TRIGGER trig_x_bool BEFORE INSERT ON x FOR EACH ROW EXECUTE
PROCEDURE
trigger_x(true);
   
   The docs clearly state what the valid values are and the literal 'true'
   is not one of them (TRUE is). See this:
   
   http://www.postgresql.org/docs/9.0/interactive/datatype-boolean.html
  
  What are you trying to accomplish? CREATE OR REPLACE FUNCTION
  trigger_x() does not declare any formal-parameters, so calling it with
  arguments doesn't make sense. I'm surprised creating the other triggers
  didn't produce an error stating No function defined with the name
  trigger_ix and the given argument-type.
 
 Read the docs. Parameters for triggers are not passed as normal function
 parameters. Thats why you access them via via TG_ARGV in plpgsql.
 
 The grammar accepts only a very limited amount of parameters there:
Err

TriggerFuncArg:
Iconst
{
char buf[64];
snprintf(buf, sizeof(buf), %d, $1);
$$ = makeString(pstrdup(buf));
}
| FCONST
{ $$ = makeString($1); }
| Sconst
{ $$ = makeString($1); }
| BCONST
{ $$ = makeString($1); }
| XCONST
{ $$ = makeString($1); }
| ColId 
{ $$ = makeString($1); }

That is integers, floating point, strings, bitstrings, hexstrings and column 
references (???).

How that exact list came to exist I do not know.

Andres

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Szymon Guz 
On 11 May 2011 11:01, Andreas Joseph Krogh andr...@officenet.no wrote:

 På onsdag 11. mai 2011 kl 10:56:19 skrev t...@fuzzy.cz:
   Hi,
   I was trying to create a trigger with parameters. I've found a
 potential
   bug
   when the param is boolean.
  
   Here is code replicating the bug:
  
   CREATE TABLE x(x TEXT);
  
   CREATE OR REPLACE FUNCTION trigger_x() RETURNS TRIGGER AS $$
   BEGIN
   RETURN NEW;
   END; $$ LANGUAGE PLPGSQL;
  
   CREATE TRIGGER trig_x_text BEFORE INSERT ON x FOR EACH ROW EXECUTE
   PROCEDURE
   trigger_x('text');
   CREATE TRIGGER trig_x_int BEFORE INSERT ON x FOR EACH ROW EXECUTE
   PROCEDURE
   trigger_x(10);
   CREATE TRIGGER trig_x_float BEFORE INSERT ON x FOR EACH ROW EXECUTE
   PROCEDURE trigger_x(42.0);
   CREATE TRIGGER trig_x_bool BEFORE INSERT ON x FOR EACH ROW EXECUTE
   PROCEDURE
   trigger_x(true);
  
   ERROR:  syntax error at or near true
   LINE 1: ... INSERT ON x FOR EACH ROW EXECUTE PROCEDURE trigger_x(true);
 
  The docs clearly state what the valid values are and the literal 'true'
 is
  not one of them (TRUE is). See this:
 
  http://www.postgresql.org/docs/9.0/interactive/datatype-boolean.html

 What are you trying to accomplish? CREATE OR REPLACE FUNCTION trigger_x()
 does not declare any formal-parameters, so calling it with arguments doesn't
 make sense. I'm surprised creating the other triggers didn't produce an
 error stating No function defined with the name trigger_ix and the given
 argument-type.


That's how you define trigger function. Later you can use params when
defining trigger.

regards
Szymon

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Andreas Joseph Krogh 
P onsdag 11. mai 2011 kl 11:30:51 skrev Szymon Guz mabew...@gmail.com:
 

On 11 May 2011 11:01, Andreas Joseph Krogh andr...@officenet.no wrote:
 P onsdag 11. mai 2011 kl 10:56:19 skrev t...@fuzzy.cz:


  Hi,
  I was trying to create a trigger with parameters. I've found a potential
  bug
  when the param is boolean.
 
  Here is code replicating the bug:
 
  CREATE TABLE x(x TEXT);
 
  CREATE OR REPLACE FUNCTION trigger_x() RETURNS TRIGGER AS $$
  BEGIN
  RETURN NEW;
  END; $$ LANGUAGE PLPGSQL;
 
  CREATE TRIGGER trig_x_text BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE
  trigger_x('text');
  CREATE TRIGGER trig_x_int BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE
  trigger_x(10);
  CREATE TRIGGER trig_x_float BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE trigger_x(42.0);
  CREATE TRIGGER trig_x_bool BEFORE INSERT ON x FOR EACH ROW EXECUTE
  PROCEDURE
  trigger_x(true);
 
  ERROR: syntax error at or near true
  LINE 1: ... INSERT ON x FOR EACH ROW EXECUTE PROCEDURE trigger_x(true);

 The docs clearly state what the valid values are and the literal 'true' is
 not one of them (TRUE is). See this:

 http://www.postgresql.org/docs/9.0/interactive/datatype-boolean.html


What are you trying to accomplish? CREATE OR REPLACE FUNCTION trigger_x() does not declare any formal-parameters, so calling it with arguments doesn't make sense. I'm surprised creating the other triggers didn't produce an error stating No function defined with the name trigger_ix and the given argument-type.



That's how you define trigger function. Later you can use params when defining trigger.


Pardon my ignorance:-)

--
Andreas Joseph Krogh andr...@officenet.no
Senior Software Developer / CTO
Public key: http://home.officenet.no/~andreak/public_key.asc
+-+
OfficeNet AS  | The most difficult thing in the world is to |
Rosenholmveien 25   | know how to do a thing and to watch|
1414 Trollsen | somebody else doing it wrong, without   |
NORWAY | comment.  |
Org.nr: NO 981 479 076 |  |
|  |
Tlf:  +47 24 15 38 90 |  |
Fax:  +47 24 15 38 91 |  |
Mobile: +47 909 56 963 |  |
+-+

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Andres Freund 
On Wednesday, May 11, 2011 11:50:35 AM Szymon Guz wrote:
 On 11 May 2011 11:29, Andres Freund and...@anarazel.de wrote:
  On Wednesday, May 11, 2011 11:21:34 AM Andres Freund wrote:
   On Wednesday, May 11, 2011 11:01:56 AM Andreas Joseph Krogh wrote:
På onsdag 11. mai 2011 kl 10:56:19 skrev t...@fuzzy.cz:
  CREATE TRIGGER trig_x_bool BEFORE INSERT ON x FOR EACH ROW
  EXECUTE PROCEDURE
  trigger_x(true);
 
 The docs clearly state what the valid values are and the literal
  
  'true'
  
 is not one of them (TRUE is). See this:
 
 http://www.postgresql.org/docs/9.0/interactive/datatype-boolean.htm
 l

What are you trying to accomplish? CREATE OR REPLACE FUNCTION
trigger_x() does not declare any formal-parameters, so calling it
with arguments doesn't make sense. I'm surprised creating the other
triggers didn't produce an error stating No function defined with
the name trigger_ix and the given argument-type.
   
   Read the docs. Parameters for triggers are not passed as normal
   function parameters. Thats why you access them via via TG_ARGV in
   plpgsql.
  
   The grammar accepts only a very limited amount of parameters there:
  Err
  
  TriggerFuncArg:
 Iconst
 
 {
 
 char buf[64];
 snprintf(buf, sizeof(buf), %d,
  
  $1);
  
 $$ = makeString(pstrdup(buf));
 
 }
 | 
 | FCONST
 
 { $$ = makeString($1); }
 
 | Sconst
 
 { $$ = makeString($1); }
 
 | BCONST
 
 { $$ = makeString($1); }
 
 | XCONST
 
 { $$ = makeString($1); }
 
 | ColId
  
  { $$ = makeString($1); }
  
  That is integers, floating point, strings, bitstrings, hexstrings and
  column references (???).
  
  How that exact list came to exist I do not know.
 
 My two thoughts on that:
 
 1. This list should be improved to allow booleans, and maybe other types
 
 2. Why then is it like this:
 
 it works:
 CREATE TRIGGER trig_x_10 BEFORE INSERT ON x FOR EACH ROW EXECUTE PROCEDURE
 trigger_x('true');
 
 it does not:
 CREATE TRIGGER trig_x_11 BEFORE INSERT ON x FOR EACH ROW EXECUTE PROCEDURE
 trigger_x('true'::text);
 
 the error is:
 ERROR:  syntax error at or near ::
 
 I think there is something wrong.
The grammar doesn't allow any form of expression. It only allows the above 
listed types of literals directly.
I am not really sure why it was done that way, but its been that way for a 
long time (only insignificant changes since 1997... bitstrings and hex strings 
were added after that though).

Why do you wan't to use a boolean directly if you can't use it as the type 
itself anyway?

Andres

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Szymon Guz 
On 11 May 2011 12:06, Andres Freund and...@anarazel.de wrote:


 Why do you wan't to use a boolean directly if you can't use it as the type
 itself anyway?


Yep, and this is a really good point :)
I wanted to have consistent api, so use true when I have a boolean value.
I will use 'true' and add some info on that to the procedure documentation.

regards
Szymon

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Tom Lane 
Andres Freund and...@anarazel.de writes:
 The grammar accepts only a very limited amount of parameters there:
 Err

 TriggerFuncArg:
 Iconst
 {
 char buf[64];
 snprintf(buf, sizeof(buf), %d, $1);
 $$ = makeString(pstrdup(buf));
 }
 | FCONST{ $$ = makeString($1); }
 | Sconst{ $$ = makeString($1); }
 | BCONST{ $$ = makeString($1); }
 | XCONST{ $$ = makeString($1); }
 | ColId { $$ = makeString($1); }

 That is integers, floating point, strings, bitstrings, hexstrings and column 
 references (???).

 How that exact list came to exist I do not know.

The documentation for CREATE FUNCTION says

arguments:
An optional comma-separated list of arguments to be provided to
the function when the trigger is executed. The arguments are
literal string constants. Simple names and numeric constants can
be written here, too, but they will all be converted to
strings.

The ColId case is meant to cover the simple names proviso, but of
course it fails to cover reserved words.  We could trivially fix that
by writing ColLabel instead of ColId.  My initial expectation was that
this would bloat the parser, but it seems not to: the backend gram.o
is exactly the same size after making the change, and ecpg's preproc.o
actually gets smaller (more opportunity to share states?).  So I'm
inclined to do it, rather than having to document that simple names
excludes reserved words.

A possibly more interesting question is why BCONST and XCONST were added
there.  The documentation certainly does not say or suggest that those
are legal options, and what's more the behavior could be considered
surprising:

regression=# CREATE TRIGGER trig_x_bconst BEFORE INSERT ON x FOR EACH ROW 
EXECUTE PROCEDURE trigger_x(b'1011');
CREATE TRIGGER
regression=# CREATE TRIGGER trig_x_xconst BEFORE INSERT ON x FOR EACH ROW 
EXECUTE PROCEDURE trigger_x(x'1234abcd');
CREATE TRIGGER
regression=# \d+ x
...
Triggers:
trig_x_bconst BEFORE INSERT ON x FOR EACH ROW EXECUTE PROCEDURE 
trigger_x('b1011')
trig_x_xconst BEFORE INSERT ON x FOR EACH ROW EXECUTE PROCEDURE 
trigger_x('x1234abcd')

I'm inclined to take those out, because (1) I find it shrinks the
generated grammar a tad (these productions *do* add to the size of the
state tables), and (2) if we don't, we ought to document this behavior,
and I don't want to do that either.

I see this as just a change to make in HEAD, it's not appropriate for
a back-patch.

Objections anyone?

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Andres Freund 
On Wednesday, May 11, 2011 07:25:58 PM Tom Lane wrote:
 Andres Freund and...@anarazel.de writes:
  The grammar accepts only a very limited amount of parameters there:
  Err
  
  TriggerFuncArg:
  Iconst
  
  {
  
  char buf[64];
  snprintf(buf, sizeof(buf), %d, $1);
  $$ = makeString(pstrdup(buf));
  
  }
  | 
  | FCONST{ $$ =
  | makeString($1); } Sconst{
  | $$ = makeString($1); } BCONST 
  |   { $$ = makeString($1); } XCONST 
  |   { $$ = makeString($1); } ColId  
  |   { $$ = makeString($1); }
  
  That is integers, floating point, strings, bitstrings, hexstrings and
  column references (???).
  
  How that exact list came to exist I do not know.
 
 The documentation for CREATE FUNCTION says
 
 arguments:
   An optional comma-separated list of arguments to be provided to
   the function when the trigger is executed. The arguments are
   literal string constants. Simple names and numeric constants can
   be written here, too, but they will all be converted to
   strings.
 
 The ColId case is meant to cover the simple names proviso, but of
 course it fails to cover reserved words.  We could trivially fix that
 by writing ColLabel instead of ColId.  My initial expectation was that
 this would bloat the parser, but it seems not to: the backend gram.o
 is exactly the same size after making the change, and ecpg's preproc.o
 actually gets smaller (more opportunity to share states?).  So I'm
 inclined to do it, rather than having to document that simple names
 excludes reserved words.
Good.

 A possibly more interesting question is why BCONST and XCONST were added
 there.  The documentation certainly does not say or suggest that those
 are legal options, and what's more the behavior could be considered
 surprising:
It seems to have originally been added there by Peter (as BITCONST) and then 
split by Thomas Lockhart.

See 73874a06 and eb121ba2
 regression=# CREATE TRIGGER trig_x_bconst BEFORE INSERT ON x FOR EACH ROW
 EXECUTE PROCEDURE trigger_x(b'1011'); CREATE TRIGGER
 regression=# CREATE TRIGGER trig_x_xconst BEFORE INSERT ON x FOR EACH ROW
 EXECUTE PROCEDURE trigger_x(x'1234abcd'); CREATE TRIGGER
 regression=# \d+ x
 ...
 Triggers:
 trig_x_bconst BEFORE INSERT ON x FOR EACH ROW EXECUTE PROCEDURE
 trigger_x('b1011') trig_x_xconst BEFORE INSERT ON x FOR EACH ROW EXECUTE
 PROCEDURE trigger_x('x1234abcd')
Err. Yes, that looks rather strange. And surprising.

 I'm inclined to take those out, because (1) I find it shrinks the
 generated grammar a tad (these productions *do* add to the size of the
 state tables), and (2) if we don't, we ought to document this behavior,
 and I don't want to do that either.
 I see this as just a change to make in HEAD, it's not appropriate for
 a back-patch.
I would say the above behaviour even is a bug. But given that I haven't 
seen/found anybody complaining about it fixing it properly looks pointless.
So yes, HEAD only sounds fine.

 Objections anyone?
Nope.

Is there a special reason for not using the normal function calling 
mechanisms? It looks to me as it was just done to have an easy way to store it 
in pg_trigger.tgargs.

Andres

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] potential bug in trigger with boolean params

2011-05-11 Thread Tom Lane 
Andres Freund and...@anarazel.de writes:
 Is there a special reason for not using the normal function calling 
 mechanisms? It looks to me as it was just done to have an easy way to store 
 it 
 in pg_trigger.tgargs.

Well, this is all very historical, dating from Berkeley days AFAIK.
If we had it to do over, I bet we'd do it differently --- but the pain
of changing it seems to exceed any likely benefit.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers