* [tarantool-patches] [PATCH v2 10/11] sql: move Triggers to server
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
@ 2018-06-09 9:32 ` Kirill Shcherbatov
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-28 7:18 ` Konstantin Osipov
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 11/11] sql: VDBE tests for trigger existence Kirill Shcherbatov
` (9 subsequent siblings)
10 siblings, 2 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:32 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, 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 | 124 +++++++++++++++
src/box/errcode.h | 2 +-
src/box/lua/schema.lua | 6 +
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/sqliteInt.h | 5 -
src/box/sql/tokenize.c | 29 +++-
src/box/sql/trigger.c | 281 +++++++++++++++++-----------------
src/box/sql/vdbe.c | 76 ++-------
src/box/sql/vdbe.h | 1 -
src/box/sql/vdbeaux.c | 9 --
test/box/misc.result | 1 +
test/sql-tap/identifier_case.test.lua | 4 +-
test/sql-tap/trigger1.test.lua | 14 +-
test/sql/triggers.result | 260 +++++++++++++++++++++++++++++++
test/sql/triggers.test.lua | 94 ++++++++++++
21 files changed, 737 insertions(+), 281 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 f2bf85d..c683a51 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -551,6 +551,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);
+ /** Copy SQL Triggers pointer. */
+ struct Trigger *old_value = new_space->sql_triggers;
+ new_space->sql_triggers = old_space->sql_triggers;
+ old_space->sql_triggers = old_value;
}
/**
@@ -3091,6 +3095,49 @@ lock_before_dd(struct trigger *trigger, void *event)
latch_lock(&schema_lock);
}
+static void
+triggers_task_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);
+ }
+}
+
+static void
+triggers_task_commit(struct trigger *trigger, void * /* event */)
+{
+ struct Trigger *old_trigger = (struct Trigger *)trigger->data;
+ /* DELETE, REPLACE trigger. */
+ sql_trigger_delete(sql_get(), old_trigger);
+}
+
/**
* A trigger invoked on replace in a space containing
* SQL triggers.
@@ -3100,6 +3147,83 @@ 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(triggers_task_rollback, NULL);
+ struct trigger *on_commit =
+ txn_alter_trigger_new(triggers_task_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, 0, &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, 0, &trigger_name_len);
+
+ const char *space_opts =
+ tuple_field_with_type_xc(new_tuple, 2, 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();
+
+ const char *trigger_name = sql_trigger_name(new_trigger);
+ if (strncmp(trigger_name_src, trigger_name,
+ trigger_name_len) != 0) {
+ sql_trigger_delete(sql_get(), new_trigger);
+ tnt_raise(ClientError, ER_SQL,
+ "tuple trigger name does not match extracted "
+ "from SQL");
+ }
+ uint32_t space_id = tuple_field_u32_xc(new_tuple, 1);
+ if (space_id != sql_trigger_space_id(new_trigger)) {
+ sql_trigger_delete(sql_get(), new_trigger);
+ tnt_raise(ClientError, ER_SQL,
+ "tuple 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) {
+ sql_trigger_delete(sql_get(), new_trigger);
+ diag_raise();
+ }
+
+ bool is_insert = (new_tuple != NULL && old_tuple == NULL);
+ assert(!is_insert || old_trigger == NULL);
+
+ on_commit->data = is_insert ? NULL : old_trigger;
+ on_rollback->data = new_trigger;
+ }
+
+ 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 dd5ce0a..4996565 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -463,6 +463,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]
@@ -471,6 +472,11 @@ box.schema.space.drop = function(space_id, space_name, opts)
-- Delete automatically generated sequence.
box.schema.sequence.drop(sequence_tuple[2])
end
+ local triggers = _trigger.index["space_id"]:select({space_id})
+ for i = #triggers, 1, -1 do
+ local v = triggers[i]
+ _trigger:delete{v[1]}
+ 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 b370b7c..d2aeecf 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
+ * initiated in LUA schema:delete.
+ */
+ assert(space->sql_triggers == NULL);
space->vtab->destroy(space);
}
diff --git a/src/box/space.h b/src/box/space.h
index b8d29ca..0413cd0 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 0ba0346..57211fd 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1227,9 +1227,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,
@@ -1298,42 +1295,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, 2);
- 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 35aacc3..51e42ab 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -84,6 +84,17 @@ struct Expr *
sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len);
/**
+ * 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.
@@ -92,6 +103,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 object from 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 3c3c900..9c1b203 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2285,16 +2285,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 70ebef8..5fcf6a5 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -1439,8 +1439,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 59c61c7..023e6b0 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1766,9 +1766,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/sqliteInt.h b/src/box/sql/sqliteInt.h
index 6ca59c4..276f881 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1935,7 +1935,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. */
@@ -3034,14 +3033,11 @@ struct Trigger {
char *zName; /* The name of the trigger */
/** The ID of space the trigger is refer 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 */
};
@@ -4029,7 +4025,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 3f59fc8..71a74d6 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -42,7 +42,6 @@
#include "say.h"
#include "sqliteInt.h"
-#include "tarantoolInt.h"
/* Character classes for tokenizing
*
@@ -123,6 +122,7 @@ static const char sql_ascii_class[] = {
* the #include below.
*/
#include "keywordhash.h"
+#include "tarantoolInt.h"
#define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0)
@@ -534,7 +534,12 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
if (pParse->pWithToFree)
sqlite3WithDelete(db, pParse->pWithToFree);
- sql_trigger_delete(db, pParse->pNewTrigger);
+ /*
+ * Trigger is exported with pNewTrigger field when
+ * parse_only flag is set.
+ */
+ if (!pParse->parse_only)
+ sql_trigger_delete(db, pParse->pNewTrigger);
sqlite3DbFree(db, pParse->pVList);
while (pParse->pZombieTab) {
Table *p = pParse->pZombieTab;
@@ -575,3 +580,23 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
sql_parser_destroy(&parser);
return expression;
}
+
+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) {
+ char *error = tt_static_buf();
+ snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error);
+ diag_set(ClientError, ER_SQL, error);
+ sqlite3DbFree(db, sql_error);
+ } else {
+ trigger = parser.pNewTrigger;
+ }
+ sql_parser_destroy(&parser);
+ return trigger;
+}
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index a9c686f..1b569bc 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,104 +82,128 @@ 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;
} 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;
+ goto trigger_cleanup;
+ }
+ if (space_id == BOX_ID_NIL) {
+ diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
+ pParse->rc = SQL_TARANTOOL_ERROR;
goto trigger_cleanup;
}
- /* INSTEAD of triggers are only for views and views only support INSTEAD
- * of triggers.
+ /* 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;
+ goto trigger_cleanup;
+ }
+
+ /*
+ * INSTEAD of triggers are only for views and
+ * views only support INSTEAD of triggers.
*/
- if (space_is_view(pTab) && 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) {
+ char *error = tt_static_buf();
+ snprintf(error, TT_STATIC_BUF_LEN,
+ "cannot create %s trigger on view: %s",
+ (tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER", table_name);
+ diag_set(ClientError, ER_SQL, error);
+ pParse->rc = SQL_TARANTOOL_ERROR;
goto trigger_cleanup;
}
- if (!space_is_view(pTab) && 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) {
+ char *error = tt_static_buf();
+ snprintf(error, TT_STATIC_BUF_LEN,
+ "cannot create INSTEAD OF trigger on table: %s",
+ table_name);
+ diag_set(ClientError, ER_SQL, error);
+ pParse->rc = SQL_TARANTOOL_ERROR;
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);
+ if ((pWhen != NULL && pTrigger->pWhen == NULL) ||
+ (pColumns != NULL && pTrigger->pColumns == NULL))
+ goto trigger_cleanup;
+ assert(pParse->pNewTrigger == NULL);
pParse->pNewTrigger = pTrigger;
trigger_cleanup:
@@ -186,11 +211,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) {
+ if (pParse->pNewTrigger == NULL)
sql_trigger_delete(db, pTrigger);
- } else {
+ else
assert(pParse->pNewTrigger == pTrigger);
- }
}
/*
@@ -211,7 +235,7 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
DbFixer sFix; /* Fixer object */
Token nameToken; /* Trigger name for error reporting */
- pParse->pNewTrigger = 0;
+ pParse->pNewTrigger = NULL;
if (NEVER(pParse->nErr) || !pTrig)
goto triggerfinish_cleanup;
zName = pTrig->zName;
@@ -228,10 +252,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;
@@ -290,37 +315,11 @@ 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->pNewTrigger = pTrig;
+ pTrig = NULL;
}
triggerfinish_cleanup:
@@ -331,7 +330,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->pNewTrigger || pParse->parse_only);
sqlite3DeleteTriggerStep(db, pStepList);
}
@@ -480,7 +479,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);
@@ -535,16 +533,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
@@ -567,34 +555,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, 0, "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;
}
/*
@@ -633,22 +646,17 @@ 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; p = p->pNext) {
+ if (p->op == op && checkColumnOverlap(p->pColumns, pChanges))
mask |= p->tr_tm;
- }
}
- if (pMask) {
+ if (pMask != 0)
*pMask = mask;
- }
- return (mask ? pList : 0);
+ return (mask != 0) ? trigger_list : 0;
}
/*
@@ -837,7 +845,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 == 0 || pTab->def->id == pTrigger->space_id);
assert(pTop->pVdbe);
/* Allocate the TriggerPrg and SubProgram objects. To ensure that they
@@ -954,7 +962,7 @@ getRowTrigger(Parse * pParse, /* Current parse context */
Parse *pRoot = sqlite3ParseToplevel(pParse);
TriggerPrg *pPrg;
- assert(pTrigger->zName == 0 || pTab == tableOfTrigger(pTrigger));
+ assert(pTrigger->zName == 0 || 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 +1087,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 3fe5875..2d1538e 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4686,46 +4686,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
*
@@ -4745,7 +4705,6 @@ case OP_RenameTable: {
const char *zNewTableName;
Table *pTab;
FKey *pFKey;
- Trigger *pTrig;
int iRootPage;
InitData initData;
char *argv[4] = {NULL, NULL, NULL, NULL};
@@ -4758,7 +4717,6 @@ case OP_RenameTable: {
assert(zOldTableName);
pTab = sqlite3HashFind(&db->pSchema->tblHash, zOldTableName);
assert(pTab);
- pTrig = pTab->pTrigger;
iRootPage = pTab->tnum;
zNewTableName = pOp->p4.z;
zOldTableName = sqlite3DbStrNDup(db, zOldTableName,
@@ -4799,19 +4757,21 @@ case OP_RenameTable: {
goto abort_due_to_error;
}
- pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName);
- pTab->pTrigger = pTrig;
+ space = space_by_id(space_id);
+ assert(space != NULL);
- /* 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,
+ /* Rename all triggers created on this table. */
+ for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
+ 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);
break;
@@ -4857,18 +4817,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 6f4028c..e8ee297 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -381,6 +381,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/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 40daba4..79111fd 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/triggers.result b/test/sql/triggers.result
new file mode 100644
index 0000000..d214962
--- /dev/null
+++ b/test/sql/triggers.result
@@ -0,0 +1,260 @@
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+---
+- true
+...
+-- get invariant part of the tuple
+ function inmutable_part(data) local r = {} for i, l in pairs(data) do r[#r+1]={l[1], l[3]} 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; ]])
+---
+...
+inmutable_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
+---
+...
+-- checks for LUA tuples
+tuple = {"T1T", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+inmutable_part({box.space._trigger:insert(tuple)})
+---
+- error: Duplicate key exists in unique index 'primary' in space '_trigger'
+...
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+inmutable_part({box.space._trigger:insert(tuple)})
+---
+- error: 'SQL error: tuple 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;"}}
+---
+...
+inmutable_part({box.space._trigger:insert(tuple)})
+---
+- error: 'SQL error: tuple trigger name does not match extracted from SQL'
+...
+tuple = {"T2T", 443, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+inmutable_part({box.space._trigger:insert(tuple)})
+---
+- error: 'SQL error: tuple space_id does not match the value resolved on AST building
+ from SQL'
+...
+inmutable_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;")
+---
+...
+inmutable_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; ]])
+---
+...
+inmutable_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);")
+---
+...
+-- test duplicate index error
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+---
+- error: Duplicate key exists in unique index 'sqlite_autoindex_T1_1' in space 'T1'
+...
+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;"}}
+---
+...
+inmutable_part({box.space._trigger:insert(tuple)})
+---
+- - - T2T
+ - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
+ END;'}
+...
+tuple = {"T3T", space_id, {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
+---
+...
+inmutable_part({box.space._trigger:insert(tuple)})
+---
+- - - T3T
+ - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+ END;'}
+...
+inmutable_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;")
+---
+...
+inmutable_part({box.space._trigger:delete("T3T")})
+---
+- - - T3T
+ - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+ END;'}
+...
+inmutable_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; ]])
+---
+...
+inmutable_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;")
+---
+...
+inmutable_part(box.space._trigger:select())
+---
+- []
+...
+-- test crash on error.injection
+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.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+---
+...
+box.error.injection.set("ERRINJ_WAL_IO", true)
+---
+- ok
+...
+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("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;")
+---
+...
+test_run:cmd("clear filter")
+---
+- true
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
new file mode 100644
index 0000000..98da2b9
--- /dev/null
+++ b/test/sql/triggers.test.lua
@@ -0,0 +1,94 @@
+env = require('test_run')
+test_run = env.new()
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+
+-- get invariant part of the tuple
+ function inmutable_part(data) local r = {} for i, l in pairs(data) do r[#r+1]={l[1], l[3]} 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; ]])
+inmutable_part(box.space._trigger:select())
+
+space_id = box.space._space.index["name"]:get('T1').id
+
+-- checks for LUA tuples
+tuple = {"T1T", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+inmutable_part({box.space._trigger:insert(tuple)})
+
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+inmutable_part({box.space._trigger:insert(tuple)})
+
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+inmutable_part({box.space._trigger:insert(tuple)})
+
+tuple = {"T2T", 443, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+inmutable_part({box.space._trigger:insert(tuple)})
+inmutable_part(box.space._trigger:select())
+
+box.sql.execute("DROP TABLE T1;")
+inmutable_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; ]])
+inmutable_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);")
+-- test duplicate index error
+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;"}}
+inmutable_part({box.space._trigger:insert(tuple)})
+tuple = {"T3T", space_id, {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
+inmutable_part({box.space._trigger:insert(tuple)})
+inmutable_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;")
+inmutable_part({box.space._trigger:delete("T3T")})
+inmutable_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; ]])
+inmutable_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;")
+inmutable_part(box.space._trigger:select())
+
+
+-- test crash on error.injection
+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.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+box.error.injection.set("ERRINJ_WAL_IO", true)
+box.sql.execute("CREATE INDEX t1a ON t1(a);")
+box.error.injection.set("ERRINJ_WAL_IO", false)
+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;")
+
+test_run:cmd("clear filter")
--
2.7.4
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 10/11] sql: move " Kirill Shcherbatov
@ 2018-06-13 18:53 ` Vladislav Shpilevoy
2018-06-14 16:12 ` Kirill Shcherbatov
2018-06-28 7:18 ` Konstantin Osipov
1 sibling, 1 reply; 28+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-13 18:53 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov
Thanks for the patch! See 19 minor comments below.
On 09/06/2018 12:32, Kirill Shcherbatov 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.
>
> Resolves #3273.
> ---
> src/box/alter.cc | 124 +++++++++++++++
> src/box/errcode.h | 2 +-
> src/box/lua/schema.lua | 6 +
> 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/sqliteInt.h | 5 -
> src/box/sql/tokenize.c | 29 +++-
> src/box/sql/trigger.c | 281 +++++++++++++++++-----------------
> src/box/sql/vdbe.c | 76 ++-------
> src/box/sql/vdbe.h | 1 -
> src/box/sql/vdbeaux.c | 9 --
> test/box/misc.result | 1 +
> test/sql-tap/identifier_case.test.lua | 4 +-
> test/sql-tap/trigger1.test.lua | 14 +-
> test/sql/triggers.result | 260 +++++++++++++++++++++++++++++++
> test/sql/triggers.test.lua | 94 ++++++++++++
> 21 files changed, 737 insertions(+), 281 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 f2bf85d..c683a51 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3091,6 +3095,49 @@ lock_before_dd(struct trigger *trigger, void *event)
> latch_lock(&schema_lock);
> }
>
> +static void
> +triggers_task_rollback(struct trigger *trigger, void *event)
1. On_replace_dd_trigger works when a single trigger is created, dropped
or updated. So 'triggers' here is incorrect. Lets name it
on_replace_trigger_rollback. And on_replace_trigger_commit the second
function.
> @@ -3100,6 +3147,83 @@ 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(triggers_task_rollback, NULL);
> + struct trigger *on_commit =
> + txn_alter_trigger_new(triggers_task_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, 0, &trigger_name_len);
2. Please, define separate enums for _trigger field numbers in
schema_def.h like it is done for other system spaces. I think, the
previous patch is the perfect place for it.
> + 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);
3. Incorrect alignment.
> + (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, 0, &trigger_name_len);
> +
> + const char *space_opts =
> + tuple_field_with_type_xc(new_tuple, 2, 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();
> +
> + const char *trigger_name = sql_trigger_name(new_trigger);
> + if (strncmp(trigger_name_src, trigger_name,
> + trigger_name_len) != 0) {
4. When you worked on CHECKs, I found a bug with strncmp() usage. Please,
fix it here too. Strncmp considers two strings be equal even if their
lengths are different.
> + bool is_insert = (new_tuple != NULL && old_tuple == NULL);
> + assert(!is_insert || old_trigger == NULL);
5. What is the point of is_insert? If it is true, then old_trigger == NULL,
so the line below is the same as just on_commit->data = old_trigger. It is
not?
> +
> + on_commit->data = is_insert ? NULL : old_trigger;
> + on_rollback->data = new_trigger;
> + }
> +
> + txn_on_rollback(txn, on_rollback);
> + txn_on_commit(txn, on_commit);
> }
>
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index dd5ce0a..4996565 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -471,6 +472,11 @@ box.schema.space.drop = function(space_id, space_name, opts)
> -- Delete automatically generated sequence.
> box.schema.sequence.drop(sequence_tuple[2])
> end
> + local triggers = _trigger.index["space_id"]:select({space_id})
> + for i = #triggers, 1, -1 do
> + local v = triggers[i]
> + _trigger:delete{v[1]}
> + end
6. Why not just
for _, t in _trigger.index.space_id:pairs({space_id}) do
_trigger:delete({t.name})
end
?
> diff --git a/src/box/space.c b/src/box/space.c
> index b370b7c..d2aeecf 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
> + * initiated in LUA schema:delete.
7. What is schema:delete?
> + */
> + assert(space->sql_triggers == NULL);
> space->vtab->destroy(space);
> }
>
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 3f59fc8..71a74d6 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -42,7 +42,6 @@
>
> #include "say.h"
> #include "sqliteInt.h"
> -#include "tarantoolInt.h"
>
> /* Character classes for tokenizing
> *
> @@ -123,6 +122,7 @@ static const char sql_ascii_class[] = {
> * the #include below.
> */
> #include "keywordhash.h"
> +#include "tarantoolInt.h"
8. This and the previous diff are unnecessary.
> @@ -575,3 +580,23 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
> sql_parser_destroy(&parser);
> return expression;
> }
> +
> +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) {
> + char *error = tt_static_buf();
> + snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error);
> + diag_set(ClientError, ER_SQL, error);
> + sqlite3DbFree(db, sql_error);
9. Same as in the reviews for previous patches. Why not just
diag_set(ClientError, EQ_SQL, sql_error);
?
> + } else {
> + trigger = parser.pNewTrigger;
10. You can remove
if (! parse_only)
delete(new_trigger)
from parser_destroy if here will nullify pNewTrigger like
sqlite3FinishTrigger. Parser_destroy is called much more
frequently, so lets optimize it to avoid branching when
possible.
> + }
> + sql_parser_destroy(&parser);
> + return trigger;
> +}
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index a9c686f..1b569bc 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -81,104 +82,128 @@ sqlite3BeginTrigger(Parse * pParse,
> - /* 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;
11. Forgot nErr++.
> + goto trigger_cleanup;
> + }
> + if (space_id == BOX_ID_NIL) {
> + diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
> + pParse->rc = SQL_TARANTOOL_ERROR;
12. Same. Please, check in other places yourself.
> goto trigger_cleanup;
> }
>
> @@ -567,34 +555,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)
> -/*
> - * 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)
> + struct Trigger *src_trigger = trigger != NULL ? trigger : *old_trigger;
> + assert(src_trigger != NULL);
> + struct space *space =
> + space_cache_find(src_trigger->space_id);
13. Fits in one line.
> @@ -633,22 +646,17 @@ 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; p = p->pNext) {
> + if (p->op == op && checkColumnOverlap(p->pColumns, pChanges))
> mask |= p->tr_tm;
> - }
> }
> - if (pMask) {
> + if (pMask != 0)
14. Please, apply.
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 1b569bc7b..10294455a 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -650,13 +650,14 @@ sqlite3TriggersExist(Table * pTab, /* The table the contains the triggers */
struct session *user_session = current_session();
if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0)
trigger_list = space_trigger_list(pTab->def->id);
- for (struct Trigger *p = trigger_list; p; p = p->pNext) {
- if (p->op == op && checkColumnOverlap(p->pColumns, pChanges))
+ 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 != 0)
+ if (pMask != NULL)
*pMask = mask;
- return (mask != 0) ? trigger_list : 0;
+ return mask != 0 ? trigger_list : NULL;
}
/*
> }
>
> /*
> @@ -837,7 +845,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 == 0 || pTab->def->id == pTrigger->space_id);
15. != NULL. Please, check in other places yourself.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3fe5875..2d1538e 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4799,19 +4757,21 @@ case OP_RenameTable: {
> goto abort_due_to_error;
> }
>
> - pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName);
> - pTab->pTrigger = pTrig;
> + space = space_by_id(space_id);
16. space is already got in this opcode above.
> + assert(space != NULL);
>
> - /* 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,
> + /* Rename all triggers created on this table. */
> + for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
> + 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;
> }
17. Why not this?
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 2d1538e24..98b7d95a0 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4761,15 +4761,14 @@ case OP_RenameTable: {
assert(space != NULL);
/* Rename all triggers created on this table. */
- for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
- struct Trigger *next_trigger = trigger->pNext;
+ for (struct Trigger *trigger = space->sql_triggers; trigger != NULL;
+ trigger = trigger->pNext) {
rc = tarantoolSqlite3RenameTrigger(trigger->zName,
zOldTableName, zNewTableName);
if (rc != SQLITE_OK) {
sqlite3ResetAllSchemasOfConnection(db);
goto abort_due_to_error;
}
- trigger = next_trigger;
}
> diff --git a/test/sql/triggers.result b/test/sql/triggers.result
> new file mode 100644
> index 0000000..d214962
> --- /dev/null
> +++ b/test/sql/triggers.result
> @@ -0,0 +1,260 @@
> +env = require('test_run')
> +---
> +...
> +test_run = env.new()
> +---
> +...
> +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
> +---
> +- true
> +...
> +-- get invariant part of the tuple
> + function inmutable_part(data) local r = {} for i, l in pairs(data) do r[#r+1]={l[1], l[3]} end return r end
18. 'inmutable' does not exist. Maybe 'immutable'?
In Lua you have tuple field access by name and table.insert() to append a value. So
lets use them to make the code more understandable:
for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) 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; ]])
> +---
> +...
> +inmutable_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
> +---
> +...
> +-- checks for LUA tuples
> +tuple = {"T1T", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +inmutable_part({box.space._trigger:insert(tuple)})
> +---
> +- error: Duplicate key exists in unique index 'primary' in space '_trigger'
19. Your tests looks failing. Or is it ok? Then why do you need 'immutable_part' here,
if :insert() fails before the call?
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-14 16:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
> 1. On_replace_dd_trigger works when a single trigger is created, dropped
> or updated. So 'triggers' here is incorrect. Lets name it
> on_replace_trigger_rollback. And on_replace_trigger_commit the second
> function.
static void
-triggers_task_rollback(struct trigger *trigger, void *event)
+on_replace_trigger_rollback(struct trigger *trigger, void *event)
static void
-triggers_task_commit(struct trigger *trigger, void * /* event */)
+on_replace_trigger_commit(struct trigger *trigger, void * /* event */)
struct trigger *on_rollback =
- txn_alter_trigger_new(triggers_task_rollback, NULL);
+ txn_alter_trigger_new(on_replace_trigger_rollback, NULL);
struct trigger *on_commit =
- txn_alter_trigger_new(triggers_task_commit, NULL);
+ txn_alter_trigger_new(on_replace_trigger_commit, NULL);
> 2. Please, define separate enums for _trigger field numbers in
> schema_def.h like it is done for other system spaces. I think, the
> previous patch is the perfect place for it.
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3160,7 +3160,8 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
/* DROP trigger. */
uint32_t trigger_name_len;
const char *trigger_name_src =
- tuple_field_str_xc(old_tuple, 0, &trigger_name_len);
+ 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);
@@ -3180,10 +3181,13 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
/* INSERT, REPLACE trigger. */
uint32_t trigger_name_len;
const char *trigger_name_src =
- tuple_field_str_xc(new_tuple, 0, &trigger_name_len);
+ tuple_field_str_xc(new_tuple, BOX_TRIGGER_FIELD_NAME,
+ &trigger_name_len);
const char *space_opts =
- tuple_field_with_type_xc(new_tuple, 2, MP_MAP);
+ 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);
@@ -3200,7 +3204,9 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
"tuple trigger name does not match extracted "
"from SQL");
}
- uint32_t space_id = tuple_field_u32_xc(new_tuple, 1);
+ uint32_t space_id =
+ tuple_field_u32_xc(new_tuple,
+ BOX_TRIGGER_FIELD_SPACE_ID);
> 3. Incorrect alignment.
int rc = sql_trigger_replace(sql_get(), trigger_name, NULL,
- &old_trigger);
+ &old_trigger);
> 4. When you worked on CHECKs, I found a bug with strncmp() usage. Please,
> fix it here too. Strncmp considers two strings be equal even if their
> lengths are different.
- if (strncmp(trigger_name_src, trigger_name,
- trigger_name_len) != 0) {
+ if (strlen(trigger_name) != trigger_name_len ||
+ memcmp(trigger_name_src, trigger_name,
+ trigger_name_len) != 0) {
> 5. What is the point of is_insert? If it is true, then old_trigger == NULL,
> so the line below is the same as just on_commit->data = old_trigger. It is
> not?
Yes, you are right.
on_commit->data = old_trigger; is enough.
> 6. Why not just
>
> for _, t in _trigger.index.space_id:pairs({space_id}) do
> _trigger:delete({t.name})
> end
Accepted.
- local triggers = _trigger.index["space_id"]:select({space_id})
- for i = #triggers, 1, -1 do
- local v = triggers[i]
- _trigger:delete{v[1]}
+ for _, t in _trigger.index.space_id:pairs({space_id}) do
+ _trigger:delete({t.name})
end
> 7. What is schema:delete?
- * initiated in LUA schema:delete.
+ * initiated in LUA part of deletion process.
> 8. This and the previous diff are unnecessary.
ok.
> 9. Same as in the reviews for previous patches. Why not just
if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK &&
parser.rc != SQL_TARANTOOL_ERROR)
diag_set(ClientError, ER_SQL, sql_error);
else
trigger = parser.pNewTrigger;
> 10. You can remove
> from parser_destroy if here will nullify pNewTrigger like
> sqlite3FinishTrigger. Parser_destroy is called much more
> frequently, so lets optimize it to avoid branching when
> possible.
Not this way, but it is possible:
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e43fbcb..1fe6deb 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -454,5 +454,6 @@ sql_parser_destroy(Parse *parser)
}
parser->disableLookaside = 0;
sqlite3DbFree(db, parser->zErrMsg);
+ sql_trigger_delete(db, parser->pNewTrigger);
region_destroy(&parser->region);
}
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 16e2111..e06cc0a 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -534,12 +534,6 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
if (pParse->pWithToFree)
sqlite3WithDelete(db, pParse->pWithToFree);
- /*
- * Trigger is exported with pNewTrigger field when
- * parse_only flag is set.
- */
- if (!pParse->parse_only)
- sql_trigger_delete(db, pParse->pNewTrigger);
sqlite3DbFree(db, pParse->pVList);
while (pParse->pZombieTab) {
Table *p = pParse->pZombieTab;
@@ -587,10 +581,12 @@ sql_trigger_compile(struct sqlite3 *db, const char *sql)
char *sql_error;
struct Trigger *trigger = NULL;
if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK &&
- parser.rc != SQL_TARANTOOL_ERROR)
+ parser.rc != SQL_TARANTOOL_ERROR) {
diag_set(ClientError, ER_SQL, sql_error);
- else
+ } else {
trigger = parser.pNewTrigger;
+ parser.pNewTrigger = NULL;
+ }
sql_parser_destroy(&parser);
return trigger;
}
> 11. Forgot nErr++> 12. Same. Please, check in other places yourself.
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -127,6 +127,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
if (!noErr) {
diag_set(ClientError, ER_TRIGGER_EXISTS, zName);
pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
} else {
assert(!db->init.busy);
}
@@ -138,11 +139,13 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
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;
}
@@ -168,6 +171,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
(tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER", table_name);
diag_set(ClientError, ER_SQL, error);
pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
goto trigger_cleanup;
}
if (!space->def->opts.is_view && tr_tm == TK_INSTEAD) {
@@ -177,6 +181,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
table_name);
diag_set(ClientError, ER_SQL, error);
pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
goto trigger_cleanup;
}
> 13. Fits in one line.
- struct space *space =
- space_cache_find(src_trigger->space_id);
+ struct space *space = space_cache_find(src_trigger->space_id);
> 14. Please, apply.
- for (struct Trigger *p = trigger_list; p; p = p->pNext) {
- if (p->op == op && checkColumnOverlap(p->pColumns, pChanges))
+ 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 != 0)
+ if (pMask != NULL)
*pMask = mask;
- return (mask != 0) ? trigger_list : 0;
+ return (mask != 0) ? trigger_list : NULL;
> 15. != NULL. Please, check in other places yourself.
assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id);
- assert(pTrigger->zName == 0 || pTab->def->id == pTrigger->space_id);
+ assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id);
> 16. space is already got in this opcode above.
No! That pointer become invalid with sqlite3UnlinkAndDeleteTable, sqlite3InitCallback calls. Now there is a new instance of space.
> 17. Why not this?
>
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 2d1538e24..98b7d95a0 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4761,15 +4761,14 @@ case OP_RenameTable: {
> assert(space != NULL);
>
> /* Rename all triggers created on this table. */
> - for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
> - struct Trigger *next_trigger = trigger->pNext;
> + for (struct Trigger *trigger = space->sql_triggers; trigger != NULL;
> + trigger = trigger->pNext) {
> rc = tarantoolSqlite3RenameTrigger(trigger->zName,
> zOldTableName, zNewTableName);
> if (rc != SQLITE_OK) {
> sqlite3ResetAllSchemasOfConnection(db);
> goto abort_due_to_error;
> }
> - trigger = next_trigger;
> }
>
I believe that to the moment of assignment next pointer source pointer could be already destructing and this value could be invalid.
+ /* Store pointer as trigger will be destructed. */
> 18. 'inmutable' does not exist. Maybe 'immutable'?
>
> In Lua you have tuple field access by name and table.insert() to append a value. So
> lets use them to make the code more understandable:
>
> for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end
- function inmutable_part(data) local r = {} for i, l in pairs(data) do r[#r+1]={l[1], l[3]} end return r end
+ function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end
> 19. Your tests looks failing. Or is it ok? Then why do you need 'immutable_part' here,
> if :insert() fails before the call?
Looks like rudimentary part of some old test, dropped.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 10/11] sql: move " Kirill Shcherbatov
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-28 7:18 ` Konstantin Osipov
2018-06-28 7:33 ` Kirill Shcherbatov
1 sibling, 1 reply; 28+ messages in thread
From: Konstantin Osipov @ 2018-06-28 7:18 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/06/09 12:39]:
> 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.
>
> +static void
> +triggers_task_rollback(struct trigger *trigger, void *event)
trigger_task_rollback or trigger_list_task_rollback, please don't
use plural prefixes for function names.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 28+ messages in thread
* [tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server
2018-06-28 7:18 ` Konstantin Osipov
@ 2018-06-28 7:33 ` Kirill Shcherbatov
0 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-28 7:33 UTC (permalink / raw)
To: tarantool-patches, Kostya Osipov; +Cc: Vladislav Shpilevoy
> trigger_task_rollback or trigger_list_task_rollback, please don't
> use plural prefixes for function names.
Hi! You've referenced an old patchset version,
it's not actual in many details.
[tarantool-patches] [PATCH v4 0/6] sql: remove Triggers to server
[tarantool-patches] [PATCH v4 6/8] sql: refactor AST trigger object name
sets contain actual discussion.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [tarantool-patches] [PATCH v2 11/11] sql: VDBE tests for trigger existence
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 10/11] sql: move " Kirill Shcherbatov
@ 2018-06-09 9:32 ` Kirill Shcherbatov
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 02/11] box: move db->pShchema init to sql_init Kirill Shcherbatov
` (8 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:32 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, 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.c | 35 +++++++++++++++++++++++++++++++++++
src/box/sql/main.c | 13 +++++++++++--
src/box/sql/sqliteInt.h | 20 ++++++++++++++++++++
src/box/sql/trigger.c | 20 +++++++++-----------
src/box/sql/vdbe.c | 14 ++++++++++----
src/box/sql/vdbeapi.c | 5 +++--
6 files changed, 88 insertions(+), 19 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index 57211fd..f539085 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1836,3 +1836,38 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
}
return 0;
}
+
+int
+vdbe_abort_execution_on_exists(Parse *parser, int space_id, int index_id,
+ const char *name, int tarantool_error_code,
+ bool no_error)
+{
+ Vdbe *v = sqlite3GetVdbe(parser);
+ assert(v != NULL);
+
+ sqlite3 *db = parser->db;
+ name = sqlite3DbStrDup(db, name);
+ if (name == NULL) {
+ diag_set(OutOfMemory, strlen(name) + 1, "sqlite3DbStrDup",
+ "name");
+ return -1;
+ }
+ int cursor = parser->nTab++;
+ int entity_id =
+ SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space_id, index_id);
+ emit_open_cursor(parser, cursor, entity_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, name, P4_STATIC);
+ sqlite3VdbeChangeP5(v, tarantool_error_code);
+ }
+ sqlite3VdbeAddOp1(v, OP_Close, cursor);
+ return 0;
+}
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 0acf7bc..f2334cb 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1456,8 +1456,17 @@ sqlite3_errmsg(sqlite3 * db)
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) {
+ testcase(db->pErr == 0);
+ z = (char *) sqlite3_value_text(db->pErr);
+ assert(!db->mallocFailed);
+ if (z == NULL)
+ z = sqlite3ErrStr(db->errCode);
+ } else {
+ struct diag *diag = diag_get();
+ assert(diag != NULL);
+ struct error *error = diag_last_error(diag);
+ z = error->errmsg;
}
}
return z;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 276f881..4f7dcbe 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4537,4 +4537,24 @@ 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.
+ *
+ * @param parser Parsing context.
+ * @param space_id Space to lookup identifier.
+ * @param index_id Index identifier containing string primary key.
+ * @param name Of object to test existency.
+ * @param tarantool_error_code Tarantool error to raise on object exists.
+ * @param no_error Do not raise error flag.
+ *
+ * @retval -1 on memory allocation error.
+ * @retval 0 on success.
+ */
+int
+vdbe_abort_execution_on_exists(Parse *parser, int space_id, int index_id,
+ const char *name, int tarantool_error_code,
+ bool no_error);
+
#endif /* SQLITEINT_H */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 1b569bc..92747f0 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -122,17 +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;
- } 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),
@@ -178,6 +167,15 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
pParse->rc = SQL_TARANTOOL_ERROR;
goto trigger_cleanup;
}
+ if (!pParse->parse_only) {
+ if (vdbe_abort_execution_on_exists(pParse, BOX_TRIGGER_ID, 0,
+ zName,
+ ER_TRIGGER_EXISTS,
+ (noErr != 0)) != 0) {
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ 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 2d1538e..0696cad 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -959,7 +959,7 @@ case OP_HaltIfNull: { /* in3 */
* VDBE, but do not rollback the transaction.
*
* If P4 is not null then it is an error message string.
- *
+ * If P1 is not SQL_TARANTOOL_ERROR,
* P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
*
* 0: (no change)
@@ -968,6 +968,8 @@ case OP_HaltIfNull: { /* in3 */
* 3: CHECK constraint failed: P4
* 4: FOREIGN KEY constraint failed: P4
*
+ * If P1 is SQL_TARANTOOL_ERROR, P5 contain ClientError code.
+ *
* If P5 is not zero and P4 is NULL, then everything after the ":" is
* omitted.
*
@@ -1005,9 +1007,10 @@ 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 (pOp->p5 != 0 && p->rc == SQL_TARANTOOL_ERROR) {
+ diag_set(ClientError, 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);
@@ -1029,7 +1032,10 @@ case OP_Halt: {
p->rc = SQLITE_BUSY;
} else {
assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT);
- rc = p->rc ? SQLITE_ERROR : SQLITE_DONE;
+ if (p->rc != SQL_TARANTOOL_ERROR)
+ rc = (p->rc != SQLITE_OK) ? SQLITE_ERROR : SQLITE_DONE;
+ else
+ rc = SQL_TARANTOOL_ERROR;
}
goto vdbe_return;
}
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index d35338a..0aa5c4a 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -598,8 +598,9 @@ sqlite3Step(Vdbe * p)
* contains the value that would be returned if sqlite3_finalize()
* were called on statement p.
*/
- assert(rc == SQLITE_ROW || rc == SQLITE_DONE || rc == SQLITE_ERROR
- || (rc & 0xff) == SQLITE_BUSY || rc == SQLITE_MISUSE);
+ assert(rc == SQLITE_ROW || rc == SQLITE_DONE || rc == SQLITE_ERROR ||
+ (rc & 0xff) == SQLITE_BUSY || rc == SQLITE_MISUSE ||
+ rc == SQL_TARANTOOL_ERROR);
if (p->isPrepareV2 && rc != SQLITE_ROW && rc != SQLITE_DONE) {
/* If this statement was prepared using sqlite3_prepare_v2(), and an
* error has occurred, then return the error code in p->rc to the
--
2.7.4
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 11/11] sql: VDBE tests for trigger existence Kirill Shcherbatov
@ 2018-06-13 18:53 ` Vladislav Shpilevoy
2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 1 reply; 28+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-13 18:53 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov
Thanks for the patch! See 12 comments below.
On 09/06/2018 12:32, Kirill Shcherbatov wrote:
> Trigger presence in system should be tested on each VDBE
> execution attempt, not on Parser iteration.
>
> Part of #3435, #3273
> ---
> src/box/sql.c | 35 +++++++++++++++++++++++++++++++++++
> src/box/sql/main.c | 13 +++++++++++--
> src/box/sql/sqliteInt.h | 20 ++++++++++++++++++++
> src/box/sql/trigger.c | 20 +++++++++-----------
> src/box/sql/vdbe.c | 14 ++++++++++----
> src/box/sql/vdbeapi.c | 5 +++--
> 6 files changed, 88 insertions(+), 19 deletions(-)
>
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 57211fd..f539085 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1836,3 +1836,38 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
> }
> return 0;
> }
> +
> +int
> +vdbe_abort_execution_on_exists(Parse *parser, int space_id, int index_id,
> + const char *name, int tarantool_error_code,
> + bool no_error)
1. If this function is method of parser then its prefix must be 'parser_'.
And here you do not an abortion. You emit the code of abortion if needed.
So this function can be named like this:
parser_emit_execution_halt_on_exists
2. struct Parse
3. Why is the function stored in sql.c? As far I remember sql.c/.h is a
link between sqlite and Tarantool code, but vdbe_abort_execution_on_exists
is used in sqlite only.
> +{
> + Vdbe *v = sqlite3GetVdbe(parser);
4. struct Vdbe. Please check other places yourself.
> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
> index 0acf7bc..f2334cb 100644
> --- a/src/box/sql/main.c
> +++ b/src/box/sql/main.c
> @@ -1456,8 +1456,17 @@ sqlite3_errmsg(sqlite3 * db)
> 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) {
> + testcase(db->pErr == 0);
5. What is 'testcase'? Why do you need it here?
> + z = (char *) sqlite3_value_text(db->pErr);
6. It is assigned to this value few lines above.
> + assert(!db->mallocFailed);
> + if (z == NULL)
> + z = sqlite3ErrStr(db->errCode);
> + } else {
> + struct diag *diag = diag_get();
> + assert(diag != NULL);
7. diag is never NULL, because TX thread consists of fibers, and
each fiber has diag. Please, remove the assertion and just inline
diag_get() on the next line.
> + struct error *error = diag_last_error(diag);
> + z = error->errmsg;
> }
> }
> return z;
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 276f881..4f7dcbe 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -4537,4 +4537,24 @@ 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.
> + *
> + * @param parser Parsing context.
> + * @param space_id Space to lookup identifier.
> + * @param index_id Index identifier containing string primary key.
> + * @param name Of object to test existency.
8. No such word 'existency'.
> + * @param tarantool_error_code Tarantool error to raise on object exists.
9. Out of 66.
> + * @param no_error Do not raise error flag.
> + *
> + * @retval -1 on memory allocation error.
> + * @retval 0 on success.
> + */
> +int
> +vdbe_abort_execution_on_exists(Parse *parser, int space_id, int index_id,
> + const char *name, int tarantool_error_code,
> + bool no_error);
> +
> #endif /* SQLITEINT_H */
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 1b569bc..92747f0 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -178,6 +167,15 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
> pParse->rc = SQL_TARANTOOL_ERROR;
> goto trigger_cleanup;
> }
> + if (!pParse->parse_only) {
> + if (vdbe_abort_execution_on_exists(pParse, BOX_TRIGGER_ID, 0,
> + zName,
> + ER_TRIGGER_EXISTS,
> + (noErr != 0)) != 0) {
> + pParse->rc = SQL_TARANTOOL_ERROR;
10. pParse->nErr++. Or find a way to delete nErr. It is very annoying and
useless attribute.
> + goto trigger_cleanup;
> + }
> + }
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 2d1538e..0696cad 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -959,7 +959,7 @@ case OP_HaltIfNull: { /* in3 */
> * VDBE, but do not rollback the transaction.
> *
> * If P4 is not null then it is an error message string.
> - *
> + * If P1 is not SQL_TARANTOOL_ERROR,
> * P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
11. Why here you did not put empty line after 'if P1', but did in the
next hunk?
> *
> * 0: (no change)
> @@ -968,6 +968,8 @@ case OP_HaltIfNull: { /* in3 */
> * 3: CHECK constraint failed: P4
> * 4: FOREIGN KEY constraint failed: P4
> *
> + * If P1 is SQL_TARANTOOL_ERROR, P5 contain ClientError code.
> + *
> * If P5 is not zero and P4 is NULL, then everything after the ":" is
> * omitted.
> *
> @@ -1005,9 +1007,10 @@ 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 (pOp->p5 != 0 && p->rc == SQL_TARANTOOL_ERROR) {
> + diag_set(ClientError, pOp->p5, pOp->p4.z);
12. I am afraid of this way will not work, when a code has multiple
arguments. And when type is not ClientError. And when it has a single
non-string argument. And when we want to know where the error was
generated. Here diag_set always saves filename "vdbe.c" and line "1012".
I think we should find another way to emit an error. One possible way
is to create and error object during parsing and save it by pointer in
p4. When the error occurs, the error object is set into the diag object.
But maybe you will find another way.
> + } else if (pOp->p5 != 0) {
> static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
> "FOREIGN KEY" };
> testcase( pOp->p5==1);
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-14 16:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
> 1. If this function is method of parser then its prefix must be 'parser_'.
> And here you do not an abortion. You emit the code of abortion if needed.
> So this function can be named like this:
> 2. struct Parse
> 3. Why is the function stored in sql.c? As far I remember sql.c/.h is a
> link between sqlite and Tarantool code, but vdbe_abort_execution_on_exists
> is used in sqlite only.
Moved to build.c.
+int
+parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+ int index_id, const char *name,
+ int tarantool_error_code, bool no_error)
> 4. struct Vdbe. Please check other places yourself.
+ struct Vdbe *v = sqlite3GetVdbe(parser);
+ struct sqlite3 *db = parser->db;
> 5. What is 'testcase'? Why do you need it here?
> 6. It is assigned to this value few lines above.
> 7. diag is never NULL, because TX thread consists of fibers, and
> each fiber has diag. Please, remove the assertion and just inline
> diag_get() on the next line.
@@ -1454,19 +1454,14 @@ 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 (db->errCode != SQL_TARANTOOL_ERROR) {
- testcase(db->pErr == 0);
- z = (char *) sqlite3_value_text(db->pErr);
assert(!db->mallocFailed);
+ z = (char *)sqlite3_value_text(db->pErr);
if (z == NULL)
z = sqlite3ErrStr(db->errCode);
} else {
- struct diag *diag = diag_get();
- assert(diag != NULL);
- struct error *error = diag_last_error(diag);
- z = error->errmsg;
+ z = diag_last_error(diag_get())->errmsg;
}
}
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index 276f881..4f7dcbe 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -4537,4 +4537,24 @@ 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.
>> + *
>> + * @param parser Parsing context.
>> + * @param space_id Space to lookup identifier.
>> + * @param index_id Index identifier containing string primary key.
>> + * @param name Of object to test existency.
>
> 8. No such word 'existency'.
Songs good for me (https://en.oxforddictionaries.com/definition/existency):
1The fact or state of existing; continuance of being.
2Something that exists; a being, an entity; = "existence".
- * @param name Of object to test existency.
+ * @param name Of object to test existence.
> 9. Out of 66.
+ * @param tarantool_error_code error to raise on object exists.
> 10. pParse->nErr++. Or find a way to delete nErr. It is very annoying and
> useless attribute.
+ pParse->nErr++;
> 11. Why here you did not put empty line after 'if P1', but did in the
> next hunk?
> 12. I am afraid of this way will not work, when a code has multiple
> arguments. And when type is not ClientError. And when it has a single
> non-string argument. And when we want to know where the error was
> generated. Here diag_set always saves filename "vdbe.c" and line "1012".
> I think we should find another way to emit an error. One possible way
> is to create and error object during parsing and save it by pointer in
> p4. When the error occurs, the error object is set into the diag object.
> But maybe you will find another way.
I've done this this way:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index c52c41d..0a731df 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -4175,8 +4175,9 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
int
parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
int index_id, const char *name,
- int tarantool_error_code, bool no_error)
+ struct error *error, bool no_error)
{
+ error_ref(error);
struct Vdbe *v = sqlite3GetVdbe(parser);
assert(v != NULL);
@@ -4197,11 +4198,12 @@ parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
P4_DYNAMIC);
sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
if (no_error) {
+ error_unref(error);
sqlite3VdbeAddOp0(v, OP_Halt);
} else {
sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
- ON_CONFLICT_ACTION_FAIL, 0, name, P4_STATIC);
- sqlite3VdbeChangeP5(v, tarantool_error_code);
+ ON_CONFLICT_ACTION_FAIL, 0, (void *)error,
+ P4_ERROROBJ);
}
sqlite3VdbeAddOp1(v, OP_Close, cursor);
return 0;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 4f755e4..d86ace5 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4546,7 +4546,7 @@ table_column_nullable_action(struct Table *tab, uint32_t column);
* @param space_id Space to lookup identifier.
* @param index_id Index identifier containing string primary key.
* @param name Of object to test existence.
- * @param tarantool_error_code error to raise on object exists.
+ * @param error object to raise on target exists.
* @param no_error Do not raise error flag.
*
* @retval -1 on memory allocation error.
@@ -4555,6 +4555,6 @@ table_column_nullable_action(struct Table *tab, uint32_t column);
int
parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
int index_id, const char *name,
- int tarantool_error_code, bool no_error);
+ struct error *error, bool no_error);
#endif /* SQLITEINT_H */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index d7ecdf1..4cb89fe 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -173,13 +173,13 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
goto trigger_cleanup;
}
if (!pParse->parse_only) {
+ struct error *error =
+ error_object(ClientError, ER_TRIGGER_EXISTS, zName);
if (parser_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID,
- 0,
- zName,
- ER_TRIGGER_EXISTS,
+ 0, zName, error,
(noErr != 0)) != 0) {
pParse->rc = SQL_TARANTOOL_ERROR;
pParse->nErr++;
goto trigger_cleanup;
}
}
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index dfc7b71..c3109ab 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -959,7 +959,7 @@ case OP_HaltIfNull: { /* in3 */
* VDBE, but do not rollback the transaction.
*
* If P4 is not null then it is an error message string.
- * If P1 is not SQL_TARANTOOL_ERROR,
+ *
* P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
*
* 0: (no change)
@@ -968,8 +968,6 @@ case OP_HaltIfNull: { /* in3 */
* 3: CHECK constraint failed: P4
* 4: FOREIGN KEY constraint failed: P4
*
- * If P1 is SQL_TARANTOOL_ERROR, P5 contain ClientError code.
- *
* If P5 is not zero and P4 is NULL, then everything after the ":" is
* omitted.
*
@@ -1008,8 +1006,8 @@ case OP_Halt: {
p->errorAction = (u8)pOp->p2;
p->pc = pcx;
if (p->rc) {
- if (pOp->p5 != 0 && p->rc == SQL_TARANTOOL_ERROR) {
- diag_set(ClientError, pOp->p5, pOp->p4.z);
+ if (p->rc == SQL_TARANTOOL_ERROR) {
+ diag_add_error(diag_get(), (struct error *)pOp->p4.z);
} else if (pOp->p5 != 0) {
static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
"FOREIGN KEY" };
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 77fa41a..66e6954 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -149,6 +149,8 @@ typedef struct VdbeOpList VdbeOpList;
#define P4_PTR (-18) /* P4 is a generic pointer */
#define P4_KEYDEF (-19) /* P4 is a pointer to key_def structure. */
#define P4_SPACEPTR (-20) /* P4 is a space pointer */
+/** P4 is an error object. */
+#define P4_ERROROBJ (-21)
/* Error message codes for OP_Halt */
#define P5_ConstraintNotNull 1
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 7b3c13d..8d89894 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -981,6 +981,10 @@ freeP4(sqlite3 * db, int p4type, void *p4)
}
break;
}
+ case P4_ERROROBJ: {
+ error_unref(p4);
+ break;
+ }
}
}
diff --git a/src/diag.h b/src/diag.h
index 0ccf86d..502b294 100644
--- a/src/diag.h
+++ b/src/diag.h
@@ -261,6 +261,9 @@ struct error *
BuildSocketError(const char *file, unsigned line, const char *socketname,
const char *format, ...);
+#define error_object(class, ...) \
+ Build##class(__FILE__, __LINE__, ##__VA_ARGS__);
+
#define diag_set(class, ...) do { \
/* Preserve the original errno. */ \
int save_errno = errno; \
^ permalink raw reply [flat|nested] 28+ messages in thread
* [tarantool-patches] [PATCH v2 02/11] box: move db->pShchema init to sql_init
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 10/11] sql: move " Kirill Shcherbatov
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 11/11] sql: VDBE tests for trigger existence Kirill Shcherbatov
@ 2018-06-09 9:32 ` Kirill Shcherbatov
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 04/11] sql: fix sql len in tarantoolSqlite3RenameTrigger Kirill Shcherbatov
` (7 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:32 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
As we are going to call parser on box.cfg() to recreate triggers
from SQL, we should initialize Schema as it used in sqlite3BeginTrigger.
Part of #3273.
---
src/box/sql.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index 7379cb4..93fca68 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -77,11 +77,22 @@ sql_init()
panic("failed to initialize SQL subsystem");
assert(db != NULL);
+ /*
+ * Initialize pSchema to use SQL parser on initialization:
+ * e.g. Trigger objects (compiled from SQL on tuple
+ * insertion in _trigger) need to refer it.
+ */
+ db->pSchema = sqlite3SchemaCreate(db);
+ if (db->pSchema == NULL) {
+ sqlite3_close(db);
+ panic("failed to initialize SQL Schema subsystem");
+ }
}
void
sql_load_schema()
{
+ assert(db->pSchema != NULL);
int rc;
struct session *user_session = current_session();
int commit_internal = !(user_session->sql_flags
@@ -89,7 +100,6 @@ sql_load_schema()
assert(db->init.busy == 0);
db->init.busy = 1;
- db->pSchema = sqlite3SchemaCreate(db);
rc = sqlite3InitDatabase(db);
if (rc != SQLITE_OK) {
sqlite3SchemaClear(db);
--
2.7.4
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] [PATCH v2 04/11] sql: fix sql len in tarantoolSqlite3RenameTrigger
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
` (2 preceding siblings ...)
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 02/11] box: move db->pShchema init to sql_init Kirill Shcherbatov
@ 2018-06-09 9:32 ` Kirill Shcherbatov
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 06/11] sql: refactor sql_expr_compile to return AST Kirill Shcherbatov
` (6 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:32 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
Part of #3273.
---
src/box/sql.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index 72fd5cc..82bddfb 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -690,8 +690,8 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
bool is_quoted = false;
trigger_stmt = rename_trigger(db, trigger_stmt, new_table_name, &is_quoted);
- uint32_t trigger_stmt_new_len = trigger_stmt_len + old_table_name_len -
- new_table_name_len + 2 * (!is_quoted);
+ 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) +
mp_sizeof_map(1) + mp_sizeof_str(3) +
--
2.7.4
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] [PATCH v2 06/11] sql: refactor sql_expr_compile to return AST
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
` (3 preceding siblings ...)
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 04/11] sql: fix sql len in tarantoolSqlite3RenameTrigger Kirill Shcherbatov
@ 2018-06-09 9:32 ` Kirill Shcherbatov
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 07/11] sql: move sqlite3DeleteTrigger to sql.h Kirill Shcherbatov
` (5 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:32 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
---
src/box/alter.cc | 6 +++---
src/box/sql.c | 5 +++--
src/box/sql.h | 9 ++++-----
src/box/sql/tokenize.c | 33 +++++++++++++++++++++------------
4 files changed, 31 insertions(+), 22 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index b62f8ad..f2bf85d 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -406,9 +406,9 @@ field_def_decode(struct field_def *field, const char **data,
}
if (field->default_value != NULL &&
- sql_expr_compile(sql_get(), field->default_value,
- strlen(field->default_value),
- &field->default_value_expr) != 0)
+ (field->default_value_expr =
+ sql_expr_compile(sql_get(), field->default_value,
+ strlen(field->default_value))) == NULL)
diag_raise();
}
diff --git a/src/box/sql.c b/src/box/sql.c
index 82bddfb..599cb60 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1813,8 +1813,9 @@ 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) {
+ if (expr_str != NULL &&
+ (item->pExpr = sql_expr_compile(db, expr_str, expr_str_len)) ==
+ NULL) {
sqlite3DbFree(db, item->zName);
return -1;
}
diff --git a/src/box/sql.h b/src/box/sql.h
index 23021e5..3f6cf22 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);
/**
* Store duplicate of a parsed expression into @a parser.
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 42c70a2..5b7c97d 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,19 +555,23 @@ 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;
+ return NULL;
}
sprintf(stmt, "%s%.*s", outer, expr_len, expr);
- char *unused;
- if (sqlite3RunParser(&parser, stmt, &unused) != SQLITE_OK) {
- diag_set(ClientError, ER_SQL_EXECUTE, expr);
- return -1;
+ char *sql_error;
+ if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK) {
+ char *error = tt_static_buf();
+ snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error);
+ diag_set(ClientError, ER_SQL, error);
+ sqlite3DbFree(db, sql_error);
+ } else {
+ expression = parser.parsed_expr;
}
- *result = parser.parsed_expr;
sql_parser_destroy(&parser);
- return 0;
+ return expression;
}
--
2.7.4
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 06/11] sql: refactor sql_expr_compile to return AST
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 06/11] sql: refactor sql_expr_compile to return AST Kirill Shcherbatov
@ 2018-06-13 18:53 ` Vladislav Shpilevoy
2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 1 reply; 28+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-13 18:53 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov
Thanks for the patch! See 1 comment below.
On 09/06/2018 12:32, Kirill Shcherbatov wrote:
> ---
> src/box/alter.cc | 6 +++---
> src/box/sql.c | 5 +++--
> src/box/sql.h | 9 ++++-----
> src/box/sql/tokenize.c | 33 +++++++++++++++++++++------------
> 4 files changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 42c70a2..5b7c97d 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -550,19 +555,23 @@ 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;
> + return NULL;
> }
> sprintf(stmt, "%s%.*s", outer, expr_len, expr);
>
> - char *unused;
> - if (sqlite3RunParser(&parser, stmt, &unused) != SQLITE_OK) {
> - diag_set(ClientError, ER_SQL_EXECUTE, expr);
> - return -1;
> + char *sql_error;
> + if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK) {
> + char *error = tt_static_buf();
> + snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error);
> + diag_set(ClientError, ER_SQL, error);
> + sqlite3DbFree(db, sql_error);
Why not just diag_set(ClientError, ER_SQL, sql_error); ?
> + } else {
> + expression = parser.parsed_expr;
> }
> - *result = parser.parsed_expr;
> sql_parser_destroy(&parser);
> - return 0;
> + return expression;
> }
>
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 06/11] sql: refactor sql_expr_compile to return AST
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-14 16:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
> Why not just diag_set(ClientError, ER_SQL, sql_error); ?
+ char *sql_error;
+ if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK &&
+ parser.rc != SQL_TARANTOOL_ERROR)
+ diag_set(ClientError, ER_SQL, sql_error);
+ else
+ expression = parser.parsed_expr;
^ permalink raw reply [flat|nested] 28+ messages in thread
* [tarantool-patches] [PATCH v2 07/11] sql: move sqlite3DeleteTrigger to sql.h
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
` (4 preceding siblings ...)
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 06/11] sql: refactor sql_expr_compile to return AST Kirill Shcherbatov
@ 2018-06-09 9:32 ` Kirill Shcherbatov
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 08/11] box: sort error codes in misc.test Kirill Shcherbatov
` (4 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:32 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
Part of #3273.
---
src/box/sql.h | 9 +++++++++
src/box/sql/callback.c | 2 +-
src/box/sql/sqliteInt.h | 3 +--
src/box/sql/status.c | 6 +++---
src/box/sql/tokenize.c | 2 +-
src/box/sql/trigger.c | 25 +++++++++++--------------
6 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/src/box/sql.h b/src/box/sql.h
index 3f6cf22..35aacc3 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
@@ -83,6 +84,14 @@ struct Expr *
sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len);
/**
+ * 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..e3f36e3 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -292,7 +292,7 @@ sqlite3SchemaClear(sqlite3 * db)
sqlite3HashInit(&pSchema->trigHash);
for (pElem = sqliteHashFirst(&temp2); pElem;
pElem = sqliteHashNext(pElem)) {
- sqlite3DeleteTrigger(0, (Trigger *) sqliteHashData(pElem));
+ sql_trigger_delete(0, (Trigger *) sqliteHashData(pElem));
}
sqlite3HashClear(&temp2);
sqlite3HashInit(&pSchema->tblHash);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 01351a1..ecbd573 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4027,7 +4027,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);
@@ -4035,7 +4034,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 5b7c97d..3f59fc8 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 ea35211..053dadb 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -186,7 +186,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
sqlite3IdListDelete(db, pColumns);
sql_expr_delete(db, pWhen, false);
if (!pParse->pNewTrigger) {
- sqlite3DeleteTrigger(db, pTrigger);
+ sql_trigger_delete(db, pTrigger);
} else {
assert(pParse->pNewTrigger == pTrigger);
}
@@ -328,7 +328,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 +471,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 +590,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] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 07/11] sql: move sqlite3DeleteTrigger to sql.h
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 07/11] sql: move sqlite3DeleteTrigger to sql.h Kirill Shcherbatov
@ 2018-06-13 18:53 ` Vladislav Shpilevoy
2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 1 reply; 28+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-13 18:53 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
Thanks for the patch!
Please, rationalize the patch in the commit message.
I know why the patch is needed, but Nikita, for example,
does not. Anyone else either.
On 09/06/2018 12:32, Kirill Shcherbatov wrote:
> Part of #3273.
> ---
> src/box/sql.h | 9 +++++++++
> src/box/sql/callback.c | 2 +-
> src/box/sql/sqliteInt.h | 3 +--
> src/box/sql/status.c | 6 +++---
> src/box/sql/tokenize.c | 2 +-
> src/box/sql/trigger.c | 25 +++++++++++--------------
> 6 files changed, 26 insertions(+), 21 deletions(-)
>
Please, apply this:
diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
index e3f36e301..bd8db9994 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)) {
- sql_trigger_delete(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/trigger.c b/src/box/sql/trigger.c
index 053dadbd9..5e21cfca6 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) {
+ if (pParse->pNewTrigger == NULL)
sql_trigger_delete(db, pTrigger);
- } else {
+ else
assert(pParse->pNewTrigger == pTrigger);
- }
}
/*
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 07/11] sql: move sqlite3DeleteTrigger to sql.h
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-14 16:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
On 13.06.2018 21:53, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> Please, rationalize the patch in the commit message.
> I know why the patch is needed, but Nikita, for example,
> does not. Anyone else either.
sql: move sqlite3DeleteTrigger to sql.h
As we are going to port triggers to server, we need
an instrument to release allocated memory in alter.cc.
Part of #3273.
> Please, apply this:
- for (pElem = sqliteHashFirst(&temp2); pElem;
- pElem = sqliteHashNext(pElem)) {
- sql_trigger_delete(0, (Trigger *) sqliteHashData(pElem));
- }
+ for (pElem = sqliteHashFirst(&temp2); pElem != NULL;
+ pElem = sqliteHashNext(pElem))
+ sql_trigger_delete(NULL, (Trigger *) sqliteHashData(pElem));
- if (!pParse->pNewTrigger) {
+ if (pParse->pNewTrigger == NULL)
sql_trigger_delete(db, pTrigger);
- } else {
+ else
assert(pParse->pNewTrigger == pTrigger);
- }
^ permalink raw reply [flat|nested] 28+ messages in thread
* [tarantool-patches] [PATCH v2 08/11] box: sort error codes in misc.test
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
` (5 preceding siblings ...)
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 07/11] sql: move sqlite3DeleteTrigger to sql.h Kirill Shcherbatov
@ 2018-06-09 9:32 ` Kirill Shcherbatov
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 09/11] sql: new _trigger space format with space_id Kirill Shcherbatov
` (3 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:32 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, 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 59f168f..6f4028c 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -318,173 +318,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] 28+ messages in thread* [tarantool-patches] [PATCH v2 09/11] sql: new _trigger space format with space_id
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
` (6 preceding siblings ...)
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 08/11] box: sort error codes in misc.test Kirill Shcherbatov
@ 2018-06-09 9:32 ` Kirill Shcherbatov
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 01/11] box: remove migration from alpha 1.8.2 and 1.8.4 Kirill Shcherbatov
` (2 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:32 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
As we would like to lookup triggers by space_id in future
on space deletion to delete assocoated _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 | 6 +++++-
src/box/sql.c | 18 +++++++++++-------
src/box/sql/sqliteInt.h | 2 ++
src/box/sql/trigger.c | 8 +++++---
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 ++++----
9 files changed, 35 insertions(+), 23 deletions(-)
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 11063f70a1ee98e57ef652dbbf90d7a35e64eb6a..f3d047c5badc7c6aacd7ed776382a325d1cdad54 100644
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 18bfaa9..e54b394 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -477,12 +477,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'}}}
+ {{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/sql.c b/src/box/sql.c
index 599cb60..0ba0346 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);
+ assert(tuple_field_count(tuple) == 3);
const char *field = box_tuple_field(tuple, 1);
+ assert(mp_typeof(*field) == MP_UINT);
+ uint32_t space_id = mp_decode_uint(&field);
+ field = box_tuple_field(tuple, 2);
assert(mp_typeof(*field) == MP_MAP);
mp_decode_map(&field);
const char *sql_str = mp_decode_str(&field, &key_len);
@@ -693,16 +696,17 @@ 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 +1257,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 +1312,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, 2);
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 ecbd573..6ca59c4 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3032,6 +3032,8 @@ struct Parse {
*/
struct Trigger {
char *zName; /* The name of the trigger */
+ /** The ID of space the trigger is refer 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 053dadb..a9c686f 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;
@@ -256,7 +257,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,
@@ -276,9 +277,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/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)")
--
2.7.4
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 09/11] sql: new _trigger space format with space_id
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 09/11] sql: new _trigger space format with space_id Kirill Shcherbatov
@ 2018-06-13 18:53 ` Vladislav Shpilevoy
2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 1 reply; 28+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-13 18:53 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov
Thanks for the patch! Almost ok. See 5 minor comments below.
On 09/06/2018 12:32, Kirill Shcherbatov wrote:
> As we would like to lookup triggers by space_id in future
> on space deletion to delete assocoated _trigger tuples we
1. "assocoated" - typo.
> 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 | 6 +++++-
> src/box/sql.c | 18 +++++++++++-------
> src/box/sql/sqliteInt.h | 2 ++
> src/box/sql/trigger.c | 8 +++++---
> 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 ++++----
> 9 files changed, 35 insertions(+), 23 deletions(-)
>
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 18bfaa9..e54b394 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -477,12 +477,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'}}}
> + {{0, 'string'}} }
2. Unnecessary diff. Either do this: {data, data}, or this { data, data }.
Not this: {data, data }. Same below.
> + 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/sql.c b/src/box/sql.c
> index 599cb60..0ba0346 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -693,16 +696,17 @@ 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);
3. Out of 80.
> 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,
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index ecbd573..6ca59c4 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -3032,6 +3032,8 @@ struct Parse {
> */
> struct Trigger {
> char *zName; /* The name of the trigger */
> + /** The ID of space the trigger is refer to. */
4. 'is refer to' is incorrect construction. Maybe did you mean '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 */
5. How about to test space_id is stored in _trigger? I mean test
box.space._trigger:select{} full result. Not two columns only.
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 09/11] sql: new _trigger space format with space_id
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-14 16:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
On 13.06.2018 21:53, Vladislav Shpilevoy wrote:
> Thanks for the patch! Almost ok. See 5 minor comments below.
>
> On 09/06/2018 12:32, Kirill Shcherbatov wrote:
>> As we would like to lookup triggers by space_id in future
>> on space deletion to delete assocoated _trigger tuples we
>
> 1. "assocoated" - typo.
fixed
> 2. Unnecessary diff. Either do this: {data, data}, or this { data, data }.
> Not this: {data, data }. Same below.
_index:insert{_trigger.id, 0, 'primary', 'tree', { unique = true },
- {{0, 'string'}} }
+ {{0, 'string'}}}
log.info("create index secondary on _trigger")
_index:insert{_trigger.id, 1, 'space_id', 'tree', { unique = false },
- {{1, 'unsigned'}} }
+ {{1, 'unsigned'}}}
> 3. Out of 80.
- mp_sizeof_str(trigger_stmt_new_len) + mp_sizeof_uint(space_id);
+ mp_sizeof_str(trigger_stmt_new_len) +
+ mp_sizeof_uint(space_id);
> 4. 'is refer to' is incorrect construction. Maybe did you mean 'refers to'?
- /** The ID of space the trigger is refer to. */
+ /** The ID of space the trigger refers to. */
> 5. How about to test space_id is stored in _trigger? I mean test
> box.space._trigger:select{} full result. Not two columns only.
--- a/test/sql/upgrade.test.lua
+++ b/test/sql/upgrade.test.lua
@@ -25,9 +25,16 @@ 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.id == t2t.id)
And some extra changed required by next message review:
+++ b/src/box/schema_def.h
@@ -217,6 +217,12 @@ enum {
BOX_SPACE_SEQUENCE_FIELD_IS_GENERATED = 2,
};
+enum {
+ BOX_TRIGGER_FIELD_NAME = 0,
+ BOX_TRIGGER_FIELD_SPACE_ID = 1,
+ BOX_TRIGGER_FIELD_OPTS = 2,
+};
+
@@ -1320,7 +1320,7 @@ void tarantoolSqlite3LoadSchema(InitData *init)
ptr = mp_decode_str(&field, &len);
name = strndup(ptr, len);
- field = tuple_field(tuple, 2);
+ field = tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);
@@ -670,10 +670,10 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
return SQL_TARANTOOL_ERROR;
assert(tuple != NULL);
assert(tuple_field_count(tuple) == 3);
- const char *field = box_tuple_field(tuple, 1);
+ 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, 2);
+ field = box_tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);
^ permalink raw reply [flat|nested] 28+ messages in thread
* [tarantool-patches] [PATCH v2 01/11] box: remove migration from alpha 1.8.2 and 1.8.4
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
` (7 preceding siblings ...)
2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 09/11] sql: new _trigger space format with space_id Kirill Shcherbatov
@ 2018-06-09 9:58 ` Kirill Shcherbatov
2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 03/11] sql: fix leak on CREATE TABLE and resolve self ref Kirill Shcherbatov
2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 05/11] box: port schema_find_id to C Kirill Shcherbatov
10 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:58 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
As 1.10 released before 1.8.2 and 1.8.4 we have had
some binary incompatibility between 2.0 and 1.8.2, 1.8.4.
This alphas were dropped.
---
src/box/lua/upgrade.lua | 49 +++-------
test/box/migrate.result | 123 ++++++++++++++++++++++++
test/box/migrate.test.lua | 45 +++++++++
test/box/migrate/1.10/00000000000000000003.snap | Bin 0 -> 1568 bytes
test/box/migrate/migrate.lua | 7 ++
5 files changed, 190 insertions(+), 34 deletions(-)
create mode 100644 test/box/migrate.result
create mode 100644 test/box/migrate.test.lua
create mode 100644 test/box/migrate/1.10/00000000000000000003.snap
create mode 100644 test/box/migrate/migrate.lua
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 8302f4b..18bfaa9 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -74,6 +74,7 @@ end
local function set_system_triggers(val)
box.space._space:run_triggers(val)
box.space._index:run_triggers(val)
+ box.space._trigger:run_triggers(val)
box.space._user:run_triggers(val)
box.space._func:run_triggers(val)
box.space._priv:run_triggers(val)
@@ -465,60 +466,49 @@ local function upgrade_to_1_10_0()
end
--------------------------------------------------------------------------------
--- Tarantool 1.8.2
+-- Tarantool 2.1.0
--------------------------------------------------------------------------------
-local function upgrade_to_1_8_2()
+local function upgrade_to_2_1_0()
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 MAP = setmap({})
+
+ log.info("create space _trigger")
local format = {{name='name', type='string'},
{name='opts', type='map'}}
+ _space:insert{_trigger.id, ADMIN, '_trigger', 'memtx', 0, MAP, format}
- log.info("create space _trigger")
- _space:insert{_trigger.id, ADMIN, '_trigger', 'memtx', 0, setmap({}), {}}
log.info("create index primary on _trigger")
_index:insert{_trigger.id, 0, 'primary', 'tree', { unique = true },
- {{0, 'string'}}}
-
- log.info("alter space _trigger set format")
- _trigger:format(format)
-end
-
---------------------------------------------------------------------------------
--- Tarantool 1.8.4
---------------------------------------------------------------------------------
+ {{0, 'string'}}}
-local function upgrade_to_1_8_4()
- local _space = box.space[box.schema.SPACE_ID]
- local _index = box.space[box.schema.INDEX_ID]
local stat1_ft = {{name='tbl', type='string'},
{name='idx', type='string'},
- {name='stat', type='string'}}
+ {name='stat', type='string'}}
local stat4_ft = {{name='tbl', type='string'},
{name='idx', type='string'},
{name='neq', type='string'},
{name='nlt', type='string'},
{name='ndlt', type='string'},
{name='sample', type='scalar'}}
- local MAP = setmap({})
log.info("create space _sql_stat1")
- _space:insert{box.schema.SQL_STAT1_ID, ADMIN, '_sql_stat1', 'memtx', 0,
- MAP, stat1_ft}
+ _space:insert{box.schema.SQL_STAT1_ID, ADMIN, '_sql_stat1', 'memtx', 0, MAP,
+ stat1_ft}
log.info("create index primary on _sql_stat1")
_index:insert{box.schema.SQL_STAT1_ID, 0, 'primary', 'tree',
{unique = true}, {{0, 'string'}, {1, 'string'}}}
log.info("create space _sql_stat4")
- _space:insert{box.schema.SQL_STAT4_ID, ADMIN, '_sql_stat4', 'memtx', 0,
- MAP, stat4_ft}
+ _space:insert{box.schema.SQL_STAT4_ID, ADMIN, '_sql_stat4', 'memtx', 0, MAP,
+ stat4_ft}
log.info("create index primary on _sql_stat4")
_index:insert{box.schema.SQL_STAT4_ID, 0, 'primary', 'tree',
- {unique = true}, {{0, 'string'}, {1, 'string'},
- {5, 'scalar'}}}
+ {unique = true}, {{0, 'string'}, {1, 'string'}, {5, 'scalar'}}}
-- Nullability wasn't skipable. This was fixed in 1-7.
-- Now, abscent field means NULL, so we can safely set second
@@ -530,14 +520,6 @@ local function upgrade_to_1_8_4()
box.space._schema:format(format)
end
---------------------------------------------------------------------------------
--- Tarantool 2.1.0
---------------------------------------------------------------------------------
-
-local function upgrade_to_2_1_0()
- upgrade_to_1_10_0()
-end
-
local function get_version()
local version = box.space._schema:get{'version'}
if version == nil then
@@ -563,8 +545,7 @@ local function upgrade(options)
local handlers = {
{version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = true},
{version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
- {version = mkversion(1, 8, 2), func = upgrade_to_1_8_2, auto = true},
- {version = mkversion(1, 8, 4), func = upgrade_to_1_8_4, auto = true},
+ {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
{version = mkversion(2, 1, 0), func = upgrade_to_2_1_0, auto = true}
}
diff --git a/test/box/migrate.result b/test/box/migrate.result
new file mode 100644
index 0000000..91209b8
--- /dev/null
+++ b/test/box/migrate.result
@@ -0,0 +1,123 @@
+test_run = require('test_run').new()
+---
+...
+work_dir = 'box/migrate/1.10/'
+---
+...
+test_run:cmd('create server migrate with script="box/migrate/migrate.lua", workdir="' .. work_dir .. '"')
+---
+- true
+...
+test_run:cmd('start server migrate')
+---
+- true
+...
+test_run:switch('migrate')
+---
+- true
+...
+-- test system tables
+box.space._space.index['name']:get('_trigger')
+---
+- [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'opts',
+ 'type': 'map'}]]
+...
+box.space._space.index['name']:get('_sql_stat1')
+---
+- [348, 1, '_sql_stat1', 'memtx', 0, {}, [{'name': 'tbl', 'type': 'string'}, {'name': 'idx',
+ 'type': 'string'}, {'name': 'stat', 'type': 'string'}]]
+...
+box.space._space.index['name']:get('_sql_stat4')
+---
+- [349, 1, '_sql_stat4', 'memtx', 0, {}, [{'name': 'tbl', 'type': 'string'}, {'name': 'idx',
+ 'type': 'string'}, {'name': 'neq', 'type': 'string'}, {'name': 'nlt', 'type': 'string'},
+ {'name': 'ndlt', 'type': 'string'}, {'name': 'sample', 'type': 'scalar'}]]
+...
+box.space._index:get({box.space._space.index['name']:get('_trigger').id, 0})
+---
+- [328, 0, 'primary', 'tree', {'unique': true}, [[0, 'string']]]
+...
+box.space._index:get({box.space._space.index['name']:get('_sql_stat1').id, 0})
+---
+- [348, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'string']]]
+...
+box.space._index:get({box.space._space.index['name']:get('_sql_stat4').id, 0})
+---
+- [349, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'string'], [5,
+ 'scalar']]]
+...
+box.space._schema:format()
+---
+- [{'type': 'string', 'name': 'key'}, {'type': 'any', 'name': 'value', 'is_nullable': true}]
+...
+-- test data migration
+box.space._space.index['name']:get('T1')
+---
+- [512, 1, 'T1', 'memtx', 0, {}, [{'name': 'x', 'type': 'unsigned'}]]
+...
+box.space._index:get({box.space._space.index['name']:get('T1').id, 0})
+---
+- [512, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
+...
+-- test system tables functionality
+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.space._space.index['name']:get('T')
+---
+- [513, 1, 'T', 'memtx', 1, {'sql': 'CREATE TABLE t(x INTEGER PRIMARY KEY)'}, [{'type': 'integer',
+ 'nullable_action': 'abort', 'name': 'X', 'is_nullable': false}]]
+...
+box.space._space.index['name']:get('T_OUT')
+---
+- [514, 1, 'T_OUT', 'memtx', 1, {'sql': 'CREATE TABLE t_out(x INTEGER PRIMARY KEY)'},
+ [{'type': 'integer', 'nullable_action': 'abort', 'name': 'X', 'is_nullable': false}]]
+...
+box.space._trigger:get('T1T')
+---
+- ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1);
+ END;'}]
+...
+box.sql.execute("INSERT INTO T VALUES(1);")
+---
+...
+box.space.T:select()
+---
+- - [1]
+...
+box.space.T_OUT:select()
+---
+- - [1]
+...
+box.sql.execute("SELECT * FROM T")
+---
+- - [1]
+...
+box.sql.execute("SELECT * FROM T")
+---
+- - [1]
+...
+box.sql.execute("DROP TABLE T;")
+---
+...
+box.sql.execute("DROP TABLE T_OUT;")
+---
+...
+test_run:switch('default')
+---
+- true
+...
+test_run:cmd('stop server migrate')
+---
+- true
+...
+test_run:cmd('cleanup server migrate')
+---
+- true
+...
diff --git a/test/box/migrate.test.lua b/test/box/migrate.test.lua
new file mode 100644
index 0000000..40f5ce8
--- /dev/null
+++ b/test/box/migrate.test.lua
@@ -0,0 +1,45 @@
+test_run = require('test_run').new()
+
+work_dir = 'box/migrate/1.10/'
+test_run:cmd('create server migrate with script="box/migrate/migrate.lua", workdir="' .. work_dir .. '"')
+test_run:cmd('start server migrate')
+
+test_run:switch('migrate')
+
+-- test system tables
+box.space._space.index['name']:get('_trigger')
+box.space._space.index['name']:get('_sql_stat1')
+box.space._space.index['name']:get('_sql_stat4')
+
+box.space._index:get({box.space._space.index['name']:get('_trigger').id, 0})
+box.space._index:get({box.space._space.index['name']:get('_sql_stat1').id, 0})
+box.space._index:get({box.space._space.index['name']:get('_sql_stat4').id, 0})
+
+box.space._schema:format()
+
+-- test data migration
+box.space._space.index['name']:get('T1')
+box.space._index:get({box.space._space.index['name']:get('T1').id, 0})
+
+-- test system tables functionality
+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.space._space.index['name']:get('T')
+box.space._space.index['name']:get('T_OUT')
+box.space._trigger:get('T1T')
+
+box.sql.execute("INSERT INTO T VALUES(1);")
+box.space.T:select()
+box.space.T_OUT:select()
+box.sql.execute("SELECT * FROM T")
+box.sql.execute("SELECT * FROM T")
+
+
+box.sql.execute("DROP TABLE T;")
+box.sql.execute("DROP TABLE T_OUT;")
+
+
+test_run:switch('default')
+test_run:cmd('stop server migrate')
+test_run:cmd('cleanup server migrate')
diff --git a/test/box/migrate/1.10/00000000000000000003.snap b/test/box/migrate/1.10/00000000000000000003.snap
new file mode 100644
index 0000000000000000000000000000000000000000..728d12c9157092b3cc31b2f7ed32c5219bc48d0b
diff --git a/test/box/migrate/migrate.lua b/test/box/migrate/migrate.lua
new file mode 100644
index 0000000..2853492
--- /dev/null
+++ b/test/box/migrate/migrate.lua
@@ -0,0 +1,7 @@
+#!/usr/bin/env tarantool
+
+box.cfg{
+ listen = os.getenv("LISTEN"),
+}
+
+require('console').listen(os.getenv('ADMIN'))
--
2.7.4
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] [PATCH v2 03/11] sql: fix leak on CREATE TABLE and resolve self ref
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
` (8 preceding siblings ...)
2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 01/11] box: remove migration from alpha 1.8.2 and 1.8.4 Kirill Shcherbatov
@ 2018-06-09 9:58 ` Kirill Shcherbatov
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 05/11] box: port schema_find_id to C Kirill Shcherbatov
10 siblings, 1 reply; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:58 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
---
src/box/sql.c | 8 ++++++--
src/box/sql/build.c | 14 +++++---------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index 93fca68..72fd5cc 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1860,8 +1860,12 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
sql_parser_destroy(&parser);
if (parser.rc != SQLITE_OK) {
/* Tarantool error may be already set with diag. */
- if (parser.rc != SQL_TARANTOOL_ERROR)
- diag_set(ClientError, ER_SQL, parser.zErrMsg);
+ if (parser.rc != SQL_TARANTOOL_ERROR) {
+ char *error = tt_static_buf();
+ snprintf(error, TT_STATIC_BUF_LEN, "%s", parser.zErrMsg);
+ diag_set(ClientError, ER_SQL, error);
+ sqlite3DbFree(db, parser.zErrMsg);
+ }
return -1;
}
return 0;
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 28e4d7a..3c3c900 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -565,7 +565,7 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
*/
if (!pParse->nested) {
if ((v = sqlite3GetVdbe(pParse)) == NULL)
- goto begin_table_error;
+ goto cleanup;
sqlite3VdbeCountChanges(v);
}
@@ -575,7 +575,7 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
if (zName == 0)
return;
if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
- goto begin_table_error;
+ goto cleanup;
assert(db->pSchema != NULL);
pTable = sqlite3HashFind(&db->pSchema->tblHash, zName);
@@ -587,12 +587,12 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
} else {
assert(!db->init.busy || CORRUPT_DB);
}
- goto begin_table_error;
+ goto cleanup;
}
pTable = sql_table_new(pParse, zName);
if (pTable == NULL)
- goto begin_table_error;
+ goto cleanup;
assert(pParse->pNewTable == 0);
pParse->pNewTable = pTable;
@@ -608,11 +608,7 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
if (!db->init.busy && (v = sqlite3GetVdbe(pParse)) != 0)
sql_set_multi_write(pParse, true);
- /* Normal (non-error) return. */
- return;
-
- /* If an error occurs, we jump here */
- begin_table_error:
+ cleanup:
sqlite3DbFree(db, zName);
return;
}
--
2.7.4
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 03/11] sql: fix leak on CREATE TABLE and resolve self ref
2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 03/11] sql: fix leak on CREATE TABLE and resolve self ref Kirill Shcherbatov
@ 2018-06-13 18:53 ` Vladislav Shpilevoy
2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 1 reply; 28+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-13 18:53 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
Thanks for the patch! See 1 comment below.
On 09/06/2018 12:32, Kirill Shcherbatov wrote:
> ---
> src/box/sql.c | 8 ++++++--
> src/box/sql/build.c | 14 +++++---------
> 2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 93fca68..72fd5cc 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1860,8 +1860,12 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
> sql_parser_destroy(&parser);
> if (parser.rc != SQLITE_OK) {
> /* Tarantool error may be already set with diag. */
> - if (parser.rc != SQL_TARANTOOL_ERROR)
> - diag_set(ClientError, ER_SQL, parser.zErrMsg);
> + if (parser.rc != SQL_TARANTOOL_ERROR) {
> + char *error = tt_static_buf();
> + snprintf(error, TT_STATIC_BUF_LEN, "%s", parser.zErrMsg);
> + diag_set(ClientError, ER_SQL, error);
Why do you need snprintf("%s", str)? It is the same as just 'str'.
diag_set(ClientError, ER_SQL, parser.zErrMsg);
is enough to set an error. If zErrMsg was not terminated by zero, snprintf("%s")
would not work as well. But zErrMsg is terminated.
And why do you destroy zErrMsg here? Why not in sql_parser_destroy()?
zErrMsg is struct Parse member.
> + sqlite3DbFree(db, parser.zErrMsg);
> + }
> return -1;
> }
> return 0;
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 03/11] sql: fix leak on CREATE TABLE and resolve self ref
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-14 16:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
> Why do you need snprintf("%s", str)? It is the same as just 'str'.
> is enough to set an error. If zErrMsg was not terminated by zero, snprintf("%s")
> would not work as well. But zErrMsg is terminated.
I wasn't sure that this diag_set build print string object. It doesn't matter. I've fixed this.
> And why do you destroy zErrMsg here? Why not in sql_parser_destroy()?
> zErrMsg is struct Parse member.
Ok, done.
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -453,5 +453,6 @@ sql_parser_destroy(Parse *parser)
db->lookaside.bDisable -= parser->disableLookaside;
}
parser->disableLookaside = 0;
+ sqlite3DbFree(db, parser->zErrMsg);
region_destroy(&parser->region);
}
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -815,6 +815,7 @@ transferParseError(Parse * pTo, Parse * pFrom)
} else {
sqlite3DbFree(pFrom->db, pFrom->zErrMsg);
}
+ pFrom->zErrMsg = NULL;
}
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1856,13 +1856,13 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, NULL,
expr_list);
-
- sql_parser_destroy(&parser);
+ int rc = 0;
if (parser.rc != SQLITE_OK) {
/* Tarantool error may be already set with diag. */
if (parser.rc != SQL_TARANTOOL_ERROR)
diag_set(ClientError, ER_SQL, parser.zErrMsg);
- return -1;
+ rc = -1;
}
- return 0;
+ sql_parser_destroy(&parser);
+ return rc;
}
^ permalink raw reply [flat|nested] 28+ messages in thread
* [tarantool-patches] [PATCH v2 05/11] box: port schema_find_id to C
2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
` (9 preceding siblings ...)
2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 03/11] sql: fix leak on CREATE TABLE and resolve self ref Kirill Shcherbatov
@ 2018-06-09 9:58 ` Kirill Shcherbatov
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
10 siblings, 1 reply; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-09 9:58 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
Part of #3273.
---
src/box/schema.cc | 54 ++++++++++++++++++++++++++++++++++++++----------------
src/box/schema.h | 23 ++++++++++++++++-------
src/box/user.cc | 4 +++-
3 files changed, 57 insertions(+), 24 deletions(-)
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 2ddf920..5d32e61 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -222,30 +222,52 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def,
trigger_run_xc(&on_alter_space, space);
}
-uint32_t
+int
schema_find_id(uint32_t system_space_id, uint32_t index_id,
- const char *name, uint32_t len)
+ const char *name, uint32_t len, uint32_t *object_id)
{
- if (len > BOX_NAME_MAX)
- return BOX_ID_NIL;
- struct space *space = space_cache_find_xc(system_space_id);
- struct index *index = index_find_system_xc(space, index_id);
+ if (len > BOX_NAME_MAX) {
+ diag_set(SystemError,
+ "name length %d is greater than BOX_NAME_MAX", len);
+ return -1;
+ }
+ struct space *space = space_cache_find(system_space_id);
+ if (space == NULL)
+ return -1;
+ if (!space_is_memtx(space)) {
+ diag_set(ClientError, ER_UNSUPPORTED,
+ space->engine->name, "system data");
+ return -1;
+ }
+ struct index *index = index_find(space, index_id);
+ if (index == NULL)
+ return -1;
uint32_t size = mp_sizeof_str(len);
struct region *region = &fiber()->gc;
uint32_t used = region_used(region);
- char *key = (char *) region_alloc_xc(region, size);
- auto guard = make_scoped_guard([=] { region_truncate(region, used); });
+ char *key = (char *)region_alloc(region, size);
+ if (key == NULL) {
+ diag_set(OutOfMemory, size, "region", "key");
+ return -1;
+ }
mp_encode_str(key, name, len);
-
- struct iterator *it = index_create_iterator_xc(index, ITER_EQ, key, 1);
- IteratorGuard iter_guard(it);
-
- struct tuple *tuple = iterator_next_xc(it);
- if (tuple) {
+ struct iterator *it = index_create_iterator(index, ITER_EQ, key, 1);
+ if (it == NULL) {
+ region_truncate(region, used);
+ return -1;
+ }
+ struct tuple *tuple;
+ int rc = iterator_next(it, &tuple);
+ if (rc == 0) {
/* id is always field #1 */
- return tuple_field_u32_xc(tuple, 0);
+ if (tuple == NULL)
+ *object_id = BOX_ID_NIL;
+ else if (tuple_field_u32(tuple, 0, object_id) != 0)
+ return -1;
}
- return BOX_ID_NIL;
+ iterator_delete(it);
+ region_truncate(region, used);
+ return rc;
}
/**
diff --git a/src/box/schema.h b/src/box/schema.h
index 1f7414f..cd9bb5b 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -102,6 +102,22 @@ space_is_system(struct space *space);
struct sequence *
sequence_by_id(uint32_t id);
+/**
+ * Find space id by name in specified system space with index.
+ *
+ * @param system_space_id identifier of the system space.
+ * @param index_id identifier of the index to lookup.
+ * @param name of object to lookup.
+ * @param len length of a name.
+ * @param object_id[out] object_id or BOX_ID_NIL - not found.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+schema_find_id(uint32_t system_space_id, uint32_t index_id,
+ const char *name, uint32_t len, uint32_t *object_id);
+
#if defined(__cplusplus)
} /* extern "C" */
@@ -134,13 +150,6 @@ schema_free();
struct space *schema_space(uint32_t id);
-/*
- * Find object id by object name.
- */
-uint32_t
-schema_find_id(uint32_t system_space_id, uint32_t index_id,
- const char *name, uint32_t len);
-
/**
* Insert a new function or update the old one.
*
diff --git a/src/box/user.cc b/src/box/user.cc
index 7fa66da..3e7f466 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -450,7 +450,9 @@ user_find(uint32_t uid)
struct user *
user_find_by_name(const char *name, uint32_t len)
{
- uint32_t uid = schema_find_id(BOX_USER_ID, 2, name, len);
+ uint32_t uid;
+ if (schema_find_id(BOX_USER_ID, 2, name, len, &uid) != 0)
+ diag_raise();
struct user *user = user_by_id(uid);
if (user == NULL || user->def->type != SC_USER) {
diag_set(ClientError, ER_NO_SUCH_USER, tt_cstr(name, len));
--
2.7.4
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 05/11] box: port schema_find_id to C
2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 05/11] box: port schema_find_id to C Kirill Shcherbatov
@ 2018-06-13 18:53 ` Vladislav Shpilevoy
2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 1 reply; 28+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-13 18:53 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
Thanks for the patch! See 3 comments below.
On 09/06/2018 12:32, Kirill Shcherbatov wrote:
> Part of #3273.
> ---
> src/box/schema.cc | 54 ++++++++++++++++++++++++++++++++++++++----------------
> src/box/schema.h | 23 ++++++++++++++++-------
> src/box/user.cc | 4 +++-
> 3 files changed, 57 insertions(+), 24 deletions(-)
>
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 2ddf920..5d32e61 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -222,30 +222,52 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def,
> trigger_run_xc(&on_alter_space, space);
> }
>
> -uint32_t
> +int
> schema_find_id(uint32_t system_space_id, uint32_t index_id,
> - const char *name, uint32_t len)
> + const char *name, uint32_t len, uint32_t *object_id)
> {
> + if (rc == 0) {
> /* id is always field #1 */
> - return tuple_field_u32_xc(tuple, 0);
> + if (tuple == NULL)
> + *object_id = BOX_ID_NIL;
> + else if (tuple_field_u32(tuple, 0, object_id) != 0)
> + return -1;
1. You should not apply my diff as is with no inspection. Usually
I do draft fixes in review. Here is the bug - on return -1
the iterator and region leaks. This time apply this:
else if (tuple_field_u32(tuple, 0, object_id) != 0)
- return -1;
+ rc = -1;
}
> diff --git a/src/box/schema.h b/src/box/schema.h
> index 1f7414f..cd9bb5b 100644
> --- a/src/box/schema.h
> +++ b/src/box/schema.h
> @@ -102,6 +102,22 @@ space_is_system(struct space *space);
> struct sequence *
> sequence_by_id(uint32_t id);
>
> +/**
> + * Find space id by name in specified system space with index.
2. Same as on the previous review: this function searches not only
space id, but a system object id.
> + *
> + * @param system_space_id identifier of the system space.
> + * @param index_id identifier of the index to lookup.
> + * @param name of object to lookup.
> + * @param len length of a name.
> + * @param object_id[out] object_id or BOX_ID_NIL - not found.
> + *
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +int
> +schema_find_id(uint32_t system_space_id, uint32_t index_id,
> + const char *name, uint32_t len, uint32_t *object_id);
3. Invalid alignment.
> +
> #if defined(__cplusplus)
> } /* extern "C" */
>
^ permalink raw reply [flat|nested] 28+ messages in thread* [tarantool-patches] Re: [PATCH v2 05/11] box: port schema_find_id to C
2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-14 16:12 ` Kirill Shcherbatov
0 siblings, 0 replies; 28+ messages in thread
From: Kirill Shcherbatov @ 2018-06-14 16:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
> 1. You should not apply my diff as is with no inspection. Usually
> I do draft fixes in review. Here is the bug - on return -1
> the iterator and region leaks. This time apply this:
- return -1;
+ rc = -1;
> 2. Same as on the previous review: this function searches not only
> space id, but a system object id.
/**
- * Find space id by name in specified system space with index.
+ * Find object id by name in specified system space with index.
*
- * @param system_space_id identifier of the system space.
+ * @param system_space_id identifier of the system object.
* @param index_id identifier of the index to lookup.
* @param name of object to lookup.
* @param len length of a name.
> 3. Invalid alignment@@ -115,8 +115,8 @@ sequence_by_id(uint32_t id);
* @retval -1 on error.
*/
int
-schema_find_id(uint32_t system_space_id, uint32_t index_id,
- const char *name, uint32_t len, uint32_t *object_id);
+schema_find_id(uint32_t system_space_id, uint32_t index_id, const char *name,
+ uint32_t len, uint32_t *object_id);
^ permalink raw reply [flat|nested] 28+ messages in thread