* [tarantool-patches] [PATCH v4 1/6] sql: refactor sql_expr_compile to return AST
2018-06-20 10:46 [tarantool-patches] [PATCH v4 0/6] sql: remove Triggers to server Kirill Shcherbatov
@ 2018-06-20 10:46 ` Kirill Shcherbatov
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 2/6] sql: move sqlite3DeleteTrigger to sql.h Kirill Shcherbatov
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Kirill Shcherbatov @ 2018-06-20 10:46 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
---
src/box/alter.cc | 12 +++++++-----
src/box/sql.c | 10 ++++++----
src/box/sql.h | 9 ++++-----
src/box/sql/prepare.c | 10 ++++++++++
src/box/sql/sqliteInt.h | 3 ++-
src/box/sql/tokenize.c | 40 ++++++++++++++++++++++++++--------------
6 files changed, 55 insertions(+), 29 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 5784e27..706eab4 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -405,11 +405,13 @@ field_def_decode(struct field_def *field, const char **data,
"string, scalar and any fields"));
}
- if (field->default_value != NULL &&
- sql_expr_compile(sql_get(), field->default_value,
- strlen(field->default_value),
- &field->default_value_expr) != 0)
- diag_raise();
+ const char *dv = field->default_value;
+ if (dv != NULL) {
+ field->default_value_expr = sql_expr_compile(sql_get(), dv,
+ strlen(dv));
+ if (field->default_value_expr == NULL)
+ diag_raise();
+ }
}
/**
diff --git a/src/box/sql.c b/src/box/sql.c
index 82f3d6d..0ae33b6 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1815,10 +1815,12 @@ sql_check_list_item_init(struct ExprList *expr_list, int column,
return -1;
}
}
- if (expr_str != NULL && sql_expr_compile(db, expr_str, expr_str_len,
- &item->pExpr) != 0) {
- sqlite3DbFree(db, item->zName);
- return -1;
+ if (expr_str != NULL) {
+ item->pExpr = sql_expr_compile(db, expr_str, expr_str_len);
+ if (item->pExpr == NULL) {
+ sqlite3DbFree(db, item->zName);
+ return -1;
+ }
}
return 0;
}
diff --git a/src/box/sql.h b/src/box/sql.h
index 834f951..ef9e099 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -75,13 +75,12 @@ struct Table;
* @param db SQL context handle.
* @param expr Expression to parse.
* @param expr_len Length of @an expr.
- * @param[out] result Result: AST structure.
*
- * @retval Error code if any.
+ * @retval NULL on error.
+ * @retval not NULL Expr AST pointer on success.
*/
-int
-sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len,
- struct Expr **result);
+struct Expr *
+sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len);
/**
* This routine executes parser on 'CREATE VIEW ...' statement
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e43fbcb..bddc21a 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -454,5 +454,15 @@ sql_parser_destroy(Parse *parser)
}
parser->disableLookaside = 0;
sqlite3DbFree(db, parser->zErrMsg);
+ switch (parser->parsed_ast_type) {
+ case AST_TYPE_SELECT:
+ sql_select_delete(db, parser->parsed_ast.select);
+ break;
+ case AST_TYPE_EXPR:
+ sql_expr_delete(db, parser->parsed_ast.expr, false);
+ break;
+ default:
+ assert(parser->parsed_ast_type == AST_TYPE_UNDEFINED);
+ }
region_destroy(&parser->region);
}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 47360fa..88644c6 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2852,7 +2852,8 @@ struct TriggerPrg {
};
enum ast_type {
- AST_TYPE_SELECT = 0,
+ AST_TYPE_UNDEFINED = 0,
+ AST_TYPE_SELECT,
AST_TYPE_EXPR,
ast_type_MAX
};
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 3fdf14d..5654e63 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -42,6 +42,7 @@
#include "say.h"
#include "sqliteInt.h"
+#include "tarantoolInt.h"
/* Character classes for tokenizing
*
@@ -510,8 +511,13 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
}
if (pParse->rc != SQLITE_OK && pParse->rc != SQLITE_DONE
&& pParse->zErrMsg == 0) {
- pParse->zErrMsg =
- sqlite3MPrintf(db, "%s", sqlite3ErrStr(pParse->rc));
+ const char *error;
+ if (is_tarantool_error(pParse->rc) &&
+ tarantoolErrorMessage() != NULL)
+ error = tarantoolErrorMessage();
+ else
+ error = sqlite3ErrStr(pParse->rc);
+ pParse->zErrMsg = sqlite3MPrintf(db, "%s", error);
}
assert(pzErrMsg != 0);
if (pParse->zErrMsg) {
@@ -539,9 +545,8 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
return nErr;
}
-int
-sql_expr_compile(sqlite3 *db, const char *expr, int expr_len,
- struct Expr **result)
+struct Expr *
+sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
{
const char *outer = "SELECT ";
int len = strlen(outer) + expr_len;
@@ -550,22 +555,25 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len,
sql_parser_create(&parser, db);
parser.parse_only = true;
+ struct Expr *expression = NULL;
char *stmt = (char *)region_alloc(&parser.region, len + 1);
if (stmt == NULL) {
diag_set(OutOfMemory, len + 1, "region_alloc", "stmt");
- return -1;
+ goto end;
}
sprintf(stmt, "%s%.*s", outer, expr_len, expr);
char *unused;
if (sqlite3RunParser(&parser, stmt, &unused) != SQLITE_OK ||
parser.parsed_ast_type != AST_TYPE_EXPR) {
- diag_set(ClientError, ER_SQL_EXECUTE, expr);
- return -1;
+ diag_set(ClientError, ER_SQL_EXECUTE, stmt);
+ } else {
+ expression = parser.parsed_ast.expr;
+ parser.parsed_ast.expr = NULL;
}
- *result = parser.parsed_ast.expr;
+end:
sql_parser_destroy(&parser);
- return 0;
+ return expression;
}
struct Select *
@@ -574,14 +582,18 @@ sql_view_compile(struct sqlite3 *db, const char *view_stmt)
struct Parse parser;
sql_parser_create(&parser, db);
parser.parse_only = true;
+
+ struct Select *select = NULL;
+
char *unused;
if (sqlite3RunParser(&parser, view_stmt, &unused) != SQLITE_OK ||
parser.parsed_ast_type != AST_TYPE_SELECT) {
diag_set(ClientError, ER_SQL_EXECUTE, view_stmt);
- sql_parser_destroy(&parser);
- return NULL;
+ } else {
+ select = parser.parsed_ast.select;
+ parser.parsed_ast.select = NULL;
}
- struct Select *result = parser.parsed_ast.select;
+
sql_parser_destroy(&parser);
- return result;
+ return select;
}
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] [PATCH v4 2/6] sql: move sqlite3DeleteTrigger to sql.h
2018-06-20 10:46 [tarantool-patches] [PATCH v4 0/6] sql: remove Triggers to server Kirill Shcherbatov
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 1/6] sql: refactor sql_expr_compile to return AST Kirill Shcherbatov
@ 2018-06-20 10:46 ` Kirill Shcherbatov
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 3/6] box: sort error codes in misc.test Kirill Shcherbatov
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Kirill Shcherbatov @ 2018-06-20 10:46 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
As we are going to port triggers to server, we need
an instrument to release allocated memory in alter.cc.
Part of #3273.
---
src/box/sql.h | 9 +++++++++
src/box/sql/callback.c | 7 +++----
src/box/sql/sqliteInt.h | 3 +--
src/box/sql/status.c | 6 +++---
src/box/sql/tokenize.c | 2 +-
src/box/sql/trigger.c | 30 +++++++++++++-----------------
6 files changed, 30 insertions(+), 27 deletions(-)
diff --git a/src/box/sql.h b/src/box/sql.h
index ef9e099..90bba1a 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -66,6 +66,7 @@ struct Expr;
struct Parse;
struct Select;
struct Table;
+struct Trigger;
/**
* Perform parsing of provided expression. This is done by
@@ -94,6 +95,14 @@ struct Select *
sql_view_compile(struct sqlite3 *db, const char *view_stmt);
/**
+ * Free AST pointed by trigger.
+ * @param db SQL handle.
+ * @param trigger AST object.
+ */
+void
+sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger);
+
+/**
* Store duplicate of a parsed expression into @a parser.
* @param parser Parser context.
* @param select Select to extract from.
diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
index 8289c05..bd8db99 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -290,10 +290,9 @@ sqlite3SchemaClear(sqlite3 * db)
temp1 = pSchema->tblHash;
temp2 = pSchema->trigHash;
sqlite3HashInit(&pSchema->trigHash);
- for (pElem = sqliteHashFirst(&temp2); pElem;
- pElem = sqliteHashNext(pElem)) {
- sqlite3DeleteTrigger(0, (Trigger *) sqliteHashData(pElem));
- }
+ for (pElem = sqliteHashFirst(&temp2); pElem != NULL;
+ pElem = sqliteHashNext(pElem))
+ sql_trigger_delete(NULL, (Trigger *) sqliteHashData(pElem));
sqlite3HashClear(&temp2);
sqlite3HashInit(&pSchema->tblHash);
for (pElem = sqliteHashFirst(&temp1); pElem;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 88644c6..a9388f1 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4091,7 +4091,6 @@ TriggerStep *sqlite3TriggerInsertStep(sqlite3 *, Token *, IdList *,
TriggerStep *sqlite3TriggerUpdateStep(sqlite3 *, Token *, ExprList *, Expr *,
u8);
TriggerStep *sqlite3TriggerDeleteStep(sqlite3 *, Token *, Expr *);
-void sqlite3DeleteTrigger(sqlite3 *, Trigger *);
void sqlite3UnlinkAndDeleteTrigger(sqlite3 *, const char *);
u32 sqlite3TriggerColmask(Parse *, Trigger *, ExprList *, int, int, Table *,
int);
@@ -4099,7 +4098,7 @@ u32 sqlite3TriggerColmask(Parse *, Trigger *, ExprList *, int, int, Table *,
#define sqlite3IsToplevel(p) ((p)->pToplevel==0)
#else
#define sqlite3TriggersExist(C,D,E,F) 0
-#define sqlite3DeleteTrigger(A,B)
+#define sql_trigger_delete(A,B)
#define sqlite3DropTriggerPtr(A,B)
#define sqlite3UnlinkAndDeleteTrigger(A,B,C)
#define sqlite3CodeRowTrigger(A,B,C,D,E,F,G,H,I)
diff --git a/src/box/sql/status.c b/src/box/sql/status.c
index 84f07cc..dda91c5 100644
--- a/src/box/sql/status.c
+++ b/src/box/sql/status.c
@@ -256,9 +256,9 @@ sqlite3_db_status(sqlite3 * db, /* The database connection whose status is desir
for (p = sqliteHashFirst(&pSchema->trigHash); p;
p = sqliteHashNext(p)) {
- sqlite3DeleteTrigger(db,
- (Trigger *)
- sqliteHashData(p));
+ sql_trigger_delete(db,
+ (Trigger *)
+ sqliteHashData(p));
}
for (p = sqliteHashFirst(&pSchema->tblHash); p;
p = sqliteHashNext(p)) {
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 5654e63..1e84c1e 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -534,7 +534,7 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
if (pParse->pWithToFree)
sqlite3WithDelete(db, pParse->pWithToFree);
- sqlite3DeleteTrigger(db, pParse->pNewTrigger);
+ sql_trigger_delete(db, pParse->pNewTrigger);
sqlite3DbFree(db, pParse->pVList);
while (pParse->pZombieTab) {
Table *p = pParse->pZombieTab;
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index ad8e243..fd1cb50 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -185,11 +185,10 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
sqlite3SrcListDelete(db, pTableName);
sqlite3IdListDelete(db, pColumns);
sql_expr_delete(db, pWhen, false);
- if (!pParse->pNewTrigger) {
- sqlite3DeleteTrigger(db, pTrigger);
- } else {
+ if (pParse->pNewTrigger == NULL)
+ sql_trigger_delete(db, pTrigger);
+ else
assert(pParse->pNewTrigger == pTrigger);
- }
}
/*
@@ -328,7 +327,7 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
/* No need to free zName sinceif we reach this point
alloc for it either wasn't called at all or failed. */
}
- sqlite3DeleteTrigger(db, pTrig);
+ sql_trigger_delete(db, pTrig);
assert(!pParse->pNewTrigger);
sqlite3DeleteTriggerStep(db, pStepList);
}
@@ -471,20 +470,17 @@ sqlite3TriggerDeleteStep(sqlite3 * db, /* Database connection */
return pTriggerStep;
}
-/*
- * Recursively delete a Trigger structure
- */
void
-sqlite3DeleteTrigger(sqlite3 * db, Trigger * pTrigger)
+sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger)
{
- if (pTrigger == 0)
+ if (trigger == NULL)
return;
- sqlite3DeleteTriggerStep(db, pTrigger->step_list);
- sqlite3DbFree(db, pTrigger->zName);
- sqlite3DbFree(db, pTrigger->table);
- sql_expr_delete(db, pTrigger->pWhen, false);
- sqlite3IdListDelete(db, pTrigger->pColumns);
- sqlite3DbFree(db, pTrigger);
+ sqlite3DeleteTriggerStep(db, trigger->step_list);
+ sqlite3DbFree(db, trigger->zName);
+ sqlite3DbFree(db, trigger->table);
+ sql_expr_delete(db, trigger->pWhen, false);
+ sqlite3IdListDelete(db, trigger->pColumns);
+ sqlite3DbFree(db, trigger);
}
/*
@@ -593,7 +589,7 @@ sqlite3UnlinkAndDeleteTrigger(sqlite3 * db, const char *zName)
pp = &((*pp)->pNext)) ;
*pp = (*pp)->pNext;
}
- sqlite3DeleteTrigger(db, pTrigger);
+ sql_trigger_delete(db, pTrigger);
user_session->sql_flags |= SQLITE_InternChanges;
}
}
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] [PATCH v4 3/6] box: sort error codes in misc.test
2018-06-20 10:46 [tarantool-patches] [PATCH v4 0/6] sql: remove Triggers to server Kirill Shcherbatov
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 1/6] sql: refactor sql_expr_compile to return AST Kirill Shcherbatov
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 2/6] sql: move sqlite3DeleteTrigger to sql.h Kirill Shcherbatov
@ 2018-06-20 10:46 ` Kirill Shcherbatov
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 4/6] sql: new _trigger space format with space_id Kirill Shcherbatov
` (2 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Kirill Shcherbatov @ 2018-06-20 10:46 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
As error codes were not sorted, changing any of error
constants significantly change test case output. This
cause unnecessary changes on each such commit.
---
test/box/misc.result | 325 +++++++++++++++++++++++++------------------------
test/box/misc.test.lua | 4 +-
2 files changed, 166 insertions(+), 163 deletions(-)
diff --git a/test/box/misc.result b/test/box/misc.result
index a2bbdca..930d0a8 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -319,173 +319,174 @@ type(require('yaml').encode(box.slab.info()));
----------------
t = {}
for k,v in pairs(box.error) do
- table.insert(t, 'box.error.'..tostring(k)..' : '..tostring(v))
+ if type(v) == 'number' then
+ t[v] = 'box.error.'..tostring(k)
+ end
end;
---
...
t;
---
-- - 'box.error.UNKNOWN_REPLICA : 62'
- - 'box.error.WRONG_INDEX_RECORD : 106'
- - 'box.error.NO_SUCH_TRIGGER : 34'
- - 'box.error.SEQUENCE_EXISTS : 146'
- - 'box.error.CHECKPOINT_IN_PROGRESS : 120'
- - 'box.error.FIELD_TYPE : 23'
- - 'box.error.SQL_BIND_PARAMETER_MAX : 156'
- - 'box.error.WRONG_SPACE_FORMAT : 141'
- - 'box.error.SQL_BIND_TYPE : 155'
- - 'box.error.UNKNOWN_UPDATE_OP : 28'
- - 'box.error.WRONG_COLLATION_OPTIONS : 151'
- - 'box.error.CURSOR_NO_TRANSACTION : 80'
- - 'box.error.TUPLE_REF_OVERFLOW : 86'
- - 'box.error.ALTER_SEQUENCE : 143'
- - 'box.error.INVALID_XLOG_NAME : 75'
- - 'box.error.SAVEPOINT_EMPTY_TX : 60'
- - 'box.error.NO_SUCH_FUNCTION : 51'
- - 'box.error.ROLE_LOOP : 87'
- - 'box.error.TUPLE_NOT_FOUND : 4'
- - 'box.error.LOADING : 116'
- - 'box.error.VIEW_MISSING_SQL : 161'
- - 'box.error.BACKUP_IN_PROGRESS : 129'
- - 'box.error.DROP_USER : 44'
- - 'box.error.MODIFY_INDEX : 14'
- - 'box.error.PASSWORD_MISMATCH : 47'
- - 'box.error.UNSUPPORTED_ROLE_PRIV : 98'
- - 'box.error.ACCESS_DENIED : 42'
- - 'box.error.CANT_CREATE_COLLATION : 150'
- - 'box.error.USER_EXISTS : 46'
- - 'box.error.WAL_IO : 40'
- - 'box.error.PROC_RET : 21'
- - 'box.error.PRIV_GRANTED : 89'
- - 'box.error.CREATE_SPACE : 9'
- - 'box.error.GRANT : 88'
- - 'box.error.INVALID_INDEX_FILE : 131'
- - 'box.error.UNKNOWN_SCHEMA_OBJECT : 49'
- - 'box.error.WRONG_DD_VERSION : 140'
- - 'box.error.CREATE_ROLE : 84'
- - 'box.error.VINYL_MAX_TUPLE_SIZE : 139'
- - 'box.error.LOAD_FUNCTION : 99'
- - 'box.error.INVALID_XLOG : 74'
- - 'box.error.PRIV_NOT_GRANTED : 91'
- - 'box.error.TRANSACTION_CONFLICT : 97'
- - 'box.error.GUEST_USER_PASSWORD : 96'
- - 'box.error.PROC_C : 102'
- - 'box.error.INVALID_RUN_FILE : 132'
- - 'box.error.NONMASTER : 6'
- - 'box.error.MEMTX_MAX_TUPLE_SIZE : 110'
- - 'box.error.DROP_FUNCTION : 71'
- - 'box.error.CFG : 59'
- - 'box.error.NO_SUCH_FIELD : 37'
- - 'box.error.CONNECTION_TO_SELF : 117'
- - 'box.error.FUNCTION_MAX : 54'
- - 'box.error.ILLEGAL_PARAMS : 1'
- - 'box.error.PARTIAL_KEY : 136'
- - 'box.error.SAVEPOINT_NO_TRANSACTION : 114'
- - 'box.error.LOAD_MODULE : 138'
- - 'box.error.FUNCTION_LANGUAGE : 100'
- - 'box.error.ROLE_GRANTED : 90'
- - 'box.error.CHECKPOINT_ROLLBACK : 134'
- - 'box.error.NO_SUCH_USER : 45'
- - 'box.error.CANT_UPDATE_PRIMARY_KEY : 94'
- - 'box.error.EXACT_MATCH : 19'
- - 'box.error.ROLE_EXISTS : 83'
- - 'box.error.REPLICASET_UUID_IS_RO : 65'
- - 'box.error.INDEX_TYPE : 13'
- - 'box.error.NO_SUCH_PROC : 33'
- - 'box.error.MEMORY_ISSUE : 2'
- - 'box.error.KEY_PART_TYPE : 18'
- - 'box.error.CREATE_FUNCTION : 50'
- - 'box.error.ALREADY_RUNNING : 126'
- - 'box.error.SQL_BIND_VALUE : 154'
- - 'box.error.NO_SUCH_INDEX : 35'
- - 'box.error.UNKNOWN_RTREE_INDEX_DISTANCE_TYPE : 103'
- - 'box.error.TUPLE_FOUND : 3'
- - 'box.error.VIEW_IS_RO : 113'
- - 'box.error.LOCAL_INSTANCE_ID_IS_READ_ONLY : 128'
- - 'box.error.FUNCTION_EXISTS : 52'
- - 'box.error.UPDATE_ARG_TYPE : 26'
- - 'box.error.FOREIGN_KEY_CONSTRAINT : 162'
- - 'box.error.CROSS_ENGINE_TRANSACTION : 81'
- - 'box.error.ACTION_MISMATCH : 160'
- - 'box.error.FORMAT_MISMATCH_INDEX_PART : 27'
- - 'box.error.injection : table: <address>
- - 'box.error.FUNCTION_TX_ACTIVE : 30'
- - 'box.error.SQL_BIND_NOT_FOUND : 159'
- - 'box.error.RELOAD_CFG : 58'
- - 'box.error.NO_SUCH_ENGINE : 57'
- - 'box.error.COMMIT_IN_SUB_STMT : 122'
- - 'box.error.SQL_EXECUTE : 157'
- - 'box.error.NULLABLE_MISMATCH : 153'
- - 'box.error.LAST_DROP : 15'
- - 'box.error.NO_SUCH_ROLE : 82'
- - 'box.error.DECOMPRESSION : 124'
- - 'box.error.CREATE_SEQUENCE : 142'
- - 'box.error.CREATE_USER : 43'
- - 'box.error.SPACE_FIELD_IS_DUPLICATE : 149'
- - 'box.error.INSTANCE_UUID_MISMATCH : 66'
- - 'box.error.SEQUENCE_OVERFLOW : 147'
- - 'box.error.SYSTEM : 115'
- - 'box.error.KEY_PART_IS_TOO_LONG : 118'
- - 'box.error.TUPLE_FORMAT_LIMIT : 16'
- - 'box.error.BEFORE_REPLACE_RET : 53'
- - 'box.error.NO_SUCH_SAVEPOINT : 61'
- - 'box.error.TRUNCATE_SYSTEM_SPACE : 137'
- - 'box.error.VY_QUOTA_TIMEOUT : 135'
- - 'box.error.WRONG_INDEX_OPTIONS : 108'
- - 'box.error.INVALID_VYLOG_FILE : 133'
- - 'box.error.INDEX_FIELD_COUNT_LIMIT : 127'
- - 'box.error.READ_VIEW_ABORTED : 130'
- - 'box.error.USER_MAX : 56'
- - 'box.error.PROTOCOL : 104'
- - 'box.error.TUPLE_NOT_ARRAY : 22'
- - 'box.error.KEY_PART_COUNT : 31'
- - 'box.error.ALTER_SPACE : 12'
- - 'box.error.ACTIVE_TRANSACTION : 79'
- - 'box.error.EXACT_FIELD_COUNT : 38'
- - 'box.error.DROP_SEQUENCE : 144'
- - 'box.error.INVALID_MSGPACK : 20'
- - 'box.error.MORE_THAN_ONE_TUPLE : 41'
- - 'box.error.RTREE_RECT : 101'
- - 'box.error.SUB_STMT_MAX : 121'
- - 'box.error.UNKNOWN_REQUEST_TYPE : 48'
- - 'box.error.SPACE_EXISTS : 10'
- - 'box.error.PROC_LUA : 32'
- - 'box.error.ROLE_NOT_GRANTED : 92'
- - 'box.error.NO_SUCH_SPACE : 36'
- - 'box.error.WRONG_INDEX_PARTS : 107'
- - 'box.error.DROP_SPACE : 11'
- - 'box.error.MIN_FIELD_COUNT : 39'
- - 'box.error.REPLICASET_UUID_MISMATCH : 63'
- - 'box.error.UPDATE_FIELD : 29'
- - 'box.error.COMPRESSION : 119'
- - 'box.error.INVALID_ORDER : 68'
- - 'box.error.INDEX_EXISTS : 85'
- - 'box.error.SPLICE : 25'
- - 'box.error.UNKNOWN : 0'
- - 'box.error.DROP_PRIMARY_KEY : 17'
- - 'box.error.NULLABLE_PRIMARY : 152'
- - 'box.error.NO_SUCH_SEQUENCE : 145'
- - 'box.error.SQL : 158'
- - 'box.error.INVALID_UUID : 64'
- - 'box.error.INJECTION : 8'
- - 'box.error.TIMEOUT : 78'
- - 'box.error.IDENTIFIER : 70'
- - 'box.error.ITERATOR_TYPE : 72'
- - 'box.error.REPLICA_MAX : 73'
- - 'box.error.MISSING_REQUEST_FIELD : 69'
- - 'box.error.MISSING_SNAPSHOT : 93'
- - 'box.error.WRONG_SPACE_OPTIONS : 111'
- - 'box.error.READONLY : 7'
- - 'box.error.UNSUPPORTED : 5'
- - 'box.error.UPDATE_INTEGER_OVERFLOW : 95'
- - 'box.error.NO_CONNECTION : 77'
- - 'box.error.INVALID_XLOG_ORDER : 76'
- - 'box.error.UPSERT_UNIQUE_SECONDARY_KEY : 105'
- - 'box.error.ROLLBACK_IN_SUB_STMT : 123'
- - 'box.error.WRONG_SCHEMA_VERSION : 109'
- - 'box.error.UNSUPPORTED_INDEX_FEATURE : 112'
- - 'box.error.INDEX_PART_TYPE_MISMATCH : 24'
- - 'box.error.INVALID_XLOG_TYPE : 125'
+- 0: box.error.UNKNOWN
+ 1: box.error.ILLEGAL_PARAMS
+ 2: box.error.MEMORY_ISSUE
+ 3: box.error.TUPLE_FOUND
+ 4: box.error.TUPLE_NOT_FOUND
+ 5: box.error.UNSUPPORTED
+ 6: box.error.NONMASTER
+ 7: box.error.READONLY
+ 8: box.error.INJECTION
+ 9: box.error.CREATE_SPACE
+ 10: box.error.SPACE_EXISTS
+ 11: box.error.DROP_SPACE
+ 12: box.error.ALTER_SPACE
+ 13: box.error.INDEX_TYPE
+ 14: box.error.MODIFY_INDEX
+ 15: box.error.LAST_DROP
+ 16: box.error.TUPLE_FORMAT_LIMIT
+ 17: box.error.DROP_PRIMARY_KEY
+ 18: box.error.KEY_PART_TYPE
+ 19: box.error.EXACT_MATCH
+ 20: box.error.INVALID_MSGPACK
+ 21: box.error.PROC_RET
+ 22: box.error.TUPLE_NOT_ARRAY
+ 23: box.error.FIELD_TYPE
+ 24: box.error.INDEX_PART_TYPE_MISMATCH
+ 25: box.error.SPLICE
+ 26: box.error.UPDATE_ARG_TYPE
+ 27: box.error.FORMAT_MISMATCH_INDEX_PART
+ 28: box.error.UNKNOWN_UPDATE_OP
+ 29: box.error.UPDATE_FIELD
+ 30: box.error.FUNCTION_TX_ACTIVE
+ 31: box.error.KEY_PART_COUNT
+ 32: box.error.PROC_LUA
+ 33: box.error.NO_SUCH_PROC
+ 34: box.error.NO_SUCH_TRIGGER
+ 35: box.error.NO_SUCH_INDEX
+ 36: box.error.NO_SUCH_SPACE
+ 37: box.error.NO_SUCH_FIELD
+ 38: box.error.EXACT_FIELD_COUNT
+ 39: box.error.MIN_FIELD_COUNT
+ 40: box.error.WAL_IO
+ 41: box.error.MORE_THAN_ONE_TUPLE
+ 42: box.error.ACCESS_DENIED
+ 43: box.error.CREATE_USER
+ 44: box.error.DROP_USER
+ 45: box.error.NO_SUCH_USER
+ 46: box.error.USER_EXISTS
+ 47: box.error.PASSWORD_MISMATCH
+ 48: box.error.UNKNOWN_REQUEST_TYPE
+ 49: box.error.UNKNOWN_SCHEMA_OBJECT
+ 50: box.error.CREATE_FUNCTION
+ 51: box.error.NO_SUCH_FUNCTION
+ 52: box.error.FUNCTION_EXISTS
+ 53: box.error.BEFORE_REPLACE_RET
+ 54: box.error.FUNCTION_MAX
+ 56: box.error.USER_MAX
+ 57: box.error.NO_SUCH_ENGINE
+ 58: box.error.RELOAD_CFG
+ 59: box.error.CFG
+ 60: box.error.SAVEPOINT_EMPTY_TX
+ 61: box.error.NO_SUCH_SAVEPOINT
+ 62: box.error.UNKNOWN_REPLICA
+ 63: box.error.REPLICASET_UUID_MISMATCH
+ 64: box.error.INVALID_UUID
+ 65: box.error.REPLICASET_UUID_IS_RO
+ 66: box.error.INSTANCE_UUID_MISMATCH
+ 68: box.error.INVALID_ORDER
+ 69: box.error.MISSING_REQUEST_FIELD
+ 70: box.error.IDENTIFIER
+ 71: box.error.DROP_FUNCTION
+ 72: box.error.ITERATOR_TYPE
+ 73: box.error.REPLICA_MAX
+ 74: box.error.INVALID_XLOG
+ 75: box.error.INVALID_XLOG_NAME
+ 76: box.error.INVALID_XLOG_ORDER
+ 77: box.error.NO_CONNECTION
+ 78: box.error.TIMEOUT
+ 79: box.error.ACTIVE_TRANSACTION
+ 80: box.error.CURSOR_NO_TRANSACTION
+ 81: box.error.CROSS_ENGINE_TRANSACTION
+ 82: box.error.NO_SUCH_ROLE
+ 83: box.error.ROLE_EXISTS
+ 84: box.error.CREATE_ROLE
+ 85: box.error.INDEX_EXISTS
+ 86: box.error.TUPLE_REF_OVERFLOW
+ 87: box.error.ROLE_LOOP
+ 88: box.error.GRANT
+ 89: box.error.PRIV_GRANTED
+ 90: box.error.ROLE_GRANTED
+ 91: box.error.PRIV_NOT_GRANTED
+ 92: box.error.ROLE_NOT_GRANTED
+ 93: box.error.MISSING_SNAPSHOT
+ 94: box.error.CANT_UPDATE_PRIMARY_KEY
+ 95: box.error.UPDATE_INTEGER_OVERFLOW
+ 96: box.error.GUEST_USER_PASSWORD
+ 97: box.error.TRANSACTION_CONFLICT
+ 98: box.error.UNSUPPORTED_ROLE_PRIV
+ 99: box.error.LOAD_FUNCTION
+ 100: box.error.FUNCTION_LANGUAGE
+ 101: box.error.RTREE_RECT
+ 102: box.error.PROC_C
+ 103: box.error.UNKNOWN_RTREE_INDEX_DISTANCE_TYPE
+ 104: box.error.PROTOCOL
+ 105: box.error.UPSERT_UNIQUE_SECONDARY_KEY
+ 106: box.error.WRONG_INDEX_RECORD
+ 107: box.error.WRONG_INDEX_PARTS
+ 108: box.error.WRONG_INDEX_OPTIONS
+ 109: box.error.WRONG_SCHEMA_VERSION
+ 110: box.error.MEMTX_MAX_TUPLE_SIZE
+ 111: box.error.WRONG_SPACE_OPTIONS
+ 112: box.error.UNSUPPORTED_INDEX_FEATURE
+ 113: box.error.VIEW_IS_RO
+ 114: box.error.SAVEPOINT_NO_TRANSACTION
+ 115: box.error.SYSTEM
+ 116: box.error.LOADING
+ 117: box.error.CONNECTION_TO_SELF
+ 118: box.error.KEY_PART_IS_TOO_LONG
+ 119: box.error.COMPRESSION
+ 120: box.error.CHECKPOINT_IN_PROGRESS
+ 121: box.error.SUB_STMT_MAX
+ 122: box.error.COMMIT_IN_SUB_STMT
+ 123: box.error.ROLLBACK_IN_SUB_STMT
+ 124: box.error.DECOMPRESSION
+ 125: box.error.INVALID_XLOG_TYPE
+ 126: box.error.ALREADY_RUNNING
+ 127: box.error.INDEX_FIELD_COUNT_LIMIT
+ 128: box.error.LOCAL_INSTANCE_ID_IS_READ_ONLY
+ 129: box.error.BACKUP_IN_PROGRESS
+ 130: box.error.READ_VIEW_ABORTED
+ 131: box.error.INVALID_INDEX_FILE
+ 132: box.error.INVALID_RUN_FILE
+ 133: box.error.INVALID_VYLOG_FILE
+ 134: box.error.CHECKPOINT_ROLLBACK
+ 135: box.error.VY_QUOTA_TIMEOUT
+ 136: box.error.PARTIAL_KEY
+ 137: box.error.TRUNCATE_SYSTEM_SPACE
+ 138: box.error.LOAD_MODULE
+ 139: box.error.VINYL_MAX_TUPLE_SIZE
+ 140: box.error.WRONG_DD_VERSION
+ 141: box.error.WRONG_SPACE_FORMAT
+ 142: box.error.CREATE_SEQUENCE
+ 143: box.error.ALTER_SEQUENCE
+ 144: box.error.DROP_SEQUENCE
+ 145: box.error.NO_SUCH_SEQUENCE
+ 146: box.error.SEQUENCE_EXISTS
+ 147: box.error.SEQUENCE_OVERFLOW
+ 149: box.error.SPACE_FIELD_IS_DUPLICATE
+ 150: box.error.CANT_CREATE_COLLATION
+ 151: box.error.WRONG_COLLATION_OPTIONS
+ 152: box.error.NULLABLE_PRIMARY
+ 153: box.error.NULLABLE_MISMATCH
+ 154: box.error.SQL_BIND_VALUE
+ 155: box.error.SQL_BIND_TYPE
+ 156: box.error.SQL_BIND_PARAMETER_MAX
+ 157: box.error.SQL_EXECUTE
+ 158: box.error.SQL
+ 159: box.error.SQL_BIND_NOT_FOUND
+ 160: box.error.ACTION_MISMATCH
+ 161: box.error.VIEW_MISSING_SQL
+ 162: box.error.FOREIGN_KEY_CONSTRAINT
...
test_run:cmd("setopt delimiter ''");
---
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index 073a748..e24228a 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -111,7 +111,9 @@ type(require('yaml').encode(box.slab.info()));
----------------
t = {}
for k,v in pairs(box.error) do
- table.insert(t, 'box.error.'..tostring(k)..' : '..tostring(v))
+ if type(v) == 'number' then
+ t[v] = 'box.error.'..tostring(k)
+ end
end;
t;
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] [PATCH v4 4/6] sql: new _trigger space format with space_id
2018-06-20 10:46 [tarantool-patches] [PATCH v4 0/6] sql: remove Triggers to server Kirill Shcherbatov
` (2 preceding siblings ...)
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 3/6] box: sort error codes in misc.test Kirill Shcherbatov
@ 2018-06-20 10:46 ` Kirill Shcherbatov
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 5/6] sql: move Triggers to server Kirill Shcherbatov
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 6/6] sql: VDBE tests for trigger existence Kirill Shcherbatov
5 siblings, 0 replies; 15+ messages in thread
From: Kirill Shcherbatov @ 2018-06-20 10:46 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
As we would like to lookup triggers by space_id in future
on space deletion to delete associated _trigger tuples we
need to introduce new field space_id as secondary key.
Part of #3273.
---
src/box/bootstrap.snap | Bin 1698 -> 1704 bytes
src/box/lua/upgrade.lua | 4 ++
src/box/schema_def.h | 7 ++++
src/box/sql.c | 21 +++++++----
src/box/sql/sqliteInt.h | 2 +
src/box/sql/trigger.c | 8 ++--
test/app-tap/tarantoolctl.test.lua | 2 +-
test/box-py/bootstrap.result | 5 ++-
test/box/access_misc.result | 4 +-
test/box/access_sysview.result | 4 +-
test/box/alter.result | 1 +
test/sql/gh2141-delete-trigger-drop-table.result | 4 +-
test/sql/gh2141-delete-trigger-drop-table.test.lua | 4 +-
test/sql/persistency.result | 8 ++--
test/sql/persistency.test.lua | 8 ++--
test/sql/upgrade.result | 41 ++++++++++++++++++---
test/sql/upgrade.test.lua | 10 ++++-
17 files changed, 97 insertions(+), 36 deletions(-)
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 7258f47..f112a93 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -471,12 +471,16 @@ local function upgrade_to_2_1_0()
log.info("create space _trigger")
local format = {{name='name', type='string'},
+ {name='space_id', type='unsigned'},
{name='opts', type='map'}}
_space:insert{_trigger.id, ADMIN, '_trigger', 'memtx', 0, MAP, format}
log.info("create index primary on _trigger")
_index:insert{_trigger.id, 0, 'primary', 'tree', { unique = true },
{{0, 'string'}}}
+ log.info("create index secondary on _trigger")
+ _index:insert{_trigger.id, 1, 'space_id', 'tree', { unique = false },
+ {{1, 'unsigned'}}}
local stat1_ft = {{name='tbl', type='string'},
{name='idx', type='string'},
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index eb07332..b9a5fa5 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -217,6 +217,13 @@ enum {
BOX_SPACE_SEQUENCE_FIELD_IS_GENERATED = 2,
};
+/** _trigger fields. */
+enum {
+ BOX_TRIGGER_FIELD_NAME = 0,
+ BOX_TRIGGER_FIELD_SPACE_ID = 1,
+ BOX_TRIGGER_FIELD_OPTS = 2,
+};
+
/*
* Different objects which can be subject to access
* control.
diff --git a/src/box/sql.c b/src/box/sql.c
index 0ae33b6..1b5e4a2 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -669,8 +669,11 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
if (box_index_get(BOX_TRIGGER_ID, 0, key_begin, key, &tuple) != 0)
return SQL_TARANTOOL_ERROR;
assert(tuple != NULL);
- assert(tuple_field_count(tuple) == 2);
- const char *field = box_tuple_field(tuple, 1);
+ assert(tuple_field_count(tuple) == 3);
+ const char *field = box_tuple_field(tuple, BOX_TRIGGER_FIELD_SPACE_ID);
+ assert(mp_typeof(*field) == MP_UINT);
+ uint32_t space_id = mp_decode_uint(&field);
+ field = box_tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);
assert(mp_typeof(*field) == MP_MAP);
mp_decode_map(&field);
const char *sql_str = mp_decode_str(&field, &key_len);
@@ -693,16 +696,18 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
uint32_t trigger_stmt_new_len = trigger_stmt_len + new_table_name_len -
old_table_name_len + 2 * (!is_quoted);
assert(trigger_stmt_new_len > 0);
- key_len = mp_sizeof_array(2) + mp_sizeof_str(trig_name_len) +
+ key_len = mp_sizeof_array(3) + mp_sizeof_str(trig_name_len) +
mp_sizeof_map(1) + mp_sizeof_str(3) +
- mp_sizeof_str(trigger_stmt_new_len);
+ mp_sizeof_str(trigger_stmt_new_len) +
+ mp_sizeof_uint(space_id);
char *new_tuple = (char*)region_alloc(&fiber()->gc, key_len);
if (new_tuple == NULL) {
diag_set(OutOfMemory, key_len, "region_alloc", "new_tuple");
return SQL_TARANTOOL_ERROR;
}
- char *new_tuple_end = mp_encode_array(new_tuple, 2);
+ char *new_tuple_end = mp_encode_array(new_tuple, 3);
new_tuple_end = mp_encode_str(new_tuple_end, trig_name, trig_name_len);
+ new_tuple_end = mp_encode_uint(new_tuple_end, space_id);
new_tuple_end = mp_encode_map(new_tuple_end, 1);
new_tuple_end = mp_encode_str(new_tuple_end, "sql", 3);
new_tuple_end = mp_encode_str(new_tuple_end, trigger_stmt,
@@ -1253,7 +1258,7 @@ void tarantoolSqlite3LoadSchema(InitData *init)
init, TARANTOOL_SYS_TRIGGER_NAME,
BOX_TRIGGER_ID, 0,
"CREATE TABLE \""TARANTOOL_SYS_TRIGGER_NAME"\" ("
- "\"name\" TEXT PRIMARY KEY, \"opts\")"
+ "\"name\" TEXT PRIMARY KEY, \"space_id\" INT, \"opts\")"
);
sql_schema_put(
@@ -1308,14 +1313,14 @@ void tarantoolSqlite3LoadSchema(InitData *init)
const char *field, *ptr;
char *name, *sql;
unsigned len;
- assert(tuple_field_count(tuple) == 2);
+ assert(tuple_field_count(tuple) == 3);
field = tuple_field(tuple, 0);
assert (field != NULL);
ptr = mp_decode_str(&field, &len);
name = strndup(ptr, len);
- field = tuple_field(tuple, 1);
+ field = tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);
assert (field != NULL);
mp_decode_array(&field);
ptr = mp_decode_str(&field, &len);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index a9388f1..91e32ab 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3038,6 +3038,8 @@ struct Parse {
*/
struct Trigger {
char *zName; /* The name of the trigger */
+ /** The ID of space the trigger refers to. */
+ uint32_t space_id;
char *table; /* The table or view to which the trigger applies */
u8 op; /* One of TK_DELETE, TK_UPDATE, TK_INSERT */
u8 tr_tm; /* One of TRIGGER_BEFORE, TRIGGER_AFTER */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index fd1cb50..122c283 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -169,6 +169,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
if (pTrigger == 0)
goto trigger_cleanup;
pTrigger->zName = zName;
+ pTrigger->space_id = pTab->def->id;
zName = 0;
pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName);
pTrigger->pSchema = db->pSchema;
@@ -255,7 +256,7 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
/* makerecord(cursor(iRecord), [reg(iFirstCol), reg(iFirstCol+1)]) */
iFirstCol = pParse->nMem + 1;
- pParse->nMem += 2;
+ pParse->nMem += 3;
iRecord = ++pParse->nMem;
zOpts = sqlite3DbMallocRaw(pParse->db,
@@ -275,9 +276,10 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
sqlite3VdbeAddOp4(v,
OP_String8, 0, iFirstCol, 0,
zName, P4_DYNAMIC);
- sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 1,
+ sqlite3VdbeAddOp2(v, OP_Integer, pTrig->space_id, iFirstCol + 1);
+ sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 2,
MSGPACK_SUBTYPE, zOpts, P4_DYNAMIC);
- sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 2, iRecord);
+ sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 3, iRecord);
sqlite3VdbeAddOp2(v, OP_IdxInsert, iCursor, iRecord);
/* Do not account nested operations: the count of such
* operations depends on Tarantool data dictionary internals,
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index cee9164..02ef5ea 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -339,7 +339,7 @@ do
check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 1 --replica 2", "\n", 3)
check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 2", "\n", 0)
check_ctlcat_snap(test_i, dir, "--space=280", "---\n", 21)
- check_ctlcat_snap(test_i, dir, "--space=288", "---\n", 46)
+ check_ctlcat_snap(test_i, dir, "--space=288", "---\n", 47)
end)
end)
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index ad9501b..b5c16c0 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -65,8 +65,8 @@ box.space._space:select{}
{'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
- [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
'type': 'string'}]]
- - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'opts',
- 'type': 'map'}]]
+ - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id',
+ 'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]]
- [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
'type': 'unsigned'}]]
- [340, 1, '_space_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
@@ -122,6 +122,7 @@ box.space._index:select{}
- [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
- [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
- [328, 0, 'primary', 'tree', {'unique': true}, [[0, 'string']]]
+ - [328, 1, 'space_id', 'tree', {'unique': false}, [[1, 'unsigned']]]
- [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
- [340, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
- [340, 1, 'sequence', 'tree', {'unique': false}, [[1, 'unsigned']]]
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 19e82bb..5a2563d 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -798,8 +798,8 @@ box.space._space:select()
{'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
- [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
'type': 'string'}]]
- - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'opts',
- 'type': 'map'}]]
+ - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id',
+ 'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]]
- [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
'type': 'unsigned'}]]
- [340, 1, '_space_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result
index 3f93b5e..ae04266 100644
--- a/test/box/access_sysview.result
+++ b/test/box/access_sysview.result
@@ -234,7 +234,7 @@ box.session.su('guest')
...
#box.space._vindex:select{}
---
-- 47
+- 48
...
#box.space._vuser:select{}
---
@@ -262,7 +262,7 @@ box.session.su('guest')
...
#box.space._vindex:select{}
---
-- 47
+- 48
...
#box.space._vuser:select{}
---
diff --git a/test/box/alter.result b/test/box/alter.result
index 217c41d..c41b52f 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -224,6 +224,7 @@ _index:select{}
- [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
- [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
- [328, 0, 'primary', 'tree', {'unique': true}, [[0, 'string']]]
+ - [328, 1, 'space_id', 'tree', {'unique': false}, [[1, 'unsigned']]]
- [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
- [340, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
- [340, 1, 'sequence', 'tree', {'unique': false}, [[1, 'unsigned']]]
diff --git a/test/sql/gh2141-delete-trigger-drop-table.result b/test/sql/gh2141-delete-trigger-drop-table.result
index ba7016c..ec5a380 100644
--- a/test/sql/gh2141-delete-trigger-drop-table.result
+++ b/test/sql/gh2141-delete-trigger-drop-table.result
@@ -24,7 +24,7 @@ box.sql.execute("CREATE TRIGGER tt_ad AFTER DELETE ON t BEGIN SELECT 1; END")
---
...
-- check that these triggers exist
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
---
- - ['TT_AD', !!binary gaNzcWzZOkNSRUFURSBUUklHR0VSIHR0X2FkIEFGVEVSIERFTEVURSBPTiB0IEJFR0lOIFNFTEVDVCAxOyBFTkQ=]
- ['TT_AI', !!binary gaNzcWzZOkNSRUFURSBUUklHR0VSIHR0X2FpIEFGVEVSIElOU0VSVCBPTiB0IEJFR0lOIFNFTEVDVCAxOyBFTkQ=]
@@ -38,7 +38,7 @@ box.sql.execute("DROP TABLE t")
---
...
-- check that triggers were dropped with deleted table
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
---
- []
...
diff --git a/test/sql/gh2141-delete-trigger-drop-table.test.lua b/test/sql/gh2141-delete-trigger-drop-table.test.lua
index e6a030c..87110a4 100644
--- a/test/sql/gh2141-delete-trigger-drop-table.test.lua
+++ b/test/sql/gh2141-delete-trigger-drop-table.test.lua
@@ -11,10 +11,10 @@ box.sql.execute("CREATE TRIGGER tt_bd BEFORE DELETE ON t BEGIN SELECT 1; END")
box.sql.execute("CREATE TRIGGER tt_ad AFTER DELETE ON t BEGIN SELECT 1; END")
-- check that these triggers exist
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
-- drop table
box.sql.execute("DROP TABLE t")
-- check that triggers were dropped with deleted table
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
diff --git a/test/sql/persistency.result b/test/sql/persistency.result
index 7a7f6b8..d85d7cc 100644
--- a/test/sql/persistency.result
+++ b/test/sql/persistency.result
@@ -140,7 +140,7 @@ box.sql.execute("INSERT INTO barfoo VALUES ('foobar', 1000)")
box.sql.execute("CREATE TRIGGER tfoobar AFTER INSERT ON foobar BEGIN INSERT INTO barfoo VALUES ('trigger test', 9999); END")
---
...
-box.sql.execute("SELECT * FROM \"_trigger\"");
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"");
---
- - ['TFOOBAR', !!binary gaNzcWzZaUNSRUFURSBUUklHR0VSIHRmb29iYXIgQUZURVIgSU5TRVJUIE9OIGZvb2JhciBCRUdJTiBJTlNFUlQgSU5UTyBiYXJmb28gVkFMVUVTICgndHJpZ2dlciB0ZXN0JywgOTk5OSk7IEVORA==]
...
@@ -166,7 +166,7 @@ box.sql.execute("SELECT a FROM t1 ORDER BY b, a LIMIT 10 OFFSET 20;");
...
test_run:cmd('restart server default');
-- prove that trigger survived
-box.sql.execute("SELECT * FROM \"_trigger\"");
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"");
---
- - ['TFOOBAR', !!binary gaNzcWzZaUNSRUFURSBUUklHR0VSIHRmb29iYXIgQUZURVIgSU5TRVJUIE9OIGZvb2JhciBCRUdJTiBJTlNFUlQgSU5UTyBiYXJmb28gVkFMVUVTICgndHJpZ2dlciB0ZXN0JywgOTk5OSk7IEVORA==]
...
@@ -179,7 +179,7 @@ box.sql.execute("SELECT * FROM barfoo WHERE foo = 9999");
- - ['trigger test', 9999]
...
-- and still persistent
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
---
- - ['TFOOBAR', !!binary gaNzcWzZaUNSRUFURSBUUklHR0VSIHRmb29iYXIgQUZURVIgSU5TRVJUIE9OIGZvb2JhciBCRUdJTiBJTlNFUlQgSU5UTyBiYXJmb28gVkFMVUVTICgndHJpZ2dlciB0ZXN0JywgOTk5OSk7IEVORA==]
...
@@ -193,7 +193,7 @@ box.sql.execute("DROP TRIGGER tfoobar")
- error: 'no such trigger: TFOOBAR'
...
-- Should be empty
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
---
- []
...
diff --git a/test/sql/persistency.test.lua b/test/sql/persistency.test.lua
index bd05545..e994a62 100644
--- a/test/sql/persistency.test.lua
+++ b/test/sql/persistency.test.lua
@@ -49,7 +49,7 @@ box.sql.execute("INSERT INTO barfoo VALUES ('foobar', 1000)")
-- create a trigger
box.sql.execute("CREATE TRIGGER tfoobar AFTER INSERT ON foobar BEGIN INSERT INTO barfoo VALUES ('trigger test', 9999); END")
-box.sql.execute("SELECT * FROM \"_trigger\"");
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"");
-- Many entries
box.sql.execute("CREATE TABLE t1(a,b,c,PRIMARY KEY(b,c));")
@@ -59,21 +59,21 @@ box.sql.execute("SELECT a FROM t1 ORDER BY b, a LIMIT 10 OFFSET 20;");
test_run:cmd('restart server default');
-- prove that trigger survived
-box.sql.execute("SELECT * FROM \"_trigger\"");
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"");
-- ... functional
box.sql.execute("INSERT INTO foobar VALUES ('foobar trigger test', 8888)")
box.sql.execute("SELECT * FROM barfoo WHERE foo = 9999");
-- and still persistent
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
-- and can be dropped just once
box.sql.execute("DROP TRIGGER tfoobar")
-- Should error
box.sql.execute("DROP TRIGGER tfoobar")
-- Should be empty
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
-- prove barfoo2 still exists
box.sql.execute("INSERT INTO barfoo VALUES ('xfoo', 1)")
diff --git a/test/sql/upgrade.result b/test/sql/upgrade.result
index 9668a1d..6f7b115 100644
--- a/test/sql/upgrade.result
+++ b/test/sql/upgrade.result
@@ -19,8 +19,8 @@ test_run:switch('upgrade')
-- test system tables
box.space._space.index['name']:get('_trigger')
---
-- [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'opts',
- 'type': 'map'}]]
+- [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id',
+ 'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]]
...
box.space._space.index['name']:get('_sql_stat1')
---
@@ -69,6 +69,9 @@ box.sql.execute("CREATE TABLE t_out(x INTEGER PRIMARY KEY);")
box.sql.execute("CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1); END;")
---
...
+box.sql.execute("CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(2); END;")
+---
+...
box.space._space.index['name']:get('T')
---
- [513, 1, 'T', 'memtx', 1, {'sql': 'CREATE TABLE t(x INTEGER PRIMARY KEY)'}, [{'affinity': 68,
@@ -79,10 +82,37 @@ box.space._space.index['name']:get('T_OUT')
- [514, 1, 'T_OUT', 'memtx', 1, {'sql': 'CREATE TABLE t_out(x INTEGER PRIMARY KEY)'},
[{'affinity': 68, 'type': 'integer', 'nullable_action': 'abort', 'name': 'X', 'is_nullable': false}]]
...
-box.space._trigger:get('T1T')
+t1t = box.space._trigger:get('T1T')
+---
+...
+t2t = box.space._trigger:get('T2T')
+---
+...
+t1t.name
+---
+- T1T
+...
+t1t.opts
+---
+- {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1);
+ END;'}
+...
+t2t.name
+---
+- T2T
+...
+t2t.opts
+---
+- {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(2);
+ END;'}
+...
+assert(t1t.space_id == t2t.space_id)
---
-- ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1);
- END;'}]
+- true
+...
+assert(t1t.space_id == box.space.T.id)
+---
+- true
...
box.sql.execute("INSERT INTO T VALUES(1);")
---
@@ -94,6 +124,7 @@ box.space.T:select()
box.space.T_OUT:select()
---
- - [1]
+ - [2]
...
box.sql.execute("SELECT * FROM T")
---
diff --git a/test/sql/upgrade.test.lua b/test/sql/upgrade.test.lua
index d0add86..1619795 100644
--- a/test/sql/upgrade.test.lua
+++ b/test/sql/upgrade.test.lua
@@ -25,9 +25,17 @@ box.space._index:get({box.space._space.index['name']:get('T1').id, 0})
box.sql.execute("CREATE TABLE t(x INTEGER PRIMARY KEY);")
box.sql.execute("CREATE TABLE t_out(x INTEGER PRIMARY KEY);")
box.sql.execute("CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1); END;")
+box.sql.execute("CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(2); END;")
box.space._space.index['name']:get('T')
box.space._space.index['name']:get('T_OUT')
-box.space._trigger:get('T1T')
+t1t = box.space._trigger:get('T1T')
+t2t = box.space._trigger:get('T2T')
+t1t.name
+t1t.opts
+t2t.name
+t2t.opts
+assert(t1t.space_id == t2t.space_id)
+assert(t1t.space_id == box.space.T.id)
box.sql.execute("INSERT INTO T VALUES(1);")
box.space.T:select()
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] [PATCH v4 5/6] sql: move Triggers to server
2018-06-20 10:46 [tarantool-patches] [PATCH v4 0/6] sql: remove Triggers to server Kirill Shcherbatov
` (3 preceding siblings ...)
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 4/6] sql: new _trigger space format with space_id Kirill Shcherbatov
@ 2018-06-20 10:46 ` Kirill Shcherbatov
2018-06-24 15:24 ` [tarantool-patches] " n.pettik
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 6/6] sql: VDBE tests for trigger existence Kirill Shcherbatov
5 siblings, 1 reply; 15+ messages in thread
From: Kirill Shcherbatov @ 2018-06-20 10:46 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
Introduced sql_triggers field in space structure.
Changed parser logic to do not insert built triggers, just only
to do parsing. All triggers insertions and deletions are operated
via on_replace_dd_trigger now.
Resolves #3273.
---
src/box/alter.cc | 133 ++++++++++++++++
src/box/errcode.h | 2 +-
src/box/lua/schema.lua | 4 +
src/box/space.c | 5 +
src/box/space.h | 2 +
src/box/sql.c | 39 -----
src/box/sql.h | 50 ++++++
src/box/sql/build.c | 8 +-
src/box/sql/fkey.c | 2 -
src/box/sql/insert.c | 6 +-
src/box/sql/prepare.c | 3 +
src/box/sql/sqliteInt.h | 8 +-
src/box/sql/tokenize.c | 24 ++-
src/box/sql/trigger.c | 289 +++++++++++++++++-----------------
src/box/sql/vdbe.c | 82 ++--------
src/box/sql/vdbe.h | 1 -
src/box/sql/vdbeaux.c | 9 --
test/box/misc.result | 1 +
test/engine/iterator.result | 2 +-
test/sql-tap/identifier_case.test.lua | 4 +-
test/sql-tap/trigger1.test.lua | 14 +-
test/sql/errinj.result | 45 ++++++
test/sql/errinj.test.lua | 16 ++
test/sql/triggers.result | 196 +++++++++++++++++++++++
test/sql/triggers.test.lua | 74 +++++++++
25 files changed, 734 insertions(+), 285 deletions(-)
create mode 100644 test/sql/triggers.result
create mode 100644 test/sql/triggers.test.lua
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 706eab4..83e5807 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -553,6 +553,10 @@ space_swap_triggers(struct space *new_space, struct space *old_space)
rlist_swap(&new_space->before_replace, &old_space->before_replace);
rlist_swap(&new_space->on_replace, &old_space->on_replace);
rlist_swap(&new_space->on_stmt_begin, &old_space->on_stmt_begin);
+ /** Swap SQL Triggers pointer. */
+ struct Trigger *new_value = new_space->sql_triggers;
+ new_space->sql_triggers = old_space->sql_triggers;
+ old_space->sql_triggers = new_value;
}
/**
@@ -3244,6 +3248,53 @@ lock_before_dd(struct trigger *trigger, void *event)
}
/**
+ * Trigger invoked on rollback in the _trigger space.
+ */
+static void
+on_replace_trigger_rollback(struct trigger *trigger, void *event)
+{
+ struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
+ struct Trigger *old_trigger = (struct Trigger *)trigger->data;
+ struct Trigger *new_trigger;
+
+ if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) {
+ /* DELETE trigger. */
+ if (sql_trigger_replace(sql_get(),
+ sql_trigger_name(old_trigger),
+ old_trigger, &new_trigger) != 0)
+ panic("Out of memory on insertion into trigger hash");
+ assert(new_trigger == NULL);
+ } else if (stmt->new_tuple != NULL && stmt->old_tuple == NULL) {
+ /* INSERT trigger. */
+ int rc = sql_trigger_replace(sql_get(),
+ sql_trigger_name(old_trigger),
+ NULL, &new_trigger);
+ (void)rc;
+ assert(rc == 0);
+ assert(new_trigger == old_trigger);
+ sql_trigger_delete(sql_get(), new_trigger);
+ } else {
+ /* REPLACE trigger. */
+ if (sql_trigger_replace(sql_get(),
+ sql_trigger_name(old_trigger),
+ old_trigger, &new_trigger) != 0)
+ panic("Out of memory on insertion into trigger hash");
+ assert(old_trigger != new_trigger);
+ sql_trigger_delete(sql_get(), new_trigger);
+ }
+}
+
+/**
+ * Trigger invoked on commit in the _trigger space.
+ */
+static void
+on_replace_trigger_commit(struct trigger *trigger, void * /* event */)
+{
+ struct Trigger *old_trigger = (struct Trigger *)trigger->data;
+ sql_trigger_delete(sql_get(), old_trigger);
+}
+
+/**
* A trigger invoked on replace in a space containing
* SQL triggers.
*/
@@ -3252,6 +3303,88 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
{
struct txn *txn = (struct txn *) event;
txn_check_singlestatement_xc(txn, "Space _trigger");
+ struct txn_stmt *stmt = txn_current_stmt(txn);
+ struct tuple *old_tuple = stmt->old_tuple;
+ struct tuple *new_tuple = stmt->new_tuple;
+
+ struct trigger *on_rollback =
+ txn_alter_trigger_new(on_replace_trigger_rollback, NULL);
+ struct trigger *on_commit =
+ txn_alter_trigger_new(on_replace_trigger_commit, NULL);
+
+ if (old_tuple != NULL && new_tuple == NULL) {
+ /* DROP trigger. */
+ uint32_t trigger_name_len;
+ const char *trigger_name_src =
+ tuple_field_str_xc(old_tuple, BOX_TRIGGER_FIELD_NAME,
+ &trigger_name_len);
+ char *trigger_name =
+ (char *)region_alloc_xc(&fiber()->gc,
+ trigger_name_len + 1);
+ memcpy(trigger_name, trigger_name_src, trigger_name_len);
+ trigger_name[trigger_name_len] = 0;
+
+ struct Trigger *old_trigger;
+ int rc = sql_trigger_replace(sql_get(), trigger_name, NULL,
+ &old_trigger);
+ (void)rc;
+ assert(rc == 0);
+ assert(old_trigger != NULL);
+
+ on_commit->data = old_trigger;
+ on_rollback->data = old_trigger;
+ } else {
+ /* INSERT, REPLACE trigger. */
+ uint32_t trigger_name_len;
+ const char *trigger_name_src =
+ tuple_field_str_xc(new_tuple, BOX_TRIGGER_FIELD_NAME,
+ &trigger_name_len);
+
+ const char *space_opts =
+ tuple_field_with_type_xc(new_tuple,
+ BOX_TRIGGER_FIELD_OPTS,
+ MP_MAP);
+ struct space_opts opts;
+ struct region *region = &fiber()->gc;
+ space_opts_decode(&opts, space_opts, region);
+ struct Trigger *new_trigger =
+ sql_trigger_compile(sql_get(), opts.sql);
+ if (new_trigger == NULL)
+ diag_raise();
+
+ auto new_trigger_guard = make_scoped_guard([=] {
+ sql_trigger_delete(sql_get(), new_trigger);
+ });
+
+ const char *trigger_name = sql_trigger_name(new_trigger);
+ if (strlen(trigger_name) != trigger_name_len ||
+ memcmp(trigger_name_src, trigger_name,
+ trigger_name_len) != 0) {
+ tnt_raise(ClientError, ER_SQL,
+ "trigger name does not match extracted "
+ "from SQL");
+ }
+ uint32_t space_id =
+ tuple_field_u32_xc(new_tuple,
+ BOX_TRIGGER_FIELD_SPACE_ID);
+ if (space_id != sql_trigger_space_id(new_trigger)) {
+ tnt_raise(ClientError, ER_SQL,
+ "trigger space_id does not match the value "
+ "resolved on AST building from SQL");
+ }
+
+ struct Trigger *old_trigger;
+ if (sql_trigger_replace(sql_get(), trigger_name, new_trigger,
+ &old_trigger) != 0)
+ diag_raise();
+
+ on_commit->data = old_trigger;
+ on_rollback->data = new_trigger;
+ new_trigger_guard.is_active = false;
+ }
+
+ txn_on_rollback(txn, on_rollback);
+ txn_on_commit(txn, on_commit);
}
struct trigger alter_space_on_replace_space = {
diff --git a/src/box/errcode.h b/src/box/errcode.h
index ba52880..eb05936 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -107,7 +107,7 @@ struct errcode_record {
/* 52 */_(ER_FUNCTION_EXISTS, "Function '%s' already exists") \
/* 53 */_(ER_BEFORE_REPLACE_RET, "Invalid return value of space:before_replace trigger: expected tuple or nil, got %s") \
/* 54 */_(ER_FUNCTION_MAX, "A limit on the total number of functions has been reached: %u") \
- /* 55 */_(ER_UNUSED4, "") \
+ /* 55 */_(ER_TRIGGER_EXISTS, "Trigger '%s' already exists") \
/* 56 */_(ER_USER_MAX, "A limit on the total number of users has been reached: %u") \
/* 57 */_(ER_NO_SUCH_ENGINE, "Space engine '%s' does not exist") \
/* 58 */_(ER_RELOAD_CFG, "Can't set option '%s' dynamically") \
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 8bd0269..d856eaf 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -502,6 +502,7 @@ box.schema.space.drop = function(space_id, space_name, opts)
check_param_table(opts, { if_exists = 'boolean' })
local _space = box.space[box.schema.SPACE_ID]
local _index = box.space[box.schema.INDEX_ID]
+ local _trigger = box.space[box.schema.TRIGGER_ID]
local _vindex = box.space[box.schema.VINDEX_ID]
local _truncate = box.space[box.schema.TRUNCATE_ID]
local _space_sequence = box.space[box.schema.SPACE_SEQUENCE_ID]
@@ -510,6 +511,9 @@ box.schema.space.drop = function(space_id, space_name, opts)
-- Delete automatically generated sequence.
box.schema.sequence.drop(sequence_tuple[2])
end
+ for _, t in _trigger.index.space_id:pairs({space_id}) do
+ _trigger:delete({t.name})
+ end
local keys = _vindex:select(space_id)
for i = #keys, 1, -1 do
local v = keys[i]
diff --git a/src/box/space.c b/src/box/space.c
index 1606430..0809317 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -209,6 +209,11 @@ space_delete(struct space *space)
trigger_destroy(&space->on_replace);
trigger_destroy(&space->on_stmt_begin);
space_def_delete(space->def);
+ /*
+ * SQL Triggers should be deleted with
+ * on_replace_dd_trigger on deletion from _trigger.
+ */
+ assert(space->sql_triggers == NULL);
space->vtab->destroy(space);
}
diff --git a/src/box/space.h b/src/box/space.h
index b25f841..64aa8c7 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -146,6 +146,8 @@ struct space {
struct rlist on_replace;
/** Triggers fired before space statement */
struct rlist on_stmt_begin;
+ /** SQL Trigger list. */
+ struct Trigger *sql_triggers;
/**
* The number of *enabled* indexes in the space.
*
diff --git a/src/box/sql.c b/src/box/sql.c
index 1b5e4a2..2d22881 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1228,9 +1228,6 @@ space_foreach_put_cb(struct space *space, void *udata)
/* Load database schema from Tarantool. */
void tarantoolSqlite3LoadSchema(InitData *init)
{
- box_iterator_t *it;
- box_tuple_t *tuple;
-
sql_schema_put(
init, TARANTOOL_SYS_SCHEMA_NAME,
BOX_SCHEMA_ID, 0,
@@ -1299,42 +1296,6 @@ void tarantoolSqlite3LoadSchema(InitData *init)
init->rc = SQL_TARANTOOL_ERROR;
return;
}
-
- /* Read _trigger */
- it = box_index_iterator(BOX_TRIGGER_ID, 0, ITER_GE,
- nil_key, nil_key + sizeof(nil_key));
-
- if (it == NULL) {
- init->rc = SQL_TARANTOOL_ITERATOR_FAIL;
- return;
- }
-
- while (box_iterator_next(it, &tuple) == 0 && tuple != NULL) {
- const char *field, *ptr;
- char *name, *sql;
- unsigned len;
- assert(tuple_field_count(tuple) == 3);
-
- field = tuple_field(tuple, 0);
- assert (field != NULL);
- ptr = mp_decode_str(&field, &len);
- name = strndup(ptr, len);
-
- field = tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);
- assert (field != NULL);
- mp_decode_array(&field);
- ptr = mp_decode_str(&field, &len);
- assert (strncmp(ptr, "sql", 3) == 0);
-
- ptr = mp_decode_str(&field, &len);
- sql = strndup(ptr, len);
-
- sql_schema_put(init, name, 0, 0, sql);
-
- free(name);
- free(sql);
- }
- box_iterator_free(it);
}
/*********************************************************************
diff --git a/src/box/sql.h b/src/box/sql.h
index 90bba1a..7dc3f80 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -95,6 +95,17 @@ struct Select *
sql_view_compile(struct sqlite3 *db, const char *view_stmt);
/**
+ * Perform parsing of provided SQL request and construct trigger AST.
+ * @param db SQL context handle.
+ * @param sql request to parse.
+ *
+ * @retval NULL on error
+ * @retval not NULL Trigger AST pointer on success.
+ */
+struct Trigger *
+sql_trigger_compile(struct sqlite3 *db, const char *sql);
+
+/**
* Free AST pointed by trigger.
* @param db SQL handle.
* @param trigger AST object.
@@ -103,6 +114,45 @@ void
sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger);
/**
+ * Get server triggers list by space_id.
+ * @param space_id Space ID.
+ *
+ * @retval trigger AST list.
+ */
+struct Trigger *
+space_trigger_list(uint32_t space_id);
+
+/**
+ * Perform replace trigger in SQL internals with new AST object.
+ * @param db SQL handle.
+ * @param name a name of the trigger.
+ * @param trigger AST object to insert.
+ * @param[out] old_trigger Old object if exists.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+sql_trigger_replace(struct sqlite3 *db, const char *name,
+ struct Trigger *trigger, struct Trigger **old_trigger);
+
+/**
+ * Get trigger name by trigger AST object.
+ * @param trigger AST object.
+ * @return trigger name string.
+ */
+const char *
+sql_trigger_name(struct Trigger *trigger);
+
+/**
+ * Get space_id of the space that trigger has been built for.
+ * @param trigger AST object.
+ * @return space identifier.
+ */
+uint32_t
+sql_trigger_space_id(struct Trigger *trigger);
+
+/**
* Store duplicate of a parsed expression into @a parser.
* @param parser Parser context.
* @param select Select to extract from.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 592c9a6..c8bfad7 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2112,16 +2112,14 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
/*
* Drop all triggers associated with the table being
* dropped. Code is generated to remove entries from
- * _trigger. OP_DropTrigger will remove it from internal
- * SQL structures.
+ * _trigger. on_replace_dd_trigger will remove it from
+ * internal SQL structures.
*
* Do not account triggers deletion - they will be
* accounted in DELETE from _space below.
*/
parse_context->nested++;
- Table *table = sqlite3HashFind(&parse_context->db->pSchema->tblHash,
- space->def->name);
- struct Trigger *trigger = table->pTrigger;
+ struct Trigger *trigger = space->sql_triggers;
while (trigger != NULL) {
sqlite3DropTriggerPtr(parse_context, trigger);
trigger = trigger->pNext;
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index e3fff37..ce63ff0 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -1429,8 +1429,6 @@ fkActionTrigger(Parse * pParse, /* Parse context */
pStep->op = TK_UPDATE;
}
pStep->pTrig = pTrigger;
- pTrigger->pSchema = pTab->pSchema;
- pTrigger->pTabSchema = pTab->pSchema;
pFKey->apTrigger[iAction] = pTrigger;
pTrigger->op = (pChanges ? TK_UPDATE : TK_DELETE);
}
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 70555c3..9f03829 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1748,9 +1748,9 @@ xferOptimization(Parse * pParse, /* Parser context */
*/
return 0;
}
- if (pDest->pTrigger) {
- return 0; /* tab1 must not have triggers */
- }
+ /* The pDest must not have triggers. */
+ if (space_trigger_list(pDest->def->id) != NULL)
+ return 0;
if (onError == ON_CONFLICT_ACTION_DEFAULT) {
if (pDest->iPKey >= 0)
onError = pDest->keyConf;
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index bddc21a..629f68e 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -461,6 +461,9 @@ sql_parser_destroy(Parse *parser)
case AST_TYPE_EXPR:
sql_expr_delete(db, parser->parsed_ast.expr, false);
break;
+ case AST_TYPE_TRIGGER:
+ sql_trigger_delete(db, parser->parsed_ast.trigger);
+ break;
default:
assert(parser->parsed_ast_type == AST_TYPE_UNDEFINED);
}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 91e32ab..72803fa 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1934,7 +1934,6 @@ struct Table {
#ifndef SQLITE_OMIT_ALTERTABLE
int addColOffset; /* Offset in CREATE TABLE stmt to add a new column */
#endif
- Trigger *pTrigger; /* List of triggers stored in pSchema */
Schema *pSchema; /* Schema that contains this table */
Table *pNextZombie; /* Next on the Parse.pZombieTab list */
/** Space definition with Tarantool metadata. */
@@ -2855,6 +2854,7 @@ enum ast_type {
AST_TYPE_UNDEFINED = 0,
AST_TYPE_SELECT,
AST_TYPE_EXPR,
+ AST_TYPE_TRIGGER,
ast_type_MAX
};
@@ -2949,7 +2949,6 @@ struct Parse {
Vdbe *pReprepare; /* VM being reprepared (sqlite3Reprepare()) */
const char *zTail; /* All SQL text past the last semicolon parsed */
Table *pNewTable; /* A table being constructed by CREATE TABLE */
- Trigger *pNewTrigger; /* Trigger under construct by a CREATE TRIGGER */
Table *pZombieTab; /* List of Table objects to delete after code gen */
TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */
With *pWith; /* Current WITH clause, or NULL */
@@ -2967,6 +2966,7 @@ struct Parse {
union {
struct Expr *expr;
struct Select *select;
+ struct Trigger *trigger;
} parsed_ast;
};
@@ -3040,14 +3040,11 @@ struct Trigger {
char *zName; /* The name of the trigger */
/** The ID of space the trigger refers to. */
uint32_t space_id;
- char *table; /* The table or view to which the trigger applies */
u8 op; /* One of TK_DELETE, TK_UPDATE, TK_INSERT */
u8 tr_tm; /* One of TRIGGER_BEFORE, TRIGGER_AFTER */
Expr *pWhen; /* The WHEN clause of the expression (may be NULL) */
IdList *pColumns; /* If this is an UPDATE OF <column-list> trigger,
the <column-list> is stored here */
- Schema *pSchema; /* Schema containing the trigger */
- Schema *pTabSchema; /* Schema containing the table */
TriggerStep *step_list; /* Link list of trigger program steps */
Trigger *pNext; /* Next trigger associated with the table */
};
@@ -4093,7 +4090,6 @@ TriggerStep *sqlite3TriggerInsertStep(sqlite3 *, Token *, IdList *,
TriggerStep *sqlite3TriggerUpdateStep(sqlite3 *, Token *, ExprList *, Expr *,
u8);
TriggerStep *sqlite3TriggerDeleteStep(sqlite3 *, Token *, Expr *);
-void sqlite3UnlinkAndDeleteTrigger(sqlite3 *, const char *);
u32 sqlite3TriggerColmask(Parse *, Trigger *, ExprList *, int, int, Table *,
int);
#define sqlite3ParseToplevel(p) ((p)->pToplevel ? (p)->pToplevel : (p))
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 1e84c1e..edcc45b 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -450,7 +450,7 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
return SQLITE_NOMEM_BKPT;
}
assert(pParse->pNewTable == 0);
- assert(pParse->pNewTrigger == 0);
+ assert(pParse->parsed_ast.trigger == NULL);
assert(pParse->nVar == 0);
assert(pParse->pVList == 0);
while (1) {
@@ -534,7 +534,6 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
if (pParse->pWithToFree)
sqlite3WithDelete(db, pParse->pWithToFree);
- sql_trigger_delete(db, pParse->pNewTrigger);
sqlite3DbFree(db, pParse->pVList);
while (pParse->pZombieTab) {
Table *p = pParse->pZombieTab;
@@ -597,3 +596,24 @@ sql_view_compile(struct sqlite3 *db, const char *view_stmt)
sql_parser_destroy(&parser);
return select;
}
+
+struct Trigger *
+sql_trigger_compile(struct sqlite3 *db, const char *sql)
+{
+ struct Parse parser;
+ sql_parser_create(&parser, db);
+ parser.parse_only = true;
+ char *sql_error;
+ struct Trigger *trigger = NULL;
+ if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK ||
+ parser.parsed_ast_type != AST_TYPE_TRIGGER) {
+ if (parser.rc != SQL_TARANTOOL_ERROR)
+ diag_set(ClientError, ER_SQL, sql_error);
+ } else {
+ trigger = parser.parsed_ast.trigger;
+ parser.parsed_ast.trigger = NULL;
+ }
+
+ sql_parser_destroy(&parser);
+ return trigger;
+}
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 122c283..e4c936d 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -33,6 +33,7 @@
* This file contains the implementation for TRIGGERs
*/
+#include "box/schema.h"
#include "sqliteInt.h"
#include "tarantoolInt.h"
#include "vdbeInt.h"
@@ -81,115 +82,141 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
)
{
Trigger *pTrigger = 0; /* The new trigger */
- Table *pTab; /* Table that the trigger fires off of */
char *zName = 0; /* Name of the trigger */
sqlite3 *db = pParse->db; /* The database connection */
DbFixer sFix; /* State vector for the DB fixer */
- /* Do not account nested operations: the count of such
- * operations depends on Tarantool data dictionary internals,
- * such as data layout in system spaces.
+ /*
+ * Do not account nested operations: the count of such
+ * operations depends on Tarantool data dictionary
+ * internals, such as data layout in system spaces.
*/
if (!pParse->nested) {
Vdbe *v = sqlite3GetVdbe(pParse);
if (v != NULL)
sqlite3VdbeCountChanges(v);
}
- assert(pName != 0); /* pName->z might be NULL, but not pName itself */
+ /* pName->z might be NULL, but not pName itself. */
+ assert(pName != NULL);
assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE);
assert(op > 0 && op < 0xff);
- if (!pTableName || db->mallocFailed) {
+ if (pTableName == NULL || db->mallocFailed)
goto trigger_cleanup;
- }
- /* Ensure the table name matches database name and that the table exists */
+ /*
+ * Ensure the table name matches database name and that
+ * the table exists.
+ */
if (db->mallocFailed)
goto trigger_cleanup;
assert(pTableName->nSrc == 1);
sqlite3FixInit(&sFix, pParse, "trigger", pName);
- if (sqlite3FixSrcList(&sFix, pTableName)) {
+ if (sqlite3FixSrcList(&sFix, pTableName) != 0)
goto trigger_cleanup;
- }
- pTab = sql_list_lookup_table(pParse, pTableName);
- if (!pTab) {
- goto trigger_cleanup;
- }
- /* Check that the trigger name is not reserved and that no trigger of the
- * specified name exists
- */
zName = sqlite3NameFromToken(db, pName);
- if (!zName || SQLITE_OK != sqlite3CheckIdentifierName(pParse, zName)) {
+ if (zName == NULL)
goto trigger_cleanup;
- }
- if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) {
+
+ if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
+ goto trigger_cleanup;
+
+ if (!pParse->parse_only &&
+ sqlite3HashFind(&db->pSchema->trigHash, zName) != NULL) {
if (!noErr) {
- sqlite3ErrorMsg(pParse, "trigger %s already exists",
- zName);
+ diag_set(ClientError, ER_TRIGGER_EXISTS, zName);
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
} else {
assert(!db->init.busy);
}
goto trigger_cleanup;
}
- /* Do not create a trigger on a system table */
- if (sqlite3StrNICmp(pTab->def->name, "sqlite_", 7) == 0) {
- sqlite3ErrorMsg(pParse,
- "cannot create trigger on system table");
+ const char *table_name = pTableName->a[0].zName;
+ uint32_t space_id;
+ if (schema_find_id(BOX_SPACE_ID, 2, table_name, strlen(table_name),
+ &space_id) != 0) {
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
+ goto trigger_cleanup;
+ }
+ if (space_id == BOX_ID_NIL) {
+ diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
+ goto trigger_cleanup;
+ }
+
+ /* Do not create a trigger on a system table. */
+ if (sqlite3StrNICmp(table_name, "sqlite_", 7) == 0) {
+ diag_set(ClientError, ER_SQL,
+ "cannot create trigger on system table");
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
goto trigger_cleanup;
}
- /* INSTEAD of triggers are only for views and views only support INSTEAD
- * of triggers.
+ /*
+ * INSTEAD of triggers are only for views and
+ * views only support INSTEAD of triggers.
*/
- if (pTab->def->opts.is_view && tr_tm != TK_INSTEAD) {
- sqlite3ErrorMsg(pParse, "cannot create %s trigger on view: %S",
- (tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER",
- pTableName, 0);
+ struct space *space = space_cache_find(space_id);
+ assert(space != NULL);
+ if (space->def->opts.is_view && tr_tm != TK_INSTEAD) {
+ diag_set(ClientError, ER_SQL, tt_sprintf("cannot create %s "\
+ "trigger on view: %s", tr_tm == TK_BEFORE ? "BEFORE" :
+ "AFTER", table_name));
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
goto trigger_cleanup;
}
- if (!pTab->def->opts.is_view && tr_tm == TK_INSTEAD) {
- sqlite3ErrorMsg(pParse, "cannot create INSTEAD OF"
- " trigger on table: %S", pTableName, 0);
+ if (!space->def->opts.is_view && tr_tm == TK_INSTEAD) {
+ diag_set(ClientError, ER_SQL, tt_sprintf("cannot create "\
+ "INSTEAD OF trigger on table: %s", table_name));
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
goto trigger_cleanup;
}
- /* INSTEAD OF triggers can only appear on views and BEFORE triggers
+ /*
+ * INSTEAD OF triggers can only appear on views and BEFORE triggers
* cannot appear on views. So we might as well translate every
* INSTEAD OF trigger into a BEFORE trigger. It simplifies code
* elsewhere.
*/
- if (tr_tm == TK_INSTEAD) {
+ if (tr_tm == TK_INSTEAD)
tr_tm = TK_BEFORE;
- }
- /* Build the Trigger object */
- pTrigger = (Trigger *) sqlite3DbMallocZero(db, sizeof(Trigger));
- if (pTrigger == 0)
+ /* Build the Trigger object. */
+ pTrigger = (Trigger *)sqlite3DbMallocZero(db, sizeof(Trigger));
+ if (pTrigger == NULL)
goto trigger_cleanup;
+ pTrigger->space_id = space_id;
pTrigger->zName = zName;
- pTrigger->space_id = pTab->def->id;
- zName = 0;
- pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName);
- pTrigger->pSchema = db->pSchema;
- pTrigger->pTabSchema = pTab->pSchema;
+ zName = NULL;
+
pTrigger->op = (u8) op;
pTrigger->tr_tm = tr_tm == TK_BEFORE ? TRIGGER_BEFORE : TRIGGER_AFTER;
pTrigger->pWhen = sqlite3ExprDup(db, pWhen, EXPRDUP_REDUCE);
pTrigger->pColumns = sqlite3IdListDup(db, pColumns);
- assert(pParse->pNewTrigger == 0);
- pParse->pNewTrigger = pTrigger;
+ if ((pWhen != NULL && pTrigger->pWhen == NULL) ||
+ (pColumns != NULL && pTrigger->pColumns == NULL))
+ goto trigger_cleanup;
+ assert(pParse->parsed_ast.trigger == NULL);
+ pParse->parsed_ast.trigger = pTrigger;
+ pParse->parsed_ast_type = AST_TYPE_TRIGGER;
trigger_cleanup:
sqlite3DbFree(db, zName);
sqlite3SrcListDelete(db, pTableName);
sqlite3IdListDelete(db, pColumns);
sql_expr_delete(db, pWhen, false);
- if (pParse->pNewTrigger == NULL)
+ if (pParse->parsed_ast.trigger == NULL)
sql_trigger_delete(db, pTrigger);
else
- assert(pParse->pNewTrigger == pTrigger);
+ assert(pParse->parsed_ast.trigger == pTrigger);
}
/*
@@ -202,7 +229,8 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
Token * pAll /* Token that describes the complete CREATE TRIGGER */
)
{
- Trigger *pTrig = pParse->pNewTrigger; /* Trigger being finished */
+ /* Trigger being finished. */
+ Trigger *pTrig = pParse->parsed_ast.trigger;
char *zName; /* Name of trigger */
char *zSql = 0; /* SQL text */
char *zOpts = 0; /* MsgPack containing SQL options */
@@ -210,7 +238,7 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
DbFixer sFix; /* Fixer object */
Token nameToken; /* Trigger name for error reporting */
- pParse->pNewTrigger = 0;
+ pParse->parsed_ast.trigger = NULL;
if (NEVER(pParse->nErr) || !pTrig)
goto triggerfinish_cleanup;
zName = pTrig->zName;
@@ -227,10 +255,11 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
goto triggerfinish_cleanup;
}
- /* if we are not initializing,
- * generate byte code to insert a new trigger into Tarantool.
+ /*
+ * Generate byte code to insert a new trigger into
+ * Tarantool for non-parsing mode or export trigger.
*/
- if (!db->init.busy) {
+ if (!pParse->parse_only) {
Vdbe *v;
int zOptsSz;
Table *pSysTrigger;
@@ -289,37 +318,12 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
sqlite3VdbeAddOp1(v, OP_Close, iCursor);
- /* parseschema3(reg(iFirstCol), ref(iFirstCol)+1) */
- iFirstCol = pParse->nMem + 1;
- pParse->nMem += 2;
-
sql_set_multi_write(pParse, false);
- sqlite3VdbeAddOp4(v,
- OP_String8, 0, iFirstCol, 0,
- zName, P4_STATIC);
-
- sqlite3VdbeAddOp4(v,
- OP_String8, 0, iFirstCol + 1, 0,
- zSql, P4_DYNAMIC);
sqlite3ChangeCookie(pParse);
- sqlite3VdbeAddParseSchema3Op(v, iFirstCol);
- }
-
- if (db->init.busy) {
- Trigger *pLink = pTrig;
- Hash *pHash = &db->pSchema->trigHash;
- pTrig = sqlite3HashInsert(pHash, zName, pTrig);
- if (pTrig) {
- sqlite3OomFault(db);
- } else if (pLink->pSchema == pLink->pTabSchema) {
- Table *pTab;
- pTab =
- sqlite3HashFind(&pLink->pTabSchema->tblHash,
- pLink->table);
- assert(pTab != 0);
- pLink->pNext = pTab->pTrigger;
- pTab->pTrigger = pLink;
- }
+ } else {
+ pParse->parsed_ast.trigger = pTrig;
+ pParse->parsed_ast_type = AST_TYPE_TRIGGER;
+ pTrig = NULL;
}
triggerfinish_cleanup:
@@ -330,7 +334,7 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
alloc for it either wasn't called at all or failed. */
}
sql_trigger_delete(db, pTrig);
- assert(!pParse->pNewTrigger);
+ assert(pParse->parsed_ast.trigger == NULL || pParse->parse_only);
sqlite3DeleteTriggerStep(db, pStepList);
}
@@ -479,7 +483,6 @@ sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger)
return;
sqlite3DeleteTriggerStep(db, trigger->step_list);
sqlite3DbFree(db, trigger->zName);
- sqlite3DbFree(db, trigger->table);
sql_expr_delete(db, trigger->pWhen, false);
sqlite3IdListDelete(db, trigger->pColumns);
sqlite3DbFree(db, trigger);
@@ -534,16 +537,6 @@ sqlite3DropTrigger(Parse * pParse, SrcList * pName, int noErr)
}
/*
- * Return a pointer to the Table structure for the table that a trigger
- * is set on.
- */
-static Table *
-tableOfTrigger(Trigger * pTrigger)
-{
- return sqlite3HashFind(&pTrigger->pTabSchema->tblHash, pTrigger->table);
-}
-
-/*
* Drop a trigger given a pointer to that trigger.
*/
void
@@ -566,34 +559,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
sqlite3ChangeCookie(pParse);
- sqlite3VdbeAddOp4(v, OP_DropTrigger, 0, 0, 0, pTrigger->zName,
- 0);
}
}
-/*
- * Remove a trigger from the hash tables of the sqlite* pointer.
- */
-void
-sqlite3UnlinkAndDeleteTrigger(sqlite3 * db, const char *zName)
+int
+sql_trigger_replace(struct sqlite3 *db, const char *name,
+ struct Trigger *trigger, struct Trigger **old_trigger)
{
- Trigger *pTrigger;
- Hash *pHash;
- struct session *user_session = current_session();
-
- pHash = &(db->pSchema->trigHash);
- pTrigger = sqlite3HashInsert(pHash, zName, 0);
- if (ALWAYS(pTrigger)) {
- if (pTrigger->pSchema == pTrigger->pTabSchema) {
- Table *pTab = tableOfTrigger(pTrigger);
- Trigger **pp;
- for (pp = &pTab->pTrigger; *pp != pTrigger;
- pp = &((*pp)->pNext)) ;
- *pp = (*pp)->pNext;
- }
- sql_trigger_delete(db, pTrigger);
- user_session->sql_flags |= SQLITE_InternChanges;
+ assert(db->pSchema != NULL);
+ assert(trigger == NULL || strcmp(name, trigger->zName) == 0);
+ struct Hash *hash = &db->pSchema->trigHash;
+ *old_trigger = sqlite3HashInsert(hash, name, trigger);
+ if (*old_trigger == trigger) {
+ diag_set(OutOfMemory, sizeof(struct HashElem),
+ "sqlite3HashInsert", "ret");
+ return -1;
}
+ struct Trigger *src_trigger = trigger != NULL ? trigger : *old_trigger;
+ assert(src_trigger != NULL);
+ struct space *space = space_cache_find(src_trigger->space_id);
+ assert(space != NULL);
+
+ if (*old_trigger != NULL) {
+ struct Trigger **pp;
+ for (pp = &space->sql_triggers; *pp != *old_trigger;
+ pp = &((*pp)->pNext));
+ *pp = (*pp)->pNext;
+ }
+ if (trigger != NULL) {
+ trigger->pNext = space->sql_triggers;
+ space->sql_triggers = trigger;
+ }
+ return 0;
+}
+
+const char *
+sql_trigger_name(struct Trigger *trigger)
+{
+ return trigger->zName;
+}
+
+uint32_t
+sql_trigger_space_id(struct Trigger *trigger)
+{
+ return trigger->space_id;
+}
+
+struct Trigger *
+space_trigger_list(uint32_t space_id)
+{
+ struct space *space = space_cache_find(space_id);
+ assert(space != NULL);
+ assert(space->def != NULL);
+ return space->sql_triggers;
}
/*
@@ -632,22 +650,18 @@ sqlite3TriggersExist(Table * pTab, /* The table the contains the triggers */
)
{
int mask = 0;
- Trigger *pList = 0;
- Trigger *p;
+ struct Trigger *trigger_list = NULL;
struct session *user_session = current_session();
-
- if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0) {
- pList = pTab->pTrigger;
- }
- for (p = pList; p; p = p->pNext) {
- if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) {
+ if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0)
+ trigger_list = space_trigger_list(pTab->def->id);
+ for (struct Trigger *p = trigger_list; p != NULL; p = p->pNext) {
+ if (p->op == op && checkColumnOverlap(p->pColumns,
+ pChanges) != 0)
mask |= p->tr_tm;
- }
}
- if (pMask) {
+ if (pMask != NULL)
*pMask = mask;
- }
- return (mask ? pList : 0);
+ return mask != 0 ? trigger_list : NULL;
}
/*
@@ -837,7 +851,7 @@ codeRowTrigger(Parse * pParse, /* Current parse context */
Parse *pSubParse; /* Parse context for sub-vdbe */
int iEndTrigger = 0; /* Label to jump to if WHEN is false */
- assert(pTrigger->zName == 0 || pTab == tableOfTrigger(pTrigger));
+ assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id);
assert(pTop->pVdbe);
/* Allocate the TriggerPrg and SubProgram objects. To ensure that they
@@ -954,7 +968,7 @@ getRowTrigger(Parse * pParse, /* Current parse context */
Parse *pRoot = sqlite3ParseToplevel(pParse);
TriggerPrg *pPrg;
- assert(pTrigger->zName == 0 || pTab == tableOfTrigger(pTrigger));
+ assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id);
/* It may be that this trigger has already been coded (or is in the
* process of being coded). If this is the case, then an entry with
@@ -1079,15 +1093,6 @@ sqlite3CodeRowTrigger(Parse * pParse, /* Parse context */
assert((op == TK_UPDATE) == (pChanges != 0));
for (p = pTrigger; p; p = p->pNext) {
-
- /* Sanity checking: The schema for the trigger and for the table are
- * always defined. The trigger must be in the same schema as the table
- * or else it must be a TEMP trigger.
- */
- assert(p->pSchema != 0);
- assert(p->pTabSchema != 0);
- assert(p->pSchema == p->pTabSchema);
-
/* Determine whether we should code this trigger */
if (p->op == op
&& p->tr_tm == tr_tm
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 5b5cf83..c1df880 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4710,46 +4710,6 @@ case OP_ParseSchema2: {
break;
}
-/* Opcode: ParseSchema3 P1 * * * *
- * Synopsis: name=r[P1] sql=r[P1+1]
- *
- * Create trigger named r[P1] w/ DDL SQL stored in r[P1+1]
- * in database P2
- */
-case OP_ParseSchema3: {
- InitData initData;
- Mem *pRec;
- char zPgnoBuf[16];
- char *argv[4] = {NULL, zPgnoBuf, NULL, NULL};
- assert(db->pSchema != NULL);
-
- initData.db = db;
- initData.pzErrMsg = &p->zErrMsg;
-
- assert(db->init.busy==0);
- db->init.busy = 1;
- initData.rc = SQLITE_OK;
- assert(!db->mallocFailed);
-
- pRec = &aMem[pOp->p1];
- argv[0] = pRec[0].z;
- argv[1] = "0";
- argv[2] = pRec[1].z;
- sqlite3InitCallback(&initData, 3, argv, NULL);
-
- rc = initData.rc;
- db->init.busy = 0;
-
- if (rc) {
- sqlite3ResetAllSchemasOfConnection(db);
- if (rc==SQLITE_NOMEM) {
- goto no_mem;
- }
- goto abort_due_to_error;
- }
- break;
-}
-
/* Opcode: RenameTable P1 * * P4 *
* Synopsis: P1 = root, P4 = name
*
@@ -4769,7 +4729,6 @@ case OP_RenameTable: {
const char *zNewTableName;
Table *pTab;
FKey *pFKey;
- Trigger *pTrig;
int iRootPage;
InitData initData;
char *argv[4] = {NULL, NULL, NULL, NULL};
@@ -4778,11 +4737,12 @@ case OP_RenameTable: {
space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1);
space = space_by_id(space_id);
assert(space);
+ /* Rename space op doesn't change triggers. */
+ struct Trigger *triggers = space->sql_triggers;
zOldTableName = space_name(space);
assert(zOldTableName);
pTab = sqlite3HashFind(&db->pSchema->tblHash, zOldTableName);
assert(pTab);
- pTrig = pTab->pTrigger;
iRootPage = pTab->tnum;
zNewTableName = pOp->p4.z;
zOldTableName = sqlite3DbStrNDup(db, zOldTableName,
@@ -4823,18 +4783,22 @@ case OP_RenameTable: {
goto abort_due_to_error;
}
- pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName);
- pTab->pTrigger = pTrig;
-
- /* Rename all trigger created on this table.*/
- for (; pTrig; pTrig = pTrig->pNext) {
- sqlite3DbFree(db, pTrig->table);
- pTrig->table = sqlite3DbStrNDup(db, zNewTableName,
- sqlite3Strlen30(zNewTableName));
- pTrig->pTabSchema = pTab->pSchema;
- rc = tarantoolSqlite3RenameTrigger(pTrig->zName,
+ /*
+ * Rebuild 'CREATE TRIGGER' expressions of all triggers
+ * created on this table. Sure, this action is not atomic
+ * due to lack of transactional DDL, but just do the best
+ * effort.
+ */
+ for (struct Trigger *trigger = triggers; trigger != NULL; ) {
+ /* Store pointer as trigger will be destructed. */
+ struct Trigger *next_trigger = trigger->pNext;
+ rc = tarantoolSqlite3RenameTrigger(trigger->zName,
zOldTableName, zNewTableName);
- if (rc) goto abort_due_to_error;
+ if (rc != SQLITE_OK) {
+ sqlite3ResetAllSchemasOfConnection(db);
+ goto abort_due_to_error;
+ }
+ trigger = next_trigger;
}
sqlite3DbFree(db, (void*)zOldTableName);
sqlite3DbFree(db, (void*)zSqlStmt);
@@ -4881,18 +4845,6 @@ case OP_DropIndex: {
break;
}
-/* Opcode: DropTrigger P1 * * P4 *
- *
- * Remove the internal (in-memory) data structures that describe
- * the trigger named P4 in database P1. This is called after a trigger
- * is dropped from disk (using the Destroy opcode) in order to keep
- * the internal representation of the
- * schema consistent with what is on disk.
- */
-case OP_DropTrigger: {
- sqlite3UnlinkAndDeleteTrigger(db, pOp->p4.z);
- break;
-}
#ifndef SQLITE_OMIT_TRIGGER
/* Opcode: Program P1 P2 P3 P4 P5
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 68d542c..77fa41a 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -238,7 +238,6 @@ void sqlite3VdbeVerifyNoResultRow(Vdbe * p);
VdbeOp *sqlite3VdbeAddOpList(Vdbe *, int nOp, VdbeOpList const *aOp,
int iLineno);
void sqlite3VdbeAddParseSchema2Op(Vdbe * p, int, int);
-void sqlite3VdbeAddParseSchema3Op(Vdbe * p, int);
void sqlite3VdbeAddRenameTableOp(Vdbe * p, int, char *);
void sqlite3VdbeChangeOpcode(Vdbe *, u32 addr, u8);
void sqlite3VdbeChangeP1(Vdbe *, u32 addr, int P1);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 679bd0b..7b3c13d 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -410,15 +410,6 @@ sqlite3VdbeAddParseSchema2Op(Vdbe * p, int iRec, int n)
sqlite3VdbeAddOp3(p, OP_ParseSchema2, iRec, n, 0);
}
-/*
- * Add an OP_ParseSchema3 opcode which in turn will create a trigger
- */
-void
-sqlite3VdbeAddParseSchema3Op(Vdbe * p, int iRec)
-{
- sqlite3VdbeAddOp2(p, OP_ParseSchema3, iRec, 0);
-}
-
void
sqlite3VdbeAddRenameTableOp(Vdbe * p, int iTab, char* zNewName)
{
diff --git a/test/box/misc.result b/test/box/misc.result
index 930d0a8..3b273a8 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -382,6 +382,7 @@ t;
52: box.error.FUNCTION_EXISTS
53: box.error.BEFORE_REPLACE_RET
54: box.error.FUNCTION_MAX
+ 55: box.error.TRIGGER_EXISTS
56: box.error.USER_MAX
57: box.error.NO_SUCH_ENGINE
58: box.error.RELOAD_CFG
diff --git a/test/engine/iterator.result b/test/engine/iterator.result
index 4984076..a36761d 100644
--- a/test/engine/iterator.result
+++ b/test/engine/iterator.result
@@ -4211,7 +4211,7 @@ s:replace{35}
...
state, value = gen(param,state)
---
-- error: 'builtin/box/schema.lua:1045: usage: next(param, state)'
+- error: 'builtin/box/schema.lua:1049: usage: next(param, state)'
...
value
---
diff --git a/test/sql-tap/identifier_case.test.lua b/test/sql-tap/identifier_case.test.lua
index 3f6dc07..5e7573a 100755
--- a/test/sql-tap/identifier_case.test.lua
+++ b/test/sql-tap/identifier_case.test.lua
@@ -170,8 +170,8 @@ test:do_execsql_test(
data = {
{ 1, [[ trigger1 ]], {0}},
- { 2, [[ Trigger1 ]], {1, "trigger TRIGGER1 already exists"}},
- { 3, [["TRIGGER1"]], {1, "trigger TRIGGER1 already exists"}},
+ { 2, [[ Trigger1 ]], {1, "Trigger 'TRIGGER1' already exists"}},
+ { 3, [["TRIGGER1"]], {1, "Trigger 'TRIGGER1' already exists"}},
{ 4, [["trigger1" ]], {0}}
}
diff --git a/test/sql-tap/trigger1.test.lua b/test/sql-tap/trigger1.test.lua
index dab3281..167beea 100755
--- a/test/sql-tap/trigger1.test.lua
+++ b/test/sql-tap/trigger1.test.lua
@@ -43,7 +43,7 @@ test:do_catchsql_test(
END;
]], {
-- <trigger1-1.1.1>
- 1, "no such table: NO_SUCH_TABLE"
+ 1, "Space 'NO_SUCH_TABLE' does not exist"
-- </trigger1-1.1.1>
})
@@ -55,7 +55,7 @@ test:do_catchsql_test(
END;
]], {
-- <trigger1-1.1.2>
- 1, "no such table: NO_SUCH_TABLE"
+ 1, "Space 'NO_SUCH_TABLE' does not exist"
-- </trigger1-1.1.2>
})
@@ -101,7 +101,7 @@ test:do_catchsql_test(
END
]], {
-- <trigger1-1.2.1>
- 1, "trigger TR1 already exists"
+ 1, "Trigger 'TR1' already exists"
-- </trigger1-1.2.1>
})
@@ -113,7 +113,7 @@ test:do_catchsql_test(
END
]], {
-- <trigger1-1.2.2>
- 1, [[trigger TR1 already exists]]
+ 1, [[Trigger 'TR1' already exists]]
-- </trigger1-1.2.2>
})
@@ -251,7 +251,7 @@ test:do_catchsql_test(
end;
]], {
-- <trigger1-1.12>
- 1, "cannot create INSTEAD OF trigger on table: T1"
+ 1, "SQL error: cannot create INSTEAD OF trigger on table: T1"
-- </trigger1-1.12>
})
@@ -265,7 +265,7 @@ test:do_catchsql_test(
end;
]], {
-- <trigger1-1.13>
- 1, "cannot create BEFORE trigger on view: V1"
+ 1, "SQL error: cannot create BEFORE trigger on view: V1"
-- </trigger1-1.13>
})
@@ -280,7 +280,7 @@ test:do_catchsql_test(
end;
]], {
-- <trigger1-1.14>
- 1, "cannot create AFTER trigger on view: V1"
+ 1, "SQL error: cannot create AFTER trigger on view: V1"
-- </trigger1-1.14>
})
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index 911b1ce..b004270 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -116,3 +116,48 @@ box.sql.execute('DROP TABLE test')
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
---
...
+----
+---- gh-3273: Move SQL TRIGGERs into server.
+----
+box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER);");
+---
+...
+box.sql.execute("CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER);");
+---
+...
+box.error.injection.set("ERRINJ_WAL_IO", true)
+---
+- ok
+...
+box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+---
+- error: Failed to write to disk
+...
+box.sql.execute("CREATE INDEX t1a ON t1(a);")
+---
+- error: Failed to write to disk
+...
+box.error.injection.set("ERRINJ_WAL_IO", false)
+---
+- ok
+...
+box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+---
+...
+box.sql.execute("INSERT INTO t1 VALUES (3, 3);")
+---
+...
+box.sql.execute("SELECT * from t1");
+---
+- - [3, 3]
+...
+box.sql.execute("SELECT * from t2");
+---
+- - [1, 1]
+...
+box.sql.execute("DROP TABLE t1;")
+---
+...
+box.sql.execute("DROP TABLE t2;")
+---
+...
diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
index 63d3063..f559099 100644
--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -44,3 +44,19 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
box.sql.execute('DROP TABLE test')
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+
+----
+---- gh-3273: Move SQL TRIGGERs into server.
+----
+box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER);");
+box.sql.execute("CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER);");
+box.error.injection.set("ERRINJ_WAL_IO", true)
+box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+box.sql.execute("CREATE INDEX t1a ON t1(a);")
+box.error.injection.set("ERRINJ_WAL_IO", false)
+box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+box.sql.execute("INSERT INTO t1 VALUES (3, 3);")
+box.sql.execute("SELECT * from t1");
+box.sql.execute("SELECT * from t2");
+box.sql.execute("DROP TABLE t1;")
+box.sql.execute("DROP TABLE t2;")
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
new file mode 100644
index 0000000..090b546
--- /dev/null
+++ b/test/sql/triggers.result
@@ -0,0 +1,196 @@
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+-- get invariant part of the tuple
+ function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end
+---
+...
+--
+-- gh-3273: Move Triggers to server
+--
+box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
+---
+...
+box.sql.execute("CREATE TABLE t2(x INTEGER PRIMARY KEY);")
+---
+...
+box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- - - T1T
+ - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+ END; '}
+...
+space_id = box.space._space.index["name"]:get('T1').id
+---
+...
+assert(box.space.T1.id == space_id)
+---
+- true
+...
+-- checks for LUA tuples
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+box.space._trigger:insert(tuple)
+---
+- error: 'SQL error: trigger name does not match extracted from SQL'
+...
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+box.space._trigger:insert(tuple)
+---
+- error: 'SQL error: trigger name does not match extracted from SQL'
+...
+tuple = {"T2T", box.space.T1.id + 1, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+box.space._trigger:insert(tuple)
+---
+- error: 'SQL error: trigger space_id does not match the value resolved on AST building
+ from SQL'
+...
+immutable_part(box.space._trigger:select())
+---
+- - - T1T
+ - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+ END; '}
+...
+box.sql.execute("DROP TABLE T1;")
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- []
+...
+box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
+---
+...
+box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- - - T1T
+ - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+ END; '}
+...
+space_id = box.space._space.index["name"]:get('T1').id
+---
+...
+-- test, didn't trigger t1t degrade
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+---
+...
+box.sql.execute("SELECT * FROM t2;")
+---
+- - [1]
+...
+box.sql.execute("DELETE FROM t2;")
+---
+...
+-- test triggers
+tuple = {"T2T", space_id, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}}
+---
+...
+_ = box.space._trigger:insert(tuple)
+---
+...
+tuple = {"T3T", space_id, {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
+---
+...
+_ = box.space._trigger:insert(tuple)
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- - - T1T
+ - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+ END; '}
+ - - T2T
+ - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
+ END;'}
+ - - T3T
+ - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+ END;'}
+...
+box.sql.execute("INSERT INTO t1 VALUES(2);")
+---
+...
+box.sql.execute("SELECT * FROM t2;")
+---
+- - [1]
+ - [2]
+ - [3]
+...
+box.sql.execute("DELETE FROM t2;")
+---
+...
+-- test t1t after t2t and t3t drop
+box.sql.execute("DROP TRIGGER T2T;")
+---
+...
+_ = box.space._trigger:delete("T3T")
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- - - T1T
+ - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+ END; '}
+...
+box.sql.execute("INSERT INTO t1 VALUES(3);")
+---
+...
+box.sql.execute("SELECT * FROM t2;")
+---
+- - [1]
+...
+box.sql.execute("DELETE FROM t2;")
+---
+...
+-- insert new SQL t2t and t3t
+box.sql.execute([[CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END; ]])
+---
+...
+box.sql.execute([[CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END; ]])
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- - - T1T
+ - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+ END; '}
+ - - T2T
+ - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
+ END; '}
+ - - T3T
+ - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+ END; '}
+...
+box.sql.execute("INSERT INTO t1 VALUES(4);")
+---
+...
+box.sql.execute("SELECT * FROM t2;")
+---
+- - [1]
+ - [2]
+ - [3]
+...
+-- clean up
+box.sql.execute("DROP TABLE t1;")
+---
+...
+box.sql.execute("DROP TABLE t2;")
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- []
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
new file mode 100644
index 0000000..8e3f0c5
--- /dev/null
+++ b/test/sql/triggers.test.lua
@@ -0,0 +1,74 @@
+env = require('test_run')
+test_run = env.new()
+
+-- get invariant part of the tuple
+ function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end
+
+--
+-- gh-3273: Move Triggers to server
+--
+
+box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
+box.sql.execute("CREATE TABLE t2(x INTEGER PRIMARY KEY);")
+box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
+immutable_part(box.space._trigger:select())
+
+space_id = box.space._space.index["name"]:get('T1').id
+assert(box.space.T1.id == space_id)
+
+-- checks for LUA tuples
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+
+tuple = {"T2T", box.space.T1.id + 1, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+immutable_part(box.space._trigger:select())
+
+box.sql.execute("DROP TABLE T1;")
+immutable_part(box.space._trigger:select())
+
+box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
+box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
+immutable_part(box.space._trigger:select())
+
+space_id = box.space._space.index["name"]:get('T1').id
+
+-- test, didn't trigger t1t degrade
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+
+-- test triggers
+tuple = {"T2T", space_id, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}}
+_ = box.space._trigger:insert(tuple)
+tuple = {"T3T", space_id, {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
+_ = box.space._trigger:insert(tuple)
+immutable_part(box.space._trigger:select())
+box.sql.execute("INSERT INTO t1 VALUES(2);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+-- test t1t after t2t and t3t drop
+box.sql.execute("DROP TRIGGER T2T;")
+_ = box.space._trigger:delete("T3T")
+immutable_part(box.space._trigger:select())
+box.sql.execute("INSERT INTO t1 VALUES(3);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+-- insert new SQL t2t and t3t
+box.sql.execute([[CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END; ]])
+box.sql.execute([[CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END; ]])
+immutable_part(box.space._trigger:select())
+box.sql.execute("INSERT INTO t1 VALUES(4);")
+box.sql.execute("SELECT * FROM t2;")
+
+-- clean up
+box.sql.execute("DROP TABLE t1;")
+box.sql.execute("DROP TABLE t2;")
+immutable_part(box.space._trigger:select())
+
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 5/6] sql: move Triggers to server Kirill Shcherbatov
@ 2018-06-24 15:24 ` n.pettik
2018-06-24 20:41 ` Vladislav Shpilevoy
2018-06-25 15:27 ` Kirill Shcherbatov
0 siblings, 2 replies; 15+ messages in thread
From: n.pettik @ 2018-06-24 15:24 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Shcherbatov
> On 20 Jun 2018, at 13:46, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
>
> Introduced sql_triggers field in space structure.
> Changed parser logic to do not insert built triggers, just only
> to do parsing. All triggers insertions and deletions are operated
> via on_replace_dd_trigger now.
>
This patch in fact introduces a lot of changes. I would devote much more
informative commit message to describe them. I had to investigate
a lot of things which could be explained in comments/commit message.
For instance, why do we still have to hold triggers in a separate hash?
To invoke triggers on a particular space you can refer to space->triggers.
Deletion of trigger occurs at VDBE runtime, so you just need to make sure
that _trigger contains record with given name.
> /**
> @@ -3244,6 +3248,53 @@ lock_before_dd(struct trigger *trigger, void *event)
> }
>
> /**
> + * Trigger invoked on rollback in the _trigger space.
> + */
Could you write more informative comment which would include information
concerning each action, i.e. why we delete tuple on insertion etc.
E.g.:
/* INSERT trigger. */
int rc = sql_trigger_replace(sql_get(),
sql_trigger_name(old_trigger),
NULL, &new_trigger);
It seems to be misleading since comment says ‘INSERT trigger’,
but instead you delete trigger from hash.
I understand that you mean ‘on rollback of insertion we must delete
already inserted in hash trigger’, but lets make it more clear.
> +static void
> +on_replace_trigger_rollback(struct trigger *trigger, void *event)
> +{
> + struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
> + struct Trigger *old_trigger = (struct Trigger *)trigger->data;
It is quite terrible: now we have struct Trigger and struct trigger,
which quite similar in their names. What do you think about renaming
struct Trigger to struct sql_trigger?
> + struct Trigger *new_trigger;
> +
> + if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) {
> + /* DELETE trigger. */
> + if (sql_trigger_replace(sql_get(),
> + sql_trigger_name(old_trigger),
> + old_trigger, &new_trigger) != 0)
> + panic("Out of memory on insertion into trigger hash");
> + assert(new_trigger == NULL);
> + } else if (stmt->new_tuple != NULL && stmt->old_tuple == NULL) {
> + /* INSERT trigger. */
> + int rc = sql_trigger_replace(sql_get(),
> + sql_trigger_name(old_trigger),
> + NULL, &new_trigger);
> + (void)rc;
> + assert(rc == 0);
> + assert(new_trigger == old_trigger);
> + sql_trigger_delete(sql_get(), new_trigger);
> + } else {
> + /* REPLACE trigger. */
> + if (sql_trigger_replace(sql_get(),
> + sql_trigger_name(old_trigger),
> + old_trigger, &new_trigger) != 0)
> + panic("Out of memory on insertion into trigger hash");
> + assert(old_trigger != new_trigger);
> + sql_trigger_delete(sql_get(), new_trigger);
> + }
> +}
> +
>
> @@ -3252,6 +3303,88 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
> {
> + const char *space_opts =
> + tuple_field_with_type_xc(new_tuple,
> + BOX_TRIGGER_FIELD_OPTS,
> + MP_MAP);
Why do we still hold string of ‘CREATE TRIGGER’ statement as a map?
AFAIU trigger can’t feature any functional elements except for mentioned one.
> + struct space_opts opts;
> + struct region *region = &fiber()->gc;
> + space_opts_decode(&opts, space_opts, region);
> + struct Trigger *new_trigger =
> + sql_trigger_compile(sql_get(), opts.sql);
> + if (new_trigger == NULL)
> + diag_raise();
>
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 90bba1a..7dc3f80 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -95,6 +95,17 @@ struct Select *
> sql_view_compile(struct sqlite3 *db, const char *view_stmt);
>
> /**
> + * Perform parsing of provided SQL request and construct trigger AST.
> + * @param db SQL context handle.
> + * @param sql request to parse.
> + *
> + * @retval NULL on error
> + * @retval not NULL Trigger AST pointer on success.
> + */
> +struct Trigger *
> +sql_trigger_compile(struct sqlite3 *db, const char *sql);
> +
> +/**
> * Free AST pointed by trigger.
> * @param db SQL handle.
> * @param trigger AST object.
> @@ -103,6 +114,45 @@ void
> sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger);
>
> /**
> + * Get server triggers list by space_id.
I would mention that this function must take valid space id.
> + * @param space_id Space ID.
> + *
> + * @retval trigger AST list.
> + */
> +struct Trigger *
> +space_trigger_list(uint32_t space_id);
> +
>
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 122c283..e4c936d 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -33,6 +33,7 @@
> * This file contains the implementation for TRIGGERs
> */
>
> +#include "box/schema.h"
> #include "sqliteInt.h"
> #include "tarantoolInt.h"
> #include "vdbeInt.h"
> @@ -81,115 +82,141 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
> )
> {
I see that you have slightly refactored this function. I propose to make
an effort to completely refactor this function. Moreover, comment to this
function is no longer relevant:
>A Trigger
>* structure is generated based on the information available and stored
>* in pParse->pNewTrigger.
> + if (space_id == BOX_ID_NIL) {
> + diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
> + pParse->rc = SQL_TARANTOOL_ERROR;
> + pParse->nErr++;
You are repeating these two lines 6 times across the code:
> + pParse->rc = SQL_TARANTOOL_ERROR;
> + pParse->nErr++;
I suggest to add another one exit label, which would perform exactly
these actions.
> + goto trigger_cleanup;
> + }
> +
> + /* Do not create a trigger on a system table. */
> + if (sqlite3StrNICmp(table_name, "sqlite_", 7) == 0) {
I suggest to substitute this obsolete check with Tarantool’s one:
you can use space_is_system() function.
> + diag_set(ClientError, ER_SQL,
> + "cannot create trigger on system table");
> + pParse->rc = SQL_TARANTOOL_ERROR;
> + pParse->nErr++;
> goto trigger_cleanup;
> }
>
> - /* INSTEAD of triggers are only for views and views only support INSTEAD
> - * of triggers.
> + /*
> + * INSTEAD of triggers are only for views and
> + * views only support INSTEAD of triggers.
> */
> - if (pTab->def->opts.is_view && tr_tm != TK_INSTEAD) {
> - sqlite3ErrorMsg(pParse, "cannot create %s trigger on view: %S",
> - (tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER",
> - pTableName, 0);
Well, personally I would move these checks (i.e. testing that trigger created on VIEW
must be <INSTEAD OF>, and trigger created on table can’t be <INSTEAD OF>’) to
on_replace_dd_trigger. Otherwise, you may catch severe errors after creation of such
‘Inconsistent’ triggers from Lua.
> @@ -566,34 +559,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)
Lets rename this function and fix comment for it. I suggest to call it vdbe_code_drop_trigger(),
vdbe_emit_drop_trigger() or whatever meaningful name you prefer.
> sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
>
> sqlite3ChangeCookie(pParse);
> - sqlite3VdbeAddOp4(v, OP_DropTrigger, 0, 0, 0, pTrigger->zName,
> - 0);
> }
> }
>
> -/*
> - * Remove a trigger from the hash tables of the sqlite* pointer.
> - */
> -void
> -sqlite3UnlinkAndDeleteTrigger(sqlite3 * db, const char *zName)
> +int
> +sql_trigger_replace(struct sqlite3 *db, const char *name,
> + struct Trigger *trigger, struct Trigger **old_trigger)
I would give it a name like sql_trigger_hash_replace().
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 5b5cf83..c1df880 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4710,46 +4710,6 @@ case OP_ParseSchema2: {
> break;
> }
>
> /* Opcode: RenameTable P1 * * P4 *
> * Synopsis: P1 = root, P4 = name
> *
> @@ -4823,18 +4783,22 @@ case OP_RenameTable: {
> goto abort_due_to_error;
> }
>
> - pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName);
> - pTab->pTrigger = pTrig;
> -
> - /* Rename all trigger created on this table.*/
> - for (; pTrig; pTrig = pTrig->pNext) {
> - sqlite3DbFree(db, pTrig->table);
> - pTrig->table = sqlite3DbStrNDup(db, zNewTableName,
> - sqlite3Strlen30(zNewTableName));
> - pTrig->pTabSchema = pTab->pSchema;
> - rc = tarantoolSqlite3RenameTrigger(pTrig->zName,
> + /*
> + * Rebuild 'CREATE TRIGGER' expressions of all triggers
> + * created on this table. Sure, this action is not atomic
> + * due to lack of transactional DDL, but just do the best
> + * effort.
> + */
> + for (struct Trigger *trigger = triggers; trigger != NULL; ) {
> + /* Store pointer as trigger will be destructed. */
> + struct Trigger *next_trigger = trigger->pNext;
> + rc = tarantoolSqlite3RenameTrigger(trigger->zName,
> zOldTableName, zNewTableName);
> - if (rc) goto abort_due_to_error;
> + if (rc != SQLITE_OK) {
> + sqlite3ResetAllSchemasOfConnection(db);
Are you sure that in a case of error we must delete all triggers from hash?
> + goto abort_due_to_error;
> + }
> + trigger = next_trigger;
> }
> sqlite3DbFree(db, (void*)zOldTableName);
> sqlite3DbFree(db, (void*)zSqlStmt);
> @@ -4881,18 +4845,6 @@ case OP_DropIndex: {
> break;
> }
>
> -/* Opcode: DropTrigger P1 * * P4 *
> - *
> - * Remove the internal (in-memory) data structures that describe
> - * the trigger named P4 in database P1. This is called after a trigger
> - * is dropped from disk (using the Destroy opcode) in order to keep
> - * the internal representation of the
> - * schema consistent with what is on disk.
> - */
> -case OP_DropTrigger: {
> - sqlite3UnlinkAndDeleteTrigger(db, pOp->p4.z);
> - break;
> -}
> #ifndef SQLITE_OMIT_TRIGGER
I would also delete all mentions of SQLITE_OMIT_TRIGGER (within a separate patch),
since triggers are always enabled in our SQL.
> +...
> diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
> index 63d3063..f559099 100644
> --- a/test/sql/errinj.test.lua
> +++ b/test/sql/errinj.test.lua
> @@ -44,3 +44,19 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
>
> box.sql.execute('DROP TABLE test')
> box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> +
> +----
> +---- gh-3273: Move SQL TRIGGERs into server.
> +----
> +box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER);");
> +box.sql.execute("CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER);");
> +box.error.injection.set("ERRINJ_WAL_IO", true)
> +box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
> +box.sql.execute("CREATE INDEX t1a ON t1(a);")
> +box.error.injection.set("ERRINJ_WAL_IO", false)
I would also add the same tests with failed drop of trigger.
> diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
> new file mode 100644
> index 0000000..8e3f0c5
> --- /dev/null
> +++ b/test/sql/triggers.test.lua
> @@ -0,0 +1,74 @@
> +env = require('test_run')
> +test_run = env.new()
> +
> +-- get invariant part of the tuple
Lets start comments with capital letter and end with dots.
Also, it doesn’t seem to be obvious what you mean by ‘invariant’.
> + function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end
> +
> +--
> +-- gh-3273: Move Triggers to server
> +--
> +
> +box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
> +box.sql.execute("CREATE TABLE t2(x INTEGER PRIMARY KEY);")
> +box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
> +immutable_part(box.space._trigger:select())
> +
> +space_id = box.space._space.index["name"]:get('T1').id
> +assert(box.space.T1.id == space_id)
How could they mismatch? I guess there are a lot of tests which
check such things.
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server
2018-06-24 15:24 ` [tarantool-patches] " n.pettik
@ 2018-06-24 20:41 ` Vladislav Shpilevoy
2018-06-25 15:27 ` Kirill Shcherbatov
1 sibling, 0 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-24 20:41 UTC (permalink / raw)
To: tarantool-patches; +Cc: Nikita Pettik, Kirill Shcherbatov
>> @@ -3252,6 +3303,88 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
>> {
>> + const char *space_opts =
>> + tuple_field_with_type_xc(new_tuple,
>> + BOX_TRIGGER_FIELD_OPTS,
>> + MP_MAP);
>
> Why do we still hold string of ‘CREATE TRIGGER’ statement as a map?
> AFAIU trigger can’t feature any functional elements except for mentioned one.
In SQL - can not, but when we will persist Lua trigger, the map contains: body,
after/before/instead, delete/insert/replace etc. So lets leave it be map.
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server
2018-06-24 15:24 ` [tarantool-patches] " n.pettik
2018-06-24 20:41 ` Vladislav Shpilevoy
@ 2018-06-25 15:27 ` Kirill Shcherbatov
2018-06-26 14:05 ` n.pettik
1 sibling, 1 reply; 15+ messages in thread
From: Kirill Shcherbatov @ 2018-06-25 15:27 UTC (permalink / raw)
To: tarantool-patches; +Cc: n.pettik
> For instance, why do we still have to hold triggers in a separate hash?
> To invoke triggers on a particular space you can refer to space->triggers.
> Deletion of trigger occurs at VDBE runtime, so you just need to make sure
> that _trigger contains record with given name.
Introduced sql_triggers field in space structure.
Changed parser logic to do not insert built triggers, just only
to do parsing. All triggers insertions and deletions are operated
via on_replace_dd_trigger now.
Global DB hash has been kept as we still ned to get Trigger AST
by name on trigger deletion.
> Could you write more informative comment which would include information
> concerning each action, i.e. why we delete tuple on insertion etc.
/**
* Trigger invoked on rollback in the _trigger space.
+ * Insert old sql_trigger AST stored in trigger->data,
+ * destruct new object in any.
*/
static void
on_replace_trigger_rollback(struct trigger *trigger, void *event)
@@ -3286,6 +3288,7 @@ on_replace_trigger_rollback(struct trigger *trigger, void *event)
/**
* Trigger invoked on commit in the _trigger space.
+ * Drop useless old sql_trigger AST object if any.
*/
> E.g.:
>
> /* INSERT trigger. */
> int rc = sql_trigger_replace(sql_get(),
> sql_trigger_name(old_trigger),
> NULL, &new_trigger);
>
> It seems to be misleading since comment says ‘INSERT trigger’,
> but instead you delete trigger from hash.
> I understand that you mean ‘on rollback of insertion we must delete
> already inserted in hash trigger’, but lets make it more clear.
I've changed comment to contain "Rollback" word;
/* Rollback DELETE trigger. */
/* Rollback INSERT trigger. */
/* Rollback REPLACE trigger. */
>
>> +static void
>> +on_replace_trigger_rollback(struct trigger *trigger, void *event)
>> +{
>> + struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
>> + struct Trigger *old_trigger = (struct Trigger *)trigger->data;
>
> It is quite terrible: now we have struct Trigger and struct trigger,
> which quite similar in their names. What do you think about renaming
> struct Trigger to struct sql_trigger?
Ok, done code refactoring in separate commit.
> Why do we still hold string of ‘CREATE TRIGGER’ statement as a map?
> AFAIU trigger can’t feature any functional elements except for mentioned one.
Vlad already answered this in this thread question.
> I would mention that this function must take valid space id.
- * @param space_id Space ID.
+ * @param space_id valid Space ID.
> I see that you have slightly refactored this function. I propose to make
> an effort to completely refactor this function. Moreover, comment to this
> function is no longer relevant:
Ok, as a part on separate refactoring commit.
> You are repeating these two lines 6 times across the code:
>
> I suggest to add another one exit label, which would perform exactly
> these actions.
+set_tarantool_error_and_cleanup:
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
+ goto trigger_cleanup;
and so on.
>
>> + goto trigger_cleanup;
>> + }
>> +
>> + /* Do not create a trigger on a system table. */
>> + if (sqlite3StrNICmp(table_name, "sqlite_", 7) == 0) {
>
> I suggest to substitute this obsolete check with Tarantool’s one:
> you can use space_is_system() function.
- if (sqlite3StrNICmp(table_name, "sqlite_", 7) == 0) {
+ if (space_is_system(space_id)) {
> Well, personally I would move these checks (i.e. testing that trigger created on VIEW
> must be <INSTEAD OF>, and trigger created on table can’t be <INSTEAD OF>’) to
> on_replace_dd_trigger. Otherwise, you may catch severe errors after creation of such
> ‘Inconsistent’ triggers from Lua.
Good idea,
moved to sql_trigger_replace.
I've also added some tests:
+++ b/test/sql/triggers.test.lua
@@ -71,3 +71,37 @@ box.sql.execute("DROP TABLE t1;")
box.sql.execute("DROP TABLE t2;")
immutable_part(box.space._trigger:select())
+-- Test target tables restricts.
+box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY,b);")
+space_id = box.space.T1.id
+
+tuple = {"T1T", space_id, {sql = [[
+create trigger t1t instead of update on t1 for each row begin
+ delete from t1 WHERE a=old.a+2;
+end;]]}}
+box.space._trigger:insert(tuple)
+
+box.sql.execute("CREATE VIEW V1 AS SELECT * FROM t1;")
+space_id = box.space.V1.id
+
+tuple = {"V1T", space_id, {sql = [[
+create trigger v1t before update on v1 for each row begin
+ delete from t1 WHERE a=old.a+2;
+end;]]}}
+box.space._trigger:insert(tuple)
+
+tuple = {"V1T", space_id, {sql = [[
+create trigger v1t AFTER update on v1 for each row begin
+ delete from t1 WHERE a=old.a+2;
+end;]]}}
+box.space._trigger:insert(tuple)
+
+space_id = box.space._sql_stat1.id
+tuple = {"T1T", space_id, {sql = [[
+create trigger t1t instead of update on "_sql_stat1" for each row begin
+ delete from t1 WHERE a=old.a+2;
+end;]]}}
+box.space._trigger:insert(tuple)
+
+box.sql.execute("DROP VIEW V1;")
+box.sql.execute("DROP TABLE T1;")
>
>> @@ -566,34 +559,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)
>
> Lets rename this function and fix comment for it. I suggest to call it vdbe_code_drop_trigger(),
> vdbe_emit_drop_trigger() or whatever meaningful name you prefer.
Ok, as a part of next refactoring commit.
> I would give it a name like sql_trigger_hash_replace().
I can't agree. This function not only updates the cache, but also insert AST into space trigger list.
> Are you sure that in a case of error we must delete all triggers from hash?Hm, can't remember, how I've wrote this code.
Yes, a non-trivial place.
Like this:
if (rc != SQLITE_OK) {
sqlite3CommitInternalChanges();
goto abort_due_to_error;
}
> I would also delete all mentions of SQLITE_OMIT_TRIGGER (within a separate patch),
> since triggers are always enabled in our SQL.
Ok, as a part of next refactoring commit.
>
>> +...
>> diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
>> index 63d3063..f559099 100644
>> --- a/test/sql/errinj.test.lua
>> +++ b/test/sql/errinj.test.lua
>> @@ -44,3 +44,19 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
>>
>> box.sql.execute('DROP TABLE test')
>> box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>> +
>> +----
>> +---- gh-3273: Move SQL TRIGGERs into server.
>> +----
>> +box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER);");
>> +box.sql.execute("CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER);");
>> +box.error.injection.set("ERRINJ_WAL_IO", true)
>> +box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
>> +box.sql.execute("CREATE INDEX t1a ON t1(a);")
>> +box.error.injection.set("ERRINJ_WAL_IO", false)
>
> I would also add the same tests with failed drop of trigger.
Good idea,
+box.error.injection.set("ERRINJ_WAL_IO", true)
+box.sql.execute("DROP TRIGGER t1t;")
+box.error.injection.set("ERRINJ_WAL_IO", false)
+box.sql.execute("DELETE FROM t1;")
+box.sql.execute("DELETE FROM t2;")
+box.sql.execute("INSERT INTO t1 VALUES (3, 3);")
+box.sql.execute("SELECT * from t1");
+box.sql.execute("SELECT * from t2");
>
>> diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
>> new file mode 100644
>> index 0000000..8e3f0c5
>> --- /dev/null
>> +++ b/test/sql/triggers.test.lua
>> @@ -0,0 +1,74 @@
>> +env = require('test_run')
>> +test_run = env.new()
>> +
>> +-- get invariant part of the tuple
>
> Lets start comments with capital letter and end with dots.
+-- Test triggers.
.. so on.
> Also, it doesn’t seem to be obvious what you mean by ‘invariant’.
--- get invariant part of the tuple
+-- Get invariant part of the tuple; name and opts don't change.
> How could they mismatch? I guess there are a lot of tests which
> check such things.
-assert(box.space.T1.id == space_id)
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server
2018-06-25 15:27 ` Kirill Shcherbatov
@ 2018-06-26 14:05 ` n.pettik
2018-06-26 16:13 ` Kirill Shcherbatov
0 siblings, 1 reply; 15+ messages in thread
From: n.pettik @ 2018-06-26 14:05 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Shcherbatov
> On 25 Jun 2018, at 18:27, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
>
>> For instance, why do we still have to hold triggers in a separate hash?
>> To invoke triggers on a particular space you can refer to space->triggers.
>> Deletion of trigger occurs at VDBE runtime, so you just need to make sure
>> that _trigger contains record with given name.
> Introduced sql_triggers field in space structure.
> Changed parser logic to do not insert built triggers, just only
> to do parsing. All triggers insertions and deletions are operated
> via on_replace_dd_trigger now.
> Global DB hash has been kept as we still ned to get Trigger AST
> by name on trigger deletion.
Now it is possible to delete trigger using only its name, since
in on_replace_dd_trigger() we would have space_id from tuple.
So, we came to agreement to delete global hash of triggers.
>
>> Could you write more informative comment which would include information
>> concerning each action, i.e. why we delete tuple on insertion etc.
> /**
> * Trigger invoked on rollback in the _trigger space.
> + * Insert old sql_trigger AST stored in trigger->data,
> + * destruct new object in any.
*Since rollback trigger is invoked after insertion to hash and space,
we have to delete it from those structures and release memory.
Vice versa, after deletion of trigger we must return it back to hash and space*.
>> Well, personally I would move these checks (i.e. testing that trigger created on VIEW
>> must be <INSTEAD OF>, and trigger created on table can’t be <INSTEAD OF>’) to
>> on_replace_dd_trigger. Otherwise, you may catch severe errors after creation of such
>> ‘Inconsistent’ triggers from Lua.
> Good idea,
> moved to sql_trigger_replace.
It would be great if you provided diff: right here, at the end of letter or
as a next patch version. Don’t forget next time.
>>
>>> @@ -566,34 +559,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)
>>
>> Lets rename this function and fix comment for it. I suggest to call it vdbe_code_drop_trigger(),
>> vdbe_emit_drop_trigger() or whatever meaningful name you prefer.
> Ok, as a part of next refactoring commit.
Please, sent that patch or attach diff.
>
>> I would give it a name like sql_trigger_hash_replace().
> I can't agree. This function not only updates the cache, but also insert AST into space trigger list.
Ok, as you wish.
>
>> Are you sure that in a case of error we must delete all triggers from hash?Hm, can't remember, how I've wrote this code.
> Yes, a non-trivial place.
> Like this:
Add cautionary (or FIXME) comment which would explain that
in case of RENAME’s fail we are totally fucked up: part of triggers
remain with old space’s name. Thus, after creation of new space with
the name of old one, it appears with triggers which haven’t been created.
>
> if (rc != SQLITE_OK) {
> sqlite3CommitInternalChanges();
> goto abort_due_to_error;
> }
>
>
>> I would also delete all mentions of SQLITE_OMIT_TRIGGER (within a separate patch),
>> since triggers are always enabled in our SQL.
> Ok, as a part of next refactoring commit.
Please, sent that patch or attach diff.
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server
2018-06-26 14:05 ` n.pettik
@ 2018-06-26 16:13 ` Kirill Shcherbatov
0 siblings, 0 replies; 15+ messages in thread
From: Kirill Shcherbatov @ 2018-06-26 16:13 UTC (permalink / raw)
To: tarantool-patches; +Cc: n.pettik
> Now it is possible to delete trigger using only its name, since
> in on_replace_dd_trigger() we would have space_id from tuple.
> So, we came to agreement to delete global hash of triggers.
I'll send a patch.
> It would be great if you provided diff: right here, at the end of letter or
> as a next patch version. Don’t forget next time.
Ok, I'll try do not forget next time.
> Please, sent that patch or attach diff.
I'll send a patch.
> Add cautionary (or FIXME) comment which would explain that
> in case of RENAME’s fail we are totally fucked up: part of triggers
> remain with old space’s name. Thus, after creation of new space with
> the name of old one, it appears with triggers which haven’t been created.
+ /*
+ * FIXME: In the case of error,
+ * part of triggers would have invalid
+ * space name in tuple so can not been
+ * persisted.
+ * Server could be restarted.
+ * In this case, rename table back and
+ * try again.
+ */
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] [PATCH v4 6/6] sql: VDBE tests for trigger existence
2018-06-20 10:46 [tarantool-patches] [PATCH v4 0/6] sql: remove Triggers to server Kirill Shcherbatov
` (4 preceding siblings ...)
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 5/6] sql: move Triggers to server Kirill Shcherbatov
@ 2018-06-20 10:46 ` Kirill Shcherbatov
2018-06-24 15:25 ` [tarantool-patches] " n.pettik
5 siblings, 1 reply; 15+ messages in thread
From: Kirill Shcherbatov @ 2018-06-20 10:46 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
Trigger presence in system should be tested on each VDBE
execution attempt, not on Parser iteration.
Part of #3435, #3273
---
src/box/sql/build.c | 42 ++++++++++++++++++++++++++++++++++++++++++
src/box/sql/main.c | 11 ++++++++---
src/box/sql/sqliteInt.h | 24 ++++++++++++++++++++++++
src/box/sql/trigger.c | 25 +++++++++++++------------
src/box/sql/vdbe.c | 18 +++++++++++-------
test/sql-tap/view.test.lua | 8 ++++----
test/sql/view.result | 4 ++--
7 files changed, 104 insertions(+), 28 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index c8bfad7..fff7c19 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3971,4 +3971,46 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
sqlite3DbFree(db, pWith);
}
}
+
+int
+vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+ int index_id, const char *name_src,
+ int tarantool_error_code,
+ const char *error_src, bool no_error)
+{
+ struct Vdbe *v = sqlite3GetVdbe(parser);
+ assert(v != NULL);
+
+ struct sqlite3 *db = parser->db;
+ char *name = sqlite3DbStrDup(db, name_src);
+ if (name == NULL) {
+ size_t size = strlen(name_src) + 1;
+ diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name");
+ return -1;
+ }
+ char *error = sqlite3DbStrDup(db, error_src);
+ if (error == NULL) {
+ sqlite3DbFree(db, name);
+ size_t size = strlen(error_src) + 1;
+ diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error");
+ return -1;
+ }
+
+ int cursor = parser->nTab++;
+ vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id));
+
+ int name_reg = parser->nMem++;
+ int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
+ P4_DYNAMIC);
+ sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
+ if (no_error) {
+ sqlite3VdbeAddOp0(v, OP_Halt);
+ } else {
+ sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
+ ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
+ sqlite3VdbeChangeP5(v, tarantool_error_code);
+ }
+ sqlite3VdbeAddOp1(v, OP_Close, cursor);
+ return 0;
+}
#endif /* !defined(SQLITE_OMIT_CTE) */
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 0acf7bc..e435c01 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1454,11 +1454,16 @@ sqlite3_errmsg(sqlite3 * db)
z = sqlite3ErrStr(SQLITE_NOMEM_BKPT);
} else {
testcase(db->pErr == 0);
- z = (char *)sqlite3_value_text(db->pErr);
assert(!db->mallocFailed);
- if (z == 0) {
- z = sqlite3ErrStr(db->errCode);
+ if (db->errCode != SQL_TARANTOOL_ERROR) {
+ assert(!db->mallocFailed);
+ z = (char *)sqlite3_value_text(db->pErr);
+ if (z == NULL)
+ z = sqlite3ErrStr(db->errCode);
+ } else {
+ z = diag_last_error(diag_get())->errmsg;
}
+ assert(z != NULL);
}
return z;
}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 72803fa..acda23d 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4627,4 +4627,28 @@ extern int sqlite3InitDatabase(sqlite3 * db);
enum on_conflict_action
table_column_nullable_action(struct Table *tab, uint32_t column);
+/**
+ * Generate VDBE code to halt execution with correct error if
+ * the object with specified name is already present in
+ * specified space.
+ * The function does not begin to hold the passed error pointer
+ * to memory.
+ *
+ * @param parser Parsing context.
+ * @param space_id Space to lookup identifier.
+ * @param index_id Index identifier containing string primary key.
+ * @param name_src Of object to test existence.
+ * @param tarantool_error_code to set on halt.
+ * @param error_src string to notify on halt.
+ * @param no_error Do not raise error flag.
+ *
+ * @retval -1 on memory allocation error.
+ * @retval 0 on success.
+ */
+int
+vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+ int index_id, const char *name_src,
+ int tarantool_error_code,
+ const char *error_src, bool no_error);
+
#endif /* SQLITEINT_H */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index e4c936d..8ccb646 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -122,18 +122,6 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
goto trigger_cleanup;
- if (!pParse->parse_only &&
- sqlite3HashFind(&db->pSchema->trigHash, zName) != NULL) {
- if (!noErr) {
- diag_set(ClientError, ER_TRIGGER_EXISTS, zName);
- pParse->rc = SQL_TARANTOOL_ERROR;
- pParse->nErr++;
- } else {
- assert(!db->init.busy);
- }
- goto trigger_cleanup;
- }
-
const char *table_name = pTableName->a[0].zName;
uint32_t space_id;
if (schema_find_id(BOX_SPACE_ID, 2, table_name, strlen(table_name),
@@ -179,6 +167,19 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
pParse->nErr++;
goto trigger_cleanup;
}
+ if (!pParse->parse_only) {
+ const char *error_msg =
+ tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS), zName);
+ if (vdbe_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID,
+ 0, zName,
+ ER_TRIGGER_EXISTS,
+ error_msg,
+ (noErr != 0)) != 0) {
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
+ goto trigger_cleanup;
+ }
+ }
/*
* INSTEAD OF triggers can only appear on views and BEFORE triggers
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c1df880..487b026 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -960,7 +960,9 @@ case OP_HaltIfNull: { /* in3 */
*
* If P4 is not null then it is an error message string.
*
- * P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
+ * If P1 is SQL_TARANTOOL_ERROR then P5 is a ClientError code and
+ * P4 is error message to set. Else P5 is a value between 0 and 4,
+ * inclusive, that modifies the P4 string.
*
* 0: (no change)
* 1: NOT NULL contraint failed: P4
@@ -968,8 +970,8 @@ case OP_HaltIfNull: { /* in3 */
* 3: CHECK constraint failed: P4
* 4: FOREIGN KEY constraint failed: P4
*
- * If P5 is not zero and P4 is NULL, then everything after the ":" is
- * omitted.
+ * If P5 is not zero and P4 is NULL, then everything after the
+ * ":" is omitted.
*
* There is an implied "Halt 0 0 0" instruction inserted at the very end of
* every program. So a jump past the last instruction of the program
@@ -1005,9 +1007,11 @@ case OP_Halt: {
p->rc = pOp->p1;
p->errorAction = (u8)pOp->p2;
p->pc = pcx;
- assert(pOp->p5<=4);
if (p->rc) {
- if (pOp->p5) {
+ if (p->rc == SQL_TARANTOOL_ERROR) {
+ assert(pOp->p4.z != NULL);
+ box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
+ } else if (pOp->p5 != 0) {
static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
"FOREIGN KEY" };
testcase( pOp->p5==1);
@@ -2999,8 +3003,8 @@ case OP_CheckViewReferences: {
struct space *space = space_by_id(space_id);
assert(space != NULL);
if (space->def->view_ref_count > 0) {
- sqlite3VdbeError(p,"Can't drop table %s: other views depend "
- "on this space", space->def->name);
+ diag_set(ClientError, ER_DROP_SPACE, space->def->name,
+ "other views depend on this space");
rc = SQL_TARANTOOL_ERROR;
goto abort_due_to_error;
}
diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
index 5dc9503..ac1a27d 100755
--- a/test/sql-tap/view.test.lua
+++ b/test/sql-tap/view.test.lua
@@ -127,7 +127,7 @@ test:do_catchsql_test(
DROP TABLE t1;
]], {
-- <view-1.6>
- 1, "Can't drop table T1: other views depend on this space"
+ 1, "Can't drop space 'T1': other views depend on this space"
-- </view-1.6>
})
@@ -1185,7 +1185,7 @@ test:do_catchsql_test(
DROP TABLE t11;
]], {
-- <view-23.2>
- 1, "Can't drop table T11: other views depend on this space"
+ 1, "Can't drop space 'T11': other views depend on this space"
-- </view-23.2>
})
@@ -1195,7 +1195,7 @@ test:do_catchsql_test(
DROP TABLE t12;
]], {
-- <view-23.3>
- 1, "Can't drop table T12: other views depend on this space"
+ 1, "Can't drop space 'T12': other views depend on this space"
-- </view-23.3>
})
@@ -1205,7 +1205,7 @@ test:do_catchsql_test(
DROP TABLE t13;
]], {
-- <view-23.4>
- 1, "Can't drop table T13: other views depend on this space"
+ 1, "Can't drop space 'T13': other views depend on this space"
-- </view-23.4>
})
diff --git a/test/sql/view.result b/test/sql/view.result
index 0b9dc55..b033f19 100644
--- a/test/sql/view.result
+++ b/test/sql/view.result
@@ -124,7 +124,7 @@ box.sql.execute("CREATE VIEW v2 AS SELECT * FROM t2;");
...
box.sql.execute("DROP TABLE t2;");
---
-- error: 'Can''t drop table T2: other views depend on this space'
+- error: 'Can''t drop space ''T2'': other views depend on this space'
...
sp = box.space._space:get{box.space.T2.id};
---
@@ -134,7 +134,7 @@ sp = box.space._space:replace(sp);
...
box.sql.execute("DROP TABLE t2;");
---
-- error: 'Can''t drop table T2: other views depend on this space'
+- error: 'Can''t drop space ''T2'': other views depend on this space'
...
box.sql.execute("DROP VIEW v2;");
---
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] Re: [PATCH v4 6/6] sql: VDBE tests for trigger existence
2018-06-20 10:46 ` [tarantool-patches] [PATCH v4 6/6] sql: VDBE tests for trigger existence Kirill Shcherbatov
@ 2018-06-24 15:25 ` n.pettik
2018-06-25 15:27 ` Kirill Shcherbatov
0 siblings, 1 reply; 15+ messages in thread
From: n.pettik @ 2018-06-24 15:25 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Shcherbatov
> On 20 Jun 2018, at 13:46, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
>
> Trigger presence in system should be
Why? If you didn’t check it, you simply would get ‘duplicate key error’.
So, you did it to display more informative error..?
> tested on each VDBE
> execution attempt, not on Parser iteration.
And? After this sentence you should explain changes which you patch provides.
>
> Part of #3435, #3273
> ---
> src/box/sql/build.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> src/box/sql/main.c | 11 ++++++++---
> src/box/sql/sqliteInt.h | 24 ++++++++++++++++++++++++
> src/box/sql/trigger.c | 25 +++++++++++++------------
> src/box/sql/vdbe.c | 18 +++++++++++-------
> test/sql-tap/view.test.lua | 8 ++++----
> test/sql/view.result | 4 ++--
> 7 files changed, 104 insertions(+), 28 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index c8bfad7..fff7c19 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3971,4 +3971,46 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
> sqlite3DbFree(db, pWith);
> }
> }
> +
> +int
> +vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
I would call it vdbe_emit_halt_if_exists().
Or vdbe_emit_halt_on_duplication()...
> + int index_id, const char *name_src,
> + int tarantool_error_code,
> + const char *error_src, bool no_error)
> +{
> + struct Vdbe *v = sqlite3GetVdbe(parser);
> + assert(v != NULL);
> +
> + struct sqlite3 *db = parser->db;
> + char *name = sqlite3DbStrDup(db, name_src);
> + if (name == NULL) {
> + size_t size = strlen(name_src) + 1;
> + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name”);
In previous patch, you used sqlite3DbMalloc() function without using diag_set(),
since in case of fail it would call sqlite3OomFault(db);
Lets handle all usages of sqlite3DbMalloc() in the same way.
> + return -1;
> + }
> + char *error = sqlite3DbStrDup(db, error_src);
> + if (error == NULL) {
> + sqlite3DbFree(db, name);
> + size_t size = strlen(error_src) + 1;
> + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error”);
The same is here.
> + return -1;
> + }
> +
> + int cursor = parser->nTab++;
> + vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id));
> +
> + int name_reg = parser->nMem++;
> + int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
> + P4_DYNAMIC);
> + sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
> + if (no_error) {
> + sqlite3VdbeAddOp0(v, OP_Halt);
> + } else {
> + sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
> + ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
Why do you set ON_CONFLICT_ACTION_FAIL? You can just skip this arg with 0.
> + sqlite3VdbeChangeP5(v, tarantool_error_code);
> + }
You may refactor code this way (if you like it):
+++ b/src/box/sql/build.c
@@ -4003,11 +4003,10 @@ vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
P4_DYNAMIC);
sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
- if (no_error) {
- sqlite3VdbeAddOp0(v, OP_Halt);
- } else {
- sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
- ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
+ int op_halt_addr = sqlite3VdbeAddOp0(v, OP_Halt);
+ if (!no_error) {
+ sqlite3VdbeChangeP1(v, op_halt_addr, SQL_TARANTOOL_ERROR);
+ sqlite3VdbeChangeP4(v, op_halt_addr, error, P4_DYNAMIC);
sqlite3VdbeChangeP5(v, tarantool_error_code);
}
> + sqlite3VdbeAddOp1(v, OP_Close, cursor);
> + return 0;
> +}
> #endif /* !defined(SQLITE_OMIT_CTE) */
Why this func is under SQLITE_OMIT_CTE guard?
> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
> index 0acf7bc..e435c01 100644
> --- a/src/box/sql/main.c
> +++ b/src/box/sql/main.c
> @@ -1454,11 +1454,16 @@ sqlite3_errmsg(sqlite3 * db)
> z = sqlite3ErrStr(SQLITE_NOMEM_BKPT);
> } else {
> testcase(db->pErr == 0);
> - z = (char *)sqlite3_value_text(db->pErr);
> assert(!db->mallocFailed);
> - if (z == 0) {
> - z = sqlite3ErrStr(db->errCode);
> + if (db->errCode != SQL_TARANTOOL_ERROR) {
> + assert(!db->mallocFailed);
Why have you duplicated this assert? It occurs 2 lines above.
> + z = (char *)sqlite3_value_text(db->pErr);
> + if (z == NULL)
> + z = sqlite3ErrStr(db->errCode);
> + } else {
> + z = diag_last_error(diag_get())->errmsg;
> }
> + assert(z != NULL);
> }
> return z;
> }
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 72803fa..acda23d 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -4627,4 +4627,28 @@ extern int sqlite3InitDatabase(sqlite3 * db);
> enum on_conflict_action
> table_column_nullable_action(struct Table *tab, uint32_t column);
>
> +/**
> + * Generate VDBE code to halt execution with correct error if
> + * the object with specified name is already present in
> + * specified space.
> + * The function does not begin to hold the passed error pointer
> + * to memory.
Rephrase last sentence - I can’t parse it..
> + *
> + * @param parser Parsing context.
> + * @param space_id Space to lookup identifier.
> + * @param index_id Index identifier containing string primary key.
How can index id contain ’string primary key’?
> + * @param name_src Of object to test existence.
Name of object to test on existence.
> + * @param tarantool_error_code to set on halt.
> + * @param error_src string to notify on halt.
Error message to display on VDBE halt.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index c1df880..487b026 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -960,7 +960,9 @@ case OP_HaltIfNull: { /* in3 */
> *
> * If P4 is not null then it is an error message string.
> *
> - * P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
> + * If P1 is SQL_TARANTOOL_ERROR then P5 is a ClientError code and
> + * P4 is error message to set. Else P5 is a value between 0 and 4,
> + * inclusive, that modifies the P4 string.
> *
> * 0: (no change)
> * 1: NOT NULL contraint failed: P4
> @@ -968,8 +970,8 @@ case OP_HaltIfNull: { /* in3 */
> * 3: CHECK constraint failed: P4
> * 4: FOREIGN KEY constraint failed: P4
> *
> - * If P5 is not zero and P4 is NULL, then everything after the ":" is
> - * omitted.
> + * If P5 is not zero and P4 is NULL, then everything after the
> + * ":" is omitted.
Looks like redundant diff. If you wanted to make comments fit into 66 chars,
you would better do it for the whole comment.
> *
> * There is an implied "Halt 0 0 0" instruction inserted at the very end of
> * every program. So a jump past the last instruction of the program
> @@ -1005,9 +1007,11 @@ case OP_Halt: {
> p->rc = pOp->p1;
> p->errorAction = (u8)pOp->p2;
> p->pc = pcx;
> - assert(pOp->p5<=4);
> if (p->rc) {
> - if (pOp->p5) {
> + if (p->rc == SQL_TARANTOOL_ERROR) {
> + assert(pOp->p4.z != NULL);
> + box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
> + } else if (pOp->p5 != 0) {
> static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
> "FOREIGN KEY" };
> testcase( pOp->p5==1);
> @@ -2999,8 +3003,8 @@ case OP_CheckViewReferences: {
> struct space *space = space_by_id(space_id);
> assert(space != NULL);
> if (space->def->view_ref_count > 0) {
> - sqlite3VdbeError(p,"Can't drop table %s: other views depend "
> - "on this space", space->def->name);
> + diag_set(ClientError, ER_DROP_SPACE, space->def->name,
> + "other views depend on this space”);
Why did you provide this diff? AFAIK in this particular case these two calls
are *almost* equivalent.
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] Re: [PATCH v4 6/6] sql: VDBE tests for trigger existence
2018-06-24 15:25 ` [tarantool-patches] " n.pettik
@ 2018-06-25 15:27 ` Kirill Shcherbatov
2018-06-26 14:49 ` n.pettik
0 siblings, 1 reply; 15+ messages in thread
From: Kirill Shcherbatov @ 2018-06-25 15:27 UTC (permalink / raw)
To: tarantool-patches; +Cc: n.pettik
> Why? If you didn’t check it, you simply would get ‘duplicate key error’.
> So, you did it to display more informative error..?
>
>> tested on each VDBE
>> execution attempt, not on Parser iteration.
>
> And? After this sentence you should explain changes which you patch provides.
Trigger presence in system should be tested on each VDBE
execution attempt, not on Parser iteration as system state
could be changed between opcode run.
With this patch, such checks are the part of VDBE program
looking to the _trigger space and raise error message if
tuple with specified key already exists.
> I would call it vdbe_emit_halt_if_exists().
> Or vdbe_emit_halt_on_duplication()...
+vdbe_emit_halt_if_exists(struct Parse *parser, int space_id, int index_id,
> In previous patch, you used sqlite3DbMalloc() function without using diag_set(),
> since in case of fail it would call sqlite3OomFault(db);
> Lets handle all usages of sqlite3DbMalloc() in the same way.
Ok.
> The same is here.
Ok.
> Why do you set ON_CONFLICT_ACTION_FAIL? You can just skip this arg with 0.
Ok.
> Why this func is under SQLITE_OMIT_CTE guard?
Moved.
> Why have you duplicated this assert? It occurs 2 lines above.
Fixed.
> Rephrase last sentence - I can’t parse it..
+ * The function allocates error and name resources for VDBE itself.
> How can index id contain ’string primary key’?
> Name of object to test on existence.
> Error message to display on VDBE halt.
Ok.
> Looks like redundant diff. If you wanted to make comments fit into 66 chars,
> you would better do it for the whole comment.
Refactored while comment.
> Why did you provide this diff? AFAIK in this particular case these two calls
> are *almost* equivalent.
No, you set SQL_TARANTOOL_ERROR here that require diag error been set with my patch.
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] Re: [PATCH v4 6/6] sql: VDBE tests for trigger existence
2018-06-25 15:27 ` Kirill Shcherbatov
@ 2018-06-26 14:49 ` n.pettik
0 siblings, 0 replies; 15+ messages in thread
From: n.pettik @ 2018-06-26 14:49 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Shcherbatov
I have noticed, that in DEBUG mode there are a lot of compile errors:
/Users/n.pettik/tarantool/src/box/sql/trigger.c:102:6: error: variable 'trigger_name' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
if (sqlite3FixSrcList(&fixdb, table) != 0)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:156:20: note: uninitialized use occurs here
sqlite3DbFree(db, trigger_name);
^~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:102:2: note: remove the 'if' if its condition is always false
if (sqlite3FixSrcList(&fixdb, table) != 0)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:98:6: error: variable 'trigger_name' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
if (db->mallocFailed)
^~~~~~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:156:20: note: uninitialized use occurs here
sqlite3DbFree(db, trigger_name);
^~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:98:2: note: remove the 'if' if its condition is always false
if (db->mallocFailed)
^~~~~~~~~~~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:91:6: error: variable 'trigger_name' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
if (table == NULL || db->mallocFailed)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:156:20: note: uninitialized use occurs here
sqlite3DbFree(db, trigger_name);
^~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:91:2: note: remove the 'if' if its condition is always false
if (table == NULL || db->mallocFailed)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:91:6: error: variable 'trigger_name' is used uninitialized whenever '||' condition is true [-Werror,-Wsometimes-uninitialized]
if (table == NULL || db->mallocFailed)
^~~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:156:20: note: uninitialized use occurs here
sqlite3DbFree(db, trigger_name);
^~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:91:6: note: remove the '||' if its condition is always false
if (table == NULL || db->mallocFailed)
^~~~~~~~~~~~~~~~
/Users/n.pettik/tarantool/src/box/sql/trigger.c:105:2: note: variable 'trigger_name' is declared here
char *trigger_name = sqlite3NameFromToken(db, name);
^
4 errors generated.
They are not shown as errors (but displayed as warnings) in Travis,
since in RELEASE mode -Werror is not set.
^ permalink raw reply [flat|nested] 15+ messages in thread