[tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions
- From: Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx>
- To: tarantool-patches@xxxxxxxxxxxxx, Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
- Date: Mon, 16 Jul 2018 12:43:08 +0300
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.
Thank you for review, I've fixed all problems on branch and squashed.
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.
2. Please, use ER_NULLABLE_PRIMARY.
3. This is checked already, so the rest of hunk below can be removed.
Fixed with review fixes.
4. This test does not work, it fails on syntax, but must fail on action.
Ok, I've hacked to make it work this way.
============================================
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/sql/build.c | 86 +++++++++++++++++++++++++++++++++----------
src/box/sql/parse.y | 7 +++-
test/sql/on-conflict.result | 21 +++++++++++
test/sql/on-conflict.test.lua | 8 ++++
4 files changed, 101 insertions(+), 21 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 8fba373..925193e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -653,7 +653,12 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token *
pType)
struct field_def *column_def = &p->def->fields[p->def->field_count];
memcpy(column_def, &field_def_default, sizeof(field_def_default));
column_def->name = z;
- column_def->nullable_action = ON_CONFLICT_ACTION_NONE;
+ /*
+ * Marker on_conflict_action_MAX is used to detect
+ * attempts to define NULL multiple time or to detect
+ * invalid primary key definition.
+ */
+ column_def->nullable_action = on_conflict_action_MAX;
column_def->is_nullable = true;
if (pType->n == 0) {
@@ -701,9 +706,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_MAX) {
+ /* 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 = onError;
+ field->is_nullable = action_is_nullable(onError);
}
/*
@@ -1283,6 +1301,24 @@ hasColumn(const i16 * aiCol, int nCol, int x)
return 0;
}
+static int
+field_def_create_for_pk(struct Parse *parser, struct field_def *field,
+ const char *space_name)
+{
+ if (field->nullable_action != ON_CONFLICT_ACTION_ABORT &&
+ field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
+ field->nullable_action != on_conflict_action_MAX) {
+ diag_set(ClientError, ER_NULLABLE_PRIMARY, space_name);
+ parser->rc = SQL_TARANTOOL_ERROR;
+ parser->nErr++;
+ return -1;
+ } else if (field->nullable_action == on_conflict_action_MAX) {
+ field->nullable_action = ON_CONFLICT_ACTION_ABORT;
+ field->is_nullable = false;
+ }
+ return 0;
+}
+
/*
* This routine runs at the end of parsing a CREATE TABLE statement.
* The job of this routine is to convert both
@@ -1301,18 +1337,8 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
Index *pPk;
int i, j;
sqlite3 *db = pParse->db;
-
- /* Mark every PRIMARY KEY column as NOT NULL (except for imposter
tables)
- */
- 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;
- }
- }
- }
+ struct field_def *fields = pTab->def->fields;
+ const char *space_name = pTab->def->name;
/* Locate the PRIMARY KEY index. Or, if this table was originally
* an INTEGER PRIMARY KEY table, create a new PRIMARY KEY index.
@@ -1320,7 +1346,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];
+ 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));
@@ -1343,11 +1372,17 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
* "PRIMARY KEY(a,b,a,b,c,b,c,d)" into just "PRIMARY
KEY(a,b,c,d)". Later
* code assumes the PRIMARY KEY contains no repeated columns.
*/
- for (i = j = 1; i < pPk->nColumn; i++) {
- if (hasColumn(pPk->aiColumn, j, pPk->aiColumn[i])) {
+ u16 pk_columns = pPk->nColumn;
+ for (i = j = 0; i < pk_columns; i++) {
+ uint32_t part_no = pPk->aiColumn[i];
+ if (hasColumn(pPk->aiColumn, j, part_no)) {
pPk->nColumn--;
} else {
- pPk->aiColumn[j++] = pPk->aiColumn[i];
+ pPk->aiColumn[j++] = part_no;
+ if (field_def_create_for_pk(pParse,
+ &fields[part_no],
+ space_name) != 0)
+ return;
}
}
pPk->nColumn = j;
@@ -1736,6 +1771,17 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
return;
} else {
convertToWithoutRowidTable(pParse, p);
+ if (pParse->nErr > 0)
+ return;
+ }
+ }
+
+ /* Set default on_nullable action if required. */
+ struct field_def *field = p->def->fields;
+ for (uint32_t i = 0; i < p->def->field_count; ++i, ++field) {
+ if (field->nullable_action == on_conflict_action_MAX) {
+ field->nullable_action = ON_CONFLICT_ACTION_NONE;
+ field->is_nullable = true;
}
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ac935fd..e2bdc4a 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -276,7 +276,12 @@ 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(R). {
+ sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);
+ /* Trigger nullability mismatch error if required. */
+ if (R != ON_CONFLICT_ACTION_DEFAULT)
+ sqlite3AddNotNull(pParse, R);
+}
ccons ::= NOT NULL onconf(R). {sqlite3AddNotNull(pParse, R);}
ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
{sqlite3AddPrimaryKey(pParse,0,R,I,Z);}
diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result
index c0d0de0..2970cb8 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: Primary index of the space 'TE17' can not contain nullable parts
+...
+box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT
IGNORE);")
+---
+- error: 'SQL error: NULL declaration for column ''B'' of table ''TEST'' has
been
+ already set to ''none'''
+...
+box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a,
b, c))")
+---
+- error: Primary index of the space 'TEST' can not contain nullable parts
+...
diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua
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.
+--
+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))")
--
2.7.4
Other related posts:
