[tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx>, tarantool-patches@xxxxxxxxxxxxx
  • Date: Fri, 13 Jul 2018 23:03:43 +0300

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'
+...

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018