Re: [Tarantool-patches] [PATCH v5 5/5] sql: refactor PRAGMA-related code

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

I've pushed my review fixes on top of this commit. See it below
and on the branch. If you agree, then squash. Otherwise lets
discuss.

================================================================================

commit 253847585514e0abc0f098c13ab77048a807d8d1
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Fri Dec 27 18:22:53 2019 +0300

    Review fixes 5/5

diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 3f3dac3be..28ba64500 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -237,15 +237,12 @@ sql_pragma_collation_list(struct Parse *parse_context)
 {
 	struct Vdbe *v = sqlGetVdbe(parse_context);
 	assert(v != NULL);
-	int i = 0;
-	struct space *space = space_by_name("_collation");
 	/* 16 is enough to encode 0 len array */
 	char key_buf[16];
-	char *key_end = key_buf;
-	key_end = mp_encode_array(key_end, 0);
+	char *key_end = mp_encode_array(key_buf, 0);
 	box_tuple_t *tuple;
-	box_iterator_t* it;
-	it = box_index_iterator(space->def->id, 0, ITER_ALL, key_buf, key_end);
+	box_iterator_t *it = box_index_iterator(BOX_COLLATION_ID, 0, ITER_ALL,
+						key_buf, key_end);
 	if (it == NULL) {
 		parse_context->is_aborted = true;
 		return;
@@ -253,9 +250,9 @@ sql_pragma_collation_list(struct Parse *parse_context)
 	int rc = box_iterator_next(it, &tuple);
 	assert(rc == 0);
 	(void) rc;
-	for (i = 0; tuple != NULL; i++, box_iterator_next(it, &tuple)) {
-		/* 1 is name field number */
-		const char *str = tuple_field_cstr(tuple, 1);
+	for (int i = 0; tuple != NULL; i++, box_iterator_next(it, &tuple)) {
+		const char *str = tuple_field_cstr(tuple,
+						   BOX_COLLATION_FIELD_NAME);
 		assert(str != NULL);
 		/* this procedure should reallocate and copy str */
 		sqlVdbeMultiLoad(v, 1, "is", i, str);
@@ -309,18 +306,18 @@ sql_pragma_foreign_key_list(struct Parse *parse_context, const char *table_name)
 	struct fk_constraint *fk_c;
 	rlist_foreach_entry(fk_c, &space->child_fk_constraint, in_child_space) {
 		struct fk_constraint_def *fk_def = fk_c->def;
+		struct space *parent = space_by_id(fk_def->parent_id);
+		assert(parent != NULL);
+		const char *on_delete_action =
+			fk_constraint_action_strs[fk_def->on_delete];
+		const char *on_update_action =
+			fk_constraint_action_strs[fk_def->on_update];
 		for (uint32_t j = 0; j < fk_def->field_count; j++) {
-			struct space *parent = space_by_id(fk_def->parent_id);
-			assert(parent != NULL);
 			uint32_t ch_fl = fk_def->links[j].child_field;
 			const char *child_col = space->def->fields[ch_fl].name;
 			uint32_t pr_fl = fk_def->links[j].parent_field;
 			const char *parent_col =
 				parent->def->fields[pr_fl].name;
-			const char *on_delete_action =
-				fk_constraint_action_strs[fk_def->on_delete];
-			const char *on_update_action =
-				fk_constraint_action_strs[fk_def->on_update];
 			sqlVdbeMultiLoad(v, 1, "iissssss", i, j,
 					 parent->def->name, child_col,
 					 parent_col, on_delete_action,
@@ -332,21 +329,20 @@ sql_pragma_foreign_key_list(struct Parse *parse_context, const char *table_name)
 }
 
 void
-sqlPragma(Parse *pParse, Token *pragma, Token *table, Token *index)
+sqlPragma(struct Parse *pParse, struct Token *pragma, struct Token *table,
+	  struct Token *index)
 {
-	char *pragma_name = NULL;
 	char *table_name = NULL;
 	char *index_name = NULL;
-	sql *db = pParse->db;
-	Vdbe *v = sqlGetVdbe(pParse);
-	const struct PragmaName *pPragma;
+	struct sql *db = pParse->db;
+	struct Vdbe *v = sqlGetVdbe(pParse);
 
 	if (v == NULL)
 		return;
 	sqlVdbeRunOnlyOnce(v);
 	pParse->nMem = 2;
 
-	pragma_name = sql_name_from_token(db, pragma);
+	char *pragma_name = sql_name_from_token(db, pragma);
 	if (pragma_name == NULL) {
 		pParse->is_aborted = true;
 		goto pragma_out;
@@ -367,7 +363,7 @@ sqlPragma(Parse *pParse, Token *pragma, Token *table, Token *index)
 	}
 
 	/* Locate the pragma in the lookup table */
-	pPragma = pragmaLocate(pragma_name);
+	const struct PragmaName *pPragma = pragmaLocate(pragma_name);
 	if (pPragma == 0) {
 		diag_set(ClientError, ER_SQL_NO_SUCH_PRAGMA, pragma_name);
 		pParse->is_aborted = true;
diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h
index 178746b35..f319272d4 100644
--- a/src/box/sql/pragma.h
+++ b/src/box/sql/pragma.h
@@ -1,17 +1,11 @@
 /** List of ID of pragmas. */
 enum
 {
-	/** Pragma collation_list. */
 	PRAGMA_COLLATION_LIST = 0,
-	/** Pragma foreign_key_list. */
 	PRAGMA_FOREIGN_KEY_LIST,
-	/** Pragma index_info. */
 	PRAGMA_INDEX_INFO,
-	/** Pragma index_list. */
 	PRAGMA_INDEX_LIST,
-	/** Pragma stats. */
 	PRAGMA_STATS,
-	/** Pragma table_info. */
 	PRAGMA_TABLE_INFO,
 };
 
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 4691a3369..1d8f6c8cd 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2781,7 +2781,8 @@ int sqlInit(sql *);
  * @param index Name of the index.
  */
 void
-sqlPragma(Parse *pParse, Token *pragma, Token *table, Token *index);
+sqlPragma(struct Parse *pParse, struct Token *pragma, struct Token *table,
+	  struct Token *index);
 
 /**
  * Return true if given column is part of primary key.