* [tarantool-patches] [PATCH v2 1/1] sql: return last_insert_id via IPROTO @ 2018-08-03 10:28 imeevma 2018-08-06 15:24 ` [tarantool-patches] " Vladislav Shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: imeevma @ 2018-08-03 10:28 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy After this patch client will get last_insert_id as additional metadata when he executes some queries. Part of #2618. @TarantoolBot document Title: SQL function last_insert_id. Function last_insert_id returns first autogenerated ID in last INSERT/REPLACE statement in current session. Return value of function is undetermined when more than one INSERT/REPLACE statements executed asynchronously. Example: CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER); INSERT INTO test VALUES (NULL, 1); SELECT last_insert_id(); --- Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-generated-columns-and-values Issue: https://github.com/tarantool/tarantool/issues/2618 src/box/execute.c | 11 ++- src/box/execute.h | 1 + src/box/lua/net_box.c | 12 ++- src/box/sequence.c | 17 ++++ src/box/sequence.h | 9 +++ src/box/session.cc | 1 + src/box/session.h | 2 + src/box/sql.c | 1 + src/box/sql/func.c | 18 +++++ src/box/sql/vdbe.c | 33 ++++++++ test/sql-tap/insert3.test.lua | 27 ++++++- test/sql/errinj.result | 3 +- test/sql/iproto.result | 180 ++++++++++++++++++++++++++++++++++-------- test/sql/iproto.test.lua | 19 +++++ 14 files changed, 296 insertions(+), 38 deletions(-) diff --git a/src/box/execute.c b/src/box/execute.c index 24459b4..e2eabc7 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -42,6 +42,7 @@ #include "schema.h" #include "port.h" #include "tuple.h" +#include "session.h" const char *sql_type_strs[] = { NULL, @@ -640,8 +641,13 @@ err: if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0) goto err; int changes = sqlite3_changes(db); + int64_t last_insert_id = current_session()->sql_last_insert_id; int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + - mp_sizeof_uint(changes); + mp_sizeof_uint(changes) + + mp_sizeof_uint(SQL_INFO_LAST_INSERT_ID) + + (last_insert_id >= 0 ? + mp_sizeof_uint(last_insert_id) : + mp_sizeof_int(last_insert_id)); char *buf = obuf_alloc(out, size); if (buf == NULL) { diag_set(OutOfMemory, size, "obuf_alloc", "buf"); @@ -649,6 +655,9 @@ err: } buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT); buf = mp_encode_uint(buf, changes); + buf = mp_encode_uint(buf, SQL_INFO_LAST_INSERT_ID); + buf = last_insert_id < 0 ? mp_encode_int(buf, last_insert_id) : + mp_encode_uint(buf, last_insert_id); } iproto_reply_sql(out, &header_svp, response->sync, schema_version, keys); diff --git a/src/box/execute.h b/src/box/execute.h index f21393b..ead09fc 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -42,6 +42,7 @@ extern "C" { /** Keys of IPROTO_SQL_INFO map. */ enum sql_info_key { SQL_INFO_ROW_COUNT = 0, + SQL_INFO_LAST_INSERT_ID = 1, sql_info_key_MAX, }; diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index 308c9c7..0e24b47 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -667,16 +667,22 @@ static void netbox_decode_sql_info(struct lua_State *L, const char **data) { uint32_t map_size = mp_decode_map(data); - /* Only SQL_INFO_ROW_COUNT is available. */ assert(map_size == 1); (void) map_size; uint32_t key = mp_decode_uint(data); assert(key == SQL_INFO_ROW_COUNT); - (void) key; uint32_t row_count = mp_decode_uint(data); - lua_createtable(L, 0, 1); + key = mp_decode_uint(data); + assert(key == SQL_INFO_LAST_INSERT_ID); + (void) key; + int64_t last_insert_id = mp_typeof(**data) == MP_UINT ? + (int64_t) mp_decode_uint(data) : + mp_decode_int(data); + lua_createtable(L, 0, 2); lua_pushinteger(L, row_count); lua_setfield(L, -2, "rowcount"); + lua_pushinteger(L, last_insert_id); + lua_setfield(L, -2, "last_insert_id"); } static int diff --git a/src/box/sequence.c b/src/box/sequence.c index 35b7605..cefb3db 100644 --- a/src/box/sequence.c +++ b/src/box/sequence.c @@ -340,3 +340,20 @@ sequence_data_iterator_create(void) light_sequence_iterator_freeze(&sequence_data_index, &iter->iter); return &iter->base; } + +int64_t +sequence_get_value(struct sequence *seq) +{ + uint32_t key = seq->def->id; + uint32_t hash = sequence_hash(key); + uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key); + /* + * Number 1 is often used as start value of sequences in + * general. We are goint to use it as default value. + */ + if (pos == light_sequence_end) + return 1; + struct sequence_data data = light_sequence_get(&sequence_data_index, + pos); + return data.value; +} diff --git a/src/box/sequence.h b/src/box/sequence.h index 0f6c8da..c1a746e 100644 --- a/src/box/sequence.h +++ b/src/box/sequence.h @@ -149,6 +149,15 @@ access_check_sequence(struct sequence *seq); struct snapshot_iterator * sequence_data_iterator_create(void); +/** + * Get current value of given sequence. + * + * @param seq sequence to get value from. + * @retval current value of sequence. + */ +int64_t +sequence_get_value(struct sequence *seq); + #if defined(__cplusplus) } /* extern "C" */ #endif /* defined(__cplusplus) */ diff --git a/src/box/session.cc b/src/box/session.cc index 64714cd..2b8dab9 100644 --- a/src/box/session.cc +++ b/src/box/session.cc @@ -108,6 +108,7 @@ session_create(enum session_type type) session->type = type; session->sql_flags = default_flags; session->sql_default_engine = SQL_STORAGE_ENGINE_MEMTX; + session->sql_last_insert_id = 0; /* For on_connect triggers. */ credentials_init(&session->credentials, guest_user->auth_token, diff --git a/src/box/session.h b/src/box/session.h index df1dcbc..0bd42d0 100644 --- a/src/box/session.h +++ b/src/box/session.h @@ -92,6 +92,8 @@ union session_meta { struct session { /** Session id. */ uint64_t id; + /** First autogenerated ID in last INSERT/REPLACE query */ + int64_t sql_last_insert_id; /** SQL Tarantool Default storage engine. */ uint8_t sql_default_engine; /** SQL Connection flag for current user session */ diff --git a/src/box/sql.c b/src/box/sql.c index 2b93c3d..d9752f3 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -48,6 +48,7 @@ #include "txn.h" #include "space.h" #include "space_def.h" +#include "sequence.h" #include "index_def.h" #include "tuple.h" #include "fiber.h" diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 45056a7..eca69b7 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -38,6 +38,7 @@ #include "vdbeInt.h" #include "version.h" #include "coll.h" +#include "src/box/session.h" #include <unicode/ustring.h> #include <unicode/ucasemap.h> #include <unicode/ucnv.h> @@ -601,6 +602,22 @@ changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2) } /* + * Implementation of the last_insert_id() function. Returns first + * autogenerated ID in last INSERT/REPLACE in current session. + * + * @param context Context being used. + * @param not_used Unused. + * @param not_used2 Unused. + */ +static void +last_insert_id(sqlite3_context *context, int not_used, + sqlite3_value **not_used2) +{ + UNUSED_PARAMETER2(not_used, not_used2); + sqlite3_result_int(context, current_session()->sql_last_insert_id); +} + +/* * Implementation of the total_changes() SQL function. The return value is * the same as the sqlite3_total_changes() API function. */ @@ -1847,6 +1864,7 @@ sqlite3RegisterBuiltinFunctions(void) FUNCTION(lower, 1, 0, 1, LowerICUFunc), FUNCTION(hex, 1, 0, 0, hexFunc), FUNCTION2(ifnull, 2, 0, 0, noopFunc, SQLITE_FUNC_COALESCE), + VFUNCTION(last_insert_id, 0, 0, 0, last_insert_id), VFUNCTION(random, 0, 0, 0, randomFunc), VFUNCTION(randomblob, 1, 0, 0, randomBlob), FUNCTION(nullif, 2, 0, 1, nullifFunc), diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index ca89908..38cd501 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -567,6 +567,30 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp) } } +/** + * Check that new ID is autogenerated in current query. If it is + * this function sets sql_last_insert_id of current session as + * first autogenerated ID in last INSERT/REPLACE query executed + * in current session. + * + * @param space space to get sequence from. + * @retval true new id autogenerated. + * @retval false no new id autogenerated. + */ +static inline bool +check_last_insert_id(struct space *space) +{ + int64_t new_id = 0; + struct session *session = current_session(); + struct sequence *sequence = space->sequence; + if (sequence != NULL) + new_id = sequence_get_value(sequence); + if (session->sql_last_insert_id == new_id) + return false; + session->sql_last_insert_id = new_id; + return true; +} + /* * Execute as much of a VDBE program as we can. * This is the core of sqlite3_step(). @@ -599,6 +623,11 @@ int sqlite3VdbeExec(Vdbe *p) u64 start; /* CPU clock count at start of opcode */ #endif struct session *user_session = current_session(); + /* + * Field sql_last_insert_id of current session should be + * set no more than once for each query. + */ + bool last_insert_id_is_set = false; /*** INSERT STACK UNION HERE ***/ assert(p->magic==VDBE_MAGIC_RUN); /* sqlite3_step() verifies this */ @@ -4300,6 +4329,10 @@ case OP_IdxInsert: { /* in2 */ rc = tarantoolSqlite3Replace(pBtCur->space, pIn2->z, pIn2->z + pIn2->n); + if (rc == 0 && !last_insert_id_is_set) { + last_insert_id_is_set = + check_last_insert_id(pBtCur->space); + } } else if (pBtCur->curFlags & BTCF_TEphemCursor) { rc = tarantoolSqlite3EphemeralInsert(pBtCur->space, pIn2->z, diff --git a/test/sql-tap/insert3.test.lua b/test/sql-tap/insert3.test.lua index c0e9d95..8394cd9 100755 --- a/test/sql-tap/insert3.test.lua +++ b/test/sql-tap/insert3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(18) +test:plan(20) --!./tcltestrunner.lua -- 2005 January 13 @@ -262,6 +262,31 @@ test:do_execsql_test( -- <insert3-4.1> }) +-- gh-2618 function last_insert_id +test:execsql("CREATE TABLE t8(id INTEGER PRIMARY KEY AUTOINCREMENT, a INT);") +test:do_execsql_test( + "insert3-5.1", + [[ + INSERT INTO t8 VALUES (null, 11); + SELECT LAST_INSERT_ID(); + ]], { + -- <insert3-5.1> + 1 + -- <insert3-5.1> +}) + +test:do_execsql_test( + "insert3-5.2", + [[ + INSERT INTO t8 VALUES (null, 44), (null, 55), (null, 66); + SELECT LAST_INSERT_ID(); + ]], { + -- <insert3-5.1> + 2 + -- <insert3-5.1> +}) + + test:drop_all_tables() --------------------------------------------------------------------------- -- While developing tests for a different feature (savepoint) the following diff --git a/test/sql/errinj.result b/test/sql/errinj.result index b491d46..011b21f 100644 --- a/test/sql/errinj.result +++ b/test/sql/errinj.result @@ -73,7 +73,8 @@ while f1:status() ~= 'dead' do fiber.sleep(0) end ... insert_res --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... select_res --- diff --git a/test/sql/iproto.result b/test/sql/iproto.result index af474bc..7aedd9d 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -69,19 +69,23 @@ type(ret.rows[1]) -- Operation with rowcount result. cn:execute('insert into test values (10, 11, NULL)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('delete from test where a = 5') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('insert into test values (11, 12, NULL), (12, 12, NULL), (13, 12, NULL)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('delete from test where a = 12') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... -- SQL errors. cn:execute('insert into not_existing_table values ("kek")') @@ -330,7 +334,8 @@ cn:execute('select :value', parameters) -- gh-2608 SQL iproto DDL cn:execute('create table test2(id primary key, a, b, c)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST2.name --- @@ -338,7 +343,8 @@ box.space.TEST2.name ... cn:execute('insert into test2 values (1, 1, 1, 1)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('select * from test2') --- @@ -352,7 +358,8 @@ cn:execute('select * from test2') ... cn:execute('create index test2_a_b_index on test2(a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... #box.space.TEST2.index --- @@ -360,7 +367,8 @@ cn:execute('create index test2_a_b_index on test2(a, b)') ... cn:execute('drop table test2') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST2 --- @@ -370,100 +378,121 @@ box.space.TEST2 -- Test CREATE [IF NOT EXISTS] TABLE. cn:execute('create table test3(id primary key, a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... -- Rowcount = 1, although two tuples were created: -- for _space and for _index. cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('create table if not exists test3(id primary key)') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE VIEW [IF NOT EXISTS] and -- DROP VIEW [IF EXISTS]. cn:execute('create view test3_view(id) as select id from test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create view if not exists test3_view(id) as select id from test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop view test3_view') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop view if exists test3_view') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE INDEX [IF NOT EXISTS] and -- DROP INDEX [IF EXISTS]. cn:execute('create index test3_sec on test3(a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create index if not exists test3_sec on test3(a, b)') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop index test3_sec on test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop index if exists test3_sec on test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE TRIGGER [IF NOT EXISTS] and -- DROP TRIGGER [IF EXISTS]. cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create trigger if not exists trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop trigger trig') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop trigger if exists trig') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test DROP TABLE [IF EXISTS]. -- Create more indexes, triggers and _truncate tuple. cn:execute('create index idx1 on test3(a)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create index idx2 on test3(b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST3:truncate() --- ... cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('drop table test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop table if exists test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- -- gh-2948: sql: remove unnecessary templates for binding @@ -535,7 +564,8 @@ cn = remote.connect(box.cfg.listen) ... cn:execute('create table test (id integer primary key, a integer, b integer)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... future1 = cn:execute('insert into test values (1, 1, 1)', nil, nil, {is_async = true}) --- @@ -548,7 +578,8 @@ future3 = cn:execute('insert into test values (2, 2, 2), (3, 3, 3)', nil, nil, { ... future1:wait_result() --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... future2:wait_result() --- @@ -558,7 +589,8 @@ future2:wait_result() ... future3:wait_result() --- -- rowcount: 2 +- last_insert_id: 0 + rowcount: 2 ... future4 = cn:execute('select * from test', nil, nil, {is_async = true}) --- @@ -580,6 +612,90 @@ cn:close() box.sql.execute('drop table test') --- ... +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return autogenerated id of first inserted tuple in last insert. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +--- +... +cn = remote.connect(box.cfg.listen) +--- +... +cn:execute('insert into test values (1, 1)', nil, nil) +--- +- last_insert_id: 1 + rowcount: 1 +... +cn:execute('insert into test values (null, 2)', nil, nil) +--- +- last_insert_id: 2 + rowcount: 1 +... +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) +--- +- last_insert_id: 3 + rowcount: 2 +... +cn:execute('insert into test values (17, 2)', nil, nil) +--- +- last_insert_id: 17 + rowcount: 1 +... +cn:execute('update test set a = 11 where id == 1', nil, nil) +--- +- last_insert_id: 17 + rowcount: 1 +... +cn:execute('update test set a = 12 where id == 2', nil, nil) +--- +- last_insert_id: 17 + rowcount: 1 +... +cn:execute('update test set a = 13 where id == 3', nil, nil) +--- +- last_insert_id: 17 + rowcount: 1 +... +cn:execute('replace into test values (4, 44), (null, 100)', nil, nil) +--- +- last_insert_id: 18 + rowcount: 2 +... +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) +--- +- last_insert_id: 19 + rowcount: 2 +... +cn:execute('select * from test', nil, nil) +--- +- metadata: + - name: ID + - name: A + rows: + - [1, 11] + - [2, 12] + - [3, 13] + - [4, 44] + - [17, 2] + - [18, 100] + - [19, 2] + - [20, 3] +... +cn:execute('create table test2 (id integer primary key, a integer)') +--- +- last_insert_id: 19 + rowcount: 1 +... +cn:execute('drop table test2') +--- +- last_insert_id: 19 + rowcount: 1 +... +cn:close() +--- +... +box.sql.execute('drop table test') +--- +... box.schema.user.revoke('guest', 'read,write,execute', 'universe') --- ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index 220331b..9c58860 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -201,6 +201,25 @@ future4:wait_result() cn:close() box.sql.execute('drop table test') +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return autogenerated id of first inserted tuple in last insert. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +cn = remote.connect(box.cfg.listen) +cn:execute('insert into test values (1, 1)', nil, nil) +cn:execute('insert into test values (null, 2)', nil, nil) +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) +cn:execute('insert into test values (17, 2)', nil, nil) +cn:execute('update test set a = 11 where id == 1', nil, nil) +cn:execute('update test set a = 12 where id == 2', nil, nil) +cn:execute('update test set a = 13 where id == 3', nil, nil) +cn:execute('replace into test values (4, 44), (null, 100)', nil, nil) +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) +cn:execute('select * from test', nil, nil) +cn:execute('create table test2 (id integer primary key, a integer)') +cn:execute('drop table test2') +cn:close() +box.sql.execute('drop table test') + box.schema.user.revoke('guest', 'read,write,execute', 'universe') space = nil -- 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO 2018-08-03 10:28 [tarantool-patches] [PATCH v2 1/1] sql: return last_insert_id via IPROTO imeevma @ 2018-08-06 15:24 ` Vladislav Shpilevoy 2018-08-13 12:34 ` Imeev Mergen 0 siblings, 1 reply; 8+ messages in thread From: Vladislav Shpilevoy @ 2018-08-06 15:24 UTC (permalink / raw) To: imeevma, tarantool-patches Hi! Thanks for the patch! See 11 comments below. On 03/08/2018 13:28, imeevma@tarantool.org wrote: > After this patch client will get last_insert_id > as additional metadata when he executes some > queries. > > Part of #2618. > > @TarantoolBot document 1. Please, describe in the same request new iproto key you have introduced. > Title: SQL function last_insert_id. > Function last_insert_id returns first autogenerated ID in > last INSERT/REPLACE statement in current session. Return > value of function is undetermined when more than one > INSERT/REPLACE statements executed asynchronously. > Example: > CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER); > INSERT INTO test VALUES (NULL, 1); > SELECT last_insert_id(); > --- > Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-generated-columns-and-values > Issue: https://github.com/tarantool/tarantool/issues/2618 > > src/box/execute.c | 11 ++- > src/box/execute.h | 1 + > src/box/lua/net_box.c | 12 ++- > src/box/sequence.c | 17 ++++ > src/box/sequence.h | 9 +++ > src/box/session.cc | 1 + > src/box/session.h | 2 + > src/box/sql.c | 1 + > src/box/sql/func.c | 18 +++++ > src/box/sql/vdbe.c | 33 ++++++++ > test/sql-tap/insert3.test.lua | 27 ++++++- > test/sql/errinj.result | 3 +- > test/sql/iproto.result | 180 ++++++++++++++++++++++++++++++++++-------- > test/sql/iproto.test.lua | 19 +++++ > 14 files changed, 296 insertions(+), 38 deletions(-) > > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c > index 308c9c7..0e24b47 100644 > --- a/src/box/lua/net_box.c > +++ b/src/box/lua/net_box.c > @@ -667,16 +667,22 @@ static void > netbox_decode_sql_info(struct lua_State *L, const char **data) > { > uint32_t map_size = mp_decode_map(data); > - /* Only SQL_INFO_ROW_COUNT is available. */ > assert(map_size == 1); > (void) map_size; > uint32_t key = mp_decode_uint(data); > assert(key == SQL_INFO_ROW_COUNT); > - (void) key; > uint32_t row_count = mp_decode_uint(data); > - lua_createtable(L, 0, 1); > + key = mp_decode_uint(data); > + assert(key == SQL_INFO_LAST_INSERT_ID); > + (void) key; 2. Now the key is used and its (void) guard can be removed, it is not? > + int64_t last_insert_id = mp_typeof(**data) == MP_UINT ? > + (int64_t) mp_decode_uint(data) : > + mp_decode_int(data); > + lua_createtable(L, 0, 2); > lua_pushinteger(L, row_count); > lua_setfield(L, -2, "rowcount"); > + lua_pushinteger(L, last_insert_id); > + lua_setfield(L, -2, "last_insert_id"); > } > > static int > diff --git a/src/box/sequence.c b/src/box/sequence.c > index 35b7605..cefb3db 100644 > --- a/src/box/sequence.c > +++ b/src/box/sequence.c > @@ -340,3 +340,20 @@ sequence_data_iterator_create(void) > light_sequence_iterator_freeze(&sequence_data_index, &iter->iter); > return &iter->base; > } > + > +int64_t > +sequence_get_value(struct sequence *seq) > +{ > + uint32_t key = seq->def->id; > + uint32_t hash = sequence_hash(key); > + uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key); > + /* > + * Number 1 is often used as start value of sequences in > + * general. We are goint to use it as default value. > + */ > + if (pos == light_sequence_end) > + return 1; 3. The same comment as in the previous review. 1 is not mandatory start value and can be different in a common case, as well as 0, -1, 2 and any other constant. Use sequence_def.start. 4. Please, when sending a new patch version, explicitly answer my comments either in a new version email or in a response to my email. It is hard to iterate through two emails (with the comments and with your patch) at the same time. > + struct sequence_data data = light_sequence_get(&sequence_data_index, > + pos); > + return data.value; > +} > diff --git a/src/box/sql.c b/src/box/sql.c > index 2b93c3d..d9752f3 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -48,6 +48,7 @@ > #include "txn.h" > #include "space.h" > #include "space_def.h" > +#include "sequence.h" 5. Why? > #include "index_def.h" > #include "tuple.h" > #include "fiber.h" > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 45056a7..eca69b7 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -38,6 +38,7 @@ > #include "vdbeInt.h" > #include "version.h" > #include "coll.h" > +#include "src/box/session.h" 6. For box/ dir we do not use src/ prefix. Please, use 'box/session.h'. > #include <unicode/ustring.h> > #include <unicode/ucasemap.h> > #include <unicode/ucnv.h> > @@ -601,6 +602,22 @@ changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2) > } > > /* > + * Implementation of the last_insert_id() function. Returns first 7. As I said earlier, please, do not describe the function in a comment like it is an object. Use imperative. I see that other functions in the file use the same style, but do not follow it - this style is sqlite's obsolete one. > + * autogenerated ID in last INSERT/REPLACE in current session. > + * > + * @param context Context being used. > + * @param not_used Unused. > + * @param not_used2 Unused. > + */ > +static void > +last_insert_id(sqlite3_context *context, int not_used, > + sqlite3_value **not_used2) > +{ > + UNUSED_PARAMETER2(not_used, not_used2); > + sqlite3_result_int(context, current_session()->sql_last_insert_id); > +} > + > +/* > * Implementation of the total_changes() SQL function. The return value is > * the same as the sqlite3_total_changes() API function. > */ > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index ca89908..38cd501 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -567,6 +567,30 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp) > } > } > > +/** > + * Check that new ID is autogenerated in current query. If it is > + * this function sets sql_last_insert_id of current session as > + * first autogenerated ID in last INSERT/REPLACE query executed > + * in current session. > + * > + * @param space space to get sequence from. > + * @retval true new id autogenerated. > + * @retval false no new id autogenerated. > + */ > +static inline bool > +check_last_insert_id(struct space *space) > +{ > + int64_t new_id = 0; > + struct session *session = current_session(); > + struct sequence *sequence = space->sequence; > + if (sequence != NULL) > + new_id = sequence_get_value(sequence); 8. If sequence == NULL, this function should not do anything, it is not? I think, it is simpler to just inline it and remove some of checks. See the comment 10. > + if (session->sql_last_insert_id == new_id) > + return false; > + session->sql_last_insert_id = new_id; > + return true; > +} > + > /* > * Execute as much of a VDBE program as we can. > * This is the core of sqlite3_step(). > @@ -599,6 +623,11 @@ int sqlite3VdbeExec(Vdbe *p) > u64 start; /* CPU clock count at start of opcode */ > #endif > struct session *user_session = current_session(); > + /* > + * Field sql_last_insert_id of current session should be > + * set no more than once for each query. > + */ > + bool last_insert_id_is_set = false; 9. For flags we use 'is_' prefix. > /*** INSERT STACK UNION HERE ***/ > > assert(p->magic==VDBE_MAGIC_RUN); /* sqlite3_step() verifies this */ > @@ -4300,6 +4329,10 @@ case OP_IdxInsert: { /* in2 */ > rc = tarantoolSqlite3Replace(pBtCur->space, > pIn2->z, > pIn2->z + pIn2->n); > + if (rc == 0 && !last_insert_id_is_set) { > + last_insert_id_is_set = > + check_last_insert_id(pBtCur->space); 10. I propose to rewrite it as if (!last_insert_id_is_set && rc == 0 && space->sequence != NULL) { last_insert_id_is_set = true; user_session->sql_last_insert_id = sequence_get_value(space->sequence); } And remove check_last_insert_id. > + } > } else if (pBtCur->curFlags & BTCF_TEphemCursor) { > rc = tarantoolSqlite3EphemeralInsert(pBtCur->space, > pIn2->z,> @@ -580,6 +612,90 @@ cn:close() > box.sql.execute('drop table test') > --- > ... > +-- gh-2618 Return generated columns after INSERT in IPROTO. > +-- Return autogenerated id of first inserted tuple in last insert. 11. 'In last insert' of what or whither? ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO 2018-08-06 15:24 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-08-13 12:34 ` Imeev Mergen 2018-08-16 22:05 ` Vladislav Shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Imeev Mergen @ 2018-08-13 12:34 UTC (permalink / raw) To: Vladislav Shpilevoy, tarantool-patches Hi! Thank you for the review! On 08/06/2018 06:24 PM, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! See 11 comments below. > > On 03/08/2018 13:28, imeevma@tarantool.org wrote: >> After this patch client will get last_insert_id >> as additional metadata when he executes some >> queries. >> >> Part of #2618. >> >> @TarantoolBot document > > 1. Please, describe in the same request new iproto key > you have introduced. Done. > >> Title: SQL function last_insert_id. >> Function last_insert_id returns first autogenerated ID in >> last INSERT/REPLACE statement in current session. Return >> value of function is undetermined when more than one >> INSERT/REPLACE statements executed asynchronously. >> Example: >> CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER); >> INSERT INTO test VALUES (NULL, 1); >> SELECT last_insert_id(); >> --- >> Branch: >> https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-generated-columns-and-values >> Issue: https://github.com/tarantool/tarantool/issues/2618 >> >> src/box/execute.c | 11 ++- >> src/box/execute.h | 1 + >> src/box/lua/net_box.c | 12 ++- >> src/box/sequence.c | 17 ++++ >> src/box/sequence.h | 9 +++ >> src/box/session.cc | 1 + >> src/box/session.h | 2 + >> src/box/sql.c | 1 + >> src/box/sql/func.c | 18 +++++ >> src/box/sql/vdbe.c | 33 ++++++++ >> test/sql-tap/insert3.test.lua | 27 ++++++- >> test/sql/errinj.result | 3 +- >> test/sql/iproto.result | 180 >> ++++++++++++++++++++++++++++++++++-------- >> test/sql/iproto.test.lua | 19 +++++ >> 14 files changed, 296 insertions(+), 38 deletions(-) >> >> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c >> index 308c9c7..0e24b47 100644 >> --- a/src/box/lua/net_box.c >> +++ b/src/box/lua/net_box.c >> @@ -667,16 +667,22 @@ static void >> netbox_decode_sql_info(struct lua_State *L, const char **data) >> { >> uint32_t map_size = mp_decode_map(data); >> - /* Only SQL_INFO_ROW_COUNT is available. */ >> assert(map_size == 1); >> (void) map_size; >> uint32_t key = mp_decode_uint(data); >> assert(key == SQL_INFO_ROW_COUNT); >> - (void) key; >> uint32_t row_count = mp_decode_uint(data); >> - lua_createtable(L, 0, 1); >> + key = mp_decode_uint(data); >> + assert(key == SQL_INFO_LAST_INSERT_ID); >> + (void) key; > > 2. Now the key is used and its (void) guard can be removed, > it is not? Fixed. > >> + int64_t last_insert_id = mp_typeof(**data) == MP_UINT ? >> + (int64_t) mp_decode_uint(data) : >> + mp_decode_int(data); >> + lua_createtable(L, 0, 2); >> lua_pushinteger(L, row_count); >> lua_setfield(L, -2, "rowcount"); >> + lua_pushinteger(L, last_insert_id); >> + lua_setfield(L, -2, "last_insert_id"); >> } >> static int >> diff --git a/src/box/sequence.c b/src/box/sequence.c >> index 35b7605..cefb3db 100644 >> --- a/src/box/sequence.c >> +++ b/src/box/sequence.c >> @@ -340,3 +340,20 @@ sequence_data_iterator_create(void) >> light_sequence_iterator_freeze(&sequence_data_index, &iter->iter); >> return &iter->base; >> } >> + >> +int64_t >> +sequence_get_value(struct sequence *seq) >> +{ >> + uint32_t key = seq->def->id; >> + uint32_t hash = sequence_hash(key); >> + uint32_t pos = light_sequence_find_key(&sequence_data_index, >> hash, key); >> + /* >> + * Number 1 is often used as start value of sequences in >> + * general. We are goint to use it as default value. >> + */ >> + if (pos == light_sequence_end) >> + return 1; > > 3. The same comment as in the previous review. 1 is not mandatory start > value and can be different in a common case, as well as 0, -1, 2 and any > other constant. Use sequence_def.start. Function was removed as it isn't necessary anymore. > > 4. Please, when sending a new patch version, explicitly answer my > comments either in a new version email or in a response to my email. It > is hard to iterate through two emails (with the comments and with your > patch) at the same time. Ok, understood. > >> + struct sequence_data data = >> light_sequence_get(&sequence_data_index, >> + pos); >> + return data.value; >> +} >> diff --git a/src/box/sql.c b/src/box/sql.c >> index 2b93c3d..d9752f3 100644 >> --- a/src/box/sql.c >> +++ b/src/box/sql.c >> @@ -48,6 +48,7 @@ >> #include "txn.h" >> #include "space.h" >> #include "space_def.h" >> +#include "sequence.h" > > 5. Why? Fixed. > >> #include "index_def.h" >> #include "tuple.h" >> #include "fiber.h" >> diff --git a/src/box/sql/func.c b/src/box/sql/func.c >> index 45056a7..eca69b7 100644 >> --- a/src/box/sql/func.c >> +++ b/src/box/sql/func.c >> @@ -38,6 +38,7 @@ >> #include "vdbeInt.h" >> #include "version.h" >> #include "coll.h" >> +#include "src/box/session.h" > > 6. For box/ dir we do not use src/ prefix. > Please, use 'box/session.h'. Fixed. > >> #include <unicode/ustring.h> >> #include <unicode/ucasemap.h> >> #include <unicode/ucnv.h> >> @@ -601,6 +602,22 @@ changes(sqlite3_context * context, int NotUsed, >> sqlite3_value ** NotUsed2) >> } >> /* >> + * Implementation of the last_insert_id() function. Returns first > > 7. As I said earlier, please, do not describe the function in a > comment like it is an object. Use imperative. I see that other > functions in the file use the same style, but do not follow it - > this style is sqlite's obsolete one. Fixed. > >> + * autogenerated ID in last INSERT/REPLACE in current session. >> + * >> + * @param context Context being used. >> + * @param not_used Unused. >> + * @param not_used2 Unused. >> + */ >> +static void >> +last_insert_id(sqlite3_context *context, int not_used, >> + sqlite3_value **not_used2) >> +{ >> + UNUSED_PARAMETER2(not_used, not_used2); >> + sqlite3_result_int(context, current_session()->sql_last_insert_id); >> +} >> + >> +/* >> * Implementation of the total_changes() SQL function. The return >> value is >> * the same as the sqlite3_total_changes() API function. >> */ >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index ca89908..38cd501 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -567,6 +567,30 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp) >> } >> } >> +/** >> + * Check that new ID is autogenerated in current query. If it is >> + * this function sets sql_last_insert_id of current session as >> + * first autogenerated ID in last INSERT/REPLACE query executed >> + * in current session. >> + * >> + * @param space space to get sequence from. >> + * @retval true new id autogenerated. >> + * @retval false no new id autogenerated. >> + */ >> +static inline bool >> +check_last_insert_id(struct space *space) >> +{ >> + int64_t new_id = 0; >> + struct session *session = current_session(); >> + struct sequence *sequence = space->sequence; >> + if (sequence != NULL) >> + new_id = sequence_get_value(sequence); > > 8. If sequence == NULL, this function should not do anything, > it is not? I think, it is simpler to just inline it and > remove some of checks. See the comment 10. Unnecessary as way last_insert_id() works changed. > >> + if (session->sql_last_insert_id == new_id) >> + return false; >> + session->sql_last_insert_id = new_id; >> + return true; >> +} >> + >> /* >> * Execute as much of a VDBE program as we can. >> * This is the core of sqlite3_step(). >> @@ -599,6 +623,11 @@ int sqlite3VdbeExec(Vdbe *p) >> u64 start; /* CPU clock count at start of >> opcode */ >> #endif >> struct session *user_session = current_session(); >> + /* >> + * Field sql_last_insert_id of current session should be >> + * set no more than once for each query. >> + */ >> + bool last_insert_id_is_set = false; > > 9. For flags we use 'is_' prefix. Fixed. > >> /*** INSERT STACK UNION HERE ***/ >> assert(p->magic==VDBE_MAGIC_RUN); /* sqlite3_step() verifies >> this */ >> @@ -4300,6 +4329,10 @@ case OP_IdxInsert: { /* in2 */ >> rc = tarantoolSqlite3Replace(pBtCur->space, >> pIn2->z, >> pIn2->z + pIn2->n); >> + if (rc == 0 && !last_insert_id_is_set) { >> + last_insert_id_is_set = >> + check_last_insert_id(pBtCur->space); > > 10. I propose to rewrite it as > > if (!last_insert_id_is_set && rc == 0 && space->sequence != NULL) { > last_insert_id_is_set = true; > user_session->sql_last_insert_id = > sequence_get_value(space->sequence); > } > > And remove check_last_insert_id. Partially fixed. > >> + } >> } else if (pBtCur->curFlags & BTCF_TEphemCursor) { >> rc = tarantoolSqlite3EphemeralInsert(pBtCur->space, >> pIn2->z,> @@ -580,6 +612,90 @@ >> cn:close() >> box.sql.execute('drop table test') >> --- >> ... >> +-- gh-2618 Return generated columns after INSERT in IPROTO. >> +-- Return autogenerated id of first inserted tuple in last insert. > > 11. 'In last insert' of what or whither? Fixed. commit 2b292311e61f3c25a4c776bd75e3e6d241ff66a1 Author: Mergen Imeev <imeevma@gmail.com> Date: Tue Jul 31 16:43:38 2018 +0300 sql: return last_insert_id via IPROTO After this patch client will get last_insert_id as additional metadata when he executes some queries. Part of #2618. @TarantoolBot document Title: SQL function last_insert_id() and IPROTO key last_insert_id. Function last_insert_id() returns first primary key value autogenerated in last INSERT/REPLACE statement in current session. Return value of function is undetermined when more than one INSERT/REPLACE statements executed asynchronously. IPROTO key last_insert_id is a metadata returned through IPROTO after such queries as INSERT, REPLACE, UPDATE etc. Value of this key is equal to value returned by function last_insert_id() executed after the query. Example: CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER); INSERT INTO test VALUES (NULL, 1); SELECT last_insert_id(); diff --git a/src/box/execute.c b/src/box/execute.c index 24459b4..e2eabc7 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -42,6 +42,7 @@ #include "schema.h" #include "port.h" #include "tuple.h" +#include "session.h" const char *sql_type_strs[] = { NULL, @@ -640,8 +641,13 @@ err: if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0) goto err; int changes = sqlite3_changes(db); + int64_t last_insert_id = current_session()->sql_last_insert_id; int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + - mp_sizeof_uint(changes); + mp_sizeof_uint(changes) + + mp_sizeof_uint(SQL_INFO_LAST_INSERT_ID) + + (last_insert_id >= 0 ? + mp_sizeof_uint(last_insert_id) : + mp_sizeof_int(last_insert_id)); char *buf = obuf_alloc(out, size); if (buf == NULL) { diag_set(OutOfMemory, size, "obuf_alloc", "buf"); @@ -649,6 +655,9 @@ err: } buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT); buf = mp_encode_uint(buf, changes); + buf = mp_encode_uint(buf, SQL_INFO_LAST_INSERT_ID); + buf = last_insert_id < 0 ? mp_encode_int(buf, last_insert_id) : + mp_encode_uint(buf, last_insert_id); } iproto_reply_sql(out, &header_svp, response->sync, schema_version, keys); diff --git a/src/box/execute.h b/src/box/execute.h index f21393b..ead09fc 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -42,6 +42,7 @@ extern "C" { /** Keys of IPROTO_SQL_INFO map. */ enum sql_info_key { SQL_INFO_ROW_COUNT = 0, + SQL_INFO_LAST_INSERT_ID = 1, sql_info_key_MAX, }; diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index 308c9c7..83ba9b8 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -667,16 +667,21 @@ static void netbox_decode_sql_info(struct lua_State *L, const char **data) { uint32_t map_size = mp_decode_map(data); - /* Only SQL_INFO_ROW_COUNT is available. */ assert(map_size == 1); (void) map_size; uint32_t key = mp_decode_uint(data); assert(key == SQL_INFO_ROW_COUNT); - (void) key; uint32_t row_count = mp_decode_uint(data); - lua_createtable(L, 0, 1); + key = mp_decode_uint(data); + assert(key == SQL_INFO_LAST_INSERT_ID); + int64_t last_insert_id = mp_typeof(**data) == MP_UINT ? + (int64_t) mp_decode_uint(data) : + mp_decode_int(data); + lua_createtable(L, 0, 2); lua_pushinteger(L, row_count); lua_setfield(L, -2, "rowcount"); + lua_pushinteger(L, last_insert_id); + lua_setfield(L, -2, "last_insert_id"); } static int diff --git a/src/box/session.cc b/src/box/session.cc index 64714cd..2b8dab9 100644 --- a/src/box/session.cc +++ b/src/box/session.cc @@ -108,6 +108,7 @@ session_create(enum session_type type) session->type = type; session->sql_flags = default_flags; session->sql_default_engine = SQL_STORAGE_ENGINE_MEMTX; + session->sql_last_insert_id = 0; /* For on_connect triggers. */ credentials_init(&session->credentials, guest_user->auth_token, diff --git a/src/box/session.h b/src/box/session.h index df1dcbc..3d4049c 100644 --- a/src/box/session.h +++ b/src/box/session.h @@ -92,6 +92,8 @@ union session_meta { struct session { /** Session id. */ uint64_t id; + /** First PK autogenerated in last INSERT/REPLACE query */ + int64_t sql_last_insert_id; /** SQL Tarantool Default storage engine. */ uint8_t sql_default_engine; /** SQL Connection flag for current user session */ diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 45056a7..164bb6e 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -38,6 +38,7 @@ #include "vdbeInt.h" #include "version.h" #include "coll.h" +#include "box/session.h" #include <unicode/ustring.h> #include <unicode/ucasemap.h> #include <unicode/ucnv.h> @@ -601,6 +602,22 @@ changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2) } /* + * Return first PK autogenerated in last INSERT/REPLACE in which + * PK was generated in current session. + * + * @param context Context being used. + * @param not_used Unused. + * @param not_used2 Unused. + */ +static void +last_insert_id(sqlite3_context *context, int not_used, + sqlite3_value **not_used2) +{ + UNUSED_PARAMETER2(not_used, not_used2); + sqlite3_result_int(context, current_session()->sql_last_insert_id); +} + +/* * Implementation of the total_changes() SQL function. The return value is * the same as the sqlite3_total_changes() API function. */ @@ -1847,6 +1864,7 @@ sqlite3RegisterBuiltinFunctions(void) FUNCTION(lower, 1, 0, 1, LowerICUFunc), FUNCTION(hex, 1, 0, 0, hexFunc), FUNCTION2(ifnull, 2, 0, 0, noopFunc, SQLITE_FUNC_COALESCE), + VFUNCTION(last_insert_id, 0, 0, 0, last_insert_id), VFUNCTION(random, 0, 0, 0, randomFunc), VFUNCTION(randomblob, 1, 0, 0, randomBlob), FUNCTION(nullif, 2, 0, 1, nullifFunc), diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 13cb61e..8b5b0df 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -739,9 +739,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ if (j < 0 || nColumn == 0 || (pColumn && j >= pColumn->nId)) { if (i == pTab->iAutoIncPKey) { - sqlite3VdbeAddOp2(v, - OP_NextAutoincValue, - pTab->def->id, + sqlite3VdbeAddOp2(v, OP_Null, 0, iRegStore); continue; } @@ -824,7 +822,14 @@ sqlite3Insert(Parse * pParse, /* Parser context */ iRegStore); } } - + /* + * Replace NULL in PK with autoincrement by + * generated value + */ + if (pTab->iAutoIncPKey >= 0) { + sqlite3VdbeAddOp2(v, OP_NextAutoincValue, pTab->def->id, + regData + pTab->iAutoIncPKey); + } /* Generate code to check constraints and generate index keys and do the insertion. */ diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index dc5146f..dceb8c0 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -600,6 +600,11 @@ int sqlite3VdbeExec(Vdbe *p) u64 start; /* CPU clock count at start of opcode */ #endif struct session *user_session = current_session(); + /* + * Field sql_last_insert_id of current session should be + * set no more than once for each query. + */ + bool is_last_insert_id_set = false; /*** INSERT STACK UNION HERE ***/ assert(p->magic==VDBE_MAGIC_RUN); /* sqlite3_step() verifies this */ @@ -1147,6 +1152,10 @@ case OP_NextAutoincValue: { assert(pOp->p1 > 0); assert(pOp->p2 > 0); + pIn1 = &p->aMem[pOp->p2]; + if ((pIn1->flags & MEM_Null) == 0) + break; + struct space *space = space_by_id(pOp->p1); if (space == NULL) { rc = SQL_TARANTOOL_ERROR; @@ -1164,6 +1173,13 @@ case OP_NextAutoincValue: { pOut->flags = MEM_Int; pOut->u.i = value; + /* Set sql_last_insert_id of current session. */ + if (!is_last_insert_id_set) { + struct session *session = current_session(); + session->sql_last_insert_id = value; + is_last_insert_id_set = true; + } + break; } @@ -3745,7 +3761,7 @@ case OP_FCopy: { /* out2 */ if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) { pOut = &aMem[pOp->p2]; - if (pOut->flags & MEM_Undefined) pOut->flags = MEM_Null; + if (pOut->flags & MEM_Int) pOut->flags = MEM_Null; /* Flag is set and register is NULL -> do nothing */ } else { assert(memIsValid(pIn1)); diff --git a/test/sql-tap/insert3.test.lua b/test/sql-tap/insert3.test.lua index c0e9d95..8394cd9 100755 --- a/test/sql-tap/insert3.test.lua +++ b/test/sql-tap/insert3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(18) +test:plan(20) --!./tcltestrunner.lua -- 2005 January 13 @@ -262,6 +262,31 @@ test:do_execsql_test( -- <insert3-4.1> }) +-- gh-2618 function last_insert_id +test:execsql("CREATE TABLE t8(id INTEGER PRIMARY KEY AUTOINCREMENT, a INT);") +test:do_execsql_test( + "insert3-5.1", + [[ + INSERT INTO t8 VALUES (null, 11); + SELECT LAST_INSERT_ID(); + ]], { + -- <insert3-5.1> + 1 + -- <insert3-5.1> +}) + +test:do_execsql_test( + "insert3-5.2", + [[ + INSERT INTO t8 VALUES (null, 44), (null, 55), (null, 66); + SELECT LAST_INSERT_ID(); + ]], { + -- <insert3-5.1> + 2 + -- <insert3-5.1> +}) + + test:drop_all_tables() --------------------------------------------------------------------------- -- While developing tests for a different feature (savepoint) the following diff --git a/test/sql/errinj.result b/test/sql/errinj.result index a0ba60f..9443137 100644 --- a/test/sql/errinj.result +++ b/test/sql/errinj.result @@ -73,7 +73,8 @@ while f1:status() ~= 'dead' do fiber.sleep(0) end ... insert_res --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... select_res --- diff --git a/test/sql/gh-2981-check-autoinc.result b/test/sql/gh-2981-check-autoinc.result index b0f55e6..43bd42b 100644 --- a/test/sql/gh-2981-check-autoinc.result +++ b/test/sql/gh-2981-check-autoinc.result @@ -19,6 +19,9 @@ box.sql.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEG box.sql.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));"); --- ... +box.sql.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); +--- +... box.sql.execute("insert into t1 values (18, null);") --- ... @@ -47,6 +50,13 @@ box.sql.execute("insert into t3(s2) values (null)") --- - error: 'CHECK constraint failed: T3' ... +box.sql.execute("insert into t4 values (18, null);") +--- +... +box.sql.execute("insert into t4 values (null, null);") +--- +- error: 'CHECK constraint failed: T4' +... box.sql.execute("DROP TABLE t1") --- ... @@ -56,3 +66,6 @@ box.sql.execute("DROP TABLE t2") box.sql.execute("DROP TABLE t3") --- ... +box.sql.execute("DROP TABLE t4") +--- +... diff --git a/test/sql/gh-2981-check-autoinc.test.lua b/test/sql/gh-2981-check-autoinc.test.lua index 98a5fb4..593ff3a 100644 --- a/test/sql/gh-2981-check-autoinc.test.lua +++ b/test/sql/gh-2981-check-autoinc.test.lua @@ -7,6 +7,7 @@ box.cfg{} box.sql.execute("CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); box.sql.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19 AND s1 <> 25));"); box.sql.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));"); +box.sql.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); box.sql.execute("insert into t1 values (18, null);") box.sql.execute("insert into t1(s2) values (null);") @@ -19,7 +20,10 @@ box.sql.execute("insert into t2(s2) values (null);") box.sql.execute("insert into t3 values (9, null)") box.sql.execute("insert into t3(s2) values (null)") +box.sql.execute("insert into t4 values (18, null);") +box.sql.execute("insert into t4 values (null, null);") + box.sql.execute("DROP TABLE t1") box.sql.execute("DROP TABLE t2") box.sql.execute("DROP TABLE t3") - +box.sql.execute("DROP TABLE t4") diff --git a/test/sql/iproto.result b/test/sql/iproto.result index af474bc..5fdf49b 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -69,19 +69,23 @@ type(ret.rows[1]) -- Operation with rowcount result. cn:execute('insert into test values (10, 11, NULL)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('delete from test where a = 5') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('insert into test values (11, 12, NULL), (12, 12, NULL), (13, 12, NULL)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('delete from test where a = 12') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... -- SQL errors. cn:execute('insert into not_existing_table values ("kek")') @@ -330,7 +334,8 @@ cn:execute('select :value', parameters) -- gh-2608 SQL iproto DDL cn:execute('create table test2(id primary key, a, b, c)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST2.name --- @@ -338,7 +343,8 @@ box.space.TEST2.name ... cn:execute('insert into test2 values (1, 1, 1, 1)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('select * from test2') --- @@ -352,7 +358,8 @@ cn:execute('select * from test2') ... cn:execute('create index test2_a_b_index on test2(a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... #box.space.TEST2.index --- @@ -360,7 +367,8 @@ cn:execute('create index test2_a_b_index on test2(a, b)') ... cn:execute('drop table test2') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST2 --- @@ -370,100 +378,121 @@ box.space.TEST2 -- Test CREATE [IF NOT EXISTS] TABLE. cn:execute('create table test3(id primary key, a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... -- Rowcount = 1, although two tuples were created: -- for _space and for _index. cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('create table if not exists test3(id primary key)') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE VIEW [IF NOT EXISTS] and -- DROP VIEW [IF EXISTS]. cn:execute('create view test3_view(id) as select id from test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create view if not exists test3_view(id) as select id from test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop view test3_view') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop view if exists test3_view') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE INDEX [IF NOT EXISTS] and -- DROP INDEX [IF EXISTS]. cn:execute('create index test3_sec on test3(a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create index if not exists test3_sec on test3(a, b)') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop index test3_sec on test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop index if exists test3_sec on test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE TRIGGER [IF NOT EXISTS] and -- DROP TRIGGER [IF EXISTS]. cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create trigger if not exists trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop trigger trig') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop trigger if exists trig') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test DROP TABLE [IF EXISTS]. -- Create more indexes, triggers and _truncate tuple. cn:execute('create index idx1 on test3(a)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create index idx2 on test3(b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST3:truncate() --- ... cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('drop table test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop table if exists test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- -- gh-2948: sql: remove unnecessary templates for binding @@ -535,7 +564,8 @@ cn = remote.connect(box.cfg.listen) ... cn:execute('create table test (id integer primary key, a integer, b integer)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... future1 = cn:execute('insert into test values (1, 1, 1)', nil, nil, {is_async = true}) --- @@ -548,7 +578,8 @@ future3 = cn:execute('insert into test values (2, 2, 2), (3, 3, 3)', nil, nil, { ... future1:wait_result() --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... future2:wait_result() --- @@ -558,7 +589,8 @@ future2:wait_result() ... future3:wait_result() --- -- rowcount: 2 +- last_insert_id: 0 + rowcount: 2 ... future4 = cn:execute('select * from test', nil, nil, {is_async = true}) --- @@ -580,6 +612,91 @@ cn:close() box.sql.execute('drop table test') --- ... +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return first PK autogenerated in last INSERT/REPLACE in which +-- new PK was genereted in current session. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +--- +... +cn = remote.connect(box.cfg.listen) +--- +... +cn:execute('insert into test values (1, 1)', nil, nil) +--- +- last_insert_id: 0 + rowcount: 1 +... +cn:execute('insert into test values (null, 2)', nil, nil) +--- +- last_insert_id: 2 + rowcount: 1 +... +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) +--- +- last_insert_id: 3 + rowcount: 2 +... +cn:execute('insert into test values (17, 2)', nil, nil) +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('update test set a = 11 where id == 1', nil, nil) +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('update test set a = 12 where id == 2', nil, nil) +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('update test set a = 13 where id == 3', nil, nil) +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('replace into test values (4, 44), (null, 100)', nil, nil) +--- +- last_insert_id: 18 + rowcount: 2 +... +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) +--- +- last_insert_id: 19 + rowcount: 2 +... +cn:execute('select * from test', nil, nil) +--- +- metadata: + - name: ID + - name: A + rows: + - [1, 11] + - [2, 12] + - [3, 13] + - [4, 44] + - [17, 2] + - [18, 100] + - [19, 2] + - [20, 3] +... +cn:execute('create table test2 (id integer primary key, a integer)') +--- +- last_insert_id: 19 + rowcount: 1 +... +cn:execute('drop table test2') +--- +- last_insert_id: 19 + rowcount: 1 +... +cn:close() +--- +... +box.sql.execute('drop table test') +--- +... box.schema.user.revoke('guest', 'read,write,execute', 'universe') --- ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index 220331b..db1ade5 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -201,6 +201,26 @@ future4:wait_result() cn:close() box.sql.execute('drop table test') +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return first PK autogenerated in last INSERT/REPLACE in which +-- new PK was genereted in current session. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +cn = remote.connect(box.cfg.listen) +cn:execute('insert into test values (1, 1)', nil, nil) +cn:execute('insert into test values (null, 2)', nil, nil) +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) +cn:execute('insert into test values (17, 2)', nil, nil) +cn:execute('update test set a = 11 where id == 1', nil, nil) +cn:execute('update test set a = 12 where id == 2', nil, nil) +cn:execute('update test set a = 13 where id == 3', nil, nil) +cn:execute('replace into test values (4, 44), (null, 100)', nil, nil) +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) +cn:execute('select * from test', nil, nil) +cn:execute('create table test2 (id integer primary key, a integer)') +cn:execute('drop table test2') +cn:close() +box.sql.execute('drop table test') + box.schema.user.revoke('guest', 'read,write,execute', 'universe') space = nil ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO 2018-08-13 12:34 ` Imeev Mergen @ 2018-08-16 22:05 ` Vladislav Shpilevoy 2018-08-17 11:43 ` Imeev Mergen 0 siblings, 1 reply; 8+ messages in thread From: Vladislav Shpilevoy @ 2018-08-16 22:05 UTC (permalink / raw) To: Imeev Mergen, tarantool-patches Thanks for the fixes! See 6 comments below. > commit 2b292311e61f3c25a4c776bd75e3e6d241ff66a1 > Author: Mergen Imeev <imeevma@gmail.com> > Date: Tue Jul 31 16:43:38 2018 +0300 > > sql: return last_insert_id via IPROTO > > After this patch client will get last_insert_id > as additional metadata when he executes some > queries. > > Part of #2618. > > @TarantoolBot document > Title: SQL function last_insert_id() > and IPROTO key last_insert_id. > Function last_insert_id() returns first primary key value > autogenerated in last INSERT/REPLACE statement in current > session. Return value of function is undetermined when more > than one INSERT/REPLACE statements executed asynchronously. 1. Please, note that only for a single session of a single user. If a user opens multiple connections, then in each last_insert_id will work properly. > IPROTO key last_insert_id is a metadata returned through > IPROTO after such queries as INSERT, REPLACE, UPDATE etc. > Value of this key is equal to value returned by function > last_insert_id() executed after the query. > Example: > CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER); > INSERT INTO test VALUES (NULL, 1); > SELECT last_insert_id(); > > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c > index 308c9c7..83ba9b8 100644 > --- a/src/box/lua/net_box.c > +++ b/src/box/lua/net_box.c > @@ -667,16 +667,21 @@ static void > netbox_decode_sql_info(struct lua_State *L, const char **data) > { > uint32_t map_size = mp_decode_map(data); > - /* Only SQL_INFO_ROW_COUNT is available. */ > assert(map_size == 1); > (void) map_size; > uint32_t key = mp_decode_uint(data); > assert(key == SQL_INFO_ROW_COUNT); > - (void) key; > uint32_t row_count = mp_decode_uint(data); > - lua_createtable(L, 0, 1); > + key = mp_decode_uint(data); > + assert(key == SQL_INFO_LAST_INSERT_ID); > + int64_t last_insert_id = mp_typeof(**data) == MP_UINT ? > + (int64_t) mp_decode_uint(data) : > + mp_decode_int(data); 2. Why not mp_read_int64() ? > + lua_createtable(L, 0, 2); > lua_pushinteger(L, row_count); > lua_setfield(L, -2, "rowcount"); > + lua_pushinteger(L, last_insert_id); > + lua_setfield(L, -2, "last_insert_id"); > } > > static int > diff --git a/src/box/session.h b/src/box/session.h > index df1dcbc..3d4049c 100644 > --- a/src/box/session.h > +++ b/src/box/session.h > @@ -92,6 +92,8 @@ union session_meta { > struct session { > /** Session id. */ > uint64_t id; > + /** First PK autogenerated in last INSERT/REPLACE query */ 3. query -> statement. Sorry for nitpicking, it is Kostja O.'s whim. But on the other hand it is literally more precise - query is more likely about SELECT while statement concerns complex multi-row DML constructions. > + int64_t sql_last_insert_id; > /** SQL Tarantool Default storage engine. */ > uint8_t sql_default_engine; > /** SQL Connection flag for current user session */ > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 13cb61e..8b5b0df 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -739,9 +739,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > if (j < 0 || nColumn == 0 > || (pColumn && j >= pColumn->nId)) { > if (i == pTab->iAutoIncPKey) { > - sqlite3VdbeAddOp2(v, > - OP_NextAutoincValue, > - pTab->def->id, > + sqlite3VdbeAddOp2(v, OP_Null, 0, 4. Can we fix the bug related to OP_NextAutoincValue in a separate commit? So the patchset would consist of 2 commits. And why for iAutoInc do you generate OP_Null here? As I understand, here and on line 799 you are sure this column is NULL and can put next_auto_inc explicitly with no writing NULL and then replacing it in a separate opcode. It is not? > iRegStore); > continue; > } > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index dc5146f..dceb8c0 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3745,7 +3761,7 @@ case OP_FCopy: { /* out2 */ > > if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) { > pOut = &aMem[pOp->p2]; > - if (pOut->flags & MEM_Undefined) pOut->flags = MEM_Null; > + if (pOut->flags & MEM_Int) pOut->flags = MEM_Null; 5. Why? > /* Flag is set and register is NULL -> do nothing */ > } else { > assert(memIsValid(pIn1)); > diff --git a/test/sql/iproto.result b/test/sql/iproto.result > index af474bc..5fdf49b 100644 > --- a/test/sql/iproto.result > +++ b/test/sql/iproto.result > @@ -580,6 +612,91 @@ cn:close() > box.sql.execute('drop table test') > --- > ... > +-- gh-2618 Return generated columns after INSERT in IPROTO. > +-- Return first PK autogenerated in last INSERT/REPLACE in which > +-- new PK was genereted in current session. > +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') > +--- > +... > +cn = remote.connect(box.cfg.listen) > +--- > +... > +cn:execute('insert into test values (1, 1)', nil, nil) 6. Why do you need last two arguments be nil? It is the same as merely omit them. > +--- > +- last_insert_id: 0 > + rowcount: 1 > +... > +cn:execute('insert into test values (null, 2)', nil, nil) > +--- > +- last_insert_id: 2 > + rowcount: 1 > +... > +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) > +--- > +- last_insert_id: 3 > + rowcount: 2 > +... > +cn:execute('insert into test values (17, 2)', nil, nil) > +--- > +- last_insert_id: 3 > + rowcount: 1 > +... > +cn:execute('update test set a = 11 where id == 1', nil, nil) > +--- > +- last_insert_id: 3 > + rowcount: 1 > +... > +cn:execute('update test set a = 12 where id == 2', nil, nil) > +--- > +- last_insert_id: 3 > + rowcount: 1 > +... > +cn:execute('update test set a = 13 where id == 3', nil, nil) > +--- > +- last_insert_id: 3 > + rowcount: 1 > +... > +cn:execute('replace into test values (4, 44), (null, 100)', nil, nil) > +--- > +- last_insert_id: 18 > + rowcount: 2 > +... > +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) > +--- > +- last_insert_id: 19 > + rowcount: 2 > +... > +cn:execute('select * from test', nil, nil) > +--- > +- metadata: > + - name: ID > + - name: A > + rows: > + - [1, 11] > + - [2, 12] > + - [3, 13] > + - [4, 44] > + - [17, 2] > + - [18, 100] > + - [19, 2] > + - [20, 3] > +... ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO 2018-08-16 22:05 ` Vladislav Shpilevoy @ 2018-08-17 11:43 ` Imeev Mergen 2018-08-21 15:50 ` Vladislav Shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Imeev Mergen @ 2018-08-17 11:43 UTC (permalink / raw) To: Vladislav Shpilevoy, tarantool-patches Hello! Thank you for review! On 08/17/2018 01:05 AM, Vladislav Shpilevoy wrote: > Thanks for the fixes! See 6 comments below. > >> commit 2b292311e61f3c25a4c776bd75e3e6d241ff66a1 >> Author: Mergen Imeev <imeevma@gmail.com> >> Date: Tue Jul 31 16:43:38 2018 +0300 >> >> sql: return last_insert_id via IPROTO >> >> After this patch client will get last_insert_id >> as additional metadata when he executes some >> queries. >> >> Part of #2618. >> >> @TarantoolBot document >> Title: SQL function last_insert_id() >> and IPROTO key last_insert_id. >> Function last_insert_id() returns first primary key value >> autogenerated in last INSERT/REPLACE statement in current >> session. Return value of function is undetermined when more >> than one INSERT/REPLACE statements executed asynchronously. > > 1. Please, note that only for a single session of a single user. > If a user opens multiple connections, then in each last_insert_id > will work properly. Done. > >> IPROTO key last_insert_id is a metadata returned through >> IPROTO after such queries as INSERT, REPLACE, UPDATE etc. >> Value of this key is equal to value returned by function >> last_insert_id() executed after the query. >> Example: >> CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a >> INTEGER); >> INSERT INTO test VALUES (NULL, 1); >> SELECT last_insert_id(); >> >> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c >> index 308c9c7..83ba9b8 100644 >> --- a/src/box/lua/net_box.c >> +++ b/src/box/lua/net_box.c >> @@ -667,16 +667,21 @@ static void >> netbox_decode_sql_info(struct lua_State *L, const char **data) >> { >> uint32_t map_size = mp_decode_map(data); >> - /* Only SQL_INFO_ROW_COUNT is available. */ >> assert(map_size == 1); >> (void) map_size; >> uint32_t key = mp_decode_uint(data); >> assert(key == SQL_INFO_ROW_COUNT); >> - (void) key; >> uint32_t row_count = mp_decode_uint(data); >> - lua_createtable(L, 0, 1); >> + key = mp_decode_uint(data); >> + assert(key == SQL_INFO_LAST_INSERT_ID); >> + int64_t last_insert_id = mp_typeof(**data) == MP_UINT ? >> + (int64_t) mp_decode_uint(data) : >> + mp_decode_int(data); > > 2. Why not mp_read_int64() ? Fixed. > >> + lua_createtable(L, 0, 2); >> lua_pushinteger(L, row_count); >> lua_setfield(L, -2, "rowcount"); >> + lua_pushinteger(L, last_insert_id); >> + lua_setfield(L, -2, "last_insert_id"); >> } >> >> static int >> diff --git a/src/box/session.h b/src/box/session.h >> index df1dcbc..3d4049c 100644 >> --- a/src/box/session.h >> +++ b/src/box/session.h >> @@ -92,6 +92,8 @@ union session_meta { >> struct session { >> /** Session id. */ >> uint64_t id; >> + /** First PK autogenerated in last INSERT/REPLACE query */ > > 3. query -> statement. Sorry for nitpicking, it is Kostja O.'s > whim. But on the other hand it is literally more precise - > query is more likely about SELECT while statement concerns complex > multi-row DML constructions. Fixed. > >> + int64_t sql_last_insert_id; >> /** SQL Tarantool Default storage engine. */ >> uint8_t sql_default_engine; >> /** SQL Connection flag for current user session */ >> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c >> index 13cb61e..8b5b0df 100644 >> --- a/src/box/sql/insert.c >> +++ b/src/box/sql/insert.c >> @@ -739,9 +739,7 @@ sqlite3Insert(Parse * pParse, /* Parser >> context */ >> if (j < 0 || nColumn == 0 >> || (pColumn && j >= pColumn->nId)) { >> if (i == pTab->iAutoIncPKey) { >> - sqlite3VdbeAddOp2(v, >> - OP_NextAutoincValue, >> - pTab->def->id, >> + sqlite3VdbeAddOp2(v, OP_Null, 0, > > 4. Can we fix the bug related to OP_NextAutoincValue in a > separate commit? So the patchset would consist of 2 commits. Done. > > And why for iAutoInc do you generate OP_Null here? As I > understand, here and on line 799 you are sure this column is > NULL and can put next_auto_inc explicitly with no writing > NULL and then replacing it in a separate opcode. It is not? Decided to left as it is after discussion. > >> iRegStore); >> continue; >> } >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index dc5146f..dceb8c0 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -3745,7 +3761,7 @@ case OP_FCopy: { /* out2 */ >> >> if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) { >> pOut = &aMem[pOp->p2]; >> - if (pOut->flags & MEM_Undefined) pOut->flags = MEM_Null; >> + if (pOut->flags & MEM_Int) pOut->flags = MEM_Null; > > 5. Why? Fixed. > >> /* Flag is set and register is NULL -> do nothing */ >> } else { >> assert(memIsValid(pIn1)); >> diff --git a/test/sql/iproto.result b/test/sql/iproto.result >> index af474bc..5fdf49b 100644 >> --- a/test/sql/iproto.result >> +++ b/test/sql/iproto.result >> @@ -580,6 +612,91 @@ cn:close() >> box.sql.execute('drop table test') >> --- >> ... >> +-- gh-2618 Return generated columns after INSERT in IPROTO. >> +-- Return first PK autogenerated in last INSERT/REPLACE in which >> +-- new PK was genereted in current session. >> +box.sql.execute('create table test (id integer primary key >> autoincrement, a integer)') >> +--- >> +... >> +cn = remote.connect(box.cfg.listen) >> +--- >> +... >> +cn:execute('insert into test values (1, 1)', nil, nil) > > 6. Why do you need last two arguments be nil? It is the same > as merely omit them. Fixed. > >> +--- >> +- last_insert_id: 0 >> + rowcount: 1 >> +... >> +cn:execute('insert into test values (null, 2)', nil, nil) >> +--- >> +- last_insert_id: 2 >> + rowcount: 1 >> +... >> +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) >> +--- >> +- last_insert_id: 3 >> + rowcount: 2 >> +... >> +cn:execute('insert into test values (17, 2)', nil, nil) >> +--- >> +- last_insert_id: 3 >> + rowcount: 1 >> +... >> +cn:execute('update test set a = 11 where id == 1', nil, nil) >> +--- >> +- last_insert_id: 3 >> + rowcount: 1 >> +... >> +cn:execute('update test set a = 12 where id == 2', nil, nil) >> +--- >> +- last_insert_id: 3 >> + rowcount: 1 >> +... >> +cn:execute('update test set a = 13 where id == 3', nil, nil) >> +--- >> +- last_insert_id: 3 >> + rowcount: 1 >> +... >> +cn:execute('replace into test values (4, 44), (null, 100)', nil, nil) >> +--- >> +- last_insert_id: 18 >> + rowcount: 2 >> +... >> +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) >> +--- >> +- last_insert_id: 19 >> + rowcount: 2 >> +... >> +cn:execute('select * from test', nil, nil) >> +--- >> +- metadata: >> + - name: ID >> + - name: A >> + rows: >> + - [1, 11] >> + - [2, 12] >> + - [3, 13] >> + - [4, 44] >> + - [17, 2] >> + - [18, 100] >> + - [19, 2] >> + - [20, 3] >> +... commit c87a9ae3b3446e016956f6b52d34677b3e321286 Author: Mergen Imeev <imeevma@gmail.com> Date: Fri Aug 17 13:24:58 2018 +0300 sql: move autoincrement in vdbe This patch expands changes made in issue #2981. Now NULL in primary key always replaced by generated value inside of vdbe. It allows us to get last_insert_id with easy. Part of #2618 diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 13cb61e..f61ff99 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -739,9 +739,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ if (j < 0 || nColumn == 0 || (pColumn && j >= pColumn->nId)) { if (i == pTab->iAutoIncPKey) { - sqlite3VdbeAddOp2(v, - OP_NextAutoincValue, - pTab->def->id, + sqlite3VdbeAddOp2(v, OP_Null, 0, iRegStore); continue; } @@ -824,7 +822,14 @@ sqlite3Insert(Parse * pParse, /* Parser context */ iRegStore); } } - + /* + * Replace NULL in PK with autoincrement by + * generated value. + */ + if (pTab->iAutoIncPKey >= 0) { + sqlite3VdbeAddOp2(v, OP_NextAutoincValue, pTab->def->id, + regData + pTab->iAutoIncPKey); + } /* Generate code to check constraints and generate index keys and do the insertion. */ diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index dc5146f..30cd6b9 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1147,6 +1147,10 @@ case OP_NextAutoincValue: { assert(pOp->p1 > 0); assert(pOp->p2 > 0); + pIn2 = &p->aMem[pOp->p2]; + if ((pIn2->flags & MEM_Null) == 0) + break; + struct space *space = space_by_id(pOp->p1); if (space == NULL) { rc = SQL_TARANTOOL_ERROR; @@ -3745,7 +3749,7 @@ case OP_FCopy: { /* out2 */ if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) { pOut = &aMem[pOp->p2]; - if (pOut->flags & MEM_Undefined) pOut->flags = MEM_Null; + pOut->flags = MEM_Null; /* Flag is set and register is NULL -> do nothing */ } else { assert(memIsValid(pIn1)); diff --git a/test/sql/gh-2981-check-autoinc.result b/test/sql/gh-2981-check-autoinc.result index b0f55e6..43bd42b 100644 --- a/test/sql/gh-2981-check-autoinc.result +++ b/test/sql/gh-2981-check-autoinc.result @@ -19,6 +19,9 @@ box.sql.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEG box.sql.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));"); --- ... +box.sql.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); +--- +... box.sql.execute("insert into t1 values (18, null);") --- ... @@ -47,6 +50,13 @@ box.sql.execute("insert into t3(s2) values (null)") --- - error: 'CHECK constraint failed: T3' ... +box.sql.execute("insert into t4 values (18, null);") +--- +... +box.sql.execute("insert into t4 values (null, null);") +--- +- error: 'CHECK constraint failed: T4' +... box.sql.execute("DROP TABLE t1") --- ... @@ -56,3 +66,6 @@ box.sql.execute("DROP TABLE t2") box.sql.execute("DROP TABLE t3") --- ... +box.sql.execute("DROP TABLE t4") +--- +... diff --git a/test/sql/gh-2981-check-autoinc.test.lua b/test/sql/gh-2981-check-autoinc.test.lua index 98a5fb4..593ff3a 100644 --- a/test/sql/gh-2981-check-autoinc.test.lua +++ b/test/sql/gh-2981-check-autoinc.test.lua @@ -7,6 +7,7 @@ box.cfg{} box.sql.execute("CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); box.sql.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19 AND s1 <> 25));"); box.sql.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));"); +box.sql.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); box.sql.execute("insert into t1 values (18, null);") box.sql.execute("insert into t1(s2) values (null);") @@ -19,7 +20,10 @@ box.sql.execute("insert into t2(s2) values (null);") box.sql.execute("insert into t3 values (9, null)") box.sql.execute("insert into t3(s2) values (null)") +box.sql.execute("insert into t4 values (18, null);") +box.sql.execute("insert into t4 values (null, null);") + box.sql.execute("DROP TABLE t1") box.sql.execute("DROP TABLE t2") box.sql.execute("DROP TABLE t3") - +box.sql.execute("DROP TABLE t4") commit 010ec9b016ae6a49914b9e88544547ea9049e145 Author: Mergen Imeev <imeevma@gmail.com> Date: Tue Jul 31 16:43:38 2018 +0300 sql: return last_insert_id via IPROTO After this patch client will get last_insert_id as additional metadata when he executes some statements. Part of #2618 @TarantoolBot document Title: SQL function last_insert_id() and IPROTO key last_insert_id. Function last_insert_id() returns first primary key value autogenerated in last INSERT/REPLACE statement in current session. User can have more than one session and this function will work propertly for each one of them. Return value of function is undetermined when more than one INSERT/REPLACE statements executed asynchronously. IPROTO key last_insert_id is a metadata returned through IPROTO after such statements as INSERT, REPLACE, UPDATE etc. Value of this key is equal to value returned by function last_insert_id() executed after the statment. Example: CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER); INSERT INTO test VALUES (NULL, 1); SELECT last_insert_id(); diff --git a/src/box/execute.c b/src/box/execute.c index 24459b4..e2eabc7 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -42,6 +42,7 @@ #include "schema.h" #include "port.h" #include "tuple.h" +#include "session.h" const char *sql_type_strs[] = { NULL, @@ -640,8 +641,13 @@ err: if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0) goto err; int changes = sqlite3_changes(db); + int64_t last_insert_id = current_session()->sql_last_insert_id; int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + - mp_sizeof_uint(changes); + mp_sizeof_uint(changes) + + mp_sizeof_uint(SQL_INFO_LAST_INSERT_ID) + + (last_insert_id >= 0 ? + mp_sizeof_uint(last_insert_id) : + mp_sizeof_int(last_insert_id)); char *buf = obuf_alloc(out, size); if (buf == NULL) { diag_set(OutOfMemory, size, "obuf_alloc", "buf"); @@ -649,6 +655,9 @@ err: } buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT); buf = mp_encode_uint(buf, changes); + buf = mp_encode_uint(buf, SQL_INFO_LAST_INSERT_ID); + buf = last_insert_id < 0 ? mp_encode_int(buf, last_insert_id) : + mp_encode_uint(buf, last_insert_id); } iproto_reply_sql(out, &header_svp, response->sync, schema_version, keys); diff --git a/src/box/execute.h b/src/box/execute.h index f21393b..ead09fc 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -42,6 +42,7 @@ extern "C" { /** Keys of IPROTO_SQL_INFO map. */ enum sql_info_key { SQL_INFO_ROW_COUNT = 0, + SQL_INFO_LAST_INSERT_ID = 1, sql_info_key_MAX, }; diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index 308c9c7..4a82fca 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -667,16 +667,20 @@ static void netbox_decode_sql_info(struct lua_State *L, const char **data) { uint32_t map_size = mp_decode_map(data); - /* Only SQL_INFO_ROW_COUNT is available. */ assert(map_size == 1); (void) map_size; uint32_t key = mp_decode_uint(data); assert(key == SQL_INFO_ROW_COUNT); - (void) key; uint32_t row_count = mp_decode_uint(data); - lua_createtable(L, 0, 1); + key = mp_decode_uint(data); + assert(key == SQL_INFO_LAST_INSERT_ID); + int64_t last_insert_id; + mp_read_int64(data, &last_insert_id); + lua_createtable(L, 0, 2); lua_pushinteger(L, row_count); lua_setfield(L, -2, "rowcount"); + lua_pushinteger(L, last_insert_id); + lua_setfield(L, -2, "last_insert_id"); } static int diff --git a/src/box/session.cc b/src/box/session.cc index 64714cd..2b8dab9 100644 --- a/src/box/session.cc +++ b/src/box/session.cc @@ -108,6 +108,7 @@ session_create(enum session_type type) session->type = type; session->sql_flags = default_flags; session->sql_default_engine = SQL_STORAGE_ENGINE_MEMTX; + session->sql_last_insert_id = 0; /* For on_connect triggers. */ credentials_init(&session->credentials, guest_user->auth_token, diff --git a/src/box/session.h b/src/box/session.h index df1dcbc..6ee22bc 100644 --- a/src/box/session.h +++ b/src/box/session.h @@ -92,6 +92,11 @@ union session_meta { struct session { /** Session id. */ uint64_t id; + /** + * First primary key autogenerated in last INSERT/REPLACE + * statement in which primary key was generated. + */ + int64_t sql_last_insert_id; /** SQL Tarantool Default storage engine. */ uint8_t sql_default_engine; /** SQL Connection flag for current user session */ diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 45056a7..2607cc3 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -38,6 +38,7 @@ #include "vdbeInt.h" #include "version.h" #include "coll.h" +#include "box/session.h" #include <unicode/ustring.h> #include <unicode/ucasemap.h> #include <unicode/ucnv.h> @@ -601,6 +602,23 @@ changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2) } /* + * Return first primary key autogenerated in last INSERT/REPLACE + * statement in which primary key was generated in current + * session. + * + * @param context Context being used. + * @param not_used Unused. + * @param not_used2 Unused. + */ +static void +last_insert_id(sqlite3_context *context, int not_used, + sqlite3_value **not_used2) +{ + UNUSED_PARAMETER2(not_used, not_used2); + sqlite3_result_int(context, current_session()->sql_last_insert_id); +} + +/* * Implementation of the total_changes() SQL function. The return value is * the same as the sqlite3_total_changes() API function. */ @@ -1847,6 +1865,7 @@ sqlite3RegisterBuiltinFunctions(void) FUNCTION(lower, 1, 0, 1, LowerICUFunc), FUNCTION(hex, 1, 0, 0, hexFunc), FUNCTION2(ifnull, 2, 0, 0, noopFunc, SQLITE_FUNC_COALESCE), + VFUNCTION(last_insert_id, 0, 0, 0, last_insert_id), VFUNCTION(random, 0, 0, 0, randomFunc), VFUNCTION(randomblob, 1, 0, 0, randomBlob), FUNCTION(nullif, 2, 0, 1, nullifFunc), diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 30cd6b9..f7fbd71 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -600,6 +600,11 @@ int sqlite3VdbeExec(Vdbe *p) u64 start; /* CPU clock count at start of opcode */ #endif struct session *user_session = current_session(); + /* + * Field sql_last_insert_id of current session should be + * set no more than once for each statement. + */ + bool is_last_insert_id_set = false; /*** INSERT STACK UNION HERE ***/ assert(p->magic==VDBE_MAGIC_RUN); /* sqlite3_step() verifies this */ @@ -1168,6 +1173,13 @@ case OP_NextAutoincValue: { pOut->flags = MEM_Int; pOut->u.i = value; + /* Set sql_last_insert_id of current session. */ + if (!is_last_insert_id_set) { + struct session *session = current_session(); + session->sql_last_insert_id = value; + is_last_insert_id_set = true; + } + break; } diff --git a/test/sql-tap/insert3.test.lua b/test/sql-tap/insert3.test.lua index c0e9d95..8394cd9 100755 --- a/test/sql-tap/insert3.test.lua +++ b/test/sql-tap/insert3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(18) +test:plan(20) --!./tcltestrunner.lua -- 2005 January 13 @@ -262,6 +262,31 @@ test:do_execsql_test( -- <insert3-4.1> }) +-- gh-2618 function last_insert_id +test:execsql("CREATE TABLE t8(id INTEGER PRIMARY KEY AUTOINCREMENT, a INT);") +test:do_execsql_test( + "insert3-5.1", + [[ + INSERT INTO t8 VALUES (null, 11); + SELECT LAST_INSERT_ID(); + ]], { + -- <insert3-5.1> + 1 + -- <insert3-5.1> +}) + +test:do_execsql_test( + "insert3-5.2", + [[ + INSERT INTO t8 VALUES (null, 44), (null, 55), (null, 66); + SELECT LAST_INSERT_ID(); + ]], { + -- <insert3-5.1> + 2 + -- <insert3-5.1> +}) + + test:drop_all_tables() --------------------------------------------------------------------------- -- While developing tests for a different feature (savepoint) the following diff --git a/test/sql/errinj.result b/test/sql/errinj.result index a0ba60f..9443137 100644 --- a/test/sql/errinj.result +++ b/test/sql/errinj.result @@ -73,7 +73,8 @@ while f1:status() ~= 'dead' do fiber.sleep(0) end ... insert_res --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... select_res --- diff --git a/test/sql/iproto.result b/test/sql/iproto.result index af474bc..4e3cfe1 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -69,19 +69,23 @@ type(ret.rows[1]) -- Operation with rowcount result. cn:execute('insert into test values (10, 11, NULL)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('delete from test where a = 5') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('insert into test values (11, 12, NULL), (12, 12, NULL), (13, 12, NULL)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('delete from test where a = 12') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... -- SQL errors. cn:execute('insert into not_existing_table values ("kek")') @@ -330,7 +334,8 @@ cn:execute('select :value', parameters) -- gh-2608 SQL iproto DDL cn:execute('create table test2(id primary key, a, b, c)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST2.name --- @@ -338,7 +343,8 @@ box.space.TEST2.name ... cn:execute('insert into test2 values (1, 1, 1, 1)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('select * from test2') --- @@ -352,7 +358,8 @@ cn:execute('select * from test2') ... cn:execute('create index test2_a_b_index on test2(a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... #box.space.TEST2.index --- @@ -360,7 +367,8 @@ cn:execute('create index test2_a_b_index on test2(a, b)') ... cn:execute('drop table test2') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST2 --- @@ -370,100 +378,121 @@ box.space.TEST2 -- Test CREATE [IF NOT EXISTS] TABLE. cn:execute('create table test3(id primary key, a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... -- Rowcount = 1, although two tuples were created: -- for _space and for _index. cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('create table if not exists test3(id primary key)') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE VIEW [IF NOT EXISTS] and -- DROP VIEW [IF EXISTS]. cn:execute('create view test3_view(id) as select id from test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create view if not exists test3_view(id) as select id from test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop view test3_view') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop view if exists test3_view') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE INDEX [IF NOT EXISTS] and -- DROP INDEX [IF EXISTS]. cn:execute('create index test3_sec on test3(a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create index if not exists test3_sec on test3(a, b)') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop index test3_sec on test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop index if exists test3_sec on test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE TRIGGER [IF NOT EXISTS] and -- DROP TRIGGER [IF EXISTS]. cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create trigger if not exists trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop trigger trig') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop trigger if exists trig') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test DROP TABLE [IF EXISTS]. -- Create more indexes, triggers and _truncate tuple. cn:execute('create index idx1 on test3(a)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create index idx2 on test3(b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST3:truncate() --- ... cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('drop table test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop table if exists test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- -- gh-2948: sql: remove unnecessary templates for binding @@ -535,7 +564,8 @@ cn = remote.connect(box.cfg.listen) ... cn:execute('create table test (id integer primary key, a integer, b integer)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... future1 = cn:execute('insert into test values (1, 1, 1)', nil, nil, {is_async = true}) --- @@ -548,7 +578,8 @@ future3 = cn:execute('insert into test values (2, 2, 2), (3, 3, 3)', nil, nil, { ... future1:wait_result() --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... future2:wait_result() --- @@ -558,7 +589,8 @@ future2:wait_result() ... future3:wait_result() --- -- rowcount: 2 +- last_insert_id: 0 + rowcount: 2 ... future4 = cn:execute('select * from test', nil, nil, {is_async = true}) --- @@ -580,6 +612,91 @@ cn:close() box.sql.execute('drop table test') --- ... +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return first PK autogenerated in last INSERT/REPLACE in which +-- new PK was genereted in current session. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +--- +... +cn = remote.connect(box.cfg.listen) +--- +... +cn:execute('insert into test values (1, 1)') +--- +- last_insert_id: 0 + rowcount: 1 +... +cn:execute('insert into test values (null, 2)') +--- +- last_insert_id: 2 + rowcount: 1 +... +cn:execute('insert into test values (null, 2), (null, 3)') +--- +- last_insert_id: 3 + rowcount: 2 +... +cn:execute('insert into test values (17, 2)') +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('update test set a = 11 where id == 1') +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('update test set a = 12 where id == 2') +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('update test set a = 13 where id == 3') +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('replace into test values (4, 44), (null, 100)') +--- +- last_insert_id: 18 + rowcount: 2 +... +cn:execute('insert into test values (null, 2), (null, 3)') +--- +- last_insert_id: 19 + rowcount: 2 +... +cn:execute('select * from test') +--- +- metadata: + - name: ID + - name: A + rows: + - [1, 11] + - [2, 12] + - [3, 13] + - [4, 44] + - [17, 2] + - [18, 100] + - [19, 2] + - [20, 3] +... +cn:execute('create table test2 (id integer primary key, a integer)') +--- +- last_insert_id: 19 + rowcount: 1 +... +cn:execute('drop table test2') +--- +- last_insert_id: 19 + rowcount: 1 +... +cn:close() +--- +... +box.sql.execute('drop table test') +--- +... box.schema.user.revoke('guest', 'read,write,execute', 'universe') --- ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index 220331b..a9f55fb 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -201,6 +201,26 @@ future4:wait_result() cn:close() box.sql.execute('drop table test') +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return first PK autogenerated in last INSERT/REPLACE in which +-- new PK was genereted in current session. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +cn = remote.connect(box.cfg.listen) +cn:execute('insert into test values (1, 1)') +cn:execute('insert into test values (null, 2)') +cn:execute('insert into test values (null, 2), (null, 3)') +cn:execute('insert into test values (17, 2)') +cn:execute('update test set a = 11 where id == 1') +cn:execute('update test set a = 12 where id == 2') +cn:execute('update test set a = 13 where id == 3') +cn:execute('replace into test values (4, 44), (null, 100)') +cn:execute('insert into test values (null, 2), (null, 3)') +cn:execute('select * from test') +cn:execute('create table test2 (id integer primary key, a integer)') +cn:execute('drop table test2') +cn:close() +box.sql.execute('drop table test') + box.schema.user.revoke('guest', 'read,write,execute', 'universe') space = nil ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO 2018-08-17 11:43 ` Imeev Mergen @ 2018-08-21 15:50 ` Vladislav Shpilevoy 2018-08-22 13:31 ` Imeev Mergen 0 siblings, 1 reply; 8+ messages in thread From: Vladislav Shpilevoy @ 2018-08-21 15:50 UTC (permalink / raw) To: Imeev Mergen, tarantool-patches Hi! Thanks for the fixes! See my 5 comments below. > commit c87a9ae3b3446e016956f6b52d34677b3e321286 > Author: Mergen Imeev <imeevma@gmail.com> > Date: Fri Aug 17 13:24:58 2018 +0300 > > sql: move autoincrement in vdbe > > This patch expands changes made in issue #2981. Now NULL > in primary key always replaced by generated value inside > of vdbe. It allows us to get last_insert_id with easy. > > Part of #2618 > 1. This commit LGTM. > > commit 010ec9b016ae6a49914b9e88544547ea9049e145 > Author: Mergen Imeev <imeevma@gmail.com> > Date: Tue Jul 31 16:43:38 2018 +0300 > > sql: return last_insert_id via IPROTO > > After this patch client will get last_insert_id > as additional metadata when he executes some > statements. > > Part of #2618 > > @TarantoolBot document > Title: SQL function last_insert_id() > and IPROTO key last_insert_id. 2. Please, do not wrap Title section. TarantoolBot reads a request line by line splitting by '\n'. > Function last_insert_id() returns first primary key value > autogenerated in last INSERT/REPLACE statement in current > session. User can have more than one session and this > function will work propertly for each one of them. Return 3. Typo: 'propertly'. > value of function is undetermined when more than one > INSERT/REPLACE statements executed asynchronously. > IPROTO key last_insert_id is a metadata returned through > IPROTO after such statements as INSERT, REPLACE, UPDATE > etc. Value of this key is equal to value returned by > function last_insert_id() executed after the statment. 4. Typo: 'statment'. > Example: > CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER); > INSERT INTO test VALUES (NULL, 1); > SELECT last_insert_id(); > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 30cd6b9..f7fbd71 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -600,6 +600,11 @@ int sqlite3VdbeExec(Vdbe *p) > u64 start; /* CPU clock count at start of opcode */ > #endif > struct session *user_session = current_session(); > + /* > + * Field sql_last_insert_id of current session should be > + * set no more than once for each statement. > + */ > + bool is_last_insert_id_set = false; 5. Kirill Sch. is implementing #2370 feature that allows (as I hope for his implementation) to interrupt Vdbe on DML just like for DQL (SELECT) to return each affected tuple. It means, that VdbeExec will be called multiple times for a DML request. So lets move this flag into struct Vdbe into the flags section (next to 'isPrepareV2', 'expired', 'doingRerun' etc.) Let me admonish you before you started against 'bft' type usage. Use simple 'bool'. > /*** INSERT STACK UNION HERE ***/ > > assert(p->magic==VDBE_MAGIC_RUN); /* sqlite3_step() verifies this */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO 2018-08-21 15:50 ` Vladislav Shpilevoy @ 2018-08-22 13:31 ` Imeev Mergen 2018-08-24 10:05 ` Vladislav Shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Imeev Mergen @ 2018-08-22 13:31 UTC (permalink / raw) To: Vladislav Shpilevoy, tarantool-patches Hi! Thank you for review! On 08/21/2018 06:50 PM, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! See my 5 comments below. > >> commit c87a9ae3b3446e016956f6b52d34677b3e321286 >> Author: Mergen Imeev <imeevma@gmail.com> >> Date: Fri Aug 17 13:24:58 2018 +0300 >> >> sql: move autoincrement in vdbe >> >> This patch expands changes made in issue #2981. Now NULL >> in primary key always replaced by generated value inside >> of vdbe. It allows us to get last_insert_id with easy. >> >> Part of #2618 >> > > 1. This commit LGTM. > >> >> commit 010ec9b016ae6a49914b9e88544547ea9049e145 >> Author: Mergen Imeev <imeevma@gmail.com> >> Date: Tue Jul 31 16:43:38 2018 +0300 >> >> sql: return last_insert_id via IPROTO >> >> After this patch client will get last_insert_id >> as additional metadata when he executes some >> statements. >> >> Part of #2618 >> >> @TarantoolBot document >> Title: SQL function last_insert_id() >> and IPROTO key last_insert_id. > > 2. Please, do not wrap Title section. TarantoolBot reads > a request line by line splitting by '\n'. Fixed. > >> Function last_insert_id() returns first primary key value >> autogenerated in last INSERT/REPLACE statement in current >> session. User can have more than one session and this >> function will work propertly for each one of them. Return > > 3. Typo: 'propertly'. Fixed. > >> value of function is undetermined when more than one >> INSERT/REPLACE statements executed asynchronously. >> IPROTO key last_insert_id is a metadata returned through >> IPROTO after such statements as INSERT, REPLACE, UPDATE >> etc. Value of this key is equal to value returned by >> function last_insert_id() executed after the statment. > > 4. Typo: 'statment'. Fixed. > >> Example: >> CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a >> INTEGER); >> INSERT INTO test VALUES (NULL, 1); >> SELECT last_insert_id(); >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 30cd6b9..f7fbd71 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -600,6 +600,11 @@ int sqlite3VdbeExec(Vdbe *p) >> u64 start; /* CPU clock count at start of >> opcode */ >> #endif >> struct session *user_session = current_session(); >> + /* >> + * Field sql_last_insert_id of current session should be >> + * set no more than once for each statement. >> + */ >> + bool is_last_insert_id_set = false; > > 5. Kirill Sch. is implementing #2370 feature that allows (as I > hope for his implementation) to interrupt Vdbe on DML just like > for DQL (SELECT) to return each affected tuple. It means, that > VdbeExec will be called multiple times for a DML request. So > lets move this flag into struct Vdbe into the flags section > (next to 'isPrepareV2', 'expired', 'doingRerun' etc.) > > Let me admonish you before you started against 'bft' type usage. > Use simple 'bool'. Done. > >> /*** INSERT STACK UNION HERE ***/ >> >> assert(p->magic==VDBE_MAGIC_RUN); /* sqlite3_step() verifies >> this */ commit 2ab414e943955a89905f4f7919406d74d348a078 Author: Mergen Imeev <imeevma@gmail.com> Date: Fri Aug 17 13:24:58 2018 +0300 sql: move autoincrement in vdbe This patch expands changes made in issue #2981. Now NULL in primary key always replaced by generated value inside of vdbe. It allows us to get last_insert_id with easy. Part of #2618 diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 0e92f62..6afa0a2 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -739,9 +739,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ if (j < 0 || nColumn == 0 || (pColumn && j >= pColumn->nId)) { if (i == pTab->iAutoIncPKey) { - sqlite3VdbeAddOp2(v, - OP_NextAutoincValue, - pTab->def->id, + sqlite3VdbeAddOp2(v, OP_Null, 0, iRegStore); continue; } @@ -824,7 +822,14 @@ sqlite3Insert(Parse * pParse, /* Parser context */ iRegStore); } } - + /* + * Replace NULL in PK with autoincrement by + * generated value. + */ + if (pTab->iAutoIncPKey >= 0) { + sqlite3VdbeAddOp2(v, OP_NextAutoincValue, pTab->def->id, + regData + pTab->iAutoIncPKey); + } /* Generate code to check constraints and generate index keys and do the insertion. */ diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index dc3f5c0..c6ece15 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1157,6 +1157,10 @@ case OP_NextAutoincValue: { assert(pOp->p1 > 0); assert(pOp->p2 > 0); + pIn2 = &p->aMem[pOp->p2]; + if ((pIn2->flags & MEM_Null) == 0) + break; + struct space *space = space_by_id(pOp->p1); if (space == NULL) { rc = SQL_TARANTOOL_ERROR; @@ -3755,7 +3759,7 @@ case OP_FCopy: { /* out2 */ if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) { pOut = &aMem[pOp->p2]; - if (pOut->flags & MEM_Undefined) pOut->flags = MEM_Null; + pOut->flags = MEM_Null; /* Flag is set and register is NULL -> do nothing */ } else { assert(memIsValid(pIn1)); diff --git a/test/sql/gh-2981-check-autoinc.result b/test/sql/gh-2981-check-autoinc.result index b0f55e6..43bd42b 100644 --- a/test/sql/gh-2981-check-autoinc.result +++ b/test/sql/gh-2981-check-autoinc.result @@ -19,6 +19,9 @@ box.sql.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEG box.sql.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));"); --- ... +box.sql.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); +--- +... box.sql.execute("insert into t1 values (18, null);") --- ... @@ -47,6 +50,13 @@ box.sql.execute("insert into t3(s2) values (null)") --- - error: 'CHECK constraint failed: T3' ... +box.sql.execute("insert into t4 values (18, null);") +--- +... +box.sql.execute("insert into t4 values (null, null);") +--- +- error: 'CHECK constraint failed: T4' +... box.sql.execute("DROP TABLE t1") --- ... @@ -56,3 +66,6 @@ box.sql.execute("DROP TABLE t2") box.sql.execute("DROP TABLE t3") --- ... +box.sql.execute("DROP TABLE t4") +--- +... diff --git a/test/sql/gh-2981-check-autoinc.test.lua b/test/sql/gh-2981-check-autoinc.test.lua index 98a5fb4..593ff3a 100644 --- a/test/sql/gh-2981-check-autoinc.test.lua +++ b/test/sql/gh-2981-check-autoinc.test.lua @@ -7,6 +7,7 @@ box.cfg{} box.sql.execute("CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); box.sql.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19 AND s1 <> 25));"); box.sql.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));"); +box.sql.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); box.sql.execute("insert into t1 values (18, null);") box.sql.execute("insert into t1(s2) values (null);") @@ -19,7 +20,10 @@ box.sql.execute("insert into t2(s2) values (null);") box.sql.execute("insert into t3 values (9, null)") box.sql.execute("insert into t3(s2) values (null)") +box.sql.execute("insert into t4 values (18, null);") +box.sql.execute("insert into t4 values (null, null);") + box.sql.execute("DROP TABLE t1") box.sql.execute("DROP TABLE t2") box.sql.execute("DROP TABLE t3") - +box.sql.execute("DROP TABLE t4") commit 67d6dc1cc24a35786fab71222692672c4b01d4be Author: Mergen Imeev <imeevma@gmail.com> Date: Tue Jul 31 16:43:38 2018 +0300 sql: return last_insert_id via IPROTO After this patch client will get last_insert_id as additional metadata when he executes some statements. Part of #2618 @TarantoolBot document Title: SQL function last_insert_id() and IPROTO key last_insert_id. Function last_insert_id() returns first primary key value autogenerated in last INSERT/REPLACE statement in current session. User can have more than one session and this function will work properly for each one of them. Return value of function is undetermined when more than one INSERT/REPLACE statements executed asynchronously. IPROTO key last_insert_id is a metadata returned through IPROTO after such statements as INSERT, REPLACE, UPDATE etc. Value of this key is equal to value returned by function last_insert_id() executed after the statement. Example: CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER); INSERT INTO test VALUES (NULL, 1); SELECT last_insert_id(); diff --git a/src/box/execute.c b/src/box/execute.c index 24459b4..e2eabc7 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -42,6 +42,7 @@ #include "schema.h" #include "port.h" #include "tuple.h" +#include "session.h" const char *sql_type_strs[] = { NULL, @@ -640,8 +641,13 @@ err: if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0) goto err; int changes = sqlite3_changes(db); + int64_t last_insert_id = current_session()->sql_last_insert_id; int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + - mp_sizeof_uint(changes); + mp_sizeof_uint(changes) + + mp_sizeof_uint(SQL_INFO_LAST_INSERT_ID) + + (last_insert_id >= 0 ? + mp_sizeof_uint(last_insert_id) : + mp_sizeof_int(last_insert_id)); char *buf = obuf_alloc(out, size); if (buf == NULL) { diag_set(OutOfMemory, size, "obuf_alloc", "buf"); @@ -649,6 +655,9 @@ err: } buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT); buf = mp_encode_uint(buf, changes); + buf = mp_encode_uint(buf, SQL_INFO_LAST_INSERT_ID); + buf = last_insert_id < 0 ? mp_encode_int(buf, last_insert_id) : + mp_encode_uint(buf, last_insert_id); } iproto_reply_sql(out, &header_svp, response->sync, schema_version, keys); diff --git a/src/box/execute.h b/src/box/execute.h index f21393b..ead09fc 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -42,6 +42,7 @@ extern "C" { /** Keys of IPROTO_SQL_INFO map. */ enum sql_info_key { SQL_INFO_ROW_COUNT = 0, + SQL_INFO_LAST_INSERT_ID = 1, sql_info_key_MAX, }; diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index 308c9c7..4a82fca 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -667,16 +667,20 @@ static void netbox_decode_sql_info(struct lua_State *L, const char **data) { uint32_t map_size = mp_decode_map(data); - /* Only SQL_INFO_ROW_COUNT is available. */ assert(map_size == 1); (void) map_size; uint32_t key = mp_decode_uint(data); assert(key == SQL_INFO_ROW_COUNT); - (void) key; uint32_t row_count = mp_decode_uint(data); - lua_createtable(L, 0, 1); + key = mp_decode_uint(data); + assert(key == SQL_INFO_LAST_INSERT_ID); + int64_t last_insert_id; + mp_read_int64(data, &last_insert_id); + lua_createtable(L, 0, 2); lua_pushinteger(L, row_count); lua_setfield(L, -2, "rowcount"); + lua_pushinteger(L, last_insert_id); + lua_setfield(L, -2, "last_insert_id"); } static int diff --git a/src/box/session.cc b/src/box/session.cc index 64714cd..2b8dab9 100644 --- a/src/box/session.cc +++ b/src/box/session.cc @@ -108,6 +108,7 @@ session_create(enum session_type type) session->type = type; session->sql_flags = default_flags; session->sql_default_engine = SQL_STORAGE_ENGINE_MEMTX; + session->sql_last_insert_id = 0; /* For on_connect triggers. */ credentials_init(&session->credentials, guest_user->auth_token, diff --git a/src/box/session.h b/src/box/session.h index df1dcbc..6ee22bc 100644 --- a/src/box/session.h +++ b/src/box/session.h @@ -92,6 +92,11 @@ union session_meta { struct session { /** Session id. */ uint64_t id; + /** + * First primary key autogenerated in last INSERT/REPLACE + * statement in which primary key was generated. + */ + int64_t sql_last_insert_id; /** SQL Tarantool Default storage engine. */ uint8_t sql_default_engine; /** SQL Connection flag for current user session */ diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 45056a7..2607cc3 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -38,6 +38,7 @@ #include "vdbeInt.h" #include "version.h" #include "coll.h" +#include "box/session.h" #include <unicode/ustring.h> #include <unicode/ucasemap.h> #include <unicode/ucnv.h> @@ -601,6 +602,23 @@ changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2) } /* + * Return first primary key autogenerated in last INSERT/REPLACE + * statement in which primary key was generated in current + * session. + * + * @param context Context being used. + * @param not_used Unused. + * @param not_used2 Unused. + */ +static void +last_insert_id(sqlite3_context *context, int not_used, + sqlite3_value **not_used2) +{ + UNUSED_PARAMETER2(not_used, not_used2); + sqlite3_result_int(context, current_session()->sql_last_insert_id); +} + +/* * Implementation of the total_changes() SQL function. The return value is * the same as the sqlite3_total_changes() API function. */ @@ -1847,6 +1865,7 @@ sqlite3RegisterBuiltinFunctions(void) FUNCTION(lower, 1, 0, 1, LowerICUFunc), FUNCTION(hex, 1, 0, 0, hexFunc), FUNCTION2(ifnull, 2, 0, 0, noopFunc, SQLITE_FUNC_COALESCE), + VFUNCTION(last_insert_id, 0, 0, 0, last_insert_id), VFUNCTION(random, 0, 0, 0, randomFunc), VFUNCTION(randomblob, 1, 0, 0, randomBlob), FUNCTION(nullif, 2, 0, 1, nullifFunc), diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index c6ece15..93ef00b 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -610,6 +610,11 @@ int sqlite3VdbeExec(Vdbe *p) u64 start; /* CPU clock count at start of opcode */ #endif struct session *user_session = current_session(); + /* + * Field sql_last_insert_id of current session should be + * set no more than once for each statement. + */ + p->is_last_insert_id_set = false; /*** INSERT STACK UNION HERE ***/ assert(p->magic==VDBE_MAGIC_RUN); /* sqlite3_step() verifies this */ @@ -1178,6 +1183,13 @@ case OP_NextAutoincValue: { pOut->flags = MEM_Int; pOut->u.i = value; + /* Set sql_last_insert_id of current session. */ + if (!p->is_last_insert_id_set) { + struct session *session = current_session(); + session->sql_last_insert_id = value; + p->is_last_insert_id_set = true; + } + break; } diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index ce97f49..d768eee 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -400,6 +400,11 @@ struct Vdbe { VdbeFrame *pDelFrame; /* List of frame objects to free on VM reset */ int nFrame; /* Number of frames in pFrame list */ u32 expmask; /* Binding to these vars invalidates VM */ + /* + * True if field last_insert_id of current session was set + * in current statement. + */ + bool is_last_insert_id_set; SubProgram *pProgram; /* Linked list of all sub-programs used by VM */ AuxData *pAuxData; /* Linked list of auxdata allocations */ /* Anonymous savepoint for aborts only */ diff --git a/test/sql-tap/insert3.test.lua b/test/sql-tap/insert3.test.lua index c0e9d95..8394cd9 100755 --- a/test/sql-tap/insert3.test.lua +++ b/test/sql-tap/insert3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(18) +test:plan(20) --!./tcltestrunner.lua -- 2005 January 13 @@ -262,6 +262,31 @@ test:do_execsql_test( -- <insert3-4.1> }) +-- gh-2618 function last_insert_id +test:execsql("CREATE TABLE t8(id INTEGER PRIMARY KEY AUTOINCREMENT, a INT);") +test:do_execsql_test( + "insert3-5.1", + [[ + INSERT INTO t8 VALUES (null, 11); + SELECT LAST_INSERT_ID(); + ]], { + -- <insert3-5.1> + 1 + -- <insert3-5.1> +}) + +test:do_execsql_test( + "insert3-5.2", + [[ + INSERT INTO t8 VALUES (null, 44), (null, 55), (null, 66); + SELECT LAST_INSERT_ID(); + ]], { + -- <insert3-5.1> + 2 + -- <insert3-5.1> +}) + + test:drop_all_tables() --------------------------------------------------------------------------- -- While developing tests for a different feature (savepoint) the following diff --git a/test/sql/errinj.result b/test/sql/errinj.result index a0ba60f..9443137 100644 --- a/test/sql/errinj.result +++ b/test/sql/errinj.result @@ -73,7 +73,8 @@ while f1:status() ~= 'dead' do fiber.sleep(0) end ... insert_res --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... select_res --- diff --git a/test/sql/iproto.result b/test/sql/iproto.result index af474bc..4e3cfe1 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -69,19 +69,23 @@ type(ret.rows[1]) -- Operation with rowcount result. cn:execute('insert into test values (10, 11, NULL)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('delete from test where a = 5') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('insert into test values (11, 12, NULL), (12, 12, NULL), (13, 12, NULL)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('delete from test where a = 12') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... -- SQL errors. cn:execute('insert into not_existing_table values ("kek")') @@ -330,7 +334,8 @@ cn:execute('select :value', parameters) -- gh-2608 SQL iproto DDL cn:execute('create table test2(id primary key, a, b, c)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST2.name --- @@ -338,7 +343,8 @@ box.space.TEST2.name ... cn:execute('insert into test2 values (1, 1, 1, 1)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('select * from test2') --- @@ -352,7 +358,8 @@ cn:execute('select * from test2') ... cn:execute('create index test2_a_b_index on test2(a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... #box.space.TEST2.index --- @@ -360,7 +367,8 @@ cn:execute('create index test2_a_b_index on test2(a, b)') ... cn:execute('drop table test2') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST2 --- @@ -370,100 +378,121 @@ box.space.TEST2 -- Test CREATE [IF NOT EXISTS] TABLE. cn:execute('create table test3(id primary key, a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... -- Rowcount = 1, although two tuples were created: -- for _space and for _index. cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('create table if not exists test3(id primary key)') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE VIEW [IF NOT EXISTS] and -- DROP VIEW [IF EXISTS]. cn:execute('create view test3_view(id) as select id from test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create view if not exists test3_view(id) as select id from test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop view test3_view') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop view if exists test3_view') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE INDEX [IF NOT EXISTS] and -- DROP INDEX [IF EXISTS]. cn:execute('create index test3_sec on test3(a, b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create index if not exists test3_sec on test3(a, b)') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop index test3_sec on test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop index if exists test3_sec on test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test CREATE TRIGGER [IF NOT EXISTS] and -- DROP TRIGGER [IF EXISTS]. cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create trigger if not exists trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... cn:execute('drop trigger trig') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop trigger if exists trig') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- Test DROP TABLE [IF EXISTS]. -- Create more indexes, triggers and _truncate tuple. cn:execute('create index idx1 on test3(a)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('create index idx2 on test3(b)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... box.space.TEST3:truncate() --- ... cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') --- -- rowcount: 3 +- last_insert_id: 0 + rowcount: 3 ... cn:execute('drop table test3') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... cn:execute('drop table if exists test3') --- -- rowcount: 0 +- last_insert_id: 0 + rowcount: 0 ... -- -- gh-2948: sql: remove unnecessary templates for binding @@ -535,7 +564,8 @@ cn = remote.connect(box.cfg.listen) ... cn:execute('create table test (id integer primary key, a integer, b integer)') --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... future1 = cn:execute('insert into test values (1, 1, 1)', nil, nil, {is_async = true}) --- @@ -548,7 +578,8 @@ future3 = cn:execute('insert into test values (2, 2, 2), (3, 3, 3)', nil, nil, { ... future1:wait_result() --- -- rowcount: 1 +- last_insert_id: 0 + rowcount: 1 ... future2:wait_result() --- @@ -558,7 +589,8 @@ future2:wait_result() ... future3:wait_result() --- -- rowcount: 2 +- last_insert_id: 0 + rowcount: 2 ... future4 = cn:execute('select * from test', nil, nil, {is_async = true}) --- @@ -580,6 +612,91 @@ cn:close() box.sql.execute('drop table test') --- ... +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return first PK autogenerated in last INSERT/REPLACE in which +-- new PK was genereted in current session. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +--- +... +cn = remote.connect(box.cfg.listen) +--- +... +cn:execute('insert into test values (1, 1)') +--- +- last_insert_id: 0 + rowcount: 1 +... +cn:execute('insert into test values (null, 2)') +--- +- last_insert_id: 2 + rowcount: 1 +... +cn:execute('insert into test values (null, 2), (null, 3)') +--- +- last_insert_id: 3 + rowcount: 2 +... +cn:execute('insert into test values (17, 2)') +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('update test set a = 11 where id == 1') +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('update test set a = 12 where id == 2') +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('update test set a = 13 where id == 3') +--- +- last_insert_id: 3 + rowcount: 1 +... +cn:execute('replace into test values (4, 44), (null, 100)') +--- +- last_insert_id: 18 + rowcount: 2 +... +cn:execute('insert into test values (null, 2), (null, 3)') +--- +- last_insert_id: 19 + rowcount: 2 +... +cn:execute('select * from test') +--- +- metadata: + - name: ID + - name: A + rows: + - [1, 11] + - [2, 12] + - [3, 13] + - [4, 44] + - [17, 2] + - [18, 100] + - [19, 2] + - [20, 3] +... +cn:execute('create table test2 (id integer primary key, a integer)') +--- +- last_insert_id: 19 + rowcount: 1 +... +cn:execute('drop table test2') +--- +- last_insert_id: 19 + rowcount: 1 +... +cn:close() +--- +... +box.sql.execute('drop table test') +--- +... box.schema.user.revoke('guest', 'read,write,execute', 'universe') --- ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index 220331b..a9f55fb 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -201,6 +201,26 @@ future4:wait_result() cn:close() box.sql.execute('drop table test') +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return first PK autogenerated in last INSERT/REPLACE in which +-- new PK was genereted in current session. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +cn = remote.connect(box.cfg.listen) +cn:execute('insert into test values (1, 1)') +cn:execute('insert into test values (null, 2)') +cn:execute('insert into test values (null, 2), (null, 3)') +cn:execute('insert into test values (17, 2)') +cn:execute('update test set a = 11 where id == 1') +cn:execute('update test set a = 12 where id == 2') +cn:execute('update test set a = 13 where id == 3') +cn:execute('replace into test values (4, 44), (null, 100)') +cn:execute('insert into test values (null, 2), (null, 3)') +cn:execute('select * from test') +cn:execute('create table test2 (id integer primary key, a integer)') +cn:execute('drop table test2') +cn:close() +box.sql.execute('drop table test') + box.schema.user.revoke('guest', 'read,write,execute', 'universe') space = nil ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO 2018-08-22 13:31 ` Imeev Mergen @ 2018-08-24 10:05 ` Vladislav Shpilevoy 0 siblings, 0 replies; 8+ messages in thread From: Vladislav Shpilevoy @ 2018-08-24 10:05 UTC (permalink / raw) To: Imeev Mergen, tarantool-patches Hi! Thanks for the fixes! > commit 67d6dc1cc24a35786fab71222692672c4b01d4be > Author: Mergen Imeev <imeevma@gmail.com> > Date: Tue Jul 31 16:43:38 2018 +0300 > > sql: return last_insert_id via IPROTO > > After this patch client will get last_insert_id > as additional metadata when he executes some > statements. > > Part of #2618 > > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h > index ce97f49..d768eee 100644 > --- a/src/box/sql/vdbeInt.h > +++ b/src/box/sql/vdbeInt.h > @@ -400,6 +400,11 @@ struct Vdbe { > VdbeFrame *pDelFrame; /* List of frame objects to free on VM reset */ > int nFrame; /* Number of frames in pFrame list */ > u32 expmask; /* Binding to these vars invalidates VM */ > + /* > + * True if field last_insert_id of current session was set > + * in current statement. > + */ > + bool is_last_insert_id_set; As I asked, this flag had to be moved to the flags section of struct Vdbe, next to isPrepareV2, runOnlyOnce etc and occupy 1 bit. I did it for you this time in a separate commit on the branch. Please, look and squash. And send the whole patchset under v3 to Nikita P. > SubProgram *pProgram; /* Linked list of all sub-programs used by VM */ > AuxData *pAuxData; /* Linked list of auxdata allocations */ > /* Anonymous savepoint for aborts only */ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-08-24 10:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-03 10:28 [tarantool-patches] [PATCH v2 1/1] sql: return last_insert_id via IPROTO imeevma 2018-08-06 15:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-13 12:34 ` Imeev Mergen 2018-08-16 22:05 ` Vladislav Shpilevoy 2018-08-17 11:43 ` Imeev Mergen 2018-08-21 15:50 ` Vladislav Shpilevoy 2018-08-22 13:31 ` Imeev Mergen 2018-08-24 10:05 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox