[tarantool-patches] Re: [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE

["n.pettik" <korablev@tarantool.org>]

>>>>> + */
>>>>> +case OP_Error: {             /* jump */
>>>>> +  box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z);
>>>> 
>>>> Why not simple diag_set() ?
>>>> 
>>> I did it like this because using diag_set() would be a bit
>>> troublesome if we decided to use an argument of a different type
>>> or more than one argument.
>> 
>> Do not understand what you mean. Could you provide
>> some examples?
>> 
> Sorry, I can't. But I have beed said so by Vlad:
> 
> On 6/2/19 7:35 PM, Vladislav Shpilevoy wrote:
>> 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.
>> 
> 
> It can be ween here:
> https://www.freelists.org/post/tarantool-patches/PATCH-v1-49-sql-rework-diag-set-in-OP-Halt,1

Ok, now it’s clear: it allows us to construct string containing
error message during parsing stage, so that we avoid argument
substitution in diag_set(). It is vital since "error codes have
different arguments of various types”.

>>> Needed for #4183
>>> 
>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>> index 292168f..aab60dd 100644
>>> --- a/src/box/sql/build.c
>>> +++ b/src/box/sql/build.c
>>> @@ -3277,15 +3277,15 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
>>> int cursor = parser->nTab++;
>>> vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id));
>>> sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
>>> -  int label = sqlVdbeCurrentAddr(v);
>>> -  sqlVdbeAddOp4Int(v, cond_opcode, cursor, label + 3, key_reg,
>>> -           key_len);
>>> +  int jmp = sqlVdbeAddOp4Int(v, cond_opcode, cursor, 0, key_reg, key_len);
>>> if (no_error) {
>>>   sqlVdbeAddOp0(v, OP_Halt);
>>> } else {
>>> -    sqlVdbeAddOp4(v, OP_Halt, -1, 0, tarantool_error_code, error,
>>> +    sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, error,
>>>           P4_DYNAMIC);
>>> +    sqlVdbeAddOp1(v, OP_Halt, -1);
>>> }
>>> +  sqlVdbeJumpHere(v, jmp);
>> 
>> Are these manipulations with labels related to patch?
>> ‘label’ or ‘addr' are more appropriate names IMHO.
>> 
> I think that these manipulations should be done, since in this
> patch we should jump to different lengths depending on the value
> of no_error. This will be even more severe after the next patch.

Ok, now I see.

This patch LGTM.