Re: [Tarantool-patches] [PATCH v2 2/3] sql: set an error to diag in sql_execute() on failure

[Leonid Vasiliev <lvasiliev@tarantool.org>]

Hi! Thank you for the review.

On 11.12.2020 18:03, Nikita Pettik wrote:
> On 11 Dec 17:49, Leonid Vasiliev wrote:
>> In SQL, on failure sometimes an error sets to the diag, sometimes not.
>> And this can dived to situation as in #5537(SEGFAULT).
>> So, let's set an error to the diag if the result of `sql_execute()`
>> is a failure and there is no error in the diag.
> 
> Personally I am against this patch. In case of error SQL submodule
> must always set diagnostic error; if it does no set - something goes
> really wrong. In this case crash is better option. Firsly, it provides
> coredump which is useful in accident investigation. Secondly, unset
> diagnostics may leave SQL in inconsistent state - nobody knows how
> severe error happened.
>   

Maybe you are right. But it looks like SEGFAULT indicates a problem, but
coredump is not always helpful in this case.
Anyway, I agree to throw out this patch.

>> Part of #5537
>> ---
>>
>> After some discussion with Sergos, I added a common diag_set
>> when sql_execute() fails.
>> I wanted to add such a common error by `diag_add()` if diag
>> is not empty, but such a change would entail additional correction in tests.
>> But this patch should be included in the next release, and I want it to
>> be as small as possible. This patchset is about fixes a crash, not about
>> refactoring and improvements. For this I will create separate tasks.
>>
>>   src/box/execute.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index e14da20..87ebb44 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -687,8 +687,18 @@ sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
>>   		rc = sql_step(stmt);
>>   		assert(rc != SQL_ROW && rc != 0);
>>   	}
>> -	if (rc != SQL_DONE)
>> +	if (rc != SQL_DONE) {
>> +		/*
>> +		 * In SQL, on failure sometimes an error sets to the diag,
>> +		 * sometimes not. So, let's set an error to the diag if
>> +		 * the status is a failure and there is no error in the diag.
>> +		 */
>> +		if (diag_is_empty(diag_get())) {
>> +			diag_set(ClientError, ER_SQL_EXECUTE,
>> +				 "something went wrong");
>> +		}
>>   		return -1;
>> +	}
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.7.4
>>