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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.

> 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);