From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Kirill Shcherbatov <kshcherbatov@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions Date: Fri, 13 Jul 2018 13:26:04 +0300 [thread overview] Message-ID: <f0878260-5e53-754a-b412-0a932413ae90@tarantool.org> (raw) In-Reply-To: <3155108e339f80906e3d4a3bd3a0885ed08e8ffc.1531413185.git.kshcherbatov@tarantool.org> 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);}
next prev parent reply other threads:[~2018-07-13 10:26 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-12 16:34 [tarantool-patches] [PATCH v1 0/2] " Kirill Shcherbatov 2018-07-12 16:34 ` [tarantool-patches] [PATCH v1 1/2] " Kirill Shcherbatov 2018-07-13 10:26 ` Vladislav Shpilevoy [this message] 2018-07-13 16:05 ` [tarantool-patches] " Kirill Shcherbatov 2018-07-13 20:03 ` Vladislav Shpilevoy 2018-07-16 9:43 ` Kirill Shcherbatov 2018-07-16 10:20 ` Vladislav Shpilevoy 2018-07-16 12:27 ` Kirill Shcherbatov 2018-07-12 16:34 ` [tarantool-patches] [PATCH v1 2/2] sql: fixed possible leak in sqlite3EndTable Kirill Shcherbatov 2018-07-13 10:26 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-13 16:05 ` Kirill Shcherbatov 2018-07-13 10:26 ` [tarantool-patches] Re: [PATCH v1 0/2] sql: restrict nullable action definitions Vladislav Shpilevoy 2018-07-16 12:28 ` [tarantool-patches] [PATCH v1 2/4] sql: refactor sqlite3AddNotNull function Kirill Shcherbatov 2018-07-16 13:41 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-16 12:28 ` [tarantool-patches] [PATCH v1 4/4] sql: get rid of Column structure Kirill Shcherbatov 2018-07-16 13:40 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-16 16:33 ` Kirill Shcherbatov 2018-07-17 9:32 ` Vladislav Shpilevoy 2018-07-17 14:08 ` Kirill Shcherbatov 2018-07-17 22:01 ` Vladislav Shpilevoy 2018-07-18 7:25 ` Kirill Shcherbatov 2018-07-18 8:04 ` Vladislav Shpilevoy 2018-07-18 16:41 ` n.pettik
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=f0878260-5e53-754a-b412-0a932413ae90@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=kshcherbatov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox