From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 6FA2A45C304 for ; Wed, 16 Dec 2020 00:07:14 +0300 (MSK) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) From: Sergey Ostanevich In-Reply-To: <77019226-cbc8-f65c-a75d-6dfcadea0d9a@tarantool.org> Date: Wed, 16 Dec 2020 00:05:48 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <4E6B04B4-3CE4-4872-91C5-263D94874353@tarantool.org> References: <2bcd06c2ee8f5fcfdb8e2d0d640ab822362832f7.1607696813.git.lvasiliev@tarantool.org> <93f47160-4a9b-da02-71a0-8e757b3f1ac7@tarantool.org> <77019226-cbc8-f65c-a75d-6dfcadea0d9a@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: m.semkin@corp.mail.ru, Vladislav Shpilevoy , tarantool-patches@dev.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 =20 > On 14 Dec 2020, at 18:52, Leonid Vasiliev = wrote: >=20 > Hi! Thank you for the review. >=20 > 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. >=20 > New patch: >=20 > sql: add panic() call in sql_execute() on complete failure >=20 > 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. >=20 > Part of #5537 > --- > src/box/execute.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) >=20 > 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 =3D sql_step(stmt); > assert(rc !=3D SQL_ROW && rc !=3D 0); > } > - if (rc !=3D SQL_DONE) > + if (rc !=3D 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; > }