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

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Wed, 18 Jul 2018 23:12:26 +0300


void
-sqlite3AddNotNull(Parse * pParse, int onError)
+sql_column_nullable_action_add(struct Parse *parser,
+                            enum on_conflict_action nullable_action)

This name sounds really weird.. Why not sql_column_add_nullable_action ?

{
-     Table *p;
-     p = pParse->pNewTable;
-     if (p == 0 || NEVER(p->def->field_count < 1))
+     struct Table *p = parser->pNewTable;
+     if (p == NULL || 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_MAX) {

Why do you need on_conflict_action_MAX when you already have 
ON_CONFLICT_ACTION_DEFAULT?
Anyway, there is no action DEFAULT, it is sooner or later converted to ABORT.

+             /* 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);
+             parser->rc = SQL_TARANTOOL_ERROR;
+             parser->nErr++;
+             return;
+     }
+     field->nullable_action = nullable_action;
+     field->is_nullable = action_is_nullable(nullable_action);
}

/*
@@ -1251,7 +1271,10 @@ convertToWithoutRowidTable(Parse * pParse, Table * 
pTab)
      if (pTab->iPKey >= 0) {
              ExprList *pList;
              Token ipkToken;
-             sqlite3TokenInit(&ipkToken, 
pTab->def->fields[pTab->iPKey].name);
+             struct field_def *field = &fields[pTab->iPKey];

Could we avoid using iPKey in this function? We are going to remove it soon.

+             if (field_def_create_for_pk(pParse, field, space_name) != 0)
+                     return;
+             sqlite3TokenInit(&ipkToken, field->name);
              pList = sql_expr_list_append(pParse->db, NULL,
                                           sqlite3ExprAlloc(db, TK_ID,
                                                            &ipkToken, 0));
index b6d92f7..5ae6b0c 100644
--- a/test/sql/on-conflict.test.lua
+++ b/test/sql/on-conflict.test.lua
@@ -38,3 +38,11 @@ box.sql.execute('DROP TABLE p')
box.sql.execute('DROP TABLE e')
box.sql.execute('DROP TABLE t1')
box.sql.execute('DROP TABLE t2')
+
+--
+-- gh-3473: Primary key can be declared with NULL.

Lets write ‘can’t be declared with NULL’. Otherwise, it seems to be confusing.

+--
+box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);')
+box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);')
+box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON 
CONFLICT IGNORE);")
+box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, 
b, c))")
-- 


Other related posts:

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