From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com [209.85.208.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 175EB4696C3 for ; Fri, 3 Apr 2020 23:08:00 +0300 (MSK) Received: by mail-lj1-f195.google.com with SMTP id b1so8273754ljp.3 for ; Fri, 03 Apr 2020 13:07:59 -0700 (PDT) Date: Fri, 3 Apr 2020 23:07:56 +0300 From: Konstantin Osipov Message-ID: <20200403200756.GA20165@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org * 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? 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). > --- > 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