* [tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server @ 2018-05-31 11:22 Kirill Shcherbatov 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 1/4] box: move db->pShchema init to sql_init Kirill Shcherbatov ` (6 more replies) 0 siblings, 7 replies; 23+ messages in thread From: Kirill Shcherbatov @ 2018-05-31 11:22 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3273-no-sql-triggers Issue: https://github.com/tarantool/tarantool/issues/3273 As we are going to call parser on box.cfg() to recreate triggers from SQL, we should initialize Schema as it used in sqlite3BeginTrigger. Inroduced box_space_id_by_name to get object id by name from custom space. Fixed bug in tarantoolSqlite3RenameTrigger: sql request have had invalid length in MsgPack after ALTER TABLE NAME. Introduced sql_triggers field in space structure. Changed parser logic to do not insert builded triggers, just only to do parsing. All triggers insertions and deletions are operated via on_replace_dd_trigger now. Kirill Shcherbatov (4): box: move db->pShchema init to sql_init sql: fix sql len in tarantoolSqlite3RenameTrigger box: introduce box_space_id_by_name sql: move Triggers to server src/box/alter.cc | 57 +++++++++++ src/box/box.cc | 10 +- src/box/box.h | 14 +++ src/box/space.h | 2 + src/box/sql.c | 60 ++++------- src/box/sql.h | 62 ++++++++++++ src/box/sql/build.c | 8 +- src/box/sql/insert.c | 6 +- src/box/sql/sqliteInt.h | 2 - src/box/sql/tokenize.c | 43 +++++++- src/box/sql/trigger.c | 258 ++++++++++++++++++++++++++++-------------------- src/box/sql/vdbe.c | 58 +++-------- src/box/sql/vdbe.h | 1 - src/box/sql/vdbeaux.c | 9 -- 14 files changed, 374 insertions(+), 216 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] [PATCH v1 1/4] box: move db->pShchema init to sql_init 2018-05-31 11:22 [tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server Kirill Shcherbatov @ 2018-05-31 11:22 ` Kirill Shcherbatov 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 2/4] sql: fix sql len in tarantoolSqlite3RenameTrigger Kirill Shcherbatov ` (5 subsequent siblings) 6 siblings, 1 reply; 23+ messages in thread From: Kirill Shcherbatov @ 2018-05-31 11:22 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 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/box/sql.c b/src/box/sql.c index 7379cb4..47d1739 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -77,11 +77,18 @@ sql_init() panic("failed to initialize SQL subsystem"); assert(db != NULL); + /* Initialize pSchema to use SQL parser. */ + 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 +96,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] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/4] box: move db->pShchema init to sql_init 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 1/4] box: move db->pShchema init to sql_init Kirill Shcherbatov @ 2018-05-31 17:36 ` Vladislav Shpilevoy 2018-06-01 20:24 ` Kirill Shcherbatov 0 siblings, 1 reply; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-05-31 17:36 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches Thanks for the patch! On 31/05/2018 14:22, Kirill Shcherbatov wrote: > 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 | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/src/box/sql.c b/src/box/sql.c > index 7379cb4..47d1739 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -77,11 +77,18 @@ sql_init() > panic("failed to initialize SQL subsystem"); > > assert(db != NULL); > + /* Initialize pSchema to use SQL parser. */ To use for what? Before what? I see it in the commit message, but lets explain this in the comment as well. > + db->pSchema = sqlite3SchemaCreate(db); > + if (db->pSchema == NULL) { > + sqlite3_close(db); > + panic("failed to initialize SQL Schema subsystem"); > + } > } > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/4] box: move db->pShchema init to sql_init 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-06-01 20:24 ` Kirill Shcherbatov 0 siblings, 0 replies; 23+ messages in thread From: Kirill Shcherbatov @ 2018-06-01 20:24 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy > To use for what? Before what? I see it in the commit message, but lets > explain this in the comment as well. - /* Initialize pSchema to use SQL parser. */ + /* + * Initialize pSchema to use SQL parser on initialization: + * e.g. Trigger objects (compiled from SQL on tuple + * insertion in _trigger) need to refer it. + */ ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] [PATCH v1 2/4] sql: fix sql len in tarantoolSqlite3RenameTrigger 2018-05-31 11:22 [tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server Kirill Shcherbatov 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 1/4] box: move db->pShchema init to sql_init Kirill Shcherbatov @ 2018-05-31 11:22 ` Kirill Shcherbatov 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 3/4] box: introduce box_space_id_by_name Kirill Shcherbatov ` (4 subsequent siblings) 6 siblings, 0 replies; 23+ messages in thread From: Kirill Shcherbatov @ 2018-05-31 11:22 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 47d1739..eacb288 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -686,8 +686,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] 23+ messages in thread
* [tarantool-patches] [PATCH v1 3/4] box: introduce box_space_id_by_name 2018-05-31 11:22 [tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server Kirill Shcherbatov 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 1/4] box: move db->pShchema init to sql_init Kirill Shcherbatov 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 2/4] sql: fix sql len in tarantoolSqlite3RenameTrigger Kirill Shcherbatov @ 2018-05-31 11:22 ` Kirill Shcherbatov 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 4/4] sql: move Triggers to server Kirill Shcherbatov ` (3 subsequent siblings) 6 siblings, 1 reply; 23+ messages in thread From: Kirill Shcherbatov @ 2018-05-31 11:22 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov Part of #3273. --- src/box/box.cc | 10 ++++++++-- src/box/box.h | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/box/box.cc b/src/box/box.cc index 02722d6..9540d85 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -877,7 +877,7 @@ box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple) /* schema_find_id()-like method using only public API */ uint32_t -box_space_id_by_name(const char *name, uint32_t len) +space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len) { if (len > BOX_NAME_MAX) return BOX_ID_NIL; @@ -892,7 +892,7 @@ box_space_id_by_name(const char *name, uint32_t len) /* NOTE: error and missing key cases are indistinguishable */ box_tuple_t *tuple; - if (box_index_get(BOX_VSPACE_ID, 2, begin, end, &tuple) != 0) + if (box_index_get(system_space_id, 2, begin, end, &tuple) != 0) return BOX_ID_NIL; if (tuple == NULL) return BOX_ID_NIL; @@ -902,6 +902,12 @@ box_space_id_by_name(const char *name, uint32_t len) } uint32_t +box_space_id_by_name(const char *name, uint32_t len) +{ + return space_id_by_name(BOX_VSPACE_ID, name, len); +} + +uint32_t box_index_id_by_name(uint32_t space_id, const char *name, uint32_t len) { if (len > BOX_NAME_MAX) diff --git a/src/box/box.h b/src/box/box.h index bdd5d5c..ba33113 100644 --- a/src/box/box.h +++ b/src/box/box.h @@ -220,6 +220,20 @@ API_EXPORT int box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple); /** + * Find space id by name in specified system space. + * + * This function performs SELECT request to _vspace system space. + * \param system_space_id space to lookup name. + * \param name space name + * \param len length of \a name + * \retval BOX_ID_NIL on error or if not found (check box_error_last()) + * \retval space_id otherwise + * \sa box_index_id_by_name + */ +uint32_t +space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len); + +/** * Find space id by name. * * This function performs SELECT request to _vspace system space. -- 2.7.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 3/4] box: introduce box_space_id_by_name Kirill Shcherbatov @ 2018-05-31 17:36 ` Vladislav Shpilevoy 2018-06-01 20:24 ` Kirill Shcherbatov 0 siblings, 1 reply; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-05-31 17:36 UTC (permalink / raw) To: tarantool-patches, Kirill Shcherbatov Thanks for the patch! See 5 comments below. On 31/05/2018 14:22, Kirill Shcherbatov wrote: > Part of #3273. > --- > src/box/box.cc | 10 ++++++++-- > src/box/box.h | 14 ++++++++++++++ > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/src/box/box.cc b/src/box/box.cc > index 02722d6..9540d85 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -877,7 +877,7 @@ box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple) > > /* schema_find_id()-like method using only public API */ > uint32_t > -box_space_id_by_name(const char *name, uint32_t len) > +space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len) 1. The comment now is wrong - space_id_by_name will be used exactly for non-public API. 2. space_id_by_name must not take system_space_id as an argument. It must always use BOX_SPACE_ID. Do this: /** * ... * * Return 0 on success, -1 on error. * @param[out] space_id BOX_ID_NIL - not found. Else space_id is * saved. * * ... */ static inline int space_id_by_name_impl(system_space_id, name, len, uint32_t *space_id); uint32_t space_id_by_name(name, len) { return space_id_by_name_impl(BOX_SPACE_ID, name, len); } uint32_t box_space_id_by_name(name, len) { return space_id_by_name_impl(BOX_VSPACE_ID, name, len); } > { > if (len > BOX_NAME_MAX) > return BOX_ID_NIL; > diff --git a/src/box/box.h b/src/box/box.h > index bdd5d5c..ba33113 100644 > --- a/src/box/box.h > +++ b/src/box/box.h > @@ -220,6 +220,20 @@ API_EXPORT int > box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple); > > /** > + * Find space id by name in specified system space. > + * > + * This function performs SELECT request to _vspace system space. 3. Not _vspace. > + * \param system_space_id space to lookup name. > + * \param name space name > + * \param len length of \a name > + * \retval BOX_ID_NIL on error or if not found (check box_error_last()) > + * \retval space_id otherwise > + * \sa box_index_id_by_name > + */ > +uint32_t > +space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len); 4. space_id_by_name is not public function. You must not put it inside /** \cond public */. It must be in schema.h like other <something_by_something> functions. 5. Why can not you just port schema_find_id to C? > + > +/** > * Find space id by name. > * > * This function performs SELECT request to _vspace system space. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-06-01 20:24 ` Kirill Shcherbatov 2018-06-04 13:27 ` Vladislav Shpilevoy 0 siblings, 1 reply; 23+ messages in thread From: Kirill Shcherbatov @ 2018-06-01 20:24 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy > 1. The comment now is wrong - space_id_by_name will be used exactly for > non-public API. > 2. space_id_by_name must not take system_space_id as an argument. It must > always use BOX_SPACE_ID. > * Return 0 on success, -1 on error. > * @param[out] space_id BOX_ID_NIL - not found. Else space_id is > * saved. > 3. Not _vspace. > 4. space_id_by_name is not public function. You must not put it inside > /** \cond public */. > 5. Why can not you just port schema_find_id to C? I've ported schema_find_id to C. This required change it's signature. diff --git a/src/box/schema.cc b/src/box/schema.cc index 2ddf920..c5055ee 100644 --- a/src/box/schema.cc +++ b/src/box/schema.cc @@ -224,28 +224,53 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def, uint32_t 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 *space_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); + *space_id = BOX_ID_NIL; + 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", "new slab"); + 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); + struct iterator *it = index_create_iterator(index, ITER_EQ, key, 1); + if (it == NULL) { + region_truncate(region, used); + return -1; + } + int rc = 0; + struct tuple *tuple; + if (iterator_next(it, &tuple) != 0) { + rc = -1; + goto cleanup; + } if (tuple) { /* id is always field #1 */ - return tuple_field_u32_xc(tuple, 0); + *space_id = tuple_field_u32_xc(tuple, 0); } - return BOX_ID_NIL; +cleanup: + iterator_delete(it); + region_truncate(region, used); + return rc; } /** diff --git a/src/box/schema.h b/src/box/schema.h index 1f7414f..099f9b0 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 space to lookup name. + * @param len length of a name. + * @param space_id[out] space_id BOX_ID_NIL - not found. + * + * @retval 0 on success. + * @retval -1 on error. + */ +uint32_t +schema_find_id(uint32_t system_space_id, uint32_t index_id, + const char *name, uint32_t len, uint32_t *space_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)); ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name 2018-06-01 20:24 ` Kirill Shcherbatov @ 2018-06-04 13:27 ` Vladislav Shpilevoy 2018-06-04 19:21 ` Kirill Shcherbatov 0 siblings, 1 reply; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-06-04 13:27 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches Hello. Thanks for the patch! Please, put the whole new patch at the end of a letter after multiple '======'. It helps in review, thanks. See 6 comments below. 1. Lets change commit title: the patch does not introduce box_space_id_by_name. It was before the patch. On 01/06/2018 23:24, Kirill Shcherbatov wrote: >> 1. The comment now is wrong - space_id_by_name will be used exactly for >> non-public API. >> 2. space_id_by_name must not take system_space_id as an argument. It must >> always use BOX_SPACE_ID. >> * Return 0 on success, -1 on error. >> * @param[out] space_id BOX_ID_NIL - not found. Else space_id is >> * saved. >> 3. Not _vspace. >> 4. space_id_by_name is not public function. You must not put it inside >> /** \cond public */. >> 5. Why can not you just port schema_find_id to C? > I've ported schema_find_id to C. This required change it's signature. > > diff --git a/src/box/schema.cc b/src/box/schema.cc > index 2ddf920..c5055ee 100644 > --- a/src/box/schema.cc > +++ b/src/box/schema.cc > @@ -224,28 +224,53 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def, > > uint32_t > 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 *space_id) 2. How can you return -1, when the return type is unsigned? 3. It is not 'space_id' speaking in general. This function is used in user.cc also to search user id by the name. Please, find another name for the parameter. > { > - 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); > + *space_id = BOX_ID_NIL; > + 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", "new slab"); 4. "new slab" -> "key". OutOfMemory takes size, alloc function and the result variable name. > + 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); > + struct iterator *it = index_create_iterator(index, ITER_EQ, key, 1); > + if (it == NULL) { > + region_truncate(region, used); > + return -1; > + } > + int rc = 0; > + struct tuple *tuple; > + if (iterator_next(it, &tuple) != 0) { > + rc = -1; > + goto cleanup; > + } > if (tuple) { > /* id is always field #1 */ > - return tuple_field_u32_xc(tuple, 0); > + *space_id = tuple_field_u32_xc(tuple, 0); > } > - return BOX_ID_NIL; > +cleanup: > + iterator_delete(it); > + region_truncate(region, used); > + return rc; 5. You should avoid 'goto cleanup' here. diff --git a/src/box/schema.cc b/src/box/schema.cc index c5055eefe..a08588aef 100644 --- a/src/box/schema.cc +++ b/src/box/schema.cc @@ -257,17 +257,12 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id, region_truncate(region, used); return -1; } - int rc = 0; struct tuple *tuple; - if (iterator_next(it, &tuple) != 0) { - rc = -1; - goto cleanup; - } - if (tuple) { - /* id is always field #1 */ + int rc = iterator_next(it, &tuple); + if (rc == 0 && tuple != NULL) { + /* ID is always field #1. */ *space_id = tuple_field_u32_xc(tuple, 0); } -cleanup: iterator_delete(it); region_truncate(region, used); return rc; > } > diff --git a/src/box/schema.h b/src/box/schema.h > index 1f7414f..099f9b0 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 space to lookup name. > + * @param len length of a name. > + * @param space_id[out] space_id BOX_ID_NIL - not found. 6. Same. Do not mention space. It is a function to find ID of any system object. Not only space. This function is applicable for _index, _space, _user, _func. > + * > + * @retval 0 on success. > + * @retval -1 on error. > + */ > +uint32_t > +schema_find_id(uint32_t system_space_id, uint32_t index_id, > + const char *name, uint32_t len, uint32_t *space_id); > + > #if defined(__cplusplus) > } /* extern "C" */ ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name 2018-06-04 13:27 ` Vladislav Shpilevoy @ 2018-06-04 19:21 ` Kirill Shcherbatov 2018-06-05 13:31 ` Vladislav Shpilevoy 0 siblings, 1 reply; 23+ messages in thread From: Kirill Shcherbatov @ 2018-06-04 19:21 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy > 1. Lets change commit title: the patch does not introduce > box_space_id_by_name. It was before the patch. box: port schema_find_id to C Part of #3273. > 2. How can you return -1, when the return type is unsigned? > 3. It is not 'space_id' speaking in general. This function is used in > user.cc also to search user id by the name. Please, find another name > for the parameter. > 4. "new slab" -> "key". OutOfMemory takes size, alloc function and > the result variable name. > 5. You should avoid 'goto cleanup' here. diff --git a/src/box/schema.cc b/src/box/schema.cc index c5055ee..a85a09f 100644 --- a/src/box/schema.cc +++ b/src/box/schema.cc @@ -222,11 +222,11 @@ 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, uint32_t *space_id) + const char *name, uint32_t len, uint32_t *object_id) { - *space_id = BOX_ID_NIL; + *object_id = BOX_ID_NIL; if (len > BOX_NAME_MAX) { diag_set(SystemError, "name length %d is greater than BOX_NAME_MAX", len); @@ -248,7 +248,7 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id, uint32_t used = region_used(region); char *key = (char *)region_alloc(region, size); if (key == NULL) { - diag_set(OutOfMemory, size, "region", "new slab"); + diag_set(OutOfMemory, size, "region", "key"); return -1; } mp_encode_str(key, name, len); @@ -261,13 +261,10 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id, struct tuple *tuple; if (iterator_next(it, &tuple) != 0) { rc = -1; - goto cleanup; - } - if (tuple) { + } else if (tuple) { /* id is always field #1 */ - *space_id = tuple_field_u32_xc(tuple, 0); + *object_id = tuple_field_u32_xc(tuple, 0); } -cleanup: iterator_delete(it); region_truncate(region, used); return rc; > 6. Same. Do not mention space. It is a function to find ID of > any system object. Not only space. This function is applicable > for _index, _space, _user, _func. diff --git a/src/box/schema.h b/src/box/schema.h index 099f9b0..cd9bb5b 100644 --- a/src/box/schema.h +++ b/src/box/schema.h @@ -107,16 +107,16 @@ sequence_by_id(uint32_t id); * * @param system_space_id identifier of the system space. * @param index_id identifier of the index to lookup. - * @param name space to lookup name. + * @param name of object to lookup. * @param len length of a name. - * @param space_id[out] space_id BOX_ID_NIL - not found. + * @param object_id[out] object_id or BOX_ID_NIL - not found. * * @retval 0 on success. * @retval -1 on error. */ -uint32_t +int schema_find_id(uint32_t system_space_id, uint32_t index_id, - const char *name, uint32_t len, uint32_t *space_id); + const char *name, uint32_t len, uint32_t *object_id); #if defined(__cplusplus) } /* extern "C" */ ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name 2018-06-04 19:21 ` Kirill Shcherbatov @ 2018-06-05 13:31 ` Vladislav Shpilevoy 0 siblings, 0 replies; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-06-05 13:31 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches Hello. Thanks for the fixes! The schema_find_id is still in C++ since it has tuple_field_u32_xc. XC means exception. I removed it and slightly refactored other code. The commit is force pushed and LGTM. diff --git a/src/box/schema.cc b/src/box/schema.cc index a85a09f9d..5d32e6153 100644 --- a/src/box/schema.cc +++ b/src/box/schema.cc @@ -226,7 +226,6 @@ int schema_find_id(uint32_t system_space_id, uint32_t index_id, const char *name, uint32_t len, uint32_t *object_id) { - *object_id = BOX_ID_NIL; if (len > BOX_NAME_MAX) { diag_set(SystemError, "name length %d is greater than BOX_NAME_MAX", len); @@ -257,13 +256,14 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id, region_truncate(region, used); return -1; } - int rc = 0; struct tuple *tuple; - if (iterator_next(it, &tuple) != 0) { - rc = -1; - } else if (tuple) { + int rc = iterator_next(it, &tuple); + if (rc == 0) { /* id is always field #1 */ - *object_id = 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; } iterator_delete(it); region_truncate(region, used); ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] [PATCH v1 4/4] sql: move Triggers to server 2018-05-31 11:22 [tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server Kirill Shcherbatov ` (2 preceding siblings ...) 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 3/4] box: introduce box_space_id_by_name Kirill Shcherbatov @ 2018-05-31 11:22 ` Kirill Shcherbatov 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-01 18:51 ` Konstantin Osipov 2018-05-31 17:36 ` [tarantool-patches] Re: [PATCH v1 0/4] sql: remove " Vladislav Shpilevoy ` (2 subsequent siblings) 6 siblings, 2 replies; 23+ messages in thread From: Kirill Shcherbatov @ 2018-05-31 11:22 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 builded 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 | 57 +++++++++++ src/box/space.h | 2 + src/box/sql.c | 48 ++------- src/box/sql.h | 62 ++++++++++++ src/box/sql/build.c | 8 +- src/box/sql/insert.c | 6 +- src/box/sql/sqliteInt.h | 2 - src/box/sql/tokenize.c | 43 +++++++- src/box/sql/trigger.c | 258 ++++++++++++++++++++++++++++-------------------- src/box/sql/vdbe.c | 58 +++-------- src/box/sql/vdbe.h | 1 - src/box/sql/vdbeaux.c | 9 -- 12 files changed, 343 insertions(+), 211 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index b62f8ad..6e4f15d 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -53,6 +53,7 @@ #include "version.h" #include "sequence.h" #include "sql.h" +#include "box.h" /** * chap-sha1 of empty string, i.e. @@ -551,6 +552,9 @@ 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. */ + new_space->sql_triggers = old_space->sql_triggers; + old_space->sql_triggers = NULL; } /** @@ -733,6 +737,20 @@ alter_space_commit(struct trigger *trigger, void *event) trigger_run_xc(&on_alter_space, alter->new_space); + if (alter->new_space->sql_triggers != NULL && + strcmp(alter->new_space->def->name, + alter->old_space->def->name) != 0) { + /* + * The function below either changes the name of + * all triggers, or does not change any of them. + * It should be last action in alter_space_commit + * as it is difficult to guarantee its rollback. + */ + if (sql_trigger_list_alter_table_name_transactional( + alter->new_space->sql_triggers, + alter->new_space->def->name) != 0) + diag_raise(); + } alter->new_space = NULL; /* for alter_space_delete(). */ /* * Delete the old version of the space, we are not @@ -3100,6 +3118,45 @@ 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); + if (stmt->old_tuple != NULL) { + uint32_t trigger_name_len; + const char *trigger_name_src = + tuple_field_str_xc(stmt->old_tuple, 0, + &trigger_name_len); + /* Can't use static buff as TT_STATIC_BUF_LEN + * is not enough for identifier max len test. */ + char *trigger_name = + (char *)region_alloc_xc(&fiber()->gc, trigger_name_len); + sprintf(trigger_name, "%.*s", trigger_name_len, + trigger_name_src); + if (sql_trigger_delete_by_name(sql_get(), trigger_name) != 0) + diag_raise(); + } + if (stmt->new_tuple != NULL) { + const char *space_opts = + tuple_field_with_type_xc(stmt->new_tuple, 1, MP_MAP); + struct space_opts opts; + struct region *region = &fiber()->gc; + space_opts_decode(&opts, space_opts, region); + + struct Trigger *trigger; + if (sql_trigger_compile(sql_get(), opts.sql, &trigger) != 0) + diag_raise(); + assert(trigger != NULL); + const char *table_name = + sql_trigger_get_table_name(trigger); + if (table_name == NULL) + diag_raise(); + uint32_t space_id = + space_id_by_name(BOX_SPACE_ID, table_name, + strlen(table_name)); + if (space_id == BOX_ID_NIL) + diag_raise(); + if (sql_trigger_insert(space_id, trigger) != 0) + diag_raise(); + } } struct trigger alter_space_on_replace_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 eacb288..0ff6e73 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1219,9 +1219,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, @@ -1290,42 +1287,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) == 2); - - field = tuple_field(tuple, 0); - assert (field != NULL); - ptr = mp_decode_str(&field, &len); - name = strndup(ptr, len); - - field = tuple_field(tuple, 1); - 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); } /********************************************************************* @@ -1733,6 +1694,15 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) return space->def->fields[fieldno].default_value_expr; } +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; +} + struct space_def * sql_ephemeral_space_def_new(Parse *parser, const char *name) { diff --git a/src/box/sql.h b/src/box/sql.h index 23021e5..f21b745 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 @@ -84,6 +85,67 @@ sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len, struct Expr **result); /** + * Perform parsing of provided SQL request and construct trigger AST. + * @param db SQL context handle. + * @param sql request to parse. + * @param[out] trigger Result: AST structure. + * + * @retval Error code if any. + */ +int +sql_trigger_compile(struct sqlite3 *db, const char *sql, + struct Trigger **trigger); + +/** + * Remove a trigger from the hash tables of the sqlite* pointer. + * @param db SQL handle. + * @param trigger_name to delete. + * + * @retval Error code if any. + */ +int +sql_trigger_delete_by_name(struct sqlite3 *db, const char *trigger_name); + +/** + * Get server triggers list by space_id. + * @param space_id Space ID. + * @retval NULL on error. + * @param not NULL on success. + */ +struct Trigger * +space_trigger_list(uint32_t space_id); + +/** + * Perform insert trigger in appropriate space. + * @param space_id id of the space to append trigger. + * @param trigger object to append. + * + * @retval Error code if any. + */ +int +sql_trigger_insert(uint32_t space_id, struct Trigger *trigger); + +/** + * Get table name of specified trigger. + * @param trigger containing a table name. + * @param new_table_name with new_name (optional). + * @retval table name string. + */ +const char * +sql_trigger_get_table_name(struct Trigger *trigger); + +/** + * Rename specified trigger list. + * @param trigger containing a table name. + * @param new_table_name with new_name (optional). + * + * @retval Error code if any. + */ +int +sql_trigger_list_alter_table_name_transactional(struct Trigger *trigger, + const char *new_table_name); + +/** * 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 9cd1cde..475d8aa 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2289,16 +2289,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/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 b23cb1a..39393c5 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. */ @@ -4028,7 +4027,6 @@ 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); #define sqlite3ParseToplevel(p) ((p)->pToplevel ? (p)->pToplevel : (p)) diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 59df75f..24db578 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -39,6 +39,7 @@ #include <stdlib.h> #include <unicode/utf8.h> #include <unicode/uchar.h> +#include "box/schema.h" #include "say.h" #include "sqliteInt.h" @@ -528,7 +529,10 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg) if (pParse->pWithToFree) sqlite3WithDelete(db, pParse->pWithToFree); - sqlite3DeleteTrigger(db, pParse->pNewTrigger); + /* Trigger is exporting with pNewTrigger field when + * parse_only flag is set. */ + if (!pParse->parse_only) + sqlite3DeleteTrigger(db, pParse->pNewTrigger); sqlite3DbFree(db, pParse->pVList); while (pParse->pZombieTab) { Table *p = pParse->pZombieTab; @@ -564,3 +568,40 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len, sql_parser_destroy(&parser); return 0; } + +int +sql_trigger_compile(struct sqlite3 *db, const char *sql, + struct Trigger **trigger) +{ + struct Parse parser; + sql_parser_create(&parser, db); + parser.parse_only = true; + char *unused; + if (sqlite3RunParser(&parser, sql, &unused) != SQLITE_OK) { + diag_set(ClientError, ER_SQL_EXECUTE, sql); + return -1; + } + *trigger = parser.pNewTrigger; + sql_parser_destroy(&parser); + return 0; +} + + +int +sql_trigger_insert(uint32_t space_id, struct Trigger *trigger) +{ + struct space *space = space_cache_find(space_id); + assert(space != NULL); + struct Hash *pHash = &trigger->pSchema->trigHash; + char *zName = trigger->zName; + void *ret = sqlite3HashInsert(pHash, zName, trigger); + if (ret != NULL) { + /* Triggers couldn't present in hash. + * So this is definitely a memory error. */ + diag_set(OutOfMemory, 0, "sqlite3HashInsert", "hash"); + return -1; + } + trigger->pNext = space->sql_triggers; + space->sql_triggers = trigger; + return 0; +} diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index ea35211..00f161e 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -33,6 +33,9 @@ * This file contains the implementation for TRIGGERs */ +#include "box/box.h" +#include "box/tuple.h" +#include "box/schema.h" #include "sqliteInt.h" #include "tarantoolInt.h" #include "vdbeInt.h" @@ -81,7 +84,8 @@ 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 */ + /* Table that the trigger fires off of. */ + struct Table *table = NULL; char *zName = 0; /* Name of the trigger */ sqlite3 *db = pParse->db; /* The database connection */ DbFixer sFix; /* State vector for the DB fixer */ @@ -99,60 +103,65 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE); assert(op > 0 && op < 0xff); - if (!pTableName || db->mallocFailed) { + if (!pTableName || db->mallocFailed) goto trigger_cleanup; - } /* 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)) { - goto trigger_cleanup; - } - pTab = sql_list_lookup_table(pParse, pTableName); - if (!pTab) { + if (sqlite3FixSrcList(&sFix, pTableName)) 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 (!noErr) { - sqlite3ErrorMsg(pParse, "trigger %s already exists", - zName); - } else { - assert(!db->init.busy); + + /* FIXME: Move all checks in VDBE #3435. */ + if (!pParse->parse_only) { + if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK) + goto trigger_cleanup; + + table = sql_list_lookup_table(pParse, pTableName); + if (table == NULL) + goto trigger_cleanup; + + if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) { + if (!noErr) { + sqlite3ErrorMsg(pParse, + "trigger %s already exists", + zName); + } else { + assert(!db->init.busy); + } + goto trigger_cleanup; } - 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"); - goto trigger_cleanup; - } + /* Do not create a trigger on a system table */ + if (sqlite3StrNICmp(table->def->name, "sqlite_", 7) == 0) { + sqlite3ErrorMsg(pParse, + "cannot create trigger on system table"); + 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); - goto trigger_cleanup; - } - if (!space_is_view(pTab) && tr_tm == TK_INSTEAD) { - sqlite3ErrorMsg(pParse, "cannot create INSTEAD OF" - " trigger on table: %S", pTableName, 0); - goto trigger_cleanup; + /* INSTEAD of triggers are only for views and + * views only support INSTEAD of triggers. + */ + if (table->def->opts.is_view && tr_tm != TK_INSTEAD) { + sqlite3ErrorMsg(pParse, + "cannot create %s trigger on view: %S", + (tr_tm == TK_BEFORE) ? + "BEFORE" : "AFTER", + pTableName, 0); + goto trigger_cleanup; + } + if (!table->def->opts.is_view && tr_tm == TK_INSTEAD) { + sqlite3ErrorMsg(pParse, + "cannot create INSTEAD OF trigger " + "on table: %S", pTableName, 0); + goto trigger_cleanup; + } } /* INSTEAD OF triggers can only appear on views and BEFORE triggers @@ -160,9 +169,8 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s * 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)); @@ -170,13 +178,22 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s goto trigger_cleanup; pTrigger->zName = zName; zName = 0; - pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName); + pTrigger->table = strdup(pTableName->a[0].zName); + if (pTrigger->table == NULL) { + diag_set(OutOfMemory, strlen(pTableName->a[0].zName), "strdup", + "pTrigger->table"); + pParse->rc = SQL_TARANTOOL_ERROR; + goto trigger_cleanup; + } pTrigger->pSchema = db->pSchema; - pTrigger->pTabSchema = pTab->pSchema; + pTrigger->pTabSchema = db->pSchema; 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); + if ((pWhen != NULL && pTrigger->pWhen == NULL) || + (pColumns != NULL && pTrigger->pColumns == NULL)) + goto trigger_cleanup; assert(pParse->pNewTrigger == 0); pParse->pNewTrigger = pTrigger; @@ -210,7 +227,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; @@ -227,10 +244,9 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */ goto triggerfinish_cleanup; } - /* if we are not initializing, - * generate byte code to insert a new trigger into Tarantool. - */ - if (!db->init.busy) { + /* Generate byte code to insert a new trigger into + * Tarantool for non-parsig mode or export trigger. */ + if (!pParse->parse_only) { Vdbe *v; int zOptsSz; Table *pSysTrigger; @@ -293,32 +309,10 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */ 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: @@ -329,7 +323,7 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */ alloc for it either wasn't called at all or failed. */ } sqlite3DeleteTrigger(db, pTrig); - assert(!pParse->pNewTrigger); + assert(!pParse->pNewTrigger || pParse->parse_only); sqlite3DeleteTriggerStep(db, pStepList); } @@ -481,7 +475,7 @@ sqlite3DeleteTrigger(sqlite3 * db, Trigger * pTrigger) return; sqlite3DeleteTriggerStep(db, pTrigger->step_list); sqlite3DbFree(db, pTrigger->zName); - sqlite3DbFree(db, pTrigger->table); + free(pTrigger->table); sql_expr_delete(db, pTrigger->pWhen, false); sqlite3IdListDelete(db, pTrigger->pColumns); sqlite3DbFree(db, pTrigger); @@ -568,34 +562,35 @@ 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_delete_by_name(struct sqlite3 *db, const char *trigger_name) { - 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)) ; + struct Hash *hash = &(db->pSchema->trigHash); + struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL); + assert(trigger != NULL); + + if (trigger->pSchema == trigger->pTabSchema) { + uint32_t space_id = space_id_by_name(BOX_SPACE_ID, + trigger->table, strlen(trigger->table)); + struct space *space = space_by_id(space_id); + /* Space could be already deleted. */ + if (space != NULL) { + struct Trigger **pp; + for (pp = &space->sql_triggers; + *pp != trigger; + pp = &((*pp)->pNext)); *pp = (*pp)->pNext; } - sqlite3DeleteTrigger(db, pTrigger); - user_session->sql_flags |= SQLITE_InternChanges; } + + sqlite3DeleteTrigger(db, trigger); + user_session->sql_flags |= SQLITE_InternChanges; + return 0; } /* @@ -634,22 +629,18 @@ sqlite3TriggersExist(Table * pTab, /* The table the contains the triggers */ ) { int mask = 0; - Trigger *pList = 0; - Trigger *p; + struct Trigger *trigger_list = NULL; struct session *user_session = current_session(); - - if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0) { - pList = pTab->pTrigger; - } - for (p = pList; p; p = p->pNext) { - if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) { + if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0) + trigger_list = space_trigger_list(pTab->def->id); + struct Trigger *p; + for (p = trigger_list; p; p = p->pNext) { + if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) mask |= p->tr_tm; - } } - if (pMask) { + if (pMask) *pMask = mask; - } - return (mask ? pList : 0); + return (mask ? trigger_list : 0); } /* @@ -1155,4 +1146,55 @@ sqlite3TriggerColmask(Parse * pParse, /* Parse context */ return mask; } +const char * +sql_trigger_get_table_name(struct Trigger *trigger) +{ + return trigger->table; +} + +int +sql_trigger_list_alter_table_name_transactional(struct Trigger *trigger, + const char *new_table_name) +{ + struct names_list_t { + char *name; + struct names_list_t *next; + } *names_list = NULL; + + int rc = 0; + for (struct Trigger *t = trigger; t != NULL; t = t->pNext) { + struct names_list_t *node = + region_alloc(&fiber()->gc, sizeof(*node)); + if (rc |= (node == NULL)) { + diag_set(OutOfMemory, sizeof(*node), "region_alloc", + "node"); + break; + } + node->name = strdup(new_table_name); + if (rc |= (node->name == NULL)) { + diag_set(OutOfMemory, strlen(new_table_name), "strdup", + "node->name"); + break; + } + node->next = names_list; + names_list = node; + } + if (rc != 0) + goto error; + + for (struct Trigger *t = trigger; t != NULL; t = t->pNext) { + free(t->table); + t->table = names_list->name; + names_list = names_list->next; + } + return 0; + +error: + while (names_list != NULL) { + free(names_list->name); + names_list = names_list->next; + } + return -1; +} + #endif /* !defined(SQLITE_OMIT_TRIGGER) */ diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 3fe5875..f60c122 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4693,36 +4693,7 @@ case OP_ParseSchema2: { * 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; - } + unreachable(); break; } @@ -4745,7 +4716,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,11 +4728,11 @@ 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, sqlite3Strlen30(zOldTableName)); + rc = tarantoolSqlite3RenameTable(pTab->tnum, zNewTableName, &zSqlStmt); if (rc) goto abort_due_to_error; @@ -4799,19 +4769,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 trigger 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; @@ -4866,7 +4838,7 @@ case OP_DropIndex: { * schema consistent with what is on disk. */ case OP_DropTrigger: { - sqlite3UnlinkAndDeleteTrigger(db, pOp->p4.z); + unreachable(); break; } #ifndef SQLITE_OMIT_TRIGGER 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 63f1978..7a5ac6f 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) { -- 2.7.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 4/4] sql: move Triggers to server Kirill Shcherbatov @ 2018-05-31 17:36 ` Vladislav Shpilevoy 2018-06-01 20:24 ` Kirill Shcherbatov 2018-06-01 18:51 ` Konstantin Osipov 1 sibling, 1 reply; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-05-31 17:36 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches Thanks for the patch! See 24 comments below. 1. /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses] if (rc |= (node == NULL)) { ~~~^~~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: note: place parentheses around the assignment to silence this warning if (rc |= (node == NULL)) { ^ ( ) /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: note: use '!=' to turn this compound assignment into an inequality comparison if (rc |= (node == NULL)) { ^~ != /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1174:10: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses] if (rc |= (node->name == NULL)) { ~~~^~~~~~~~~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1174:10: note: place parentheses around the assignment to silence this warning if (rc |= (node->name == NULL)) { ^ ( ) /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1174:10: note: use '!=' to turn this compound assignment into an inequality comparison if (rc |= (node->name == NULL)) { ^~ != On 31/05/2018 14:22, Kirill Shcherbatov wrote: > Introduced sql_triggers field in space structure. > Changed parser logic to do not insert builded 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 | 57 +++++++++++ > src/box/space.h | 2 + > src/box/sql.c | 48 ++------- > src/box/sql.h | 62 ++++++++++++ > src/box/sql/build.c | 8 +- > src/box/sql/insert.c | 6 +- > src/box/sql/sqliteInt.h | 2 - > src/box/sql/tokenize.c | 43 +++++++- > src/box/sql/trigger.c | 258 ++++++++++++++++++++++++++++-------------------- > src/box/sql/vdbe.c | 58 +++-------- > src/box/sql/vdbe.h | 1 - > src/box/sql/vdbeaux.c | 9 -- > 12 files changed, 343 insertions(+), 211 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index b62f8ad..6e4f15d 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -733,6 +737,20 @@ alter_space_commit(struct trigger *trigger, void *event) > > trigger_run_xc(&on_alter_space, alter->new_space); > > + if (alter->new_space->sql_triggers != NULL && > + strcmp(alter->new_space->def->name, > + alter->old_space->def->name) != 0) { > + /* > + * The function below either changes the name of > + * all triggers, or does not change any of them. > + * It should be last action in alter_space_commit > + * as it is difficult to guarantee its rollback. > + */ > + if (sql_trigger_list_alter_table_name_transactional( > + alter->new_space->sql_triggers, > + alter->new_space->def->name) != 0) 2. It does not work. box.cfg{} 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; ]]) box.space.T1:rename('T3') box.space._trigger:select{} I still see the trigger with the old table name. I do not think we should do in-memory non-persistent rename. Please, investigate what other databases do. > + diag_raise(); > + } > alter->new_space = NULL; /* for alter_space_delete(). */ > /* > * Delete the old version of the space, we are not > @@ -3100,6 +3118,45 @@ 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); > + if (stmt->old_tuple != NULL) { > + uint32_t trigger_name_len; > + const char *trigger_name_src = > + tuple_field_str_xc(stmt->old_tuple, 0, > + &trigger_name_len); > + /* Can't use static buff as TT_STATIC_BUF_LEN > + * is not enough for identifier max len test. */ > + char *trigger_name = > + (char *)region_alloc_xc(&fiber()->gc, trigger_name_len); > + sprintf(trigger_name, "%.*s", trigger_name_len, > + trigger_name_src); > + if (sql_trigger_delete_by_name(sql_get(), trigger_name) != 0) > + diag_raise(); 3. If WAL write fails, you could not rollback because the trigger is already deleted now. Please, implement on_rollback/on_commit triggers. You must delete the old trigger on commit only. > + } > + if (stmt->new_tuple != NULL) { > + const char *space_opts = > + tuple_field_with_type_xc(stmt->new_tuple, 1, MP_MAP); > + struct space_opts opts; > + struct region *region = &fiber()->gc; > + space_opts_decode(&opts, space_opts, region); > + > + struct Trigger *trigger; > + if (sql_trigger_compile(sql_get(), opts.sql, &trigger) != 0) > + diag_raise(); > + assert(trigger != NULL); > + const char *table_name = > + sql_trigger_get_table_name(trigger); > + if (table_name == NULL) > + diag_raise(); > + uint32_t space_id = > + space_id_by_name(BOX_SPACE_ID, table_name, > + strlen(table_name)); > + if (space_id == BOX_ID_NIL) > + diag_raise(); > + if (sql_trigger_insert(space_id, trigger) != 0) > + diag_raise(); 4. On rollback you must delete this trigger. > + } > } > diff --git a/src/box/sql.h b/src/box/sql.h > index 23021e5..f21b745 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 > @@ -84,6 +85,67 @@ sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len, > struct Expr **result); > > /** > + * Perform parsing of provided SQL request and construct trigger AST. > + * @param db SQL context handle. > + * @param sql request to parse. > + * @param[out] trigger Result: AST structure. > + * > + * @retval Error code if any. 5. More specifically? > + */ > +int > +sql_trigger_compile(struct sqlite3 *db, const char *sql, > + struct Trigger **trigger); 6. Why can not you just return Trigger *? Not NULL on success, NULL on error. It is a common practice. > + > +/** > + * Remove a trigger from the hash tables of the sqlite* pointer. > + * @param db SQL handle. > + * @param trigger_name to delete. 7. You delete not the trigger name, but the trigger object. > + * > + * @retval Error code if any. > + */ > +int > +sql_trigger_delete_by_name(struct sqlite3 *db, const char *trigger_name); > + > +/** > + * Get server triggers list by space_id. > + * @param space_id Space ID. > + * @retval NULL on error. 8. Are you sure? > + * @param not NULL on success. > + */ > +struct Trigger * > +space_trigger_list(uint32_t space_id); > + > +/** > + * Perform insert trigger in appropriate space. > + * @param space_id id of the space to append trigger. > + * @param trigger object to append. > + * > + * @retval Error code if any. > + */ > +int > +sql_trigger_insert(uint32_t space_id, struct Trigger *trigger); 9. Why can not you use space *? In alter.cc you have it in txn_stmt. > + > +/** > + * Get table name of specified trigger. > + * @param trigger containing a table name. > + * @param new_table_name with new_name (optional). > + * @retval table name string. > + */ > +const char * > +sql_trigger_get_table_name(struct Trigger *trigger); 10. sql_trigger_table_name. On getter methods you may omit 'get'. > 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) 11. As far as I remember, in Table.def id now is always 0. It is not updated after insertion into _space, so you can not use it. Use tnum for a while. Or please make a patch, that updates def.id in the same place where tnum. > + return 0; > if (onError == ON_CONFLICT_ACTION_DEFAULT) { > if (pDest->iPKey >= 0) > onError = pDest->keyConf; > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index 59df75f..24db578 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -39,6 +39,7 @@ > #include <stdlib.h> > #include <unicode/utf8.h> > #include <unicode/uchar.h> > +#include "box/schema.h" > > #include "say.h" > #include "sqliteInt.h" > @@ -528,7 +529,10 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > > if (pParse->pWithToFree) > sqlite3WithDelete(db, pParse->pWithToFree); > - sqlite3DeleteTrigger(db, pParse->pNewTrigger); > + /* Trigger is exporting with pNewTrigger field when > + * parse_only flag is set. */ 12. Please, obey Tarantool comment style. In other places too. I will not comment each explicitly from now. > + if (!pParse->parse_only) > + sqlite3DeleteTrigger(db, pParse->pNewTrigger); > sqlite3DbFree(db, pParse->pVList); > while (pParse->pZombieTab) { > Table *p = pParse->pZombieTab; > @@ -564,3 +568,40 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len, > sql_parser_destroy(&parser); > return 0; > } > + > +int > +sql_trigger_compile(struct sqlite3 *db, const char *sql, > + struct Trigger **trigger) > +{ > + struct Parse parser; > + sql_parser_create(&parser, db); > + parser.parse_only = true; > + char *unused; 13. Why have you skipped the error message? (Please, do not respond with just a diff. Try to answer on the questions. This and others.) > + if (sqlite3RunParser(&parser, sql, &unused) != SQLITE_OK) { > + diag_set(ClientError, ER_SQL_EXECUTE, sql); > + return -1; > + } > + *trigger = parser.pNewTrigger; > + sql_parser_destroy(&parser); > + return 0; > +} > + > + 14. Extra new line. > +int > +sql_trigger_insert(uint32_t space_id, struct Trigger *trigger) > +{ > + struct space *space = space_cache_find(space_id); > + assert(space != NULL); > + struct Hash *pHash = &trigger->pSchema->trigHash; > + char *zName = trigger->zName; > + void *ret = sqlite3HashInsert(pHash, zName, trigger); > + if (ret != NULL) { > + /* Triggers couldn't present in hash. > + * So this is definitely a memory error. */ > + diag_set(OutOfMemory, 0, "sqlite3HashInsert", "hash"); > + return -1; > + } > + trigger->pNext = space->sql_triggers; > + space->sql_triggers = trigger; > + return 0; > +} > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index ea35211..00f161e 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -33,6 +33,9 @@ > * This file contains the implementation for TRIGGERs > */ > > +#include "box/box.h" > +#include "box/tuple.h" 15. Why do you need tuple.h? I removed it and nothing is changed. > +#include "box/schema.h" > #include "sqliteInt.h" > #include "tarantoolInt.h" > #include "vdbeInt.h" > @@ -99,60 +103,65 @@ sqlite3BeginTrigger(Parse * pParse > /* 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)) { > - goto trigger_cleanup; > - } > - pTab = sql_list_lookup_table(pParse, pTableName); > - if (!pTab) { > + if (sqlite3FixSrcList(&sFix, pTableName)) > 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 (!noErr) { > - sqlite3ErrorMsg(pParse, "trigger %s already exists", > - zName); > - } else { > - assert(!db->init.busy); > + > + /* FIXME: Move all checks in VDBE #3435. */ > + if (!pParse->parse_only) { > + if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK) > + goto trigger_cleanup; > + > + table = sql_list_lookup_table(pParse, pTableName); > + if (table == NULL) > + goto trigger_cleanup; > + > + if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) { 16. Why do you need extra () after & ? > + if (!noErr) { > + sqlite3ErrorMsg(pParse, > + "trigger %s already exists", > + zName); > + } else { > + assert(!db->init.busy); > + } > + goto trigger_cleanup; > } > - goto trigger_cleanup; > @@ -170,13 +178,22 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s > goto trigger_cleanup; > pTrigger->zName = zName; > zName = 0; > - pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName); > + pTrigger->table = strdup(pTableName->a[0].zName); 17. Looks like sqlite3DeleteTriggerStep() leaks. It does not delete 'table'. Maybe it is the original SQLite bug. Can you please check? > + if (pTrigger->table == NULL) { > + diag_set(OutOfMemory, strlen(pTableName->a[0].zName), "strdup", > + "pTrigger->table"); > + pParse->rc = SQL_TARANTOOL_ERROR; > + goto trigger_cleanup; > + } > pTrigger->pSchema = db->pSchema; > - pTrigger->pTabSchema = pTab->pSchema; > + pTrigger->pTabSchema = db->pSchema; > 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); > + if ((pWhen != NULL && pTrigger->pWhen == NULL) || > + (pColumns != NULL && pTrigger->pColumns == NULL)) > + goto trigger_cleanup; 18. Why? > @@ -634,22 +629,18 @@ sqlite3TriggersExist(Table * pTab, /* The table the contains the triggers */ > ) > { > int mask = 0; > - Trigger *pList = 0; > - Trigger *p; > + struct Trigger *trigger_list = NULL; > struct session *user_session = current_session(); > - > - if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0) { > - pList = pTab->pTrigger; > - } > - for (p = pList; p; p = p->pNext) { > - if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) { > + if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0) > + trigger_list = space_trigger_list(pTab->def->id); > + struct Trigger *p; > + for (p = trigger_list; p; p = p->pNext) { 19. Why not for (struct Trigger *p = ... ? > + if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) > mask |= p->tr_tm; > - } > } > - if (pMask) { > + if (pMask) > *pMask = mask; > - } > - return (mask ? pList : 0); > + return (mask ? trigger_list : 0); 20. Extra (). > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 3fe5875..f60c122 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4693,36 +4693,7 @@ case OP_ParseSchema2: { > * in database P2 > */ > case OP_ParseSchema3: { 21. Why have not you deleted it? > - 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; > - } > + unreachable(); > break; > } > @@ -4758,11 +4728,11 @@ 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, > sqlite3Strlen30(zOldTableName)); > + 22. Garbage diff. > rc = tarantoolSqlite3RenameTable(pTab->tnum, zNewTableName, > &zSqlStmt); > if (rc) goto abort_due_to_error; > @@ -4866,7 +4838,7 @@ case OP_DropIndex: { > * schema consistent with what is on disk. > */ > case OP_DropTrigger: { > - sqlite3UnlinkAndDeleteTrigger(db, pOp->p4.z); > + unreachable(); 23. Same as 21. > break; > } > #ifndef SQLITE_OMIT_TRIGGER 24. Where are tests on triggers create/drop via Lua and direct _trigger manipulations? ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-06-01 20:24 ` Kirill Shcherbatov 2018-06-01 20:25 ` Kirill Shcherbatov 0 siblings, 1 reply; 23+ messages in thread From: Kirill Shcherbatov @ 2018-06-01 20:24 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy You better take a lock to the new patch version at the branch. I'll send it with next message. > /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: note: use '!=' to turn this compound assignment into an inequality comparison > if (rc |= (node == NULL)) { > ^~ - if (rc |= (node == NULL)) { + if (node == NULL) { + rc = -1; > 2. It does not work. > I do not think we should do in-memory non-persistent rename. Please, > investigate what other databases do. SQLite3, MySQL allows rename table with triggers. > 3. If WAL write fails, you could not rollback because the trigger is > already deleted now. Please, implement on_rollback/on_commit triggers. > You must delete the old trigger on commit only. > 4. On rollback you must delete this trigger. I've introduce space_id instead of table name in Trigger object. Diff for this explicit change is at the end of this message. >> + * @retval Error code if any. > > 5. More specifically?>> +int >> +sql_trigger_compile(struct sqlite3 *db, const char *sql, >> + struct Trigger **trigger); > > 6. Why can not you just return Trigger *? Not NULL on success, > NULL on error. It is a common practice.I followed sql_expr_compile example. But it is ok for me to refactor this function this way. diff --git a/src/box/alter.cc b/src/box/alter.cc index 63f253b..487a497 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -3141,8 +3141,9 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) struct region *region = &fiber()->gc; space_opts_decode(&opts, space_opts, region); - struct Trigger *trigger; - if (sql_trigger_compile(sql_get(), opts.sql, &trigger) != 0) + struct Trigger *trigger = + sql_trigger_compile(sql_get(), opts.sql); + if (trigger == NULL) diag_raise(); assert(trigger != NULL); const char *table_name = diff --git a/src/box/sql.h b/src/box/sql.h index f21b745..bd542c0 100644 --- a/src/box/sql.h +++ b/src/box/sql.h @@ -88,13 +88,12 @@ 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. - * @param[out] trigger Result: AST structure. * - * @retval Error code if any. + * @retval NULL on error + * @retval not NULL Trigger AST pointer on success. */ -int -sql_trigger_compile(struct sqlite3 *db, const char *sql, - struct Trigger **trigger); +struct Trigger * +sql_trigger_compile(struct sqlite3 *db, const char *sql); > 13. Why have you skipped the error message? > > (Please, do not respond with just a diff. Try to answer on > the questions. This and others.)I've fellow sql_expr_compile logic, made same behavior. @@ -577,12 +579,16 @@ sql_trigger_compile(struct sqlite3 *db, const char *sql) struct Parse parser; sql_parser_create(&parser, db); parser.parse_only = true; - char *unused; - if (sqlite3RunParser(&parser, sql, &unused) != SQLITE_OK) { + 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_EXECUTE, sql); - return NULL; + goto cleanup; } - struct Trigger *trigger = parser.pNewTrigger; + trigger = parser.pNewTrigger; +cleanup: sql_parser_destroy(&parser); return trigger; } I'll also change sql_expr_compile function in separate patch to follow this convention. >> + * Remove a trigger from the hash tables of the sqlite* pointer. >> + * @param db SQL handle. >> + * @param trigger_name to delete. > > 7. You delete not the trigger name, but the trigger object. - * @param trigger_name to delete. + * @param trigger_name a name of trigger object to delete. >> +/** >> + * Get server triggers list by space_id. >> + * @param space_id Space ID. >> + * @retval NULL on error. > > 8. Are you sure? - * @retval NULL on error. - * @param not NULL on success. + * @retval trigger AST list. >> +int >> +sql_trigger_insert(uint32_t space_id, struct Trigger *trigger); > > 9. Why can not you use space *? In alter.cc you have it in txn_stmt. txn_stmt doesn't contain valid space pointer as it is defined by trigger. > 10. sql_trigger_table_name. On getter methods you may omit 'get'.-sql_trigger_get_table_name(struct Trigger *trigger) +sql_trigger_table_name(struct Trigger *trigger) >> + /* The pDest must not have triggers. */ >> + if (space_trigger_list(pDest->def->id) != NULL) > > 11. As far as I remember, in Table.def id now is always 0. It is > not updated after insertion into _space, so you can not use it. > Use tnum for a while. Or please make a patch, that updates def.id > in the same place where tnum. I believe that it is updated in sqlite3EndTable, below the only location of updating tnum. >> + /* Trigger is exporting with pNewTrigger field when >> + * parse_only flag is set. */ > > 12. Please, obey Tarantool comment style. In other places too. I will not > comment each explicitly from now. - /* Trigger is exporting with pNewTrigger field when - * parse_only flag is set. */ + /* + * Trigger is exporting with pNewTrigger field when + * parse_only flag is set. + */ > 14. Extra new line. dropped. >> +#include "box/box.h" >> +#include "box/tuple.h" > 15. Why do you need tuple.h? I removed it and nothing is changed. -#include "box/tuple.h" > 16. Why do you need extra () after & ? - if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) { + if (sqlite3HashFind(&db->pSchema->trigHash, zName)) { > 17. Looks like sqlite3DeleteTriggerStep() leaks. It does not delete 'table'. > Maybe it is the original SQLite bug. Can you please check? triggerStepAllocate(sqlite3 * db, /* Database connection */ u8 op, /* Trigger opcode */ Token * pName /* The target name */ ) { TriggerStep *pTriggerStep = sqlite3DbMallocZero(db, sizeof(TriggerStep) + pName->n + 1); ... char *z = (char *)&pTriggerStep[1]; ... pTriggerStep->zTarget = z; Memory for name is located in same allocation that object located is. >> + if ((pWhen != NULL && pTrigger->pWhen == NULL) || >> + (pColumns != NULL && pTrigger->pColumns == NULL)) >> + goto trigger_cleanup; > > 18. Why? We have deep-cloned pWhen into pTrigger->pWhen and pColumns into pTrigger->pColumns; There could be memory allocation error, so we ensure that action have been finished correctly. >> + struct Trigger *p; >> + for (p = trigger_list; p; p = p->pNext) { > > 19. Why not for (struct Trigger *p = ... ? - struct Trigger *p; - for (p = trigger_list; p; p = p->pNext) { + for (struct Trigger *p = trigger_list; p; p = p->pNext) { >> + return (mask ? trigger_list : 0); > > 20. Extra (). - return (mask ? trigger_list : 0); + return mask ? trigger_list : 0; > >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 3fe5875..f60c122 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -4693,36 +4693,7 @@ case OP_ParseSchema2: { >> * in database P2 >> */ >> case OP_ParseSchema3: { > > 21. Why have not you deleted it? > 23. Same as 21. I've assumed that it could be useful in future or for debug. -/* 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: { - unreachable(); - 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: { - unreachable(); - break; -} >> zOldTableName = sqlite3DbStrNDup(db, zOldTableName, >> sqlite3Strlen30(zOldTableName)); >> + > > 22. Garbage diff. dropped. > 24. Where are tests on triggers create/drop via Lua and direct _trigger > manipulations? diff --git a/src/box/alter.cc b/src/box/alter.cc index d3f9d42..6d63f96 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -736,21 +736,6 @@ alter_space_commit(struct trigger *trigger, void *event) } trigger_run_xc(&on_alter_space, alter->new_space); - - if (alter->new_space->sql_triggers != NULL && - strcmp(alter->new_space->def->name, - alter->old_space->def->name) != 0) { - /* - * The function below either changes the name of - * all triggers, or does not change any of them. - * It should be last action in alter_space_commit - * as it is difficult to guarantee its rollback. - */ - if (sql_trigger_list_alter_table_name_transactional( - alter->new_space->sql_triggers, - alter->new_space->def->name) != 0) - diag_raise(); - } alter->new_space = NULL; /* for alter_space_delete(). */ /* * Delete the old version of the space, we are not @@ -3140,21 +3125,11 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) struct space_opts opts; struct region *region = &fiber()->gc; space_opts_decode(&opts, space_opts, region); - struct Trigger *trigger = sql_trigger_compile(sql_get(), opts.sql); if (trigger == NULL) diag_raise(); - assert(trigger != NULL); - const char *table_name = - sql_trigger_table_name(trigger); - if (table_name == NULL) - diag_raise(); - uint32_t space_id; - if (schema_find_id(BOX_SPACE_ID, 2, table_name, - strlen(table_name), &space_id) != 0) - diag_raise(); - if (sql_trigger_insert(space_id, trigger) != 0) + if (sql_trigger_insert(trigger) != 0) diag_raise(); } } diff --git a/src/box/sql.h b/src/box/sql.h index e6f5a02..bc96b75 100644 --- a/src/box/sql.h +++ b/src/box/sql.h @@ -114,33 +114,12 @@ space_trigger_list(uint32_t space_id); /** * Perform insert trigger in appropriate space. - * @param space_id id of the space to append trigger. * @param trigger object to append. * * @retval Error code if any. */ int -sql_trigger_insert(uint32_t space_id, struct Trigger *trigger); - -/** - * Get table name of specified trigger. - * @param trigger containing a table name. - * @param new_table_name with new_name (optional). - * @retval table name string. - */ -const char * -sql_trigger_table_name(struct Trigger *trigger); - -/** - * Rename specified trigger list. - * @param trigger containing a table name. - * @param new_table_name with new_name (optional). - * - * @retval Error code if any. - */ -int -sql_trigger_list_alter_table_name_transactional(struct Trigger *trigger, - const char *new_table_name); +sql_trigger_insert(struct Trigger *trigger); /** * Store duplicate of a parsed expression into @a parser. diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c index 70ebef8..d5585cf 100644 --- a/src/box/sql/fkey.c +++ b/src/box/sql/fkey.c @@ -1440,7 +1440,6 @@ fkActionTrigger(Parse * pParse, /* Parse context */ } 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/sqliteInt.h b/src/box/sql/sqliteInt.h index 36695e3..06930c7 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3031,14 +3031,14 @@ struct Parse { */ struct Trigger { char *zName; /* The name of the trigger */ - char *table; /* The table or view to which the trigger applies */ + /** The ID of space the triggers is refer to. */ + uint32_t space_id; 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 */ }; diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 2d9d29e..9d613de 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -597,13 +597,13 @@ cleanup: } int -sql_trigger_insert(uint32_t space_id, struct Trigger *trigger) +sql_trigger_insert(struct Trigger *trigger) { - struct space *space = space_cache_find(space_id); + struct space *space = space_cache_find(trigger->space_id); assert(space != NULL); struct Hash *pHash = &trigger->pSchema->trigHash; - char *zName = trigger->zName; - void *ret = sqlite3HashInsert(pHash, zName, trigger); + char *trigger_name = trigger->zName; + void *ret = sqlite3HashInsert(pHash, trigger_name, trigger); if (ret != NULL) { /* Triggers couldn't present in hash. * So this is definitely a memory error. */ diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index a808d3a..2fd81c0 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -177,15 +177,14 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s goto trigger_cleanup; pTrigger->zName = zName; zName = 0; - pTrigger->table = strdup(pTableName->a[0].zName); - if (pTrigger->table == NULL) { - diag_set(OutOfMemory, strlen(pTableName->a[0].zName), "strdup", - "pTrigger->table"); + /* Initialize space_id on trigger insert. */ + if (schema_find_id(BOX_SPACE_ID, 2, pTableName->a[0].zName, + strlen(pTableName->a[0].zName), + &pTrigger->space_id) != 0) { pParse->rc = SQL_TARANTOOL_ERROR; goto trigger_cleanup; } pTrigger->pSchema = db->pSchema; - pTrigger->pTabSchema = db->pSchema; pTrigger->op = (u8) op; pTrigger->tr_tm = tr_tm == TK_BEFORE ? TRIGGER_BEFORE : TRIGGER_AFTER; pTrigger->pWhen = sqlite3ExprDup(db, pWhen, EXPRDUP_REDUCE); @@ -470,7 +469,6 @@ sqlite3DeleteTrigger(sqlite3 * db, Trigger * pTrigger) return; sqlite3DeleteTriggerStep(db, pTrigger->step_list); sqlite3DbFree(db, pTrigger->zName); - free(pTrigger->table); sql_expr_delete(db, pTrigger->pWhen, false); sqlite3IdListDelete(db, pTrigger->pColumns); sqlite3DbFree(db, pTrigger); @@ -525,16 +523,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 @@ -569,20 +557,14 @@ sql_trigger_delete_by_name(struct sqlite3 *db, const char *trigger_name) struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL); assert(trigger != NULL); - if (trigger->pSchema == trigger->pTabSchema) { - uint32_t space_id; - if (schema_find_id(BOX_SPACE_ID, 2, trigger->table, - strlen(trigger->table), &space_id) != 0) - return -1; - struct space *space = space_by_id(space_id); - /* Space could be already deleted. */ - if (space != NULL) { - struct Trigger **pp; - for (pp = &space->sql_triggers; - *pp != trigger; - pp = &((*pp)->pNext)); - *pp = (*pp)->pNext; - } + struct space *space = space_by_id(trigger->space_id); + /* Space could be already deleted. */ + if (space != NULL) { + struct Trigger **pp; + for (pp = &space->sql_triggers; + *pp != trigger; + pp = &((*pp)->pNext)); + *pp = (*pp)->pNext; } sqlite3DeleteTrigger(db, trigger); @@ -825,7 +807,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 @@ -942,7 +924,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 @@ -1067,14 +1049,7 @@ 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 @@ -1142,57 +1117,4 @@ sqlite3TriggerColmask(Parse * pParse, /* Parse context */ return mask; } -const char * -sql_trigger_table_name(struct Trigger *trigger) -{ - return trigger->table; -} - -int -sql_trigger_list_alter_table_name_transactional(struct Trigger *trigger, - const char *new_table_name) -{ - struct names_list_t { - char *name; - struct names_list_t *next; - } *names_list = NULL; - - int rc = 0; - for (struct Trigger *t = trigger; t != NULL; t = t->pNext) { - struct names_list_t *node = - region_alloc(&fiber()->gc, sizeof(*node)); - if (node == NULL) { - rc = -1; - diag_set(OutOfMemory, sizeof(*node), "region_alloc", - "node"); - break; - } - node->name = strdup(new_table_name); - if (node->name == NULL) { - rc = -1; - diag_set(OutOfMemory, strlen(new_table_name), "strdup", - "node->name"); - break; - } - node->next = names_list; - names_list = node; - } - if (rc != 0) - goto error; - - for (struct Trigger *t = trigger; t != NULL; t = t->pNext) { - free(t->table); - t->table = names_list->name; - names_list = names_list->next; - } - return 0; - -error: - while (names_list != NULL) { - free(names_list->name); - names_list = names_list->next; - } - return -1; -} - #endif /* !defined(SQLITE_OMIT_TRIGGER) */ ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server 2018-06-01 20:24 ` Kirill Shcherbatov @ 2018-06-01 20:25 ` Kirill Shcherbatov 2018-06-04 13:27 ` Vladislav Shpilevoy 0 siblings, 1 reply; 23+ messages in thread From: Kirill Shcherbatov @ 2018-06-01 20:25 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy From c0775801fcdb475ee6e77dff483f4f6a84c1afc3 Mon Sep 17 00:00:00 2001 Message-Id: <c0775801fcdb475ee6e77dff483f4f6a84c1afc3.1527884700.git.kshcherbatov@gmail.com> From: Kirill Shcherbatov <kshcherbatov@tarantool.org> Date: Wed, 30 May 2018 16:03:43 +0300 Subject: [PATCH] sql: move Triggers to server 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 | 37 ++++- src/box/errcode.h | 2 +- src/box/space.h | 2 + src/box/sql.c | 48 ++---- src/box/sql.h | 44 ++++++ src/box/sql/build.c | 8 +- src/box/sql/fkey.c | 1 - src/box/sql/insert.c | 6 +- src/box/sql/sqliteInt.h | 6 +- src/box/sql/tokenize.c | 38 ++++- src/box/sql/trigger.c | 287 +++++++++++++++++----------------- src/box/sql/vdbe.c | 76 ++------- src/box/sql/vdbe.h | 1 - src/box/sql/vdbeaux.c | 9 -- test/sql-tap/identifier_case.test.lua | 4 +- test/sql-tap/trigger1.test.lua | 4 +- test/sql/triggers.test.lua | 72 +++++++++ 17 files changed, 370 insertions(+), 275 deletions(-) create mode 100644 test/sql/triggers.test.lua diff --git a/src/box/alter.cc b/src/box/alter.cc index f2bf85d..bf170a5 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -551,6 +551,9 @@ 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. */ + new_space->sql_triggers = old_space->sql_triggers; + old_space->sql_triggers = NULL; } /** @@ -732,7 +735,6 @@ alter_space_commit(struct trigger *trigger, void *event) } trigger_run_xc(&on_alter_space, alter->new_space); - alter->new_space = NULL; /* for alter_space_delete(). */ /* * Delete the old version of the space, we are not @@ -3100,6 +3102,39 @@ 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); + if (stmt->old_tuple != NULL) { + uint32_t trigger_name_len; + const char *trigger_name_src = + tuple_field_str_xc(stmt->old_tuple, 0, + &trigger_name_len); + if (sql_trigger_delete_by_name(sql_get(), trigger_name_src, + trigger_name_len) != 0) + diag_raise(); + } + if (stmt->new_tuple != NULL) { + uint32_t trigger_name_len; + const char *trigger_name_src = + tuple_field_str_xc(stmt->new_tuple, 0, + &trigger_name_len); + const char *space_opts = + tuple_field_with_type_xc(stmt->new_tuple, 1, MP_MAP); + struct space_opts opts; + struct region *region = &fiber()->gc; + space_opts_decode(&opts, space_opts, region); + struct Trigger *trigger = + sql_trigger_compile(sql_get(), opts.sql); + if (trigger == NULL) + diag_raise(); + auto trigger_guard = make_scoped_guard([=] { + sql_trigger_delete(sql_get(), trigger); + }); + if (sql_trigger_insert(trigger, trigger_name_src, + trigger_name_len) != 0) + diag_raise(); + trigger_guard.is_active = false; + } } 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/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 0eded6f..6ecb226 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1223,9 +1223,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, @@ -1294,42 +1291,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) == 2); - - field = tuple_field(tuple, 0); - assert (field != NULL); - ptr = mp_decode_str(&field, &len); - name = strndup(ptr, len); - - field = tuple_field(tuple, 1); - 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); } /********************************************************************* @@ -1737,6 +1698,15 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) return space->def->fields[fieldno].default_value_expr; } +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; +} + struct space_def * sql_ephemeral_space_def_new(Parse *parser, const char *name) { diff --git a/src/box/sql.h b/src/box/sql.h index 35aacc3..f4d909f 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,39 @@ void sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger); /** + * Remove a trigger from the hash tables of the sqlite* pointer. + * @param db SQL handle. + * @param name a name of trigger object to delete. + * @param name_len length of the name. + * + * @retval 0 on success. + * @retval -1 on error. + */ +int +sql_trigger_delete_by_name(struct sqlite3 *db, const char *name, int name_len); + +/** + * 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 insert trigger in appropriate space. + * @param trigger object to append. + * @param name a name of trigger object to insert. + * @param name_len length of the name. + * + * @retval 0 on success. + * @retval -1 on error. + */ +int +sql_trigger_insert(struct Trigger *trigger, const char *name, int name_len); + +/** * 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 28e4d7a..40f693b 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2289,16 +2289,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..d5585cf 100644 --- a/src/box/sql/fkey.c +++ b/src/box/sql/fkey.c @@ -1440,7 +1440,6 @@ fkActionTrigger(Parse * pParse, /* Parse context */ } 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 ecbd573..4c07480 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. */ @@ -3032,14 +3031,14 @@ struct Parse { */ struct Trigger { char *zName; /* The name of the trigger */ - char *table; /* The table or view to which the trigger applies */ + /** The ID of space the triggers is refer to. */ + uint32_t space_id; 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 */ }; @@ -4027,7 +4026,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 9b65064..0c851c9 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -39,6 +39,7 @@ #include <stdlib.h> #include <unicode/utf8.h> #include <unicode/uchar.h> +#include "box/schema.h" #include "say.h" #include "sqliteInt.h" @@ -122,6 +123,7 @@ static const char sql_ascii_class[] = { * the #include below. */ #include "keywordhash.h" +#include "tarantoolInt.h" #define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0) @@ -510,8 +512,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()) { + error = tarantoolErrorMessage(); + } else { + error = sqlite3ErrStr(pParse->rc); + } + pParse->zErrMsg = sqlite3MPrintf(db, "%s", error); } assert(pzErrMsg != 0); if (pParse->zErrMsg) { @@ -528,7 +535,12 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg) if (pParse->pWithToFree) sqlite3WithDelete(db, pParse->pWithToFree); - sql_trigger_delete(db, pParse->pNewTrigger); + /* + * Trigger is exporting 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; @@ -569,3 +581,23 @@ cleanup: 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_EXECUTE, sql); + goto cleanup; + } + trigger = parser.pNewTrigger; +cleanup: + sql_parser_destroy(&parser); + return trigger; +} diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index 053dadb..beaad11 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -33,6 +33,8 @@ * This file contains the implementation for TRIGGERs */ +#include "box/box.h" +#include "box/schema.h" #include "sqliteInt.h" #include "tarantoolInt.h" #include "vdbeInt.h" @@ -81,102 +83,121 @@ 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 */ + /* Table that the trigger fires off of. */ + struct Table *table = NULL; 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 != 0); assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE); assert(op > 0 && op < 0xff); - if (!pTableName || db->mallocFailed) { + if (!pTableName || 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)) 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)) { - goto trigger_cleanup; - } - if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) { - if (!noErr) { - sqlite3ErrorMsg(pParse, "trigger %s already exists", - zName); - } else { - assert(!db->init.busy); - } + if (zName == NULL) 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"); + if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK) 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); - goto trigger_cleanup; - } - if (!space_is_view(pTab) && tr_tm == TK_INSTEAD) { - sqlite3ErrorMsg(pParse, "cannot create INSTEAD OF" - " trigger on table: %S", pTableName, 0); - goto trigger_cleanup; + /* FIXME: Move all checks in VDBE #3435. */ + if (!pParse->parse_only) { + table = sql_list_lookup_table(pParse, pTableName); + if (table == NULL) + goto trigger_cleanup; + + if (sqlite3HashFind(&db->pSchema->trigHash, zName)) { + if (!noErr) { + 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(table->def->name, "sqlite_", 7) == 0) { + sqlite3ErrorMsg(pParse, + "cannot create trigger on system table"); + goto trigger_cleanup; + } + + /* + * INSTEAD of triggers are only for views and + * views only support INSTEAD of triggers. + */ + if (table->def->opts.is_view && tr_tm != TK_INSTEAD) { + sqlite3ErrorMsg(pParse, + "cannot create %s trigger on view: %S", + (tr_tm == TK_BEFORE) ? + "BEFORE" : "AFTER", + pTableName, 0); + goto trigger_cleanup; + } + if (!table->def->opts.is_view && tr_tm == TK_INSTEAD) { + sqlite3ErrorMsg(pParse, + "cannot create INSTEAD OF trigger " + "on table: %S", pTableName, 0); + 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 */ + /* Build the Trigger object. */ pTrigger = (Trigger *) sqlite3DbMallocZero(db, sizeof(Trigger)); if (pTrigger == 0) goto trigger_cleanup; pTrigger->zName = zName; zName = 0; - pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName); + /* Initialize space_id on trigger insert. */ + if (schema_find_id(BOX_SPACE_ID, 2, pTableName->a[0].zName, + strlen(pTableName->a[0].zName), + &pTrigger->space_id) != 0) { + pParse->rc = SQL_TARANTOOL_ERROR; + goto trigger_cleanup; + } pTrigger->pSchema = db->pSchema; - pTrigger->pTabSchema = pTab->pSchema; 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); + if ((pWhen != NULL && pTrigger->pWhen == NULL) || + (pColumns != NULL && pTrigger->pColumns == NULL)) + goto trigger_cleanup; assert(pParse->pNewTrigger == 0); pParse->pNewTrigger = pTrigger; @@ -210,7 +231,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; @@ -227,10 +248,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-parsig mode or export trigger. */ - if (!db->init.busy) { + if (!pParse->parse_only) { Vdbe *v; int zOptsSz; Table *pSysTrigger; @@ -288,37 +310,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: @@ -329,7 +325,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); } @@ -478,7 +474,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); @@ -533,16 +528,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 @@ -565,34 +550,68 @@ 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_delete_by_name(struct sqlite3 *db, const char *name, int name_len) { - Trigger *pTrigger; - Hash *pHash; - struct session *user_session = current_session(); + /* Name from Tarantool tuple may be not normalized. */ + uint32_t used = region_used(&fiber()->gc); + char *trigger_name = region_alloc(&fiber()->gc, name_len + 3); + if (trigger_name == NULL) { + diag_set(OutOfMemory, name_len + 3, "region_alloc", + "trigger_name"); + return -1; + } + memcpy(trigger_name, name, name_len); + trigger_name[name_len] = 0; + + struct Hash *hash = &(db->pSchema->trigHash); + struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL); + assert(trigger != NULL); + + struct space *space = space_by_id(trigger->space_id); + /* Space could be already deleted. */ + if (space != NULL) { + struct Trigger **pp; + for (pp = &space->sql_triggers; + *pp != trigger; + pp = &((*pp)->pNext)); + *pp = (*pp)->pNext; + } - 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; + sql_trigger_delete(db, trigger); + region_truncate(&fiber()->gc, used); + return 0; +} + +int +sql_trigger_insert(struct Trigger *trigger, const char *name, int name_len) +{ + if (strncmp(name, trigger->zName, name_len) != 0) { + diag_set(ClientError, ER_SQL, + "tuple trigger name does not match extracted from SQL"); + return -1; + } + + struct space *space = space_cache_find(trigger->space_id); + assert(space != NULL); + struct Hash *hash = &trigger->pSchema->trigHash; + if (sqlite3HashFind(hash, trigger->zName) != NULL) { + diag_set(ClientError, ER_TRIGGER_EXISTS, trigger->zName); + return -1; + } + + void *ret = sqlite3HashInsert(hash, trigger->zName, trigger); + if (ret == trigger) { + diag_set(OutOfMemory, 0, "sqlite3HashInsert", "ret"); + return -1; } + assert(ret == NULL); + trigger->pNext = space->sql_triggers; + space->sql_triggers = trigger; + return 0; } /* @@ -631,22 +650,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) *pMask = mask; - } - return (mask ? pList : 0); + return mask ? trigger_list : 0; } /* @@ -835,7 +849,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 @@ -952,7 +966,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 @@ -1077,14 +1091,7 @@ 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 diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 3fe5875..bba9bf1 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 trigger 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/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..ed6ee36 100755 --- a/test/sql-tap/trigger1.test.lua +++ b/test/sql-tap/trigger1.test.lua @@ -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> }) diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua new file mode 100644 index 0000000..23bdcfd --- /dev/null +++ b/test/sql/triggers.test.lua @@ -0,0 +1,72 @@ +env = require('test_run') +test_run = env.new() +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '") + +-- +-- 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; ]]) +box.space._trigger:select() + +-- checks for LUA tuples +tuple = {"T1T", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +box.space._trigger:insert(tuple) + +tuple = {"T1t", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +box.space._trigger:insert(tuple) + +tuple = {"T1T", {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +box.space._trigger:insert(tuple) + +tuple = {"T1t", {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +box.space._trigger:insert(tuple) + +box.space._trigger:select() + + +-- 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", {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}} +box.space._trigger:insert(tuple) +tuple = {"T3T", {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}} +box.space._trigger:insert(tuple) +box.space._trigger:select() +box.sql.execute("INSERT INTO t1 VALUES(2);") +box.sql.execute("SELECT * FROM t2;") +box.sql.execute("DELETE FROM t2;") + +-- test t1t after t2t and t3t drop +box.sql.execute("DROP TRIGGER T2T;") +box.space._trigger:delete("T2T") +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; ]]) +box.space._trigger:select() +box.sql.execute("INSERT INTO t1 VALUES(4);") +box.sql.execute("SELECT * FROM t2;") + +-- clean up +box.space._trigger:delete("t1t") +box.space._trigger:delete("t2t") +box.space._trigger:delete("t3t") +box.sql.execute("DROP TABLE t1;") +box.sql.execute("DROP TABLE t2;") +box.space._trigger:select() + + +test_run:cmd("clear filter") -- 2.7.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server 2018-06-01 20:25 ` Kirill Shcherbatov @ 2018-06-04 13:27 ` Vladislav Shpilevoy 2018-06-04 19:21 ` Kirill Shcherbatov 0 siblings, 1 reply; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-06-04 13:27 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches Thanks for the patch! At first, please, do not mess the comments. Try to respond on them in the same order as I had wrote. At second, please, do not skip them. At third, check the travis - it fails on box/misc test. See 22 comments below. Most of them are one-line minors. On 01/06/2018 23:25, Kirill Shcherbatov wrote: > From c0775801fcdb475ee6e77dff483f4f6a84c1afc3 Mon Sep 17 00:00:00 2001 > Message-Id: <c0775801fcdb475ee6e77dff483f4f6a84c1afc3.1527884700.git.kshcherbatov@gmail.com> > From: Kirill Shcherbatov <kshcherbatov@tarantool.org> > Date: Wed, 30 May 2018 16:03:43 +0300 > Subject: [PATCH] sql: move Triggers to server 1. Please, do not send SMTP headers in the body. > > 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 | 37 ++++- > src/box/errcode.h | 2 +- > src/box/space.h | 2 + > src/box/sql.c | 48 ++---- > src/box/sql.h | 44 ++++++ > src/box/sql/build.c | 8 +- > src/box/sql/fkey.c | 1 - > src/box/sql/insert.c | 6 +- > src/box/sql/sqliteInt.h | 6 +- > src/box/sql/tokenize.c | 38 ++++- > src/box/sql/trigger.c | 287 +++++++++++++++++----------------- > src/box/sql/vdbe.c | 76 ++------- > src/box/sql/vdbe.h | 1 - > src/box/sql/vdbeaux.c | 9 -- > test/sql-tap/identifier_case.test.lua | 4 +- > test/sql-tap/trigger1.test.lua | 4 +- > test/sql/triggers.test.lua | 72 +++++++++ > 17 files changed, 370 insertions(+), 275 deletions(-) > create mode 100644 test/sql/triggers.test.lua > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index f2bf85d..bf170a5 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -551,6 +551,9 @@ 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. */ > + new_space->sql_triggers = old_space->sql_triggers; > + old_space->sql_triggers = NULL; 2. I just have noticed it is not actual swap. Here new_space->sql_triggers are forgotten. It leads to bug on rollback, when swap_triggers_is_called again to swap new/old back. > } > > /** > @@ -732,7 +735,6 @@ alter_space_commit(struct trigger *trigger, void *event) > } > > trigger_run_xc(&on_alter_space, alter->new_space); > - > alter->new_space = NULL; /* for alter_space_delete(). */ 3. Garbage diff. > /* > * Delete the old version of the space, we are not > @@ -3100,6 +3102,39 @@ 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); > + if (stmt->old_tuple != NULL) { > + uint32_t trigger_name_len; > + const char *trigger_name_src = > + tuple_field_str_xc(stmt->old_tuple, 0, > + &trigger_name_len); > + if (sql_trigger_delete_by_name(sql_get(), trigger_name_src, > + trigger_name_len) != 0) > + diag_raise(); > + } 4. You still have not fixed the point '3.' from the previous review. If WAL write fails, you loss the deleted trigger, but must restore it back. > + if (stmt->new_tuple != NULL) { > + uint32_t trigger_name_len; > + const char *trigger_name_src = > + tuple_field_str_xc(stmt->new_tuple, 0, > + &trigger_name_len); > + const char *space_opts = > + tuple_field_with_type_xc(stmt->new_tuple, 1, MP_MAP); > + struct space_opts opts; > + struct region *region = &fiber()->gc; > + space_opts_decode(&opts, space_opts, region); > + struct Trigger *trigger = > + sql_trigger_compile(sql_get(), opts.sql); > + if (trigger == NULL) > + diag_raise(); > + auto trigger_guard = make_scoped_guard([=] { > + sql_trigger_delete(sql_get(), trigger); > + }); > + if (sql_trigger_insert(trigger, trigger_name_src, > + trigger_name_len) != 0) > + diag_raise(); > + trigger_guard.is_active = false; 5. Why do you need the guard here? You can delete trigger explicitly before diag_raise() if insert() != 0. > + } > } > > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index ecbd573..4c07480 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. */ > @@ -3032,14 +3031,14 @@ struct Parse { > */ > struct Trigger { > char *zName; /* The name of the trigger */ > - char *table; /* The table or view to which the trigger applies */ > + /** The ID of space the triggers is refer to. */ 6. triggers? There is one trigger. And 'is refer to' is not correct sentence. 'Refer' is a verb. > + uint32_t space_id; > 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 */ > }; > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index 9b65064..0c851c9 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -39,6 +39,7 @@ > #include <stdlib.h> > #include <unicode/utf8.h> > #include <unicode/uchar.h> > +#include "box/schema.h" > > #include "say.h" > #include "sqliteInt.h" > @@ -122,6 +123,7 @@ static const char sql_ascii_class[] = { > * the #include below. > */ > #include "keywordhash.h" > +#include "tarantoolInt.h" > > #define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0) > > @@ -510,8 +512,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()) { 7. != NULL > + error = tarantoolErrorMessage(); > + } else { > + error = sqlite3ErrStr(pParse->rc); > + } 8. Do not use {} when both 'if' and 'else' are one lines. > + pParse->zErrMsg = sqlite3MPrintf(db, "%s", error); > } > assert(pzErrMsg != 0); > if (pParse->zErrMsg) { > @@ -528,7 +535,12 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > > if (pParse->pWithToFree) > sqlite3WithDelete(db, pParse->pWithToFree); > - sql_trigger_delete(db, pParse->pNewTrigger); > + /* > + * Trigger is exporting with pNewTrigger field when 9. is exported. > + * 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; > @@ -569,3 +581,23 @@ cleanup: > 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_EXECUTE, sql); 10. Same as in the previous patch: error and sql_error are unused. > + goto cleanup; > + } > + trigger = parser.pNewTrigger; > +cleanup: 11. You should avoid 'goto cleanup' here. > + sql_parser_destroy(&parser); > + return trigger; > +} > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index 053dadb..beaad11 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -33,6 +33,8 @@ > * This file contains the implementation for TRIGGERs > */ > > +#include "box/box.h" 12. Unused. > +#include "box/schema.h" > #include "sqliteInt.h" > #include "tarantoolInt.h" > #include "vdbeInt.h" > @@ -81,102 +83,121 @@ 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 */ > + /* Table that the trigger fires off of. */ > + struct Table *table = NULL; > 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 != 0); > assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE); > assert(op > 0 && op < 0xff); > > - if (!pTableName || db->mallocFailed) { > + if (!pTableName || 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)) 13. != 0 > > - /* 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 */ > + /* Build the Trigger object. */ > pTrigger = (Trigger *) sqlite3DbMallocZero(db, sizeof(Trigger)); > if (pTrigger == 0) > goto trigger_cleanup; > pTrigger->zName = zName; > zName = 0; > - pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName); > + /* Initialize space_id on trigger insert. */ > + if (schema_find_id(BOX_SPACE_ID, 2, pTableName->a[0].zName, > + strlen(pTableName->a[0].zName), 14. Wrong indentation. > + &pTrigger->space_id) != 0) { > + pParse->rc = SQL_TARANTOOL_ERROR; > + goto trigger_cleanup; > + } > pTrigger->pSchema = db->pSchema; > - pTrigger->pTabSchema = pTab->pSchema; > 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); > + if ((pWhen != NULL && pTrigger->pWhen == NULL) || > + (pColumns != NULL && pTrigger->pColumns == NULL)) 15. Same. > + goto trigger_cleanup; > assert(pParse->pNewTrigger == 0); > pParse->pNewTrigger = pTrigger; > > @@ -227,10 +248,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-parsig mode or export trigger. 16. typo > */ > - if (!db->init.busy) { > + if (!pParse->parse_only) { > Vdbe *v; > int zOptsSz; > Table *pSysTrigger; > @@ -565,34 +550,68 @@ 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_delete_by_name(struct sqlite3 *db, const char *name, int name_len) > { > - Trigger *pTrigger; > - Hash *pHash; > - struct session *user_session = current_session(); > + /* Name from Tarantool tuple may be not normalized. */ > + uint32_t used = region_used(&fiber()->gc); > + char *trigger_name = region_alloc(&fiber()->gc, name_len + 3); 17. Why +3? > + if (trigger_name == NULL) { > + diag_set(OutOfMemory, name_len + 3, "region_alloc", > + "trigger_name"); > + return -1; > + } > + memcpy(trigger_name, name, name_len); > + trigger_name[name_len] = 0; > + > + struct Hash *hash = &(db->pSchema->trigHash); > + struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL); > + assert(trigger != NULL); 18. Why != NULL? Can't sqlite3HashInsert fail? > + > + struct space *space = space_by_id(trigger->space_id); > + /* Space could be already deleted. */ > + if (space != NULL) { > + struct Trigger **pp; > + for (pp = &space->sql_triggers; > + *pp != trigger; > + pp = &((*pp)->pNext)); 19. Can't it fit in a single line? > + *pp = (*pp)->pNext; > + } > > +int > +sql_trigger_insert(struct Trigger *trigger, const char *name, int name_len) > +{ > + if (strncmp(name, trigger->zName, name_len) != 0) { > + diag_set(ClientError, ER_SQL, > + "tuple trigger name does not match extracted from SQL"); 20. There are no tuples. Please, do this check in alter.cc. For this you can declare trigger_name() helper. > + return -1; > + } > + > + struct space *space = space_cache_find(trigger->space_id); > + assert(space != NULL); > + struct Hash *hash = &trigger->pSchema->trigHash; > + if (sqlite3HashFind(hash, trigger->zName) != NULL) { > + diag_set(ClientError, ER_TRIGGER_EXISTS, trigger->zName);> + return -1; > + } > + > /* > @@ -631,22 +650,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) 21. != NULL > *pMask = mask; > - } > - return (mask ? pList : 0); > + return mask ? trigger_list : 0; > } > > /* > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 3fe5875..bba9bf1 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); > + 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 trigger created on this table. */ 22. triggers > + 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; ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server 2018-06-04 13:27 ` Vladislav Shpilevoy @ 2018-06-04 19:21 ` Kirill Shcherbatov 2018-06-05 13:31 ` Vladislav Shpilevoy 0 siblings, 1 reply; 23+ messages in thread From: Kirill Shcherbatov @ 2018-06-04 19:21 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy > At third, check the travis - it fails on box/misc test. > 1. Please, do not send SMTP headers in the body. ok. > 2. I just have noticed it is not actual swap. Here new_space->sql_triggers are > forgotten. It leads to bug on rollback, when swap_triggers_is_called again > to swap new/old back. new_space->sql_triggers = old_space->sql_triggers; - old_space->sql_triggers = NULL; > 3. Garbage diff. dropped > 4. You still have not fixed the point '3.' from the previous review. > If WAL write fails, you loss the deleted trigger, but must restore it back. The patch at the end of this message contains this changes. > 5. Why do you need the guard here? You can delete trigger explicitly before > diag_raise() if insert() != 0. @@ -3127,13 +3127,11 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) sql_trigger_compile(sql_get(), opts.sql); if (trigger == NULL) diag_raise(); - auto trigger_guard = make_scoped_guard([=] { - sql_trigger_delete(sql_get(), trigger); - }); if (sql_trigger_insert(trigger, trigger_name_src, - trigger_name_len) != 0) + trigger_name_len) != 0) { + sql_trigger_delete(sql_get(), trigger); diag_raise(); - trigger_guard.is_active = false; + } } } > 6. triggers? There is one trigger. And 'is refer to' is not correct sentence. 'Refer' > is a verb. - /** The ID of space the triggers is refer to. */ + /** The ID of space the trigger is refer to. */ > 7. != NULL > 8. Do not use {} when both 'if' and 'else' are one lines. - if (is_tarantool_error(pParse->rc) && tarantoolErrorMessage()) { + if (is_tarantool_error(pParse->rc) && + tarantoolErrorMessage() != NULL) error = tarantoolErrorMessage(); - } else { + else error = sqlite3ErrStr(pParse->rc); - } > 9. is exported. - * Trigger is exporting with pNewTrigger field when + * Trigger is exported with pNewTrigger field when > 10. Same as in the previous patch: error and sql_error are unused. > 11. You should avoid 'goto cleanup' here. - goto cleanup; + } else { + trigger = parser.pNewTrigger; } - trigger = parser.pNewTrigger; -cleanup: sql_parser_destroy(&parser); return trigger; } >> +#include "box/box.h" > 12. Unused. -#include "box/box.h" >> + if (sqlite3FixSrcList(&sFix, pTableName)) > > 13. != 0 + if (sqlite3FixSrcList(&sFix, pTableName) != 0) >> + if (schema_find_id(BOX_SPACE_ID, 2, pTableName->a[0].zName, >> + strlen(pTableName->a[0].zName), > > 14. Wrong indentation. This place is slightly different now: + 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; + } >> + if ((pWhen != NULL && pTrigger->pWhen == NULL) || >> + (pColumns != NULL && pTrigger->pColumns == NULL)) > > 15. Same. if ((pWhen != NULL && pTrigger->pWhen == NULL) || - (pColumns != NULL && pTrigger->pColumns == NULL)) + (pColumns != NULL && pTrigger->pColumns == NULL)) > 16. typo - * Tarantool for non-parsig mode or export trigger. + * Tarantool for non-parsing mode or export trigger. >> + uint32_t used = region_used(&fiber()->gc); >> + char *trigger_name = region_alloc(&fiber()->gc, name_len + 3); > > 17. Why +3? Some legacy... Fixed, this place is differ now. + tuple_field_str_xc(old_tuple, 0, &trigger_name_len); + 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 Hash *hash = &(db->pSchema->trigHash); >> + struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL); >> + assert(trigger != NULL); > > 18. Why != NULL? Can't sqlite3HashInsert fail? It was assert for inconsistent tarantool state. This place is differ now, it is not actual. > >> + >> + struct space *space = space_by_id(trigger->space_id); >> + /* Space could be already deleted. */ >> + if (space != NULL) { >> + struct Trigger **pp; >> + for (pp = &space->sql_triggers; >> + *pp != trigger; >> + pp = &((*pp)->pNext)); > > 19. Can't it fit in a single line? Accounted, this place is differ now. > >> +int >> +sql_trigger_insert(struct Trigger *trigger, const char *name, int name_len) >> +{ >> + if (strncmp(name, trigger->zName, name_len) != 0) { >> + diag_set(ClientError, ER_SQL, >> + "tuple trigger name does not match extracted from SQL"); > > 20. There are no tuples. Please, do this check in alter.cc. For this you can > declare trigger_name() helper. Accounted, implemented in on_replace_dd_trigger. >> int mask = 0; >> - if (pMask) { >> + if (pMask) > > 21. != NULL if (pMask != 0) *pMask = mask; > 22. triggers + /* Rename all triggers created on this table. */ ======================================================================= 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 | 140 ++++++++++++++++++ src/box/errcode.h | 2 +- src/box/space.h | 2 + src/box/sql.c | 48 ++----- src/box/sql.h | 42 ++++++ src/box/sql/build.c | 8 +- src/box/sql/fkey.c | 2 - src/box/sql/insert.c | 6 +- src/box/sql/sqliteInt.h | 7 +- src/box/sql/tokenize.c | 28 +++- src/box/sql/trigger.c | 259 ++++++++++++++++------------------ 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 | 187 ++++++++++++++++++++++++ test/sql/triggers.test.lua | 69 +++++++++ 19 files changed, 629 insertions(+), 276 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..23cd4a3 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -551,6 +551,8 @@ 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. */ + new_space->sql_triggers = old_space->sql_triggers; } /** @@ -3091,6 +3093,55 @@ 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 txn_stmt *stmt = txn_last_stmt((struct txn*) event); + struct Trigger *old_trigger = (struct Trigger *)trigger->data; + + if (stmt->old_tuple != NULL) { + /* DELETE, REPLACE trigger. */ + sql_trigger_delete(sql_get(), old_trigger); + } +} + /** * A trigger invoked on replace in a space containing * SQL triggers. @@ -3100,6 +3151,95 @@ 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); + + char *trigger_name = NULL; + struct Trigger *new_trigger = NULL; + if (old_tuple != NULL) { + /* DROP, REPLACE trigger. */ + uint32_t trigger_name_len; + const char *trigger_name_src = + tuple_field_str_xc(old_tuple, 0, &trigger_name_len); + 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; + } + if (new_tuple != NULL) { + /* 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, 1, MP_MAP); + struct space_opts opts; + struct region *region = &fiber()->gc; + space_opts_decode(&opts, space_opts, region); + new_trigger = sql_trigger_compile(sql_get(), opts.sql); + if (new_trigger == NULL) + diag_raise(); + + if (strncmp(trigger_name_src, sql_trigger_name(new_trigger), + 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"); + } + trigger_name = sql_trigger_name(new_trigger); + } + + if (old_tuple != NULL && new_tuple == NULL) { + /* DELETE trigger. */ + 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 if (new_tuple != NULL && old_tuple == NULL) { + /* INSERT trigger. */ + 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(); + } + assert(old_trigger == NULL); + + on_commit->data = NULL; + on_rollback->data = new_trigger; + } else { + /* REPLACE trigger. */ + 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(); + } + assert(old_trigger != NULL); + + on_commit->data = old_trigger; + on_rollback->data = old_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/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 0eded6f..6ecb226 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1223,9 +1223,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, @@ -1294,42 +1291,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) == 2); - - field = tuple_field(tuple, 0); - assert (field != NULL); - ptr = mp_decode_str(&field, &len); - name = strndup(ptr, len); - - field = tuple_field(tuple, 1); - 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); } /********************************************************************* @@ -1737,6 +1698,15 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) return space->def->fields[fieldno].default_value_expr; } +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; +} + struct space_def * sql_ephemeral_space_def_new(Parse *parser, const char *name) { diff --git a/src/box/sql.h b/src/box/sql.h index 35aacc3..885f0a7 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,37 @@ 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. + */ +char * +sql_trigger_name(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 28e4d7a..40f693b 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2289,16 +2289,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 ecbd573..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. */ @@ -3032,14 +3031,13 @@ struct Parse { */ struct Trigger { char *zName; /* The name of the trigger */ - char *table; /* The table or view to which the trigger applies */ + /** The ID of space the trigger is refer to. */ + uint32_t space_id; 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 */ }; @@ -4027,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 817a282..d769d14 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -39,6 +39,7 @@ #include <stdlib.h> #include <unicode/utf8.h> #include <unicode/uchar.h> +#include "box/schema.h" #include "say.h" #include "sqliteInt.h" @@ -123,6 +124,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 +536,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 +582,22 @@ cleanup: 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, 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 053dadb..a027453 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,102 +82,127 @@ 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 != 0); assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE); assert(op > 0 && op < 0xff); - if (!pTableName || db->mallocFailed) { + if (!pTableName || 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(zName, "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)); + /* Build the Trigger object. */ + pTrigger = (Trigger *)sqlite3DbMallocZero(db, sizeof(Trigger)); if (pTrigger == 0) goto trigger_cleanup; + pTrigger->space_id = space_id; pTrigger->zName = zName; zName = 0; - pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName); - pTrigger->pSchema = db->pSchema; - pTrigger->pTabSchema = pTab->pSchema; + 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); + if ((pWhen != NULL && pTrigger->pWhen == NULL) || + (pColumns != NULL && pTrigger->pColumns == NULL)) + goto trigger_cleanup; assert(pParse->pNewTrigger == 0); pParse->pNewTrigger = pTrigger; @@ -185,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); - } } /* @@ -210,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; @@ -227,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; @@ -288,37 +314,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: @@ -329,7 +329,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); } @@ -478,7 +478,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); @@ -533,16 +532,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 @@ -565,34 +554,44 @@ 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; +} + +char * +sql_trigger_name(struct Trigger *trigger) +{ + return trigger->zName; } /* @@ -631,22 +630,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 ? trigger_list : 0; } /* @@ -835,7 +829,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 @@ -952,7 +946,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 @@ -1077,15 +1071,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..353a3c2 --- /dev/null +++ b/test/sql/triggers.result @@ -0,0 +1,187 @@ +env = require('test_run') +--- +... +test_run = env.new() +--- +... +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '") +--- +- true +... +-- +-- 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; ]]) +--- +... +box.space._trigger:select() +--- +- - ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); + END; '}] +... +-- checks for LUA tuples +tuple = {"T1T", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +--- +... +box.space._trigger:insert(tuple) +--- +- error: Duplicate key exists in unique index 'primary' in space '_trigger' +... +tuple = {"T1t", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +--- +... +box.space._trigger:insert(tuple) +--- +- error: 'SQL error: tuple trigger name does not match extracted from SQL' +... +tuple = {"T1t", {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +--- +... +box.space._trigger:insert(tuple) +--- +- error: 'SQL error: tuple trigger name does not match extracted from SQL' +... +box.space._trigger:select() +--- +- - ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); + END; '}] +... +-- 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", {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}} +--- +... +box.space._trigger:insert(tuple) +--- +- ['T2T', {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); + END;'}] +... +tuple = {"T3T", {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}} +--- +... +box.space._trigger:insert(tuple) +--- +- ['T3T', {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); + END;'}] +... +box.space._trigger:select() +--- +- - ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); + END; '}] + - ['T2T', {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); + END;'}] + - ['T3T', {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); + END;'}] +... +box.sql.execute("INSERT INTO t1 VALUES(2);") +--- +... +box.sql.execute("SELECT * FROM t2;") +--- +- - [1] + - [2] + - [3] +... +box.sql.execute("DELETE FROM t2;") +--- +... +-- test t1t after t2t and t3t drop +box.sql.execute("DROP TRIGGER T2T;") +--- +... +box.space._trigger:delete("T3T") +--- +- ['T3T', {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); + END;'}] +... +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; ]]) +--- +... +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.space._trigger:delete("T1T") +--- +- ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); + END; '}] +... +box.space._trigger:delete("T2T") +--- +- ['T2T', {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); + END; '}] +... +box.space._trigger:delete("T3T") +--- +- ['T3T', {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); + END; '}] +... +box.sql.execute("DROP TABLE t1;") +--- +... +box.sql.execute("DROP TABLE t2;") +--- +... +box.space._trigger:select() +--- +- [] +... +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..a63a71b --- /dev/null +++ b/test/sql/triggers.test.lua @@ -0,0 +1,69 @@ +env = require('test_run') +test_run = env.new() +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '") + +-- +-- 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; ]]) +box.space._trigger:select() + +-- checks for LUA tuples +tuple = {"T1T", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +box.space._trigger:insert(tuple) + +tuple = {"T1t", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +box.space._trigger:insert(tuple) + +tuple = {"T1t", {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} +box.space._trigger:insert(tuple) + +box.space._trigger:select() + + +-- 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", {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}} +box.space._trigger:insert(tuple) +tuple = {"T3T", {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}} +box.space._trigger:insert(tuple) +box.space._trigger:select() +box.sql.execute("INSERT INTO t1 VALUES(2);") +box.sql.execute("SELECT * FROM t2;") +box.sql.execute("DELETE FROM t2;") + +-- test t1t after t2t and t3t drop +box.sql.execute("DROP TRIGGER T2T;") +box.space._trigger:delete("T3T") +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; ]]) +box.space._trigger:select() +box.sql.execute("INSERT INTO t1 VALUES(4);") +box.sql.execute("SELECT * FROM t2;") + +-- clean up +box.space._trigger:delete("T1T") +box.space._trigger:delete("T2T") +box.space._trigger:delete("T3T") +box.sql.execute("DROP TABLE t1;") +box.sql.execute("DROP TABLE t2;") +box.space._trigger:select() + + +test_run:cmd("clear filter") -- 2.7.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server 2018-06-04 19:21 ` Kirill Shcherbatov @ 2018-06-05 13:31 ` Vladislav Shpilevoy 2018-06-09 9:32 ` Kirill Shcherbatov 0 siblings, 1 reply; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-06-05 13:31 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches Thanks for the fixes! See 12 comments below. I have not reviewed the whole patch since you did not fix some of the previous comments, and I have found new bugs. > 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 | 140 ++++++++++++++++++ > src/box/errcode.h | 2 +- > src/box/space.h | 2 + > src/box/sql.c | 48 ++----- > src/box/sql.h | 42 ++++++ > src/box/sql/build.c | 8 +- > src/box/sql/fkey.c | 2 - > src/box/sql/insert.c | 6 +- > src/box/sql/sqliteInt.h | 7 +- > src/box/sql/tokenize.c | 28 +++- > src/box/sql/trigger.c | 259 ++++++++++++++++------------------ > 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 | 187 ++++++++++++++++++++++++ > test/sql/triggers.test.lua | 69 +++++++++ > 19 files changed, 629 insertions(+), 276 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..23cd4a3 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -551,6 +551,8 @@ 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. */ > + new_space->sql_triggers = old_space->sql_triggers; 1. It is not swap still. Swap of A and B means that before swap A == value1, B = value2. After swap A == value2, B == value1. Here you have done this: A == B. Do you understand why exactly swap is needed here? Please, answer. Do not skip the question. 2. Space->sql_triggers leaks when I drop a space via Lua. 3. After you fix the leak, try this test to simulate WAL error and you will see a crash: box.cfg{} 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);") > } > > /** > @@ -3091,6 +3093,55 @@ 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); 4. Fits in 3 lines. > + (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); > + } > +} > + > + 5. Extra new line. > +static void > +triggers_task_commit(struct trigger *trigger, void *event) > +{ > + struct txn_stmt *stmt = txn_last_stmt((struct txn*) event); > + struct Trigger *old_trigger = (struct Trigger *)trigger->data; > + > + if (stmt->old_tuple != NULL) { 6. Why do you need stmt and stmt->old_tuple here? As I see, old_trigger != NULL is the same as stmt->old_tuple != NULL. > + /* DELETE, REPLACE trigger. */ > + sql_trigger_delete(sql_get(), old_trigger); > + } > +} > + > @@ -3100,6 +3151,95 @@ 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); > + > + char *trigger_name = NULL; > + struct Trigger *new_trigger = NULL; 7. Please, do not create two branches for each case. You can make this function more compact. > + if (new_tuple != NULL) { > + /* 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, 1, MP_MAP); > + struct space_opts opts; > + struct region *region = &fiber()->gc; > + space_opts_decode(&opts, space_opts, region); > + new_trigger = sql_trigger_compile(sql_get(), opts.sql); > + if (new_trigger == NULL) > + diag_raise(); > + > + if (strncmp(trigger_name_src, sql_trigger_name(new_trigger), > + 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"); > + } > + trigger_name = sql_trigger_name(new_trigger); > + } > + > + if (old_tuple != NULL && new_tuple == NULL) { > + /* DELETE trigger. */ > + 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 if (new_tuple != NULL && old_tuple == NULL) { > + /* INSERT trigger. */ > + 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(); > + } > + assert(old_trigger == NULL); > + > + on_commit->data = NULL; > + on_rollback->data = new_trigger; > + } else { > + /* REPLACE trigger. */ > + 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(); > + } > + assert(old_trigger != NULL); > + > + on_commit->data = old_trigger; > + on_rollback->data = old_trigger; > + } > + > + txn_on_rollback(txn, on_rollback); > + txn_on_commit(txn, on_commit); > + > + 8. Two extra lines. > } > > diff --git a/src/box/sql.h b/src/box/sql.h > index 35aacc3..885f0a7 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > +/** > + * Get trigger name by trigger AST object. > + * @param trigger AST object. > + * @return trigger name string. > + */ > +char * > +sql_trigger_name(struct Trigger *trigger); 9. Const char *. > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index 817a282..d769d14 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -39,6 +39,7 @@ > #include <stdlib.h> > #include <unicode/utf8.h> > #include <unicode/uchar.h> > +#include "box/schema.h" 10. Why? > > #include "say.h" > #include "sqliteInt.h" > @@ -123,6 +124,7 @@ static const char sql_ascii_class[] = { > * the #include below. > */ > #include "keywordhash.h" > +#include "tarantoolInt.h" 11. Same. > > #define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0) > > @@ -575,3 +582,22 @@ cleanup: > 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, sql_error); 12. Still is not fixed from the previous review. 'Error' is unused. And why do you need snprintf here? Please, do another iteration of self-review before sending the patch again. Remove all unused includes, extra lines, unused variables, unnecessary calls, fix alignment etc. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server 2018-06-05 13:31 ` Vladislav Shpilevoy @ 2018-06-09 9:32 ` Kirill Shcherbatov 0 siblings, 0 replies; 23+ messages in thread From: Kirill Shcherbatov @ 2018-06-09 9:32 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy I'll send 2.0 patchset, > 1. It is not swap still. Swap of A and B means that before swap > A == value1, B = value2. After swap A == value2, B == value1. > Here you have done this: > A == B. > > Do you understand why exactly swap is needed here? Please, answer. > Do not skip the question. Yes, it is clear for me now: this function provides an ability to rollback. diff --git a/src/box/alter.cc b/src/box/alter.cc index 23cd4a3..03c2d91 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -552,7 +552,9 @@ space_swap_triggers(struct space *new_space, struct space *old_space) 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; } > > 2. Space->sql_triggers leaks when I drop a space via Lua. I've reworked a tuple format in separate commit, I'll send a new patch set that includes it a bit later. 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); } > > 3. After you fix the leak, try this test to simulate WAL error and you > will see a crash: > > box.cfg{} > 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);") > There is no crash for now, I've included this test case: +-- 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;") > 4. Fits in 3 lines. + int rc = sql_trigger_replace(sql_get(), + sql_trigger_name(old_trigger), + NULL, &new_trigger); > 5. Extra new line. @@ -3131,7 +3130,6 @@ triggers_task_rollback(struct trigger *trigger, void *event) } } - > 6. Why do you need stmt and stmt->old_tuple here? As I see, > old_trigger != NULL is the same as stmt->old_tuple != NULL. static void triggers_task_commit(struct trigger *trigger, void *event) { - struct txn_stmt *stmt = txn_last_stmt((struct txn*) event); struct Trigger *old_trigger = (struct Trigger *)trigger->data; - - if (stmt->old_tuple != NULL) { - /* DELETE, REPLACE trigger. */ - sql_trigger_delete(sql_get(), old_trigger); - } + /* DELETE, REPLACE trigger. */ + sql_trigger_delete(sql_get(), old_trigger); } > 7. Please, do not create two branches for each case. You can make this > function more compact. > 8. Two extra lines. > 9. Const char *. -char * +const char * sql_trigger_name(struct Trigger *trigger); > 10. Why? > 11. Same. Dropped. > 12. Still is not fixed from the previous review. 'Error' is unused. > And why do you need snprintf here? char *error = tt_static_buf(); snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error); diag_set(ClientError, ER_SQL, error); sqlite3DbFree(db, sql_error); I believe this should be like this. Done same in other places. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 4/4] sql: move Triggers to server Kirill Shcherbatov 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-06-01 18:51 ` Konstantin Osipov 1 sibling, 0 replies; 23+ messages in thread From: Konstantin Osipov @ 2018-06-01 18:51 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov * Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/31 14:26]: > index b62f8ad..6e4f15d 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -53,6 +53,7 @@ > #include "version.h" > #include "sequence.h" > #include "sql.h" > +#include "box.h" > > /** Whoa, if you had to include box.h something went terribly wrong. It's a dependency loop. Please read the SOP part about managing header file dependencies and rethink dependencies in your code. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 0/4] sql: remove Triggers to server 2018-05-31 11:22 [tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server Kirill Shcherbatov ` (3 preceding siblings ...) 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 4/4] sql: move Triggers to server Kirill Shcherbatov @ 2018-05-31 17:36 ` Vladislav Shpilevoy 2018-06-04 13:27 ` Vladislav Shpilevoy 2018-06-05 13:31 ` Vladislav Shpilevoy 6 siblings, 0 replies; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-05-31 17:36 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches Hello. Thanks for the patchset! On 31/05/2018 14:22, Kirill Shcherbatov wrote: > Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3273-no-sql-triggers > Issue: https://github.com/tarantool/tarantool/issues/3273 > > As we are going to call parser on box.cfg() to recreate triggers > from SQL, we should initialize Schema as it used in sqlite3BeginTrigger. > Inroduced box_space_id_by_name to get object id by name from custom space. "Inroduced" - typo. > Fixed bug in tarantoolSqlite3RenameTrigger: sql request have had invalid > length in MsgPack after ALTER TABLE NAME. > Introduced sql_triggers field in space structure. > Changed parser logic to do not insert builded triggers, just only "builded" - no such word. > to do parsing. All triggers insertions and deletions are operated > via on_replace_dd_trigger now. Please, do not just copy-paste commit messages. Out of the context I can not understand why "we are going to call parser on box.cfg() to recreate triggers" is blocked by Schema and a mystic function sqlite3BeginTrigger. And why "Inroduced box_space_id_by_name to get object id by name from custom space."? Box_space_id_by_name exists before you patch. "Fixed bug in tarantoolSqlite3RenameTrigger" - this and the previous hunks are minor patches and can be omitted here or just mentioned as 'helper patches'. "Introduced sql_triggers field in space structure." - too minor implementation detail. > > Kirill Shcherbatov (4): > box: move db->pShchema init to sql_init > sql: fix sql len in tarantoolSqlite3RenameTrigger > box: introduce box_space_id_by_name > sql: move Triggers to server > > src/box/alter.cc | 57 +++++++++++ > src/box/box.cc | 10 +- > src/box/box.h | 14 +++ > src/box/space.h | 2 + > src/box/sql.c | 60 ++++------- > src/box/sql.h | 62 ++++++++++++ > src/box/sql/build.c | 8 +- > src/box/sql/insert.c | 6 +- > src/box/sql/sqliteInt.h | 2 - > src/box/sql/tokenize.c | 43 +++++++- > src/box/sql/trigger.c | 258 ++++++++++++++++++++++++++++-------------------- > src/box/sql/vdbe.c | 58 +++-------- > src/box/sql/vdbe.h | 1 - > src/box/sql/vdbeaux.c | 9 -- > 14 files changed, 374 insertions(+), 216 deletions(-) > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 0/4] sql: remove Triggers to server 2018-05-31 11:22 [tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server Kirill Shcherbatov ` (4 preceding siblings ...) 2018-05-31 17:36 ` [tarantool-patches] Re: [PATCH v1 0/4] sql: remove " Vladislav Shpilevoy @ 2018-06-04 13:27 ` Vladislav Shpilevoy 2018-06-05 13:31 ` Vladislav Shpilevoy 6 siblings, 0 replies; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-06-04 13:27 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches Thanks for the patch! Why did not you send it? See 1 comment below. > commit 7e6143c51e520f68db9a4fe96d6980ba65f653d8 > Author: Kirill Shcherbatov <kshcherbatov@gmail.com> > Date: Fri Jun 1 15:38:30 2018 +0300 > > sql: refactor sql_expr_compile to return AST > > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index 42c70a255..2555adbf4 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -539,9 +539,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 +549,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; > + goto cleanup; > } > 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_EXECUTE, stmt); > + goto cleanup; 1. What is happening here? 'Error' variable is unused, as well as sql_error as I can see. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH v1 0/4] sql: remove Triggers to server 2018-05-31 11:22 [tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server Kirill Shcherbatov ` (5 preceding siblings ...) 2018-06-04 13:27 ` Vladislav Shpilevoy @ 2018-06-05 13:31 ` Vladislav Shpilevoy 6 siblings, 0 replies; 23+ messages in thread From: Vladislav Shpilevoy @ 2018-06-05 13:31 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches Please, in the commit message of 'box: sort error codes in misc.test' explain why the patch is needed. It is not obvious for people out of context. On 31/05/2018 14:22, Kirill Shcherbatov wrote: > Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3273-no-sql-triggers > Issue: https://github.com/tarantool/tarantool/issues/3273 > > As we are going to call parser on box.cfg() to recreate triggers > from SQL, we should initialize Schema as it used in sqlite3BeginTrigger. > Inroduced box_space_id_by_name to get object id by name from custom space. > Fixed bug in tarantoolSqlite3RenameTrigger: sql request have had invalid > length in MsgPack after ALTER TABLE NAME. > Introduced sql_triggers field in space structure. > Changed parser logic to do not insert builded triggers, just only > to do parsing. All triggers insertions and deletions are operated > via on_replace_dd_trigger now. > > Kirill Shcherbatov (4): > box: move db->pShchema init to sql_init > sql: fix sql len in tarantoolSqlite3RenameTrigger > box: introduce box_space_id_by_name > sql: move Triggers to server > > src/box/alter.cc | 57 +++++++++++ > src/box/box.cc | 10 +- > src/box/box.h | 14 +++ > src/box/space.h | 2 + > src/box/sql.c | 60 ++++------- > src/box/sql.h | 62 ++++++++++++ > src/box/sql/build.c | 8 +- > src/box/sql/insert.c | 6 +- > src/box/sql/sqliteInt.h | 2 - > src/box/sql/tokenize.c | 43 +++++++- > src/box/sql/trigger.c | 258 ++++++++++++++++++++++++++++-------------------- > src/box/sql/vdbe.c | 58 +++-------- > src/box/sql/vdbe.h | 1 - > src/box/sql/vdbeaux.c | 9 -- > 14 files changed, 374 insertions(+), 216 deletions(-) > ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-06-09 11:06 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-31 11:22 [tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server Kirill Shcherbatov 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 1/4] box: move db->pShchema init to sql_init Kirill Shcherbatov 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-01 20:24 ` Kirill Shcherbatov 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 2/4] sql: fix sql len in tarantoolSqlite3RenameTrigger Kirill Shcherbatov 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 3/4] box: introduce box_space_id_by_name Kirill Shcherbatov 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-01 20:24 ` Kirill Shcherbatov 2018-06-04 13:27 ` Vladislav Shpilevoy 2018-06-04 19:21 ` Kirill Shcherbatov 2018-06-05 13:31 ` Vladislav Shpilevoy 2018-05-31 11:22 ` [tarantool-patches] [PATCH v1 4/4] sql: move Triggers to server Kirill Shcherbatov 2018-05-31 17:36 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-01 20:24 ` Kirill Shcherbatov 2018-06-01 20:25 ` Kirill Shcherbatov 2018-06-04 13:27 ` Vladislav Shpilevoy 2018-06-04 19:21 ` Kirill Shcherbatov 2018-06-05 13:31 ` Vladislav Shpilevoy 2018-06-09 9:32 ` Kirill Shcherbatov 2018-06-01 18:51 ` Konstantin Osipov 2018-05-31 17:36 ` [tarantool-patches] Re: [PATCH v1 0/4] sql: remove " Vladislav Shpilevoy 2018-06-04 13:27 ` Vladislav Shpilevoy 2018-06-05 13:31 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox