From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id F211946970E for ; Fri, 7 Feb 2020 01:16:49 +0300 (MSK) From: Vladislav Shpilevoy References: <20200204193200.61510-1-k.sosnin@tarantool.org> Message-ID: Date: Thu, 6 Feb 2020 23:16:48 +0100 MIME-Version: 1.0 In-Reply-To: <20200204193200.61510-1-k.sosnin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin , tarantool-patches@dev.tarantool.org Thanks for the patch! I used session_setting_find() in more places; dropped P3 argument from OP_Set; replaced region_alloc with static_alloc; added check that vdbe is not NULL, because sql_set_setting may be the first called function and may try to create a new VDBE object; renamed sql_set_setting to sql_setting_set, because in future we may add GET, and I want their C emitters have the same prefix - sql_setting_*; made SET argument expression instead of term, so I could use '?', which is really useful when dealing with strings, against SQL injections and to avoid string concatenations in user's code. ================================================================================ commit 81182c89163581cb55fa40552bde779f0bbe025b Author: Vladislav Shpilevoy Date: Thu Feb 6 22:56:40 2020 +0100 Review fixes diff --git a/src/box/session_settings.c b/src/box/session_settings.c index c7cb4f6d4..3d06a191a 100644 --- a/src/box/session_settings.c +++ b/src/box/session_settings.c @@ -324,8 +324,8 @@ session_settings_index_get(struct index *base, const char *key, uint32_t len; key = mp_decode_str(&key, &len); key = tt_cstr(key, len); - int sid = 0; - if (session_settings_set_forward(&sid, key, true, true) != 0) { + int sid = session_setting_find(key); + if (sid < 0) { *result = NULL; return 0; } @@ -426,7 +426,8 @@ session_settings_space_execute_update(struct space *space, struct txn *txn, } key = mp_decode_str(&key, &key_len); key = tt_cstr(key, key_len); - if (session_settings_set_forward(&sid, key, true, true) != 0) { + sid = session_setting_find(key); + if (sid < 0) { *result = NULL; return 0; } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 259a16ab5..9cbe25443 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3471,20 +3471,21 @@ sql_session_settings_init() } void -sql_set_setting(struct Parse *parse_context, struct Token *name, +sql_setting_set(struct Parse *parse_context, struct Token *name, struct Expr *expr) { struct Vdbe *vdbe = sqlGetVdbe(parse_context); - assert(vdbe != NULL); + if (vdbe == NULL) + goto abort; sqlVdbeCountChanges(vdbe); - char *key = sql_name_from_token(sql_get(), name); + char *key = sql_name_from_token(parse_context->db, name); if (key == NULL) goto abort; int index = session_setting_find(key); if (index >= 0) { int target = ++parse_context->nMem; sqlExprCode(parse_context, expr, target); - sqlVdbeAddOp3(vdbe, OP_Set, index, target, expr->type); + sqlVdbeAddOp2(vdbe, OP_Set, index, target); return; } diag_set(ClientError, ER_SQL_PARSER_GENERIC, diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 9d9b498a7..a01c37e19 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1542,8 +1542,8 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y). { } ///////////////////////////// The SET command //////////////////////////////// -cmd ::= SET nm(X) EQ term(Y). { - sql_set_setting(pParse,&X,Y.pExpr); +cmd ::= SET nm(X) EQ expr(Y). { + sql_setting_set(pParse,&X,Y.pExpr); } ///////////////////////////// The PRAGMA command ///////////////////////////// diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 5e192a7bb..aabada4a2 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -4518,7 +4518,7 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name, * @param value New value of the session setting. */ void -sql_set_setting(struct Parse *parse_context, struct Token *name, +sql_setting_set(struct Parse *parse_context, struct Token *name, struct Expr *value); #endif /* sqlINT_H */ diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 77dcfc57a..282a02b69 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -5262,44 +5262,51 @@ case OP_IncMaxid: { break; } -/* Opcode: Set P1 P2 P3 * * +/* Opcode: Set P1 P2 * * * * - * Set the new value of the session setting. P1 is the id of the setting - * in the session_settings array, P2 is the memory location of the - * new value, P3 is its field_type. + * Set the new value of the session setting. P1 is the id of the + * setting in the session_settings array, P2 is the register + * holding a value. */ case OP_Set: { int sid = pOp->p1; pIn1 = &aMem[pOp->p2]; assert(sid >= 0 && sid < SESSION_SETTING_COUNT); struct session_setting *setting = &session_settings[sid]; - switch (pOp->p3) { - case FIELD_TYPE_BOOLEAN: { - bool value = pIn1->u.b; - size_t size = mp_sizeof_bool(value); - char *mp_value = (char *)region_alloc(&fiber()->gc, - size); - mp_encode_bool(mp_value, value); - if (setting->set(sid, mp_value) != 0) - goto abort_due_to_error; - break; - } - case FIELD_TYPE_STRING: { - const char *str = pIn1->z; - size_t len = strlen(str); - uint32_t size = mp_sizeof_str(len); - char *mp_value = (char *)region_alloc(&fiber()->gc, - size); - mp_encode_str(mp_value, str, len); - if (setting->set(sid, mp_value) != 0) - goto abort_due_to_error; - break; + switch (setting->field_type) { + case FIELD_TYPE_BOOLEAN: { + if ((pIn1->flags & MEM_Bool) == 0) + goto invalid_type; + bool value = pIn1->u.b; + size_t size = mp_sizeof_bool(value); + char *mp_value = (char *) static_alloc(size); + mp_encode_bool(mp_value, value); + if (setting->set(sid, mp_value) != 0) + goto abort_due_to_error; + break; + } + case FIELD_TYPE_STRING: { + if ((pIn1->flags & MEM_Str) == 0) + goto invalid_type; + const char *str = pIn1->z; + size_t len = strlen(str); + uint32_t size = mp_sizeof_str(len); + char *mp_value = (char *) static_alloc(size); + if (mp_value == NULL) { + diag_set(OutOfMemory, size, "static_alloc", "mp_value"); + goto abort_due_to_error; } - default: - diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE, - session_setting_strs[sid], - field_type_strs[setting->field_type]); + mp_encode_str(mp_value, str, len); + if (setting->set(sid, mp_value) != 0) goto abort_due_to_error; + break; + } + default: + invalid_type: + diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE, + session_setting_strs[sid], + field_type_strs[setting->field_type]); + goto abort_due_to_error; } p->nChange++; break; diff --git a/test/box/session_settings.result b/test/box/session_settings.result index 9657ba58d..7c9802af6 100644 --- a/test/box/session_settings.result +++ b/test/box/session_settings.result @@ -363,7 +363,7 @@ s:get('sql_defer_foreign_keys').value | --- | - true | ... -box.execute([[set "sql_defer_foreign_keys" = false]]) +box.execute([[set "sql_defer_foreign_keys" = ?]], {false}) | --- | - row_count: 1 | ... diff --git a/test/box/session_settings.test.lua b/test/box/session_settings.test.lua index d0f7ca0f7..f2d7e03f5 100644 --- a/test/box/session_settings.test.lua +++ b/test/box/session_settings.test.lua @@ -138,7 +138,7 @@ box.execute([[set "sql_default_engine" = 'memtx']]) s:get('sql_default_engine').value box.execute([[set "sql_defer_foreign_keys" = true]]) s:get('sql_defer_foreign_keys').value -box.execute([[set "sql_defer_foreign_keys" = false]]) +box.execute([[set "sql_defer_foreign_keys" = ?]], {false}) s:get('sql_defer_foreign_keys').value settings.sql_default_engine:set(true)