From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 8C5BB45C304 for ; Mon, 14 Dec 2020 18:53:24 +0300 (MSK) References: <2bcd06c2ee8f5fcfdb8e2d0d640ab822362832f7.1607696813.git.lvasiliev@tarantool.org> <93f47160-4a9b-da02-71a0-8e757b3f1ac7@tarantool.org> From: Leonid Vasiliev Message-ID: <77019226-cbc8-f65c-a75d-6dfcadea0d9a@tarantool.org> Date: Mon, 14 Dec 2020 18:52:24 +0300 MIME-Version: 1.0 In-Reply-To: <93f47160-4a9b-da02-71a0-8e757b3f1ac7@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: Vladislav Shpilevoy , imeevma@tarantool.org, korablev@tarantool.org, sergos@tarantool.org, m.semkin@corp.mail.ru Cc: tarantool-patches@dev.tarantool.org 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; }