[tarantool-patches] Re: [PATCH 3/3] sql: move default col values to Tarantool's core
v.shpilevoy at tarantool.org
v.shpilevoy at tarantool.org
Thu Mar 29 17:00:29 MSK 2018
See below 20 comments.
> 29 марта 2018 г., в 9:42, Kirill Yukhin <kyukhin at tarantool.org> написал(а):
>
> Extract expressions parsing into separate routine to
> allow Tarantool's backend compile DEFAULT statements w/o
> SQL machinery at all. So far, for DEFAULT values no context
> is needed: only constant expressions and built-ins are allowed.
> In future, table context will be added to allow use column
> values for CHECK constraint expressions.
>
> Move DEFAULT string value to space_def. Move compiled expresion
> to field_def as well. Reason not to move compiled expression
> to tuple_field as follows: we do not want to engage parser
> during tuple validation. So, do it in alter.cc.
>
> In order to allow expression duplication in alter.cc: expose
> those routines from expr.c and make their names correspond to
> coding style.
>
> Since recovery is performed before SQL fronend initialization:
> split it into two pieces: 1. create SQL handler, enable
> all subsystems 2. Do recovery. This will allow to run
> parser during recovery, since it needs db handle so far.
>
> Part of #3235
> ---
> src/CMakeLists.txt | 2 +-
> src/box/alter.cc | 17 +
> src/box/box.cc | 3 +-
> src/box/field_def.c | 5 +-
> src/box/field_def.h | 4 +
> src/box/space_def.c | 105 ++++--
> src/box/space_def.h | 10 +-
> src/box/sql.c | 59 +++-
> src/box/sql.h | 30 ++
> src/box/sql/build.c | 18 +-
> src/box/sql/delete.c | 12 +-
> src/box/sql/expr.c | 114 ++++---
> src/box/sql/fkey.c | 13 +-
> src/box/sql/insert.c | 32 +-
> src/box/sql/main.c | 15 -
> src/box/sql/parse.c | 876 ++++++++++++++++++++++++------------------------
> src/box/sql/parse.y | 32 +-
> src/box/sql/resolve.c | 12 +-
> src/box/sql/select.c | 28 +-
> src/box/sql/sqliteInt.h | 11 +-
> src/box/sql/tokenize.c | 121 +++++++
> src/box/sql/trigger.c | 12 +-
> src/box/sql/update.c | 2 +-
> src/box/sql/wherecode.c | 2 +-
> src/box/sql/whereexpr.c | 6 +-
> 25 files changed, 928 insertions(+), 613 deletions(-)
>
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 8ab09e9..eae40c3 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -268,7 +268,7 @@ add_executable(
> ${LIBUTIL_FREEBSD_SRC}/flopen.c
> ${LIBUTIL_FREEBSD_SRC}/pidfile.c)
>
> -add_dependencies(tarantool build_bundled_libs preprocess_exports)
> +add_dependencies(tarantool build_bundled_libs preprocess_exports sql)
>
> # Re-link if exports changed
> set_target_properties(tarantool PROPERTIES LINK_DEPENDS ${exports_file})
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 299dcb0..b976d0f 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -52,6 +52,7 @@
> #include "memtx_tuple.h"
> #include "version.h"
> #include "sequence.h"
> +#include "sql.h"
>
> /**
> * chap-sha1 of empty string, i.e.
> @@ -403,6 +404,22 @@ field_def_decode(struct field_def *field, const char **data,
> tt_sprintf("collation is reasonable only for "
> "string, scalar and any fields"));
> }
> +
> + if (field->default_value != NULL) {
> + struct Expr *expr = NULL;
> + char *err = 0;
1. err = NUlL.
> + /* Recovery is performed before SQL FE is initialized.
> + * Postpone AST compilation if in process of recovery.
> + */
> + if (sql_get() != NULL) {
1. In the commit message you said:
"Since recovery is performed before SQL fronend initialization:
split it into two pieces: 1. create SQL handler, enable
all subsystems 2. Do recovery. This will allow to run
parser during recovery, since it needs db handle so far."
Why sql_get() can be NULL here?
And why do you ignore error?
In a case of error in one of next field definitions, or because of
OOM, or error further in space_def_new_from_tuple, the default_value_ast
leaks. Other field_def fields are not deleted explicitly here, because they
are on region memory, that is deleted on a transaction rollback.
> + sql_compile_expr(sql_get(),
> + field->default_value,
> + &expr,
> + &err);
> + assert(err == NULL);
> + field->default_value_ast = expr;
2. How about another name? For example, default_value_expr? I know,
formally it is AST, but actually it is expr. Or lets rename struct Expr to
struct expr_ast, or struct sql_ast or something.
> + }
> + }
> }
>
> /**
>
> diff --git a/src/box/space_def.c b/src/box/space_def.c
> index 1b3c305..7b9a664 100644
> --- a/src/box/space_def.c
> +++ b/src/box/space_def.c
> @@ -32,6 +32,7 @@
> #include "space_def.h"
> #include "diag.h"
> #include "error.h"
> +#include "sql.h"
>
> const struct space_opts space_opts_default = {
> /* .temporary = */ false,
> @@ -49,7 +50,7 @@ const struct opt_def space_opts_reg[] = {
> /**
> * Size of the space_def.
> * @param name_len Length of the space name.
> - * @param field_names_size Size of all names.
> + * @param fields Fields array of space format.
> * @param field_count Space field count.
> * @param[out] names_offset Offset from the beginning of a def to
> * a field names memory.
3. Add a comment about second out parameter, please.
> @@ -58,25 +59,37 @@ const struct opt_def space_opts_reg[] = {
> * @retval Size in bytes.
> */
> static inline size_t
> -space_def_sizeof(uint32_t name_len, uint32_t field_names_size,
> +space_def_sizeof(uint32_t name_len, const struct field_def *fields,
> uint32_t field_count, uint32_t *names_offset,
> - uint32_t *fields_offset)
> + uint32_t *fields_offset, uint32_t *def_ast_offset)
> {
> + uint32_t field_strs_size = 0;
> + uint32_t def_exprs_size = 0;
> + for (uint32_t i = 0; i < field_count; ++i) {
> + field_strs_size += strlen(fields[i].name) + 1;
> + if (fields[i].default_value != NULL) {
> + int len = strlen(fields[i].default_value);
> + field_strs_size += len + 1;
> + }
> + if (fields[i].default_value_ast != NULL) {
4. If I correctly understand, (fields[i].default_value != NULL) ==
(fields[i].default_value_ast != NULL).
So if a default_value != NULL, then it is guaranteed the
default_value_ast != NULL too. Please, correct me, or move this code
into the body of the previous 'if' and add an assertion.
> + struct Expr *expr = fields[i].default_value_ast;
> + def_exprs_size += sql_duped_expr_size(expr, 0);
> + }
> + }
> +
> *fields_offset = sizeof(struct space_def) + name_len + 1;
> *names_offset = *fields_offset + field_count * sizeof(struct field_def);
> - return *names_offset + field_names_size;
> + *def_ast_offset = *names_offset + field_strs_size;
> + return *def_ast_offset + def_exprs_size;
> }
>
> struct space_def *
> space_def_dup(const struct space_def *src)
> {
> - uint32_t names_offset, fields_offset;
> - uint32_t field_names_size = 0;
> - for (uint32_t i = 0; i < src->field_count; ++i)
> - field_names_size += strlen(src->fields[i].name) + 1;
> - size_t size = space_def_sizeof(strlen(src->name), field_names_size,
> - src->field_count, &names_offset,
> - &fields_offset);
> + uint32_t strs_offset, fields_offset, def_ast_offset;
> + size_t size = space_def_sizeof(strlen(src->name), src->fields,
> + src->field_count, &strs_offset,
> + &fields_offset, &def_ast_offset);
> struct space_def *ret = (struct space_def *) malloc(size);
> if (ret == NULL) {
> diag_set(OutOfMemory, size, "malloc", "ret");
> @@ -92,12 +105,25 @@ space_def_dup(const struct space_def *src)
> return NULL;
> }
> }
> - char *name_pos = (char *)ret + names_offset;
> + char *strs_pos = (char *)ret + strs_offset;
> + char *expr_pos = (char *)ret + def_ast_offset;
> if (src->field_count > 0) {
> ret->fields = (struct field_def *)((char *)ret + fields_offset);
> for (uint32_t i = 0; i < src->field_count; ++i) {
> - ret->fields[i].name = name_pos;
> - name_pos += strlen(name_pos) + 1;
> + ret->fields[i].name = strs_pos;
> + strs_pos += strlen(strs_pos) + 1;
> + if (src->fields[i].default_value != NULL) {
> + ret->fields[i].default_value = strs_pos;
> + strs_pos += strlen(strs_pos) + 1;
> + }
5. Same as 3.
> + struct Expr *def_expr_ast = src->fields[i].default_value_ast;
> + if (def_expr_ast != NULL) {
> + struct Expr *expr = NULL;
6. Useless initialization.
> + expr = sql_expr_dup(sql_get(), def_expr_ast,
> + 0, &expr_pos);
7. If I correctly understand the sql_expr_dup, after the expression is duplicated,
expr_pos points the byte right after the duplicated Expr. Can you please check it?
If it is true, then you must skip this line "expr_pos += sql_duped_expr_size(expr, 0);",
and I can not understand why it works now.
> + ret->fields[i].default_value_ast = expr;
> + expr_pos += sql_duped_expr_size(expr, 0);
> + }
> }
> }
> tuple_dictionary_ref(ret->dict);
> @@ -111,12 +137,10 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
> const struct space_opts *opts, const struct field_def *fields,
> uint32_t field_count)
> {
> - uint32_t field_names_size = 0;
> - for (uint32_t i = 0; i < field_count; ++i)
> - field_names_size += strlen(fields[i].name) + 1;
> - uint32_t names_offset, fields_offset;
> - size_t size = space_def_sizeof(name_len, field_names_size, field_count,
> - &names_offset, &fields_offset);
> + uint32_t strs_offset, fields_offset, def_ast_offset;
> + size_t size = space_def_sizeof(name_len, fields, field_count,
> + &strs_offset, &fields_offset,
> + &def_ast_offset);
> struct space_def *def = (struct space_def *) malloc(size);
> if (def == NULL) {
> diag_set(OutOfMemory, size, "malloc", "def");
> @@ -147,18 +171,35 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
> }
> }
> def->field_count = field_count;
> - if (field_count == 0) {
> + if (field_count == 0)
> def->fields = NULL;
> - } else {
> - char *name_pos = (char *)def + names_offset;
> + else {
> + char *strs_pos = (char *)def + strs_offset;
> + char *expr_pos = (char *)def + def_ast_offset;
> def->fields = (struct field_def *)((char *)def + fields_offset);
> for (uint32_t i = 0; i < field_count; ++i) {
> def->fields[i] = fields[i];
> - def->fields[i].name = name_pos;
> + def->fields[i].name = strs_pos;
> uint32_t len = strlen(fields[i].name);
> memcpy(def->fields[i].name, fields[i].name, len);
> def->fields[i].name[len] = 0;
> - name_pos += len + 1;
> + strs_pos += len + 1;
> +
> + if (fields[i].default_value != NULL) {
> + def->fields[i].default_value = strs_pos;
> + len = strlen(fields[i].default_value);
> + memcpy(def->fields[i].default_value, fields[i].default_value, len);
> + def->fields[i].default_value[len] = 0;
> + strs_pos += len + 1;
> + }
8. Same as 5, 6, 7.
> + struct Expr *def_expr_ast = fields[i].default_value_ast;
> + if (def_expr_ast != NULL) {
> + struct Expr *expr = NULL;
> + expr = sql_expr_dup(sql_get(), def_expr_ast,
> + 0, &expr_pos);
> + def->fields[i].default_value_ast = expr;
> + expr_pos += sql_duped_expr_size(expr, 0);
> + }
> }
> }
> return def;
> @@ -217,3 +258,17 @@ space_def_check_compatibility(const struct space_def *old_def,
> return 0;
> }
>
> +void
> +space_def_delete(struct space_def *def)
> +{
> + space_opts_destroy(&def->opts);
> + tuple_dictionary_unref(def->dict);
> + for (uint32_t i = 0; i < def->field_count; ++i) {
> + if (def->fields[i].default_value_ast != NULL) {
> + sql_expr_free(sql_get(),
> + def->fields[i].default_value_ast);
> + }
9. Why does it work, if your expressions are the part of megamallocs above?
> + }
> + TRASH(def);
> + free(def);
> +}
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 6d8ef9a..8606be6 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -56,7 +56,7 @@
> #include "xrow.h"
> #include "iproto_constants.h"
>
> -static sqlite3 *db;
> +static sqlite3 *db = 0;
10. Please, use NULL instead of 0. Common comment: assigning 0 in the
code, where the variable declaration is not strictly visible is very
confusing - it seems to be integer.
> @@ -1781,3 +1814,25 @@ tarantoolSqlGetMaxId(uint32_t space_id, uint32_t index_id, uint32_t fieldno,
>
> return tuple_field_u64(tuple, fieldno, max_id);
> }
> +
> +/**
> + * Given space_id and field number, return default value
> + * for the field.
> + * @param space_id Space ID.
> + * @param fieldno Field index.
> + * @retval Pointer to AST corresponding to default value.
> + * Can be NULL if no DEFAULT specified or it is a view.
> + */
11. Please, put a function description above a declaration instead of implementation.
We put description above implementation only if a declaration is absent.
> +struct Expr*
> +space_get_column_default_expr(uint32_t space_id, uint32_t fieldno)
> +{
> + struct space *space;
> + space = space_cache_find(space_id);
> + assert(space != NULL);
> + assert(space->def != NULL);
> + if (space->def->opts.is_view)
> + return NULL;
12. How is it possible? Must not it be an assertion?
> + assert(space->def->field_count > fieldno);
> +
> + return space->def->fields[fieldno].default_value_ast;
> +}
> diff --git a/src/box/sql.h b/src/box/sql.h
> @@ -55,6 +60,31 @@ sql_free();
> struct sqlite3 *
> sql_get();
>
> +struct Expr;
> +struct Parse;
> +struct Select;
> +
> +int
> +sql_compile_expr(struct sqlite3 *db,
> + const char *expr,
> + struct Expr **result,
> + char **err);
> +
> +void
> +sql_extract_select_expr(struct Parse *parser, struct Select *select);
> +
> +struct Expr*
> +space_get_column_default_expr(uint32_t space_id, uint32_t fieldno);
> +
> +int
> +sql_duped_expr_size(struct Expr *p, int flags);
13. How about simply expr_size, not duped? And please, name methods of a specific
struct using convention <struct_name>_<method_name>. And for struct size
methods use sizeof name (see space_def_sizeof, key_def_sizeof). For 'free' methods
we use name 'delete'. Delete means, that the struct's memory is freed. Also we
have 'destroy' methods, which frees only struct members, but not the struct itself.
Here it is expr_sizeof(), expr_delete(), expr_dup().
And please, do not use 'get' for getters. Here it is space_column_default_expr().
> +
> +struct Expr *
> +sql_expr_dup(struct sqlite3 *db, struct Expr *p, int flags, char **buffer);
> +
> +void
> +sql_expr_free(struct sqlite3 *db, struct Expr *expr);
> +
> #if defined(__cplusplus)
> } /* extern "C" { */
> #endif
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 05a7cc9..c3bd637 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1018,9 +1018,7 @@ sqlite3AddCheckConstraint(Parse * pParse, /* Parsing context */
> }
> } else
> #endif
> - {
> - sqlite3ExprDelete(pParse->db, pCheckExpr);
> - }
> + sql_expr_free(pParse->db, pCheckExpr);
14. Extra tab.
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 89dcb23..093f99e 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -492,7 +492,7 @@ sqlite3ExprForVectorField(Parse * pParse, /* Parsing context */
> * pLeft->iTable: First in an array of register holding result, or 0
> * if the result is not yet computed.
> *
> - * sqlite3ExprDelete() specifically skips the recursive delete of
> + * sql_expr_free() specifically skips the recursive delete of
> * pLeft on TK_SELECT_COLUMN nodes. But pRight is followed, so pVector
> * can be attached to pRight to cause this node to take ownership of
> * pVector. Typically there will be multiple TK_SELECT_COLUMN nodes
> @@ -935,8 +935,8 @@ sqlite3ExprAttachSubtrees(sqlite3 * db,
> {
> if (pRoot == 0) {
> assert(db->mallocFailed);
> - sqlite3ExprDelete(db, pLeft);
> - sqlite3ExprDelete(db, pRight);
> + sql_expr_free(db, pLeft);
> + sql_expr_free(db, pRight);
> } else {
> if (pRight) {
> pRoot->pRight = pRight;
> @@ -1052,8 +1052,8 @@ sqlite3ExprAnd(sqlite3 * db, Expr * pLeft, Expr * pRight)
> } else if (pRight == 0) {
> return pLeft;
> } else if (exprAlwaysFalse(pLeft) || exprAlwaysFalse(pRight)) {
> - sqlite3ExprDelete(db, pLeft);
> - sqlite3ExprDelete(db, pRight);
> + sql_expr_free(db, pLeft);
> + sql_expr_free(db, pRight);
> return sqlite3ExprAlloc(db, TK_INTEGER, &sqlite3IntTokens[0],
> 0);
> } else {
> @@ -1189,7 +1189,7 @@ sqlite3ExprDeleteNN(sqlite3 * db, Expr * p)
> assert(p->x.pList == 0 || p->pRight == 0);
> if (p->pLeft && p->op != TK_SELECT_COLUMN)
> sqlite3ExprDeleteNN(db, p->pLeft);
> - sqlite3ExprDelete(db, p->pRight);
> + sql_expr_free(db, p->pRight);
> if (ExprHasProperty(p, EP_xIsSelect)) {
> sqlite3SelectDelete(db, p->x.pSelect);
> } else {
> @@ -1203,11 +1203,16 @@ sqlite3ExprDeleteNN(sqlite3 * db, Expr * p)
> }
> }
>
> +/**
> + * Recursively free storage occupied by AST expr.
> + * @param db SQL handle.
> + * @param p Pointer to root node.
> + */
15. Same as 11.
> void
> -sqlite3ExprDelete(sqlite3 * db, Expr * p)
> +sql_expr_free(sqlite3 *db, Expr *expr)
> {
> - if (p)
> - sqlite3ExprDeleteNN(db, p);
> + if (expr != NULL)
> + sqlite3ExprDeleteNN(db, expr);
> }
>
> /*
>
> @@ -1401,29 +1412,28 @@ exprDup(sqlite3 * db, Expr * p, int dupFlags, u8 ** pzBuffer)
> if (ExprHasProperty(p, EP_xIsSelect)) {
> pNew->x.pSelect =
> sqlite3SelectDup(db, p->x.pSelect,
> - dupFlags);
> + flags);
> } else {
> pNew->x.pList =
> sqlite3ExprListDup(db, p->x.pList,
> - dupFlags);
> + flags);
> }
> }
>
> /* Fill in pNew->pLeft and pNew->pRight. */
> if (ExprHasProperty(pNew, EP_Reduced | EP_TokenOnly)) {
> - zAlloc += dupedExprNodeSize(p, dupFlags);
> + zAlloc += dupedExprNodeSize(p, flags);
> if (!ExprHasProperty(pNew, EP_TokenOnly | EP_Leaf)) {
> pNew->pLeft = p->pLeft ?
> - exprDup(db, p->pLeft, EXPRDUP_REDUCE,
> + sql_expr_dup(db, p->pLeft, EXPRDUP_REDUCE,
> &zAlloc) : 0;
16. Bad alignment.
>
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 42254dd..00e263a 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -674,10 +678,15 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> if (i == pTab->iAutoIncPKey)
> sqlite3VdbeAddOp2(v, OP_Integer, -1,
> regCols + i + 1);
> - else
> + else {
17. If 'else' body is inside {}, then its 'if' body must be too, even if
consists of single line.
>
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index e9c0b31..6d4ae7c 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -63,10 +63,12 @@
> * asterisks and the comment text.
> */
>
> -#include <box/field_def.h>
> #include <stdbool.h>
> -#include <trivia/util.h>
> +
> +#include "box/field_def.h"
> +#include "box/sql.h"
> #include "box/txn.h"
> +#include "trivia/util.h"
>
> /*
> * These #defines should enable >2GB file support on POSIX if the
> @@ -3007,6 +3009,10 @@ struct Parse {
> With *pWithToFree; /* Free this WITH object at the end of the parse */
>
> bool initiateTTrans; /* Initiate Tarantool transaction */
> + /* If set - do not emit byte code at all, just parse. */
> + bool parse_only;
> + /* If parse_only is set to true, store parsed expression. */
> + struct Expr *parsed_expr;
18. For struct members please use /** comments instead of /*.
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index f810db4..ae52731 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -652,3 +652,124 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
> assert(nErr == 0 || pParse->rc != SQLITE_OK);
> return nErr;
> }
> +
> +/**
> + * Perform parsing of provided expression. This is done by
> + * surrounding the expression w/ 'SELECT ' prefix and perform
> + * convetional parsing. Then extract result expression value from
> + * stuct Select and return it.
> + * @param db SQL context handle.
> + * @param expr Expression to parse.
> + * @param[out] result Result: AST structure.
> + * @param[out] err Error string if any.
> + *
> + * @retval Error code if any.
> + */
> +int
> +sql_compile_expr(sqlite3 *db,
> + const char *expr,
> + struct Expr **result,
> + char **err)
> +{
> + struct Parse parser;
> + int nErr = 0; /* Number of errors encountered */
> + int i; /* Loop counter */
> + void *engine; /* The LEMON-generated LALR(1) parser */
> + int token_type; /* type of the next token */
> + int last_token_type = -1; /* type of the previous token */
> + int mxSqlLen; /* Max length of an SQL string */
> +
> + assert(expr != 0);
> + mxSqlLen = db->aLimit[SQLITE_LIMIT_SQL_LENGTH];
> + memset(&parser, 0, sizeof(struct Parse));
> + parser.rc = SQLITE_OK;
> + parser.zTail = expr;
> + parser.db = db;
> + parser.pToplevel = NULL;
> + parser.parse_only = true;
> + i = 0;
> + assert(err != 0);
> + engine = sqlite3ParserAlloc(sqlite3Malloc);
> + if (engine == NULL) {
> + sqlite3OomFault(db);
> + return SQLITE_NOMEM_BKPT;
> + }
> +
> + const char *outer = "SELECT ";
> + char *stmt = malloc(strlen(outer) + strlen(expr) + 1);
19. Please, use region. You do this in a DDL transaction only, so
region usage allows you to do not free this manually.
> + if (stmt == NULL) {
> + sqlite3OomFault(db);
> + return SQLITE_NOMEM_BKPT;
> + }
> + sprintf(stmt, "%s%s", outer, expr);
> + while (1) {
20. While (true). For boolean variables and constants please use
boolean values.
More information about the Tarantool-patches
mailing list