From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5D3E745C304 for ; Fri, 11 Dec 2020 18:41:30 +0300 (MSK) References: <2bcd06c2ee8f5fcfdb8e2d0d640ab822362832f7.1607696813.git.lvasiliev@tarantool.org> <20201211150336.GG12730@tarantool.org> From: Leonid Vasiliev Message-ID: <50fc15d6-3d85-de44-3abb-455d7c2a4b70@tarantool.org> Date: Fri, 11 Dec 2020 18:40:31 +0300 MIME-Version: 1.0 In-Reply-To: <20201211150336.GG12730@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 2/3] sql: set an error to diag in sql_execute() on failure List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, m.semkin@corp.mail.ru 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 >>