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

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 tothe function when the trigger is executed.
Thearguments areliteral string constants. Simple names and numeric constants canbe written here, too, but they will all
beconverted tostrings.
 

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
INSERTON 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