Re: potential bug in trigger with boolean params

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