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 27EA52744D for ; Fri, 13 Jul 2018 16:03:48 -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 7qkbnzSuGmME for ; Fri, 13 Jul 2018 16:03:48 -0400 (EDT) Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (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 D2AD9272AD for ; Fri, 13 Jul 2018 16:03:47 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions References: <3155108e339f80906e3d4a3bd3a0885ed08e8ffc.1531413185.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 13 Jul 2018 23:03:43 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed 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: Kirill Shcherbatov , tarantool-patches@freelists.org Thanks for the fixes! See 4 comments below. I have pushed my review fixes on the branch, and they correct some of the comments. Please, finish the patch after my fixes including rest of the comments. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 8fba373..b00f4ff 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1306,11 +1324,31 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab) > */ > if (!db->init.imposterTable) { > for (i = 0; i < (int)pTab->def->field_count; i++) { > - if (pTab->aCol[i].is_primkey) { > - pTab->def->fields[i].nullable_action > - = ON_CONFLICT_ACTION_ABORT; > - pTab->def->fields[i].is_nullable = false; > + if (pTab->aCol[i].is_primkey == 0) 1. Please, remove is_primkey. It is not needed here and can be easily removed together with merging this cycle into the code below. The only place where it is still used except here is check_on_conflict_replace_entries, that can be spread over other primary key checks. > + continue; > + enum on_conflict_action action = > + pTab->def->fields[i].nullable_action; > + if (action != on_conflict_action_MAX && > + action != ON_CONFLICT_ACTION_ABORT && > + action != ON_CONFLICT_ACTION_DEFAULT) { > + const char *action_str = > + on_conflict_action_strs[ > + pTab->def->fields[i]. > + nullable_action]; > + const char *err_str = > + tt_sprintf("Cannot define PRIMARY KEY " > + "constraint on column %s " > + "with on conflict action %s", > + pTab->def->fields[i].name, > + action_str); > + diag_set(ClientError, ER_SQL, err_str); 2. Please, use ER_NULLABLE_PRIMARY. > + pParse->rc = SQL_TARANTOOL_ERROR; > + pParse->nErr++; > + return; > } > + pTab->def->fields[i].nullable_action = > + ON_CONFLICT_ACTION_ABORT; > + pTab->def->fields[i].is_nullable = false; > } > } > > @@ -1739,6 +1777,35 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ > } > } > > + /* Set default on_nullable action if required. */ > + for (uint32_t i = 0; i < p->def->field_count; i++) { > + if (p->def->fields[i].nullable_action == > + on_conflict_action_MAX) { > + p->def->fields[i].nullable_action = > + ON_CONFLICT_ACTION_NONE; > + p->def->fields[i].is_nullable = true; > + } else if (p->iPKey == (int)i && 3. This is checked already, so the rest of hunk below can be removed. > + p->def->fields[i].nullable_action == > + ON_CONFLICT_ACTION_NONE) { > + /* > + * PRIMARY KEY can not be defined with > + * ON_CONFLICT_ACTION_NONE. > + */ > + const char *action_str = > + on_conflict_action_strs[p->def->fields[i]. > + nullable_action]; > + const char *err_str = > + tt_sprintf("Cannot define PRIMARY KEY " > + "constraint on nullable column %s", > + p->def->fields[i].name, > + action_str); > + diag_set(ClientError, ER_SQL, err_str); > + pParse->rc = SQL_TARANTOOL_ERROR; > + pParse->nErr++; > + return; > + } > + } > + > diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result > index c0d0de0..5c8131a 100644 > --- a/test/sql/on-conflict.result > +++ b/test/sql/on-conflict.result > @@ -99,3 +99,24 @@ box.sql.execute('DROP TABLE t1') > box.sql.execute('DROP TABLE t2') > --- > ... > +-- > +-- gh-3473: Primary key can be declared with NULL. > +-- > +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);') > +--- > +- error: 'SQL error: NULL declaration for column ''S1'' of table ''TE17'' has been > + already set to ''none''' > +... > +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);') > +--- > +- error: 'SQL error: Cannot define PRIMARY KEY constraint on nullable column S1' > +... > +box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);") > +--- > +- error: keyword "ON" is reserved 4. This test does not work, it fails on syntax, but must fail on action. > +... > +box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))") > +--- > +- error: 'SQL error: Cannot define PRIMARY KEY constraint on column B with on conflict > + action none' > +...