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

[Roman Khabibov <roman.habibov@tarantool.org>]

Hi! Thanks for the review.

> On Nov 6, 2020, at 01:17, Nikita Pettik <korablev@tarantool.org> wrote:
> 
> On 09 Oct 16:45, Roman Khabibov wrote:
>> Move ck, fk constraint lists from struct create_table_def into new
>> defs and autoincrement into struct Parse to make the code more
>> reusable when implementing <ALTER TABLE ADD COLUMN>.
>> 
>> Needed for #3075
>> ---
>> 
>> +/**
>> + * Emit code to create sequences, indexes, check and foreign key
>> + * constraints appeared in <CREATE TABLE>.
>> + */
>> +static void
>> +vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id)
> 
> I'd move this refactoring in a separate patch. Up to you.
> 
>> @@ -3306,15 +3324,15 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
>> int
>> sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno)
>> {
>> -	if (parse_context->create_table_def.has_autoinc) {
>> +	if (parse_context->has_autoinc) {
>> 		diag_set(ClientError, ER_SQL_SYNTAX_WITH_POS,
>> 			 parse_context->line_count, parse_context->line_pos,
>> 			 "table must feature at most one AUTOINCREMENT field");
>> 		parse_context->is_aborted = true;
>> 		return -1;
>> 	}
>> -	parse_context->create_table_def.has_autoinc = true;
>> -	parse_context->create_table_def.autoinc_fieldno = fieldno;
>> +	parse_context->has_autoinc = true;
>> +	parse_context->autoinc_fieldno = fieldno;
>> 	return 0;
>> }
>> 
>> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
>> index cb0ecd2fc..21829b6f0 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;
> 
> Did you consider adding create_column_def class? Imho it would
> fit better in parse hierarchy: it would derive from create_entity_def,
> has field_def, autoinc, ck, fk members.
Yes. We discussed this a lot with Vlad. This class is implemented in
the main, last patch of the patchset. Unfortunately, the check and
fk constraints lists had to be removed separately from this class,
because the check and fk descriptions can occur separately from the
column descriptions in CREATE TABLE, e.g. " CREATE TABLE t (a INT
PRIMARY KEY, CHECK (a > 0))”. To make the code reusable, it is easier
to emit opcode for all constants after CREATE TABLE parsing. For the
same reason, autoinc is separate.

>> +};
>> +
>> +struct create_checks_def {
> 
> Why not create_ck_constraint_def? This naming would be more consistent
> with existing ck_contraint_def etc. The same for create_fk_constraint_def.
Because these are lists of constant defs. We decided to emphasize this.

> 
>> +	/** List of ck_constraint_parse objects. */
>> +	struct rlist checks;
>> +	/** Count of ck_constraint_parse objects. */
>> +	uint32_t count;
>> +};
>> +