From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C6EFE4696C3 for ; Mon, 6 Apr 2020 14:20:04 +0300 (MSK) Date: Mon, 6 Apr 2020 11:20:03 +0000 From: Nikita Pettik Message-ID: <20200406112003.GC20849@tarantool.org> References: <20200403200756.GA20165@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200403200756.GA20165@atlas> Subject: Re: [Tarantool-patches] [PATCH] sql: reset values to be bound after execution List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org On 03 Apr 23:07, Konstantin Osipov wrote: > * Nikita Pettik [20/04/03 21:23]: > > Before this patch prepared statements didn't reset bound values after > > its execution. As a result, if during next execution cycle not all > > parameters were provided, cached values would appear. For instance: > > > > prep = box.prepare('select :a, :b, :c') > > prep:execute({{[':a'] = 1}, {[':b'] = 2}, {[':c'] = 3}} > > -- [1, 2, 3] > > prep:execute({{[':a'] = 1}, {[':b'] = 2}}) > > -- [1, 2, 3] > > > > However, expected result for the last query should be [1, 2, NULL]. > > Let's fix it and always reset all binding values before next execution. > > > > Closes #4825 > > Since this is a bug fix it perhaps should go in, but something is > wrong here with the implementation. > > How is this problem solved in sqlite? There's no meta in sqlite ergo problem does not exist. But in SQLite prepared statement is modified as well: for instance see sqlite3_clear_bindings() in source code. > Execution should not change vdbe in any way. Otherwise there is > a risk to end up with a complex vdbe_cleanup_after_execution() > routine (instead of throwing away execution state altogether). Prepared statement is a state of VM, it is SQLite legacy. We call sql_stmt_reset() each time after prepared statement execution. Still it would be nice to split VM and byte-code - I already considered this option, but it would require a lot of refactoring. > > --- > > Branch: https://github.com/tarantool/tarantool/tree/np/gh-4825-cleanup-prep-stmt-bind > > Issue: https://github.com/tarantool/tarantool/issues/4825 > > > > @ChangeLog (2.3 and 2.4) > > * Reset binding value placeholders after execution of prepared statement. > > > > src/box/execute.c | 5 ++++ > > src/box/sql/sqlInt.h | 4 ++++ > > src/box/sql/vdbeapi.c | 17 ++++++++++++++ > > test/sql/prepared.result | 48 ++++++++++++++++++++++++++++++++++++++ > > test/sql/prepared.test.lua | 10 ++++++++ > > 5 files changed, 84 insertions(+) > > > > diff --git a/src/box/execute.c b/src/box/execute.c > > index 24f8529ec..66eac9814 100644 > > --- a/src/box/execute.c > > +++ b/src/box/execute.c > > @@ -711,6 +711,11 @@ sql_execute_prepared(uint32_t stmt_id, const struct sql_bind *bind, > > return sql_prepare_and_execute(sql_str, strlen(sql_str), bind, > > bind_count, port, region); > > } > > + /* > > + * Clear all set from previous execution cycle > > + * values to be bound. > > + */ > > + sql_unbind(stmt); > > if (sql_bind(stmt, bind, bind_count) != 0) > > return -1; > > enum sql_serialization_format format = sql_column_count(stmt) > 0 ? > > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > > index 1579cc92e..2d6bad424 100644 > > --- a/src/box/sql/sqlInt.h > > +++ b/src/box/sql/sqlInt.h > > @@ -658,6 +658,10 @@ sql_vfs_register(sql_vfs *, int makeDflt); > > #define SQL_STMTSTATUS_AUTOINDEX 3 > > #define SQL_STMTSTATUS_VM_STEP 4 > > > > +/** Unbind all parameters of given prepared statement. */ > > +void > > +sql_unbind(struct sql_stmt *stmt); > > + > > int > > sql_bind_blob(sql_stmt *, int, const void *, > > int n, void (*)(void *)); > > diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c > > index c984f9506..6e307e97f 100644 > > --- a/src/box/sql/vdbeapi.c > > +++ b/src/box/sql/vdbeapi.c > > @@ -934,6 +934,23 @@ sql_bind_type(struct Vdbe *v, uint32_t position, const char *type) > > return 0; > > } > > > > +void > > +sql_unbind(struct sql_stmt *stmt) > > +{ > > + struct Vdbe *v = (struct Vdbe *) stmt; > > + for (int i = 1; i < v->nVar + 1; ++i) { > > + int rc = vdbeUnbind(v, i); > > + assert(rc == 0); > > + (void) rc; > > + /* > > + * We should re-set boolean type - unassigned > > + * binding slots are assumed to contain NULL > > + * value, which has boolean type. > > + */ > > + sql_bind_type(v, i, "boolean"); > > + } > > +} > > + > > /* > > * Bind a text or BLOB value. > > */ > > diff --git a/test/sql/prepared.result b/test/sql/prepared.result > > index 5a16ea3b1..666f6e1d3 100644 > > --- a/test/sql/prepared.result > > +++ b/test/sql/prepared.result > > @@ -828,6 +828,54 @@ f2:join(); > > | - true > > | ... > > > > +-- gh-4825: make sure that values to be bound are erased after > > +-- execution, so that they don't appear in the next statement > > +-- execution. > > +-- > > +s = prepare('SELECT :a, :b, :c'); > > + | --- > > + | ... > > +execute(s.stmt_id, {{[':a'] = 1}, {[':b'] = 2}, {[':c'] = 3}}); > > + | --- > > + | - metadata: > > + | - name: :a > > + | type: integer > > + | - name: :b > > + | type: integer > > + | - name: :c > > + | type: integer > > + | rows: > > + | - [1, 2, 3] > > + | ... > > +execute(s.stmt_id, {{[':a'] = 1}, {[':b'] = 2}}); > > + | --- > > + | - metadata: > > + | - name: :a > > + | type: integer > > + | - name: :b > > + | type: integer > > + | - name: :c > > + | type: boolean > > + | rows: > > + | - [1, 2, null] > > + | ... > > +execute(s.stmt_id); > > + | --- > > + | - metadata: > > + | - name: :a > > + | type: boolean > > + | - name: :b > > + | type: boolean > > + | - name: :c > > + | type: boolean > > + | rows: > > + | - [null, null, null] > > + | ... > > +unprepare(s.stmt_id); > > + | --- > > + | - null > > + | ... > > + > > if is_remote then > > cn:close() > > box.schema.user.revoke('guest', 'read, write, execute', 'universe') > > diff --git a/test/sql/prepared.test.lua b/test/sql/prepared.test.lua > > index 85116aa47..d8e8a44cb 100644 > > --- a/test/sql/prepared.test.lua > > +++ b/test/sql/prepared.test.lua > > @@ -319,6 +319,16 @@ f2:set_joinable(true) > > f1:join(); > > f2:join(); > > > > +-- gh-4825: make sure that values to be bound are erased after > > +-- execution, so that they don't appear in the next statement > > +-- execution. > > +-- > > +s = prepare('SELECT :a, :b, :c'); > > +execute(s.stmt_id, {{[':a'] = 1}, {[':b'] = 2}, {[':c'] = 3}}); > > +execute(s.stmt_id, {{[':a'] = 1}, {[':b'] = 2}}); > > +execute(s.stmt_id); > > +unprepare(s.stmt_id); > > + > > if is_remote then > > cn:close() > > box.schema.user.revoke('guest', 'read, write, execute', 'universe') > > -- > > 2.17.1 > > -- > Konstantin Osipov, Moscow, Russia