From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A264E257AA for ; Mon, 18 Jun 2018 15:22:18 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7A6qLXtGl421 for ; Mon, 18 Jun 2018 15:22:18 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5EBF525787 for ; Mon, 18 Jun 2018 15:22:18 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence References: <2dc4c354dc72123c3447831a6ac48038eaf95f48.1528997527.git.kshcherbatov@tarantool.org> <05ff2cb5-d224-7b04-5aa8-d7d57b7c0031@tarantool.org> <04bda6c6-247c-0ad4-b78d-701d1d34c974@tarantool.org> From: Kirill Shcherbatov Message-ID: <79bd10d4-579b-92b3-e1ed-755d33febb8b@tarantool.org> Date: Mon, 18 Jun 2018 22:22:16 +0300 MIME-Version: 1.0 In-Reply-To: <04bda6c6-247c-0ad4-b78d-701d1d34c974@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: "v.shpilevoy@tarantool.org" > 1. Sorry, after discussion with Nikita we have decided to > use 'vdbe_' prefix for emitters even if they take parser in > first argument. Tnx, squashed. > 2. Mismatch of parameter names: error in header, error_src in source. Fixed. > 3. Please, reduce the mess. Separate these OOM handlers. name = sqlite3DbStrDup(db, name_src); - if (name == NULL) + if (name == NULL) { + size_t size = strlen(name_src) + 1; + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name"); return -1; + } error = sqlite3DbStrDup(db, error_src); if (error == NULL) { sqlite3DbFree(db, name); - return -1; - } - if (name == NULL || error == NULL) { - size_t size = - (name != NULL ? strlen(error_src) : - strlen(name_src)) + 1; - const char *var_name = name != NULL ? "error" : "name"; - diag_set(OutOfMemory, size, "sqlite3DbStrDup", var_name); - sqlite3DbFree(db, name); - sqlite3DbFree(db, error); + size_t size = strlen(error_src) + 1; + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error"); return -1; } > 4. This error is duplicated already above, it is not? Damn, you are right. - error = sqlite3DbStrDup(db, error); > 5. Please, use tt_sprintf. Do not declare big arrays on the > stack when possible. - char error[DIAG_ERRMSG_MAX]; - snprintf(error, DIAG_ERRMSG_MAX, - tnt_errcode_desc(ER_TRIGGER_EXISTS), zName); + const char *error_msg = + tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS), zName); > 6. Earlier this function could return either DONE or ERROR. Now > it can return SQL_TARANTOOL_ERROR, and I am not sure that the > behavior should be changed in such way. Please, explain why do you > need SQL_TARANTOOL_ERROR here. This is the sane behavior that parser has. I'd like to distinguish tarantool errors and out them from diag like Parser does. Take a look to the changes in sqlite3_errmsg.