[tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Aug 6 18:24:35 MSK 2018
Hi! Thanks for the patch! See 11 comments below.
On 03/08/2018 13:28, imeevma at 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?
More information about the Tarantool-patches
mailing list