Re: [BUGS] BUG #8198: ROW() literals not supported in an IN clause

2013-06-06 Thread Rafał Rzepecki
On Wed, Jun 5, 2013 at 7:58 AM, Amit Kapila  wrote:
> On Wednesday, June 05, 2013 5:34 AM Rafał Rzepecki wrote:
>> On Tue, Jun 4, 2013 at 12:35 PM, Amit Kapila 
>> wrote:
>> > On Saturday, June 01, 2013 9:37 PM
>> >
>> >> Row type literals constructed with ROW() cause an error when used in
>> >> an IN clause (string literals casted appropriately are allowed).
>> This
>> >> is especially problematic since many client libraries use these
>> >> literals to pass values of row-type arguments, hence making it
>> >> impossible to use them in IN-clause queries.
>> >>
>>
>> If I'm right, the proper fix would be (patch 0001; caution, not
>> tested):
>>
>> --- a/src/backend/parser/parse_expr.c
>> +++ b/src/backend/parser/parse_expr.c
>> @@ -1203,10 +1203,9 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
>> Node   *rexpr = (Node *) lfirst(l);
>> Node   *cmp;
>>
>> -   if (haveRowExpr)
>> +   if (haveRowExpr && IsA(lexpr, RowExpr))
>> {
>> -   if (!IsA(lexpr, RowExpr) ||
>> -   !IsA(rexpr, RowExpr))
>> +   if (!IsA(rexpr, RowExpr))
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>>errmsg("arguments of row IN must all
>> be row expressions"),
>>
>>
>> Since the restriction seems a rather arbitrary (at least I fail to see
>> any reason for it), it can be removed altogether (patch 0002, not
>> tested as well):
>>
>> --- a/src/backend/parser/parse_expr.c
>> +++ b/src/backend/parser/parse_expr.c
>> @@ -1203,20 +1203,12 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
>> Node   *rexpr = (Node *) lfirst(l);
>> Node   *cmp;
>>
>> -   if (haveRowExpr)
>> -   {
>> -   if (!IsA(lexpr, RowExpr) ||
>> -   !IsA(rexpr, RowExpr))
>> -   ereport(ERROR,
>> -
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> -  errmsg("arguments of row IN must
>> all be row expressions"),
>> -
>> parser_errposition(pstate, a->location)));
>> +   if (IsA(lexpr, RowExpr) && IsA(rexpr, RowExpr))
>> cmp = make_row_comparison_op(pstate,
>>
>>   a->name,
>>   (List *)
>> copyObject(((RowExpr *) lexpr)->args),
>>
>>   ((RowExpr *) rexpr)->args,
>>
>>   a->location);
>> -   }
>> else
>> cmp = (Node *) make_op(pstate,
>>a-
>> >name,
>>
>
> I had tried, both your patches have passed all regression tests (tested on 
> Windows). I feel fixing it in a way similar to your Patch-1 would be
> more appropriate as with Patch-1 it can generate meaningful error message for 
> some cases like below:
>
> postgres=# select * from the_table where ROW('abc','def') in 
> (row('foo','bar')::the_row,12);
> ERROR:  arguments of row IN must all be row expressions
> LINE 1: select * from the_table where ROW('abc','def') in (row('foo'...

Perhaps you're right, rare cases when you want to do something like
'ROW('abc','def') in (row('foo','bar')::the_row, a_column)' are, I
suppose, so exotic that working around this restriction probably won't
be much of a hassle.
--
Rafał Rzepecki


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs


Re: [BUGS] BUG #8198: ROW() literals not supported in an IN clause

2013-06-04 Thread Rafał Rzepecki
xprIn() similar to 
> transformAExprOp() will fix this issue, but not sure if there is any other 
> side effect of same.

Thanks for the analysis! This problem seems to have been introduced in
3d376fce8dd4 [1] (almost eight years ago! I guess not many people use
row types...).

I'm pretty sure the original intent was to afford some extra checks so
that conditions such as "ROW(1, 2) IN ((ROW(3, 4), ROW(5, 6, 7))"
would get rejected at parsing time (CCing the original author; please
confirm).

If I'm right, the proper fix would be (patch 0001; caution, not tested):

--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1203,10 +1203,9 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
Node   *rexpr = (Node *) lfirst(l);
Node   *cmp;

-   if (haveRowExpr)
+   if (haveRowExpr && IsA(lexpr, RowExpr))
{
-   if (!IsA(lexpr, RowExpr) ||
-   !IsA(rexpr, RowExpr))
+   if (!IsA(rexpr, RowExpr))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
   errmsg("arguments of row IN must
all be row expressions"),


Since the restriction seems a rather arbitrary (at least I fail to see
any reason for it), it can be removed altogether (patch 0002, not
tested as well):

--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1203,20 +1203,12 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
Node   *rexpr = (Node *) lfirst(l);
Node   *cmp;

-   if (haveRowExpr)
-   {
-   if (!IsA(lexpr, RowExpr) ||
-   !IsA(rexpr, RowExpr))
-   ereport(ERROR,
-   (errcode(ERRCODE_SYNTAX_ERROR),
-  errmsg("arguments of row IN must
all be row expressions"),
-
parser_errposition(pstate, a->location)));
+   if (IsA(lexpr, RowExpr) && IsA(rexpr, RowExpr))
cmp = make_row_comparison_op(pstate,

  a->name,
  (List *)
copyObject(((RowExpr *) lexpr)->args),

  ((RowExpr *) rexpr)->args,

  a->location);
-   }
else
cmp = (Node *) make_op(pstate,
   a->name,


[1] 
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3d376fce8dd45d43fb6dbeb5a08c08400a589ff8
--
Rafał Rzepecki


0001-Only-use-make_row_comparison_op-in-transformAExprIn-.patch
Description: Binary data


0002-Allow-mixing-RowExprs-and-other-expressions-in-the-R.patch
Description: Binary data

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs