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 661E9270BF for ; Fri, 13 Jul 2018 06:26:08 -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 IF7hJMlfqgJS for ; Fri, 13 Jul 2018 06:26:08 -0400 (EDT) Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 24C6E270B9 for ; Fri, 13 Jul 2018 06:26:08 -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 13:26:04 +0300 MIME-Version: 1.0 In-Reply-To: <3155108e339f80906e3d4a3bd3a0885ed08e8ffc.1531413185.git.kshcherbatov@tarantool.org> 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: tarantool-patches@freelists.org, Kirill Shcherbatov Hello. See 5 comments below. 1. Assertion. tarantool> format = {} --- ... tarantool> format[1] = {'field1', 'unsigned', nullable_action = "undefined"} --- ... tarantool> s = box.schema.create_space('test', {format = format}) Assertion failed: (field->nullable_action != ON_CONFLICT_ACTION_UNDEFINED), function field_def_decode, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/alter.cc, line 393. Process 13519 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT frame #0: 0x00007fff51b71b6e libsystem_kernel.dylib`__pthread_kill + 10 libsystem_kernel.dylib`__pthread_kill: -> 0x7fff51b71b6e <+10>: jae 0x7fff51b71b78 ; <+20> 0x7fff51b71b70 <+12>: movq %rax, %rdi 0x7fff51b71b73 <+15>: jmp 0x7fff51b68b00 ; cerror_nocancel 0x7fff51b71b78 <+20>: retq Target 0: (tarantool) stopped. On 12/07/2018 19:34, Kirill Shcherbatov wrote: > This patch dissallows define multiple "NULL", "NOT NULL" > options per column and fixes silent implicit behavior > for invalid "NULL PRIMARY KEY" construction. > > Closes #3473. > --- > src/box/alter.cc | 1 + > src/box/field_def.c | 1 + > src/box/field_def.h | 3 ++- > src/box/sql/build.c | 46 +++++++++++++++++++++++++++++++++++++++---- > src/box/sql/parse.y | 2 +- > test/sql/on-conflict.result | 13 ++++++++++++ > test/sql/on-conflict.test.lua | 6 ++++++ > 7 files changed, 66 insertions(+), 6 deletions(-) > > diff --git a/src/box/field_def.h b/src/box/field_def.h > index 05f80d4..c35e0b8 100644 > --- a/src/box/field_def.h > +++ b/src/box/field_def.h > @@ -60,7 +60,8 @@ enum field_type { > }; > > enum on_conflict_action { > - ON_CONFLICT_ACTION_NONE = 0, > + ON_CONFLICT_ACTION_UNDEFINED = 0, 2. Please, remove this new action, because * it is not really action, but just a flag; * it can be replaced with on_conflict_action_MAX or ON_CONFLICT_ACTION_DEFAULT. > + ON_CONFLICT_ACTION_NONE, > ON_CONFLICT_ACTION_ROLLBACK, > ON_CONFLICT_ACTION_ABORT, > ON_CONFLICT_ACTION_FAIL, > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 53c20a6..946b10c 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -701,9 +701,22 @@ sqlite3AddNotNull(Parse * pParse, int onError) > p = pParse->pNewTable; > if (p == 0 || NEVER(p->def->field_count < 1)) > return; > - p->def->fields[p->def->field_count - 1].nullable_action = (u8)onError; > - p->def->fields[p->def->field_count - 1].is_nullable = > - action_is_nullable(onError); > + struct field_def *field = &p->def->fields[p->def->field_count - 1]; > + if (field->nullable_action != ON_CONFLICT_ACTION_UNDEFINED) { > + /* Prevent defining nullable_action many times. */ > + const char *err_msg = > + tt_sprintf("NULL declaration for column '%s' of table " > + "'%s' has been already set to '%s'", > + field->name, p->def->name, > + on_conflict_action_strs[field-> > + nullable_action]); > + diag_set(ClientError, ER_SQL, err_msg); > + pParse->rc = SQL_TARANTOOL_ERROR; > + pParse->nErr++; > + return; > + } > + field->nullable_action = (u8)onError; 3. Why do you cast to u8 though nullable_action is enum? > + field->is_nullable = action_is_nullable(onError); > } > > /* > @@ -1728,6 +1741,31 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ > p->def->id = SQLITE_PAGENO_TO_SPACEID(p->tnum); > } > > + /* Set default on_nullable action if required. */ 4. This still works: box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))") And 'NULL' here is ignored somehow. It is incorrect and should be caught by core, in alter.cc, because it is not a grammar issue. > + for (uint32_t i = 0; i < p->def->field_count; i++) { > + if (p->def->fields[i].nullable_action == > + ON_CONFLICT_ACTION_UNDEFINED) { > + p->def->fields[i].nullable_action = > + ON_CONFLICT_ACTION_NONE; > + p->def->fields[i].is_nullable = true; > + } else if (p->iPKey == (int)i && > + p->def->fields[i].nullable_action == > + ON_CONFLICT_ACTION_NONE) { > + /* > + * PRIMARY KEY can not be defined with > + * ON_CONFLICT_ACTION_NONE. > + */ > + const char *err_str = > + tt_sprintf("Cannot define PRIMARY KEY " > + "constraint on nullable column in " > + "table '%s'", p->def->name); > + diag_set(ClientError, ER_SQL, err_str); > + pParse->rc = SQL_TARANTOOL_ERROR; > + pParse->nErr++; > + return; > + } > + } > + > if (!p->def->opts.is_view) { > if ((p->tabFlags & TF_HasPrimaryKey) == 0) { > sqlite3ErrorMsg(pParse, > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index ac935fd..6463019 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -276,7 +276,7 @@ ccons ::= DEFAULT id(X). { > // In addition to the type name, we also care about the primary key and > // UNIQUE constraints. > // > -ccons ::= NULL onconf. > +ccons ::= NULL onconf. {sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);} 5. I do this: box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE)") But ON CONFLICT makes no sense for nullable columns. > ccons ::= NOT NULL onconf(R). {sqlite3AddNotNull(pParse, R);} > ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I). > {sqlite3AddPrimaryKey(pParse,0,R,I,Z);}