Re: potential bug in trigger with boolean params - Mailing list pgsql-hackers

On Wednesday, May 11, 2011 07:25:58 PM Tom Lane wrote:
> Andres Freund <andres@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