Re: [Tarantool-patches] [PATCH v3 04/20] sql: rename sqlPrepare() to sql_stmt_compile()

[Sergey Ostanevich <sergos@tarantool.org>]

Hi! 

Thanks for the patch! Some comments below

On 20 Dec 15:47, Nikita Pettik wrote:
> sql_prepare() is going not only to compile statement, but also to save it
> to the prepared statement cache. So we'd better rename sqlPrepare()
> which is static wrapper around sql_prepare() and make it non-static.
> Where it is possible let's use sql_stmt_compile() instead of sql_prepare().
> 
> Needed for #2592
> ---
>  src/box/execute.c     |  2 +-
>  src/box/sql/analyze.c |  6 +++---
>  src/box/sql/legacy.c  |  2 +-
>  src/box/sql/prepare.c | 21 ++++++---------------
>  src/box/sql/sqlInt.h  | 14 ++++++++++++++
>  src/box/sql/vdbeapi.c |  2 +-
>  6 files changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 0b21386b5..af66447b5 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -442,7 +442,7 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
>  			struct region *region)
>  {
>  	struct sql_stmt *stmt;
> -	if (sql_prepare(sql, len, &stmt, NULL) != 0)
> +	if (sql_stmt_compile(sql, len, NULL, &stmt, NULL) != 0)
>  		return -1;
>  	assert(stmt != NULL);
>  	port_sql_create(port, stmt);
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 00ca15413..42e2a1a2f 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1359,7 +1359,7 @@ load_stat_from_space(const char *sql_select_prepare,
>  		}
>  	}
>  	sql_stmt *stmt = NULL;
> -	int rc = sql_prepare(sql_select_prepare, -1, &stmt, 0);
> +	int rc = sql_stmt_compile(sql_select_prepare, -1, NULL, &stmt, 0);
>  	if (rc)
>  		goto finalize;
>  	uint32_t current_idx_count = 0;
> @@ -1427,7 +1427,7 @@ load_stat_from_space(const char *sql_select_prepare,
>  	rc = sql_finalize(stmt);
>  	if (rc)
>  		goto finalize;
> -	rc = sql_prepare(sql_select_load, -1, &stmt, 0);
> +	rc = sql_stmt_compile(sql_select_load, -1, NULL, &stmt, 0);
>  	if (rc)
>  		goto finalize;
>  	struct index *prev_index = NULL;
> @@ -1509,7 +1509,7 @@ load_stat_to_index(const char *sql_select_load, struct index_stat **stats)
>  {
>  	assert(stats != NULL && *stats != NULL);
>  	struct sql_stmt *stmt = NULL;
> -	if (sql_prepare(sql_select_load, -1, &stmt, 0) != 0)
> +	if (sql_stmt_compile(sql_select_load, -1, NULL, &stmt, 0) != 0)
>  		return -1;
>  	uint32_t current_idx_count = 0;
>  	while (sql_step(stmt) == SQL_ROW) {
> diff --git a/src/box/sql/legacy.c b/src/box/sql/legacy.c
> index e3a2c77ca..458afd7f8 100644
> --- a/src/box/sql/legacy.c
> +++ b/src/box/sql/legacy.c
> @@ -71,7 +71,7 @@ sql_exec(sql * db,	/* The database on which the SQL executes */
>  		char **azVals = 0;
>  
>  		pStmt = 0;
> -		rc = sql_prepare(zSql, -1, &pStmt, &zLeftover);
> +		rc = sql_stmt_compile(zSql, -1, NULL, &pStmt, &zLeftover);
>  		assert(rc == 0 || pStmt == NULL);
>  		if (rc != 0)
>  			continue;
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 520b52d64..73d6866a4 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -39,18 +39,11 @@
>  #include "box/space.h"
>  #include "box/session.h"
>  
> -/*
> - * Compile the UTF-8 encoded SQL statement zSql into a statement handle.
> - */
> -static int
> -sqlPrepare(sql * db,	/* Database handle. */
> -	       const char *zSql,	/* UTF-8 encoded SQL statement. */
> -	       int nBytes,	/* Length of zSql in bytes. */
> -	       Vdbe * pReprepare,	/* VM being reprepared */
> -	       sql_stmt ** ppStmt,	/* OUT: A pointer to the prepared statement */
> -	       const char **pzTail	/* OUT: End of parsed string */
> -    )
> +int
> +sql_stmt_compile(const char *zSql, int nBytes, struct Vdbe *pReprepare,
> +		 sql_stmt **ppStmt, const char **pzTail)
>  {
> +	struct sql *db = sql_get();
>  	int rc = 0;	/* Result code */
>  	Parse sParse;		/* Parsing context */
>  	sql_parser_create(&sParse, db, current_session()->sql_flags);
> @@ -185,12 +178,10 @@ sqlReprepare(Vdbe * p)
>  {
>  	sql_stmt *pNew;
>  	const char *zSql;
> -	sql *db;
>  
>  	zSql = sql_sql((sql_stmt *) p);
>  	assert(zSql != 0);	/* Reprepare only called for prepare_v2() statements */

Comment is irrelevant, prepare_v2 is viped out?

> -	db = sqlVdbeDb(p);
> -	if (sqlPrepare(db, zSql, -1, p, &pNew, 0) != 0) {
> +	if (sql_stmt_compile(zSql, -1, p, &pNew, 0) != 0) {
>  		assert(pNew == 0);
>  		return -1;
>  	}
> @@ -206,7 +197,7 @@ int
>  sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
>  	    const char **sql_tail)
>  {
> -	int rc = sqlPrepare(sql_get(), sql, length, 0, stmt, sql_tail);
> +	int rc = sql_stmt_compile(sql, length, 0, stmt, sql_tail);
>  	assert(rc == 0 || stmt == NULL || *stmt == NULL);
>  	return rc;
>  }
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 3ca10778e..03deb733c 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -468,6 +468,20 @@ typedef void (*sql_destructor_type) (void *);
>  #define SQL_STATIC      ((sql_destructor_type)0)
>  #define SQL_TRANSIENT   ((sql_destructor_type)-1)
>  
> +/**
> + * Compile the UTF-8 encoded SQL statement into
> + * a statement handle (struct Vdbe).
> + *
> + * @param sql UTF-8 encoded SQL statement.
> + * @param sql_len Length of @sql in bytes.
> + * @param re_prepared VM being re-compiled. Can be NULL.
> + * @param[out] stmt A pointer to the compiled statement.
> + * @param[out] sql_tail End of parsed string.
> + */
> +int
> +sql_stmt_compile(const char *sql, int bytes_count, struct Vdbe *re_prepared,
> +		 sql_stmt **stmt, const char **sql_tail);
> +
>  int
>  sql_step(sql_stmt *);
>  
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index db7936e78..ab8441bc5 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -71,7 +71,7 @@ invokeProfileCallback(sql * db, Vdbe * p)
>  
>  /*
>   * The following routine destroys a virtual machine that is created by
> - * the sql_compile() routine. The integer returned is an SQL_
> + * the sql_stmt_compile() routine. The integer returned is an SQL_
>   * success/failure code that describes the result of executing the virtual
>   * machine.
>   */
> -- 
> 2.15.1
>