[tarantool-patches] [PATCH v1 0/2] sql: Create special region for parser

[tarantool-patches] [PATCH v1 0/2] sql: Create special region for parser

[Kirill Shcherbatov]

Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3438-parser-own-region
Issue: https://github.com/tarantool/tarantool/issues/3438

Kirill Shcherbatov (2):
  sql: remove parser construct, destruct to sql.h
  sql: use own region in Parser

 src/box/sql.h           | 17 +++++++++++++++++
 src/box/sql/build.c     |  6 +++---
 src/box/sql/prepare.c   | 11 +++++++++--
 src/box/sql/select.c    |  2 +-
 src/box/sql/sqliteInt.h | 29 ++---------------------------
 src/box/sql/tokenize.c  | 10 ++++++----
 6 files changed, 38 insertions(+), 37 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v1 1/2] sql: remove parser construct, destruct to sql.h

[Kirill Shcherbatov]

Moved sql_parser_create and sql_parser_destroy to sql.h to
use them not only in DDL.
Removed SELECTTRACE_ENABLED macro with conditional fields from
struct Parse to prevent different object sizes across the project.

Part of #3272.
---
 src/box/sql.h           | 17 +++++++++++++++++
 src/box/sql/prepare.c   |  9 +++++++++
 src/box/sql/sqliteInt.h | 25 -------------------------
 3 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/src/box/sql.h b/src/box/sql.h
index 3c26492..ecaf431 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -175,6 +175,23 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name);
 int
 sql_table_def_rebuild(struct sqlite3 *db, struct Table *table);
 
+/**
+ * Initialize a new parser object.
+ * A number of service allocations are performed on the region,
+ * which is also cleared in the destroy function.
+ * @param parser object to initialize.
+ * @param db SQLite object.
+ */
+void
+sql_parser_create(struct Parse *parser, struct sqlite3 *db);
+
+/**
+ * Release the parser object resources.
+ * @param parser object to release.
+ */
+void
+sql_parser_destroy(struct Parse *parser);
+
 #if defined(__cplusplus)
 } /* extern "C" { */
 #endif
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 8dc433f..8d94642 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -433,6 +433,15 @@ sqlite3_prepare_v2(sqlite3 * db,	/* Database handle. */
 }
 
 void
+sql_parser_create(struct Parse *parser, sqlite3 *db)
+{
+	memset(parser, 0, sizeof(struct Parse));
+	parser->db = db;
+	struct region *region = &fiber()->gc;
+	parser->region_initial_size = region_used(region);
+}
+
+void
 sql_parser_destroy(Parse *parser)
 {
 	assert(parser != NULL);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 950409d..abf1543 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2903,10 +2903,8 @@ struct Parse {
 	Token constraintName;	/* Name of the constraint currently being parsed */
 	int regRoot;		/* Register holding root page number for new objects */
 	int nMaxArg;		/* Max args passed to user function by sub-program */
-#ifdef SELECTTRACE_ENABLED
 	int nSelect;		/* Number of SELECT statements seen */
 	int nSelectIndent;	/* How far to indent SELECTTRACE() output */
-#endif
 	Parse *pToplevel;	/* Parse structure for main program (or NULL) */
 	Table *pTriggerTab;	/* Table triggers are being coded for */
 	u32 nQueryLoop;		/* Est number of iterations of a query (10*log2(N)) */
@@ -4537,27 +4535,4 @@ extern int sqlite3InitDatabase(sqlite3 * db);
 enum on_conflict_action
 table_column_nullable_action(struct Table *tab, uint32_t column);
 
-/**
- * Initialize a new parser object.
- * A number of service allocations are performed on the region, which is also
- * cleared in the destroy function.
- * @param parser object to initialize.
- * @param db SQLite object.
- */
-static inline void
-sql_parser_create(struct Parse *parser, sqlite3 *db)
-{
-	memset(parser, 0, sizeof(struct Parse));
-	parser->db = db;
-	struct region *region = &fiber()->gc;
-	parser->region_initial_size = region_used(region);
-}
-
-/**
- * Release the parser object resources.
- * @param parser object to release.
- */
-void
-sql_parser_destroy(struct Parse *parser);
-
 #endif				/* SQLITEINT_H */
-- 
2.7.4

[tarantool-patches] [PATCH v1 2/2] sql: use own region in Parser

[Kirill Shcherbatov]

Fixes #3438.
---
 src/box/sql/build.c     |  6 +++---
 src/box/sql/prepare.c   |  6 ++----
 src/box/sql/select.c    |  2 +-
 src/box/sql/sqliteInt.h |  4 ++--
 src/box/sql/tokenize.c  | 10 ++++++----
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index e376295..4e77084 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -636,7 +636,7 @@ sql_field_retrieve(Parse *parser, Table *table, uint32_t id)
 	if (id >= table->def->exact_field_count) {
 		uint32_t columns_new = table->def->exact_field_count;
 		columns_new = (columns_new > 0) ? 2 * columns_new : 1;
-		struct region *region = &fiber()->gc;
+		struct region *region = &parser->region;
 		field = region_alloc(region, columns_new *
 				     sizeof(table->def->fields[0]));
 		if (field == NULL) {
@@ -698,7 +698,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
 	if (sql_field_retrieve(pParse, p,
 			       (uint32_t) p->def->field_count) == NULL)
 		return;
-	struct region *region = &fiber()->gc;
+	struct region *region = &pParse->region;
 	z = region_alloc(region, pName->n + 1);
 	if (z == NULL) {
 		diag_set(OutOfMemory, pName->n + 1,
@@ -906,7 +906,7 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
 			assert(p->def != NULL);
 			struct field_def *field =
 				&p->def->fields[p->def->field_count - 1];
-			struct region *region = &fiber()->gc;
+			struct region *region = &pParse->region;
 			uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
 			field->default_value = region_alloc(region,
 							    default_length + 1);
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 8d94642..e796077 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -437,8 +437,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
 {
 	memset(parser, 0, sizeof(struct Parse));
 	parser->db = db;
-	struct region *region = &fiber()->gc;
-	parser->region_initial_size = region_used(region);
+	region_create(&parser->region, &cord()->slabc);
 }
 
 void
@@ -454,6 +453,5 @@ sql_parser_destroy(Parse *parser)
 		db->lookaside.bDisable -= parser->disableLookaside;
 	}
 	parser->disableLookaside = 0;
-	struct region *region = &fiber()->gc;
-	region_truncate(region, parser->region_initial_size);
+	region_destroy(&parser->region);
 }
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 5fbcbaf..b26565b 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1762,7 +1762,7 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table)
 	 * names for existing table.
 	 */
 	assert(table->def->fields == NULL);
-	struct region *region = &fiber()->gc;
+	struct region *region = &parse->region;
 	table->def->fields =
 		region_alloc(region, nCol * sizeof(table->def->fields[0]));
 	if (table->def->fields == NULL) {
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index abf1543..4275f72 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2913,8 +2913,8 @@ struct Parse {
 	u8 eTriggerOp;		/* TK_UPDATE, TK_INSERT or TK_DELETE */
 	u8 eOrconf;		/* Default ON CONFLICT policy for trigger steps */
 	u8 disableTriggers;	/* True to disable triggers */
-	/** Region size at the Parser launch. */
-	size_t region_initial_size;
+	/* Region to make SQL temp allocations. */
+	struct region region;
 
   /**************************************************************************
   * Fields above must be initialized to zero.  The fields that follow,
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index fc4fe25..1fd8b29 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -544,16 +544,18 @@ sql_expr_compile(sqlite3 *db, const char *expr, struct Expr **result)
 {
 	const char *outer = "SELECT ";
 	int len = strlen(outer) + strlen(expr);
-	char *stmt = (char *) region_alloc(&fiber()->gc, len + 1);
+
+	struct Parse parser;
+	sql_parser_create(&parser, db);
+	parser.parse_only = true;
+
+	char *stmt = (char *)region_alloc(&parser.region, len + 1);
 	if (stmt == NULL) {
 		diag_set(OutOfMemory, len + 1, "region_alloc", "stmt");
 		return -1;
 	}
 	sprintf(stmt, "%s%s", outer, expr);
 
-	struct Parse parser;
-	sql_parser_create(&parser, db);
-	parser.parse_only = true;
 	char *unused;
 	if (sqlite3RunParser(&parser, stmt, &unused) != SQLITE_OK) {
 		diag_set(ClientError, ER_SQL_EXECUTE, expr);
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 1/2] sql: remove parser construct, destruct to sql.h

[Vladislav Shpilevoy]

Thanks for the patch!

> sql: remove parser construct, destruct to sql.h

What is construct/destruct? It is verbs. How can you remove them?

Please, use sql_parser_create/destroy, and do not use 'remove' word here.
It means rather delete something than move.

On 31/05/2018 18:01, Kirill Shcherbatov wrote:
> Moved sql_parser_create and sql_parser_destroy to sql.h to
> use them not only in DDL.
> Removed SELECTTRACE_ENABLED macro with conditional fields from
> struct Parse to prevent different object sizes across the project.
> 
> Part of #3272.
> ---
>   src/box/sql.h           | 17 +++++++++++++++++
>   src/box/sql/prepare.c   |  9 +++++++++
>   src/box/sql/sqliteInt.h | 25 -------------------------
>   3 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 3c26492..ecaf431 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -175,6 +175,23 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name);
>   int
>   sql_table_def_rebuild(struct sqlite3 *db, struct Table *table);
>   
> +/**
> + * Initialize a new parser object.
> + * A number of service allocations are performed on the region,
> + * which is also cleared in the destroy function.
> + * @param parser object to initialize.
> + * @param db SQLite object.
> + */
> +void
> +sql_parser_create(struct Parse *parser, struct sqlite3 *db);
> +
> +/**
> + * Release the parser object resources.
> + * @param parser object to release.
> + */
> +void
> +sql_parser_destroy(struct Parse *parser);
> +
>   #if defined(__cplusplus)
>   } /* extern "C" { */
>   #endif
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 8dc433f..8d94642 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -433,6 +433,15 @@ sqlite3_prepare_v2(sqlite3 * db,	/* Database handle. */
>   }
>   
>   void
> +sql_parser_create(struct Parse *parser, sqlite3 *db)
> +{
> +	memset(parser, 0, sizeof(struct Parse));
> +	parser->db = db;
> +	struct region *region = &fiber()->gc;
> +	parser->region_initial_size = region_used(region);
> +}
> +
> +void
>   sql_parser_destroy(Parse *parser)
>   {
>   	assert(parser != NULL);
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 950409d..abf1543 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2903,10 +2903,8 @@ struct Parse {
>   	Token constraintName;	/* Name of the constraint currently being parsed */
>   	int regRoot;		/* Register holding root page number for new objects */
>   	int nMaxArg;		/* Max args passed to user function by sub-program */
> -#ifdef SELECTTRACE_ENABLED
>   	int nSelect;		/* Number of SELECT statements seen */
>   	int nSelectIndent;	/* How far to indent SELECTTRACE() output */
> -#endif
>   	Parse *pToplevel;	/* Parse structure for main program (or NULL) */
>   	Table *pTriggerTab;	/* Table triggers are being coded for */
>   	u32 nQueryLoop;		/* Est number of iterations of a query (10*log2(N)) */
> @@ -4537,27 +4535,4 @@ extern int sqlite3InitDatabase(sqlite3 * db);
>   enum on_conflict_action
>   table_column_nullable_action(struct Table *tab, uint32_t column);
>   
> -/**
> - * Initialize a new parser object.
> - * A number of service allocations are performed on the region, which is also
> - * cleared in the destroy function.
> - * @param parser object to initialize.
> - * @param db SQLite object.
> - */
> -static inline void
> -sql_parser_create(struct Parse *parser, sqlite3 *db)
> -{
> -	memset(parser, 0, sizeof(struct Parse));
> -	parser->db = db;
> -	struct region *region = &fiber()->gc;
> -	parser->region_initial_size = region_used(region);
> -}
> -
> -/**
> - * Release the parser object resources.
> - * @param parser object to release.
> - */
> -void
> -sql_parser_destroy(struct Parse *parser);
> -
>   #endif				/* SQLITEINT_H */
> 

[tarantool-patches] Re: [PATCH v1 2/2] sql: use own region in Parser

[Vladislav Shpilevoy]

Thanks for the patch! See 2 comments below.

On 31/05/2018 18:01, Kirill Shcherbatov wrote:
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 5fbcbaf..b26565b 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1762,7 +1762,7 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table)
>   	 * names for existing table.
>   	 */
>   	assert(table->def->fields == NULL);
> -	struct region *region = &fiber()->gc;
> +	struct region *region = &parse->region;
>   	table->def->fields =
>   		region_alloc(region, nCol * sizeof(table->def->fields[0]));
>   	if (table->def->fields == NULL) {
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index abf1543..4275f72 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2913,8 +2913,8 @@ struct Parse {
>   	u8 eTriggerOp;		/* TK_UPDATE, TK_INSERT or TK_DELETE */
>   	u8 eOrconf;		/* Default ON CONFLICT policy for trigger steps */
>   	u8 disableTriggers;	/* True to disable triggers */
> -	/** Region size at the Parser launch. */
> -	size_t region_initial_size;
> +	/* Region to make SQL temp allocations. */

1. Please, use /** for out of a function body comments.

> +	struct region region;
>   
>     /**************************************************************************
>     * Fields above must be initialized to zero.  The fields that follow,
2. Did you check it works now on the Kirill Yu. patch? (with additional field in
space options).

[tarantool-patches] Re: [PATCH v1 0/2] sql: Create special region for parser

[Vladislav Shpilevoy]

Hello. Thanks for the patch!

On 31/05/2018 18:01, Kirill Shcherbatov wrote:
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3438-parser-own-region
> Issue: https://github.com/tarantool/tarantool/issues/3438

Please, provide the patchset description.

> 
> Kirill Shcherbatov (2):
>    sql: remove parser construct, destruct to sql.h
>    sql: use own region in Parser
> 
>   src/box/sql.h           | 17 +++++++++++++++++
>   src/box/sql/build.c     |  6 +++---
>   src/box/sql/prepare.c   | 11 +++++++++--
>   src/box/sql/select.c    |  2 +-
>   src/box/sql/sqliteInt.h | 29 ++---------------------------
>   src/box/sql/tokenize.c  | 10 ++++++----
>   6 files changed, 38 insertions(+), 37 deletions(-)
> 

[tarantool-patches] Re: [PATCH v1 2/2] sql: use own region in Parser

[Kirill Shcherbatov]

> 1. Please, use /** for out of a function body comments.
-       /* Region to make SQL temp allocations. */
+       /** Region to make SQL temp allocations. */

> 2. Did you check it works now on the Kirill Yu. patch? (with additional field in
> space options).
Yes, I did.

[tarantool-patches] Re: [PATCH v1 1/2] sql: remove parser construct, destruct to sql.h

[Kirill Shcherbatov]

> Please, use sql_parser_create/destroy, and do not use 'remove' word here.
> It means rather delete something than move.
sql: move sql_parser_create/destroy to sql.h

[tarantool-patches] Re: [PATCH v1 0/2] sql: Create special region for parser

[Kirill Shcherbatov]

Moved sql_parser_create and sql_parser_destroy to sql.h to
use them not only in DDL.
Removed SELECTTRACE_ENABLED macro with conditional fields from
struct Parse to prevent different object sizes across the project.
Start use own region for parser to avoid unexpected memory releases.

On 31.05.2018 18:01, Kirill Shcherbatov wrote:
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3438-parser-own-region
> Issue: https://github.com/tarantool/tarantool/issues/3438
> 
> Kirill Shcherbatov (2):
>   sql: remove parser construct, destruct to sql.h
>   sql: use own region in Parser
> 
>  src/box/sql.h           | 17 +++++++++++++++++
>  src/box/sql/build.c     |  6 +++---
>  src/box/sql/prepare.c   | 11 +++++++++--
>  src/box/sql/select.c    |  2 +-
>  src/box/sql/sqliteInt.h | 29 ++---------------------------
>  src/box/sql/tokenize.c  | 10 ++++++----
>  6 files changed, 38 insertions(+), 37 deletions(-)
> 

[tarantool-patches] Re: [PATCH v1 0/2] sql: Create special region for parser

[Vladislav Shpilevoy]

LGTM.

On 31/05/2018 18:50, Kirill Shcherbatov wrote:
> Moved sql_parser_create and sql_parser_destroy to sql.h to
> use them not only in DDL.
> Removed SELECTTRACE_ENABLED macro with conditional fields from
> struct Parse to prevent different object sizes across the project.
> Start use own region for parser to avoid unexpected memory releases.
> 
> On 31.05.2018 18:01, Kirill Shcherbatov wrote:
>> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3438-parser-own-region
>> Issue: https://github.com/tarantool/tarantool/issues/3438
>>
>> Kirill Shcherbatov (2):
>>    sql: remove parser construct, destruct to sql.h
>>    sql: use own region in Parser
>>
>>   src/box/sql.h           | 17 +++++++++++++++++
>>   src/box/sql/build.c     |  6 +++---
>>   src/box/sql/prepare.c   | 11 +++++++++--
>>   src/box/sql/select.c    |  2 +-
>>   src/box/sql/sqliteInt.h | 29 ++---------------------------
>>   src/box/sql/tokenize.c  | 10 ++++++----
>>   6 files changed, 38 insertions(+), 37 deletions(-)
>>
> 

[tarantool-patches] Re: [PATCH v1 0/2] sql: Create special region for parser

[Kirill Yukhin]

Hello,
On 31 мая 18:55, Vladislav Shpilevoy wrote:
> LGTM.
I've checked the patch-set into 2.0 branch.

--
Regards, Kirill Yukhin