Re: [Tarantool-patches] [PATCH] sql: reset values to be bound after execution

[Konstantin Osipov <kostja.osipov@gmail.com>]

* 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?

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