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

[Sergey Ostanevich <sergos@tarantool.org>]

Thanks for the patch!

Effectively we just cover SIGSEGV that inevitably appear in iproto either
in box.execute - both access diag as if it set in case of non-zero result.

LGTM.

Sergos
 
> On 14 Dec 2020, at 18:52, Leonid Vasiliev <lvasiliev@tarantool.org> wrote:
> 
> Hi! Thank you for the review.
> 
> On 13.12.2020 21:30, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>> I agree with Nikita here. The change is dangerous. If there is
>> no a diag, but the query failed, it means something is very wrong,
>> and it is not safe to continue execution. A panic() would be
>> better here.
> OK. I don't mind.
> 
> New patch:
> 
> sql: add panic() call in sql_execute() on complete failure
> 
> 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 call `panic()` in that case, because something is very wrong,
> and it is not safe to continue execution.
> 
> Part of #5537
> ---
> 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..a424349 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 call `panic()` in that case, because
> +		 * something is very wrong, and it is not safe to continue
> +		 * execution.
> +		 */
> +		if (diag_is_empty(diag_get()))
> +			panic("failed to execute SQL statement");
> +
> 		return -1;
> +	}
> 	return 0;
> }