From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 6B87145C304 for ; Fri, 11 Dec 2020 18:03:38 +0300 (MSK) Date: Fri, 11 Dec 2020 15:03:36 +0000 From: Nikita Pettik Message-ID: <20201211150336.GG12730@tarantool.org> References: <2bcd06c2ee8f5fcfdb8e2d0d640ab822362832f7.1607696813.git.lvasiliev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2bcd06c2ee8f5fcfdb8e2d0d640ab822362832f7.1607696813.git.lvasiliev@tarantool.org> 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: Leonid Vasiliev Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, m.semkin@corp.mail.ru 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 >