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

[Nikita Pettik]

On 03 Apr 23:07, Konstantin Osipov wrote:
> * Nikita Pettik <korablev at 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