[tarantool-patches] Re: [PATCH 3/4] sql: simplify lua SQL executor

["n.pettik" <korablev@tarantool.org>]

Ack.

> On 3 Apr 2018, at 18:34, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> 1) Code is completely unreadable and complicated by strange
>   optimizations;
> 2) It still supposes, that there can be multiple statements in a
>   single string;
> 3) It pushes first letter of affinity together with column names
>   - why? It is not used anywhere.
> 
> Lets remove prepared statements list, affinity first letters
> pushing.
> ---
> src/box/lua/sql.c | 239 ++++++++----------------------------------------------
> 1 file changed, 35 insertions(+), 204 deletions(-)
> 
> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
> index 12b81383d..e693aeaad 100644
> --- a/src/box/lua/sql.c
> +++ b/src/box/lua/sql.c
> @@ -6,132 +6,12 @@
> #include "lua/utils.h"
> #include "info.h"
> 
> -struct prep_stmt
> -{
> -	sqlite3_stmt *stmt;
> -};
> -
> -struct prep_stmt_list
> -{
> -	uint8_t         *mem_end;   /* denotes actual size of sql_ctx struct */
> -	uint32_t         pool_size; /* mem at the end used for aux allocations;
> -				       pool grows from mem_end
> -				       towards stmt[] array */
> -	uint32_t         last_select_stmt_index; /* UINT32_MAX if no selects */
> -	uint32_t         column_count; /* in last select stmt */
> -	uint32_t         stmt_count;
> -	struct prep_stmt stmt[6];  /* overlayed with the mem pool;
> -				      actual size could be larger or smaller */
> -	/* uint8_t mem_pool[] */
> -};
> -
> -static inline int
> -prep_stmt_list_needs_free(struct prep_stmt_list *l)
> -{
> -	return (uint8_t *)(l + 1) != l->mem_end;
> -}
> -
> -/* Release resources and free the list itself, unless it was preallocated
> - * (i.e. l points to an automatic variable) */
> -static void
> -prep_stmt_list_free(struct prep_stmt_list *l)
> -{
> -	if (l == NULL)
> -		return;
> -	for (size_t i = 0, n = l->stmt_count; i < n; i++)
> -		sqlite3_finalize(l->stmt[i].stmt);
> -	if (prep_stmt_list_needs_free(l))
> -		free(l);
> -}
> -
> -static struct prep_stmt_list *
> -prep_stmt_list_init(struct prep_stmt_list *prealloc)
> -{
> -	prealloc->mem_end = (uint8_t *)(prealloc + 1);
> -	prealloc->pool_size = 0;
> -	prealloc->last_select_stmt_index = UINT32_MAX;
> -	prealloc->column_count = 0;
> -	prealloc->stmt_count = 0;
> -	return prealloc;
> -}
> -
> -/* Allocate mem from the prep_stmt_list pool.
> - * If not enough space is available, reallocates the list.
> - * If reallocation is needed but l was preallocated, old mem is left
> - * intact and a new memory chunk is allocated. */
> -static void *
> -prep_stmt_list_palloc(struct prep_stmt_list **pl,
> -		      size_t size, size_t alignment)
> -{
> -	assert((alignment & (alignment - 1)) == 0); /* 2 ^ k */
> -	assert(alignment <= __alignof__((*pl)->stmt[0]));
> -
> -	struct prep_stmt_list *l = *pl;
> -	uint32_t pool_size = l->pool_size;
> -	uint32_t pool_size_max = (uint32_t)(
> -		l->mem_end - (uint8_t *)(l->stmt + l->stmt_count)
> -	);
> -
> -	assert(UINT32_MAX - pool_size >= size);
> -	pool_size += size;
> -
> -	assert(UINT32_MAX - pool_size >= alignment - 1);
> -	pool_size += alignment - 1;
> -	pool_size &= ~(alignment - 1);
> -
> -	if (pool_size > pool_size_max) {
> -		size_t prev_size = l->mem_end - (uint8_t *)l;
> -		size_t size = prev_size;
> -		while (size < prev_size + (pool_size - pool_size_max)) {
> -			assert(SIZE_MAX - size >= size);
> -			size += size;
> -		}
> -		if (prep_stmt_list_needs_free(l)) {
> -			l = realloc(l, size);
> -			if (l == NULL)
> -				return NULL;
> -		} else {
> -			l = malloc(size);
> -			if (l == NULL)
> -				return NULL;
> -			memcpy(l, *pl, prev_size);
> -		}
> -		l->mem_end = (uint8_t *)l + size;
> -		/* move the pool data */
> -		memmove((uint8_t *)l + prev_size - l->pool_size,
> -			l->mem_end - l->pool_size,
> -			l->pool_size);
> -		*pl = l;
> -	}
> -
> -	l->pool_size = pool_size;
> -	return l->mem_end - pool_size;
> -}
> -
> -/* push new stmt; reallocate memory if needed
> - * returns a pointer to the new stmt or NULL if out of memory.
> - * If reallocation is needed but l was preallocated, old mem is left
> - * intact and a new memory chunk is allocated. */
> -static struct prep_stmt *
> -prep_stmt_list_push(struct prep_stmt_list **pl)
> -{
> -	struct prep_stmt_list *l;
> -	/* make sure we don't collide with the pool */
> -	if (prep_stmt_list_palloc(pl, sizeof(l->stmt[0]), 1
> -				  ) == NULL)
> -		return NULL;
> -	l = *pl;
> -	l->pool_size -= sizeof(l->stmt[0]);
> -	return l->stmt + (l->stmt_count++);
> -}
> -
> static void
> -lua_push_column_names(struct lua_State *L, struct prep_stmt_list *l)
> +lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
> {
> -	sqlite3_stmt *stmt = l->stmt[l->last_select_stmt_index].stmt;
> -	int n = l->column_count;
> -	lua_createtable(L, n, 0);
> -	for (int i = 0; i < n; i++) {
> +	int column_count = sqlite3_column_count(stmt);
> +	lua_createtable(L, column_count, 0);
> +	for (int i = 0; i < column_count; i++) {
> 		const char *name = sqlite3_column_name(stmt, i);
> 		lua_pushstring(L, name == NULL ? "" : name);
> 		lua_rawseti(L, -2, i+1);
> @@ -139,11 +19,9 @@ lua_push_column_names(struct lua_State *L, struct prep_stmt_list *l)
> }
> 
> static void
> -lua_push_row(struct lua_State *L, struct prep_stmt_list *l)
> +lua_push_row(struct lua_State *L, struct sqlite3_stmt *stmt)
> {
> -	sqlite3_stmt *stmt = l->stmt[l->last_select_stmt_index].stmt;
> -	int column_count = l->column_count;
> -	char *typestr = (void *)(l->mem_end - column_count);
> +	int column_count = sqlite3_column_count(stmt);
> 
> 	lua_createtable(L, column_count, 0);
> 	lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_array_metatable_ref);
> @@ -153,124 +31,77 @@ lua_push_row(struct lua_State *L, struct prep_stmt_list *l)
> 		int type = sqlite3_column_type(stmt, i);
> 		switch (type) {
> 		case SQLITE_INTEGER:
> -			typestr[i] = 'i';
> -            luaL_pushint64(L, sqlite3_column_int64(stmt, i));
> +			luaL_pushint64(L, sqlite3_column_int64(stmt, i));
> 			break;
> 		case SQLITE_FLOAT:
> -			typestr[i] = 'f';
> 			lua_pushnumber(L, sqlite3_column_double(stmt, i));
> 			break;
> 		case SQLITE_TEXT: {
> 			const void *text = sqlite3_column_text(stmt, i);
> -			typestr[i] = 's';
> 			lua_pushlstring(L, text,
> 					sqlite3_column_bytes(stmt, i));
> 			break;
> 		}
> 		case SQLITE_BLOB: {
> 			const void *blob = sqlite3_column_blob(stmt, i);
> -			typestr[i] = 'b';
> 			lua_pushlstring(L, blob,
> 					sqlite3_column_bytes(stmt, i));
> 			break;
> 		}
> 		case SQLITE_NULL:
> -			typestr[i] = '-';
> 			lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_nil_ref);
> 			break;
> 		default:
> -			typestr[i] = '?';
> 			assert(0);
> 		}
> 		lua_rawseti(L, -2, i+1);
> 	}
> -
> -	lua_pushlstring(L, typestr, column_count);
> -	lua_rawseti(L, -2, 0);
> }
> 
> static int
> lua_sql_execute(struct lua_State *L)
> {
> -	int rc;
> 	sqlite3 *db = sql_get();
> -	struct prep_stmt_list *l = NULL, stock_l;
> -	size_t length;
> -	const char *sql, *sql_end;
> -
> 	if (db == NULL)
> 		return luaL_error(L, "not ready");
> 
> -	sql = lua_tolstring(L, 1, &length);
> +	size_t length;
> +	const char *sql = lua_tolstring(L, 1, &length);
> 	if (sql == NULL)
> 		return luaL_error(L, "usage: box.sql.execute(sqlstring)");
> 
> -	assert(length <= INT_MAX);
> -	sql_end = sql + length;
> +	struct sqlite3_stmt *stmt;
> +	if (sqlite3_prepare_v2(db, sql, length, &stmt, &sql) != SQLITE_OK)
> +		goto sqlerror;
> +	assert(stmt != NULL);
> 
> -	l = prep_stmt_list_init(&stock_l);
> -	do {
> -
> -		struct prep_stmt *ps = prep_stmt_list_push(&l);
> -		if (ps == NULL)
> -			goto outofmem;
> -		rc = sqlite3_prepare_v2(db, sql, (int)(sql_end - sql),
> -					&ps->stmt, &sql);
> -		if (rc != SQLITE_OK)
> -			goto sqlerror;
> -
> -		if (ps->stmt == NULL) {
> -			/* only whitespace */
> -			assert(sql == sql_end);
> -			l->stmt_count --;
> -			break;
> -		}
> -
> -		int column_count = sqlite3_column_count(ps->stmt);
> -		if (column_count == 0) {
> -			while ((rc = sqlite3_step(ps->stmt)) == SQLITE_ROW) { ; }
> -		} else {
> -			char *typestr;
> -			l->column_count = column_count;
> -			l->last_select_stmt_index = l->stmt_count - 1;
> -
> -			assert(l->pool_size == 0);
> -			/* This might possibly call realloc() and ruin *ps.  */
> -			typestr = prep_stmt_list_palloc(&l, column_count, 1);
> -			if (typestr == NULL)
> -				goto outofmem;
> -			/* Refill *ps.  */
> -			ps = l->stmt + l->stmt_count - 1;
> -
> -			lua_settop(L, 1); /* discard any results */
> -
> -			/* create result table */
> -			lua_createtable(L, 7, 0);
> -			lua_pushvalue(L, lua_upvalueindex(1));
> -			lua_setmetatable(L, -2);
> -			lua_push_column_names(L, l);
> -			lua_rawseti(L, -2, 0);
> -
> -			int row_count = 0;
> -			while ((rc = sqlite3_step(ps->stmt)) == SQLITE_ROW) {
> -				lua_push_row(L, l);
> -				row_count++;
> -				lua_rawseti(L, -2, row_count);
> -			}
> -			l->pool_size = 0;
> +	int rc;
> +	int retval_count;
> +	if (sqlite3_column_count(stmt) == 0) {
> +		while ((rc = sqlite3_step(stmt)) == SQLITE_ROW);
> +		retval_count = 0;
> +	} else {
> +		lua_newtable(L);
> +		lua_pushvalue(L, lua_upvalueindex(1));
> +		lua_setmetatable(L, -2);
> +		lua_push_column_names(L, stmt);
> +		lua_rawseti(L, -2, 0);
> +
> +		int row_count = 0;
> +		while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
> +			lua_push_row(L, stmt);
> +			lua_rawseti(L, -2, ++row_count);
> 		}
> +		retval_count = 1;
> +	}
>         if (rc != SQLITE_OK && rc != SQLITE_DONE)
> -            goto sqlerror;
> -	} while (sql != sql_end);
> -	prep_stmt_list_free(l);
> -	return lua_gettop(L) - 1;
> +		goto sqlerror;
> +	sqlite3_finalize(stmt);
> +	return retval_count;
> sqlerror:
> 	lua_pushstring(L, sqlite3_errmsg(db));
> -	prep_stmt_list_free(l);	
> +	sqlite3_finalize(stmt);
> 	return lua_error(L);
> -outofmem:
> -	prep_stmt_list_free(l);
> -	return luaL_error(L, "out of memory");
> }
> 
> static int
> -- 
> 2.14.3 (Apple Git-98)
> 
>