From: Nikita Pettik <korablev@tarantool.org> To: Konstantin Osipov <kostja.osipov@gmail.com>, tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Subject: Re: [Tarantool-patches] [PATCH] sql: reset values to be bound after execution Date: Mon, 6 Apr 2020 11:20:03 +0000 [thread overview] Message-ID: <20200406112003.GC20849@tarantool.org> (raw) In-Reply-To: <20200403200756.GA20165@atlas> On 03 Apr 23:07, Konstantin Osipov wrote: > * Nikita Pettik <korablev@tarantool.org> [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
next prev parent reply other threads:[~2020-04-06 11:20 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-03 18:22 Nikita Pettik 2020-04-03 20:07 ` Konstantin Osipov 2020-04-06 11:20 ` Nikita Pettik [this message] 2020-04-06 21:27 ` Vladislav Shpilevoy 2020-04-07 11:40 ` Nikita Pettik 2020-04-03 21:23 ` Eugene Leonovich 2020-04-06 12:15 ` Nikita Pettik
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200406112003.GC20849@tarantool.org \ --to=korablev@tarantool.org \ --cc=kostja.osipov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] sql: reset values to be bound after execution' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox