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

[Nikita Pettik <korablev@tarantool.org>]

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.
 
> 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
>