Обсуждение: 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