[tarantool-patches] Re: [PATCH v1 4/9] sql: rework diag_set() in OP_Halt

[Imeev Mergen <imeevma@tarantool.org>]

Hi! Thank you for the review! My answers below. Also, I think I'm
going to drop this patch, since I plan to move the error code from
p5 to p2 in the next patch.

On 6/2/19 7:35 PM, Vladislav Shpilevoy wrote:
>
> On 28/05/2019 14:39, imeevma@tarantool.org wrote:
>> Prior to this patch, the way to set Tarantool error in OP_Halt was
>> too universal. It was possible to set a description of the error
>> that does not match its errcode.
> There was a concrete reason, why it was possible - because different
> error codes have different arguments of various types, and the only way
> to set an error at parsing stage is to allow to set arbitrary error
> message to any error code. Without '...', va_arg etc. Besides, we could
> use it to set correct line number and function name in future. Now you
> use diag_set, which restricts us.
>
> So why do you need that patch? We will need to revert it when an error
> appears requiring more than one argument, or an argument of not
> const char * type. That will definitely happen.
For some reason, manually creating an error message looks a bit
wrong to me. How about creating a new macro to create such
messages, for example, diag_prepare(), which takes the same
arguments as diag_set() and returns the complete error message?

>> This change will also make it
>> easier to work with an error in OP_Halt, since you no longer need
>> to create a complete error message.>
>> Example of wrong error code:
>> ...
>>
>> tarantool> box.execute('select 1 limit true')
>> ---
>> - error: Only positive integers are allowed in the LIMIT clause
>> ...
>>
>> tarantool> box.error.last().code
>> ---
>> - 0
>> ...
>> ---
> This is not because of box_error_set usage.
>
> tarantool> box.error({code = 123, reason = 'test'})
> ---
> - error: test
> ...
>
> tarantool> box.error.last().code
> ---
> - 123
> ...
>
> This example uses box_error_set() too, but the error
> code is kept. The bug in SQL could be fixed in a few lines,
> and you did it. But somewhy decided to break generic
> error creation alongside.
>
> The whole patch could be shrunk to this (+ tests):
>
>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index d3472a9..3f0b540 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -2116,6 +2116,7 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
>>   				  0, 0,
>>   				  wrong_limit_error,
>>   				  P4_STATIC);
>> +		sqlVdbeChangeP5(v, ER_SQL_EXECUTE);>
>>   		sqlVdbeResolveLabel(v, positive_limit_label);
>>   		VdbeCoverage(v);
>> @@ -2178,6 +2178,7 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
>>   					  0, 0,
>>   					  wrong_offset_error,
>>   					  P4_STATIC);
>> +			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
>>   
>>   			sqlVdbeResolveLabel(v, positive_offset_label);
>>               		sqlReleaseTempReg(pParse, r1);
True, but I think I won't need this after I move errcode from p5
to p2 in the next patch.