Thread: Why does TRIM() expect an expr_list?
In gram.y, the productions for the TRIM() expression expect an
argument of trim_list:
TRIM '(' trim_list ')'TRIM '(' TRAILING trim_list ')'TRIM '(' LEADING trim_list ')'TRIM '(' BOTH trim_list ')'
And trim_list is defined as:
trim_list: a_expr FROM expr_list { $$ = lappend($3, $1); } | FROM expr_list { $$ = $2; }
| expr_list { $$ = $1; }
But it seems wrong for trim_list to be defined in terms of
expr_list's. The way it's currently written, we allow expressions
such as:
TRIM( 'foo', now(), 4+2)
or
TRIM( LEADING FROM 'foo', 4+2)
The parser translates the TRIM expression into a call to btrim() (or
ltrim() or rtrim()) and we seem to (accidentally) make up a silly
argument list if the user includes an actual expr_list (with multiple
expressions).
The first example results in "function ltrim(unknown, timestamp with
time zone, integer) does not exist".
The second example above is translated to ltrim(4+2, 'foo').
It seems to me that trim_list should defined as:
trim_list: a_expr FROM a_expr { $$ = list_make2($3, $1); } | FROM a_expr { $$ =
list_make1($2);} | a_expr { $$ = list_make1($1); }
Am I missing something?
-- Korry
-----------------------------------------------------------------------
Korry Douglas
Senior Database Dude
EnterpriseDB Corporation
The Enterprise Postgres Company
Phone: (804)241-4301
Mobile: (620) EDB-NERD
Korry Douglas <korry.douglas@enterprisedb.com> writes:
> It seems to me that trim_list should defined as:
> trim_list: a_expr FROM a_expr { $$ = list_make2($3, $1); }
> | FROM a_expr { $$ = list_make1($2); }
> | a_expr { $$ = list_make1($1); }
> Am I missing something?
That will break the ability to call trim() with ordinary function
syntax.
We possibly could change that in conjunction with adding a straight
TRIM '(' expr_list ')' production, though.
What this looks like to me is somebody was trying to allow for future
extensions in the keyword-ized syntax, but I can't imagine the SQL
committee introducing a mix of keyword-ized and non-keyword-ized
arguments. So I agree that the expr_list cases are pretty silly
except for the bare no-keyword-anywhere path.
Actually, on closer examination I think there's another bug here.
I see this in SQL99:
<trim function> ::= TRIM <left paren> <trim operands> <right paren>
<trim operands> ::= [ [ <trim specification> ] [ <trim character> ] FROM ] <trim source>
<trim specification> ::= LEADING | TRAILING | BOTH
<trim character> ::= <character value expression>
<trim source> ::= <character value expression>
It looks to me like you're not supposed to be able to omit FROM if
you've written a <trim specification>. Should we tighten our
syntax to reject that?
regards, tom lane
On Tue, Apr 20, 2010 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > It looks to me like you're not supposed to be able to omit FROM if > you've written a <trim specification>. Should we tighten our > syntax to reject that? I would think not. If we were doing this from scratch we might choose to set it up that way, but I don't think it's worth breaking backward compatibility. I think we should just consider it a PostgreSQL extension. ...Robert
>> It seems to me that trim_list should defined as:
>
>> trim_list: a_expr FROM a_expr { $$ = list_make2($3, $1); }
>> | FROM a_expr { $$ = list_make1($2); }
>> | a_expr { $$ = list_make1($1); }
>
>> Am I missing something?
>
> That will break the ability to call trim() with ordinary function
> syntax.
>
> We possibly could change that in conjunction with adding a straight
> TRIM '(' expr_list ')' production, though.
Hmm... it seems counterintuitive to call TRIM() using ordinary
function syntax anyway. What would the argument list look like?
I think you would have to reverse the order of the arguments (and
there's no way to factor the LEADING/TRAILING/BOTH stuff into the
argument list since those map to calls to different functions).
For example to write:
TRIM( 'foo' FROM 'foobar' )
using function syntax, you would have to write:
TRIM( 'foobar', 'foo' )
As far as I know, that usage is not documented anywhere. And since
"trim" is not really a function, you can't discover the proper
argument list using \df
On the other hand, someone is surely (ab)using TRIM() that way...
> What this looks like to me is somebody was trying to allow for future
> extensions in the keyword-ized syntax, but I can't imagine the SQL
> committee introducing a mix of keyword-ized and non-keyword-ized
> arguments. So I agree that the expr_list cases are pretty silly
> except for the bare no-keyword-anywhere path.
I suspect that it was simply easier to write it that way than to code
the make_list1() invocations, but who knows.
> Actually, on closer examination I think there's another bug here.
> I see this in SQL99:
>
> <trim function> ::=
> TRIM <left paren> <trim operands> <right paren>
>
> <trim operands> ::=
> [ [ <trim specification> ] [ <trim character> ] FROM ]
> <trim source>
>
> <trim specification> ::=
> LEADING
> | TRAILING
> | BOTH
>
> <trim character> ::= <character value expression>
>
> <trim source> ::= <character value expression>
>
> It looks to me like you're not supposed to be able to omit FROM if
> you've written a <trim specification>. Should we tighten our
> syntax to reject that?
That depends on how much code you want to break. Doesn't really
matter to me.
-- Korry
-----------------------------------------------------------------------
Korry Douglas
Senior Database Dude
EnterpriseDB Corporation
The Enterprise Postgres Company
Phone: (804)241-4301
Mobile: (620) EDB-NERD
Korry Douglas <korry.douglas@enterprisedb.com> writes:
>> That will break the ability to call trim() with ordinary function
>> syntax.
> Hmm... it seems counterintuitive to call TRIM() using ordinary
> function syntax anyway. What would the argument list look like?
"foo, bar, baz", just like any other function. This is important because
we don't use the weird keyword-ized syntax when dumping out function
calls in rules and suchlike. Also, in general it's preferable to not
prevent users from creating their own functions that happen to be named
like a system function. (I think the current code fails to achieve that
last goal because of the forced function name remapping, but perhaps it
should be fixed if we're going to mess with it.)
regards, tom lane