Re: [Tarantool-patches] [PATCH v2 14/16] sql: introduce cache for prepared statemets

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> +    prep_stmt.c

sql_stmt_cache.{h,c}
> +static void
> +box_check_sql_cache_size(int size)
> +{
> +	if (size < 0) {
> +		tnt_raise(ClientError, ER_CFG, "sql_cache_size",
> +			  "must be non-negative");
> +	}
> +}

sql_stmt_cache_size?

> +void
> +box_set_prepared_stmt_cache_size(void)

Please use consistent naming. box_get_{sql_stmt_cache}_size(),
box_set_{sql_stmt_cache*}_size

> +{
> +	int cache_sz = cfg_geti("sql_cache_size");

Same here. sql_stmt_cache_size, or simply sz or s. Please
either use a shorthand like s or sz, or the same name
as the configuration variable name, to make the code simple to
grep, rename, refactor, etc.

> +	}
> +	while (! sql_cache_check_new_node_size(new_node_size))
> +		sql_prepared_stmt_cache_gc();

'Entry' is a more common name for cache entries than a 'node'.

> +	uint32_t str_hash = mh_strn_hash(sql_str, strlen(sql_str));
> +	const struct mh_strnptr_node_t hash_node = { sql_str, strlen(sql_str),
> +						     str_hash, cache_node };
> +	struct mh_strnptr_node_t *old_node = NULL;
> +	mh_int_t i = mh_strnptr_put(hash, &hash_node, &old_node, NULL);
> +	if (i == mh_end(hash)) {
> +		sql_cache_node_delete(cache, cache_node);
> +		diag_set(OutOfMemory, 0, "mh_strnptr_put", "mh_strnptr_node");
> +		return -1;
> +	}

> +sql_prepared_stmt_cache_set_size(size_t size)
> +{
> +	prep_stmt_cache.mem_quota = size;
> +	while (prep_stmt_cache.mem_used > size)
> +		sql_prepared_stmt_cache_gc();
> +}

How does this work if there are active statements which are
referenced/in use?


-- 
Konstantin Osipov, Moscow, Russia