Re: [Tarantool-patches] [PATCH v4 2/6] sql: refactor create_table_def and parse

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

Hi! Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index d55c1cd71..b9f7e4638 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1147,6 +1147,88 @@ resolve_link(struct Parse *parse_context, const struct space_def *def,
>  	return -1;
>  }
>  
> +/**
> + * Emit code to create sequences, indexes, check and foreign key
> + * constraints appeared in <CREATE TABLE>.
> + */
> +static void
> +sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id)

1. After looking at another SQL patch I realized that this patch
has the same issue - vdbe code generation methods should be started
from 'vdbe_emit_' prefix. We can't invent a new prefix in every
patch doing something with VDBE code. The code will eventually become
unreadable.

> +{
> +	assert(reg_space_id != 0);
> +	struct space *space = parse->create_table_def.new_space;
> +	assert(space != NULL);
> +	uint32_t i = 0;
> +	for (; i < space->index_count; ++i) {
> +		struct index *idx = space->index[i];
> +		vdbe_emit_create_index(parse, space->def, idx->def,
> +				       reg_space_id, idx->def->iid);
> +	}
> @@ -1923,10 +1942,10 @@ sql_create_foreign_key(struct Parse *parse_context)
>  	}
>  	if (!is_alter) {
>  		if (create_def->name.n == 0) {
> +			uint32_t i = ++parse_context->create_fkeys_def.count;
>  			constraint_name =
>  				sqlMPrintf(db, "fk_unnamed_%s_%d",
> -					   space->def->name,
> -					   ++table_def->fkey_count);
> +					   space->def->name, i);

2. The format uses '%d', and you pass uint32_t, which is usually '%u'
(not always). Why can't you use a mere 'int' for the counter? In
the new struct definitions and here.

>  		} else {
>  			constraint_name =
>  				sql_name_from_token(db, &create_def->name);
> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
> index cb0ecd2fc..504d190b2 100644
> --- a/src/box/sql/parse_def.h
> +++ b/src/box/sql/parse_def.h
> @@ -205,26 +205,20 @@ struct create_entity_def {
>  struct create_table_def {
>  	struct create_entity_def base;
>  	struct space *new_space;
> -	/**
> -	 * Number of FK constraints declared within
> -	 * CREATE TABLE statement.
> -	 */
> -	uint32_t fkey_count;
> -	/**
> -	 * Foreign key constraint appeared in CREATE TABLE stmt.
> -	 */
> -	struct rlist new_fkey;
> -	/**
> -	 * Number of CK constraints declared within
> -	 * CREATE TABLE statement.
> -	 */
> -	uint32_t check_count;
> -	/** Check constraint appeared in CREATE TABLE stmt. */
> -	struct rlist new_check;
> -	/** True, if table to be created has AUTOINCREMENT PK. */
> -	bool has_autoinc;
> -	/** Id of field with AUTOINCREMENT. */
> -	uint32_t autoinc_fieldno;
> +};
> +
> +struct create_checks_def {
> +	/** List of check constraint objects. */

3. Lets try second time. What is 'check constraint object'? I can't
find 'struct check_constraint_object' in the code. The same for
foreign key constraints below.

> +	struct rlist checks;
> +	/** Count of check constraint objects. */
> +	uint32_t count;
> +};
> +
> +struct create_fkeys_def {
> +	/** List of foreign key constraint objects. */
> +	struct rlist fkeys;
> +	/** Count of foreign key constraint objects. */
> +	uint32_t count;
>  };