[tarantool-patches] Re: [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT

Nikita Pettik korablev at tarantool.org
Wed Sep 18 20:22:27 MSK 2019


On 28 Aug 14:47, Mergen Imeev wrote:
> 
> From f71cec86f8a19c60c2c71f78d25c473e7495b2e3 Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma at gmail.com>
> Date: Tue, 27 Aug 2019 14:03:58 +0300
> Subject: [PATCH] sql: AUTOINCREMENT for multipart PK
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index b4f114e..eb0263b 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -943,11 +937,36 @@ emitNewSysSequenceRecord(Parse *pParse, int reg_seq_id, const char *seq_name)
>  
>  static int
>  emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
> +			      struct space_def *space_def,
>  			      struct index_def *idx_def)
>  {
> -	struct key_part *part = &idx_def->key_def->parts[0];
> -	int fieldno = part->fieldno;
> +	uint32_t i;
>  	char *path = NULL;
> +	uint32_t fieldno = pParse->create_table_def.autoinc_field_id;
> +
> +	char *name = pParse->create_table_def.autoinc_field_name;
> +	if (name != NULL) {
> +		for (i = 0; i < space_def->field_count; ++i) {
> +			if (strcmp(space_def->fields[i].name, name) == 0)
> +				break;
> +		}
> +		assert(i < space_def->field_count);
> +		fieldno = i;
> +	}
> +
> +	struct key_part *part;
> +	for (i = 0; i < idx_def->key_def->part_count; ++i) {
> +		part = &idx_def->key_def->parts[i];
> +		if (part->fieldno == fieldno)
> +			break;
> +	}
> +	if (i == idx_def->key_def->part_count) {
> +		diag_set(ClientError, ER_MODIFY_INDEX, idx_def->name,
> +			 space_def->name, "sequence field must be a part of "
> +			 "the index");

This check is provided in alter.cc, why do we need to duplicate it?

> +		pParse->is_aborted = true;
> +		return -1;
> +	}
>  	if (part->path != NULL) {
>  		path = sqlDbStrNDup(pParse->db, part->path, part->path_len);
>  		if (path == NULL)
> @@ -1249,9 +1268,11 @@ sqlEndTable(struct Parse *pParse)
>  						 new_space->def->name);
>  		sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
>  		/* Do an insertion into _space_sequence. */
> -		int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse,
> -							reg_space_id, reg_seq_id,
> -							new_space->index[0]->def);
> +		int reg_space_seq_record =
> +			emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
> +						      reg_seq_id,
> +						      new_space->def,
> +						      new_space->index[0]->def);
>  		sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
>  			      reg_space_seq_record);
>  	}
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index be3c5c3..371da79 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -225,8 +225,34 @@ create_table_end ::= . { sqlEndTable(pParse); }
>   */
>  
>  columnlist ::= columnlist COMMA tcons.
> -columnlist ::= columnlist COMMA columnname carglist.
> -columnlist ::= columnname carglist.
> +columnlist ::= columnlist COMMA columnname carglist autoinc(I). {
> +  if (I == 1) {
> +    if (pParse->create_table_def.has_autoinc) {

Nit: just squash these two conditions in one diff:

if (I && pParse->create_table_def.has_autoinc) ...

> +      diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table must feature "
> +               "at most one AUTOINCREMENT field");
> +      pParse->is_aborted = true;
> +      return;
> +    }
> +    pParse->create_table_def.has_autoinc = true;
> +    pParse->create_table_def.autoinc_field_id =
> +      pParse->create_table_def.new_space->def->field_count - 1;
> +    assert(pParse->create_table_def.autoinc_field_name == NULL);

Why not always use field_name? Why not move this logic to autoinc() rule?
This code duplication looks quite awful.

> +columnlist ::= columnname carglist autoinc(I). {
> +  if (I == 1) {
> +    if (pParse->create_table_def.has_autoinc) {
> +      diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table must feature "
> +               "at most one AUTOINCREMENT field");
> +      pParse->is_aborted = true;
> +      return;
> +    }
> +    pParse->create_table_def.has_autoinc = true;
> +    pParse->create_table_def.autoinc_field_id =
> +      pParse->create_table_def.new_space->def->field_count - 1;
> +    assert(pParse->create_table_def.autoinc_field_name == NULL);
> +  }
> +}
>  columnlist ::= tcons.
>  columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);}
>  
> @@ -299,8 +325,7 @@ ccons ::= NULL onconf(R).        {
>          sql_column_add_nullable_action(pParse, R);
>  }
>  ccons ::= NOT NULL onconf(R).    {sql_column_add_nullable_action(pParse, R);}
> -ccons ::= cconsname(N) PRIMARY KEY sortorder(Z) autoinc(I). {
> -  pParse->create_table_def.has_autoinc = I;
> +ccons ::= cconsname(N) PRIMARY KEY sortorder(Z). {
>    create_index_def_init(&pParse->create_index_def, NULL, &N, NULL,
>                          SQL_INDEX_TYPE_CONSTRAINT_PK, Z, false);
>    sqlAddPrimaryKey(pParse);
> @@ -369,8 +394,7 @@ init_deferred_pred_opt(A) ::= .                       {A = 0;}
>  init_deferred_pred_opt(A) ::= INITIALLY DEFERRED.     {A = 1;}
>  init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE.    {A = 0;}
>  
> -tcons ::= cconsname(N) PRIMARY KEY LP sortlist(X) autoinc(I) RP. {
> -  pParse->create_table_def.has_autoinc = I;
> +tcons ::= cconsname(N) PRIMARY KEY LP sortlist_with_autoinc(X) RP. {
>    create_index_def_init(&pParse->create_index_def, NULL, &N, X,
>                          SQL_INDEX_TYPE_CONSTRAINT_PK, SORT_ORDER_ASC, false);
>    sqlAddPrimaryKey(pParse);
> @@ -760,6 +784,63 @@ sortlist(A) ::= expr(Y) sortorder(Z). {
>    sqlExprListSetSortOrder(A,Z);
>  }
>  
> +// the sortlist_with_autoinc non-terminal stores a list of
> +// expression where each expression is a name of the column in
> +// PRIMARY KEY definition and optionally followed by ASC or DESC
> +// to indicate the sort order and by AUTOINCREMENT to indicate
> +// that sequence should be attached to the column.
> +//

Please use common commenting style (/**/).

> +%type sortlist_with_autoinc {ExprList*}
> +%destructor sortlist_with_autoinc {sql_expr_list_delete(pParse->db, $$);}
> +
> +sortlist_with_autoinc(A) ::= sortlist_with_autoinc(A) COMMA expr(Y) sortorder(Z) autoinc(I). {i

Why do you need expr? Expressions are prohibited in index definition.

> +  if (I == 1) {
> +    if (pParse->create_table_def.has_autoinc) {
> +      diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table must feature "
> +               "at most one AUTOINCREMENT field");
> +      pParse->is_aborted = true;
> +      return;
> +    }
> +    pParse->create_table_def.has_autoinc = true;
> +    struct Expr *name = sqlExprSkipCollate(Y.pExpr);
> +    if (name->op != TK_ID) {
> +      diag_set(ClientError, ER_INDEX_DEF_UNSUPPORTED, "Expressions");
> +      pParse->is_aborted = true;
> +      return;
> +    }
> +    char *name_str = region_alloc(&pParse->region, strlen(name->u.zToken) + 1);
> +    strcpy(name_str, name->u.zToken);
> +    pParse->create_table_def.autoinc_field_name = name_str;
> +    assert(pParse->create_table_def.autoinc_field_id == 0);
> +  }
> +  A = sql_expr_list_append(pParse->db,A,Y.pExpr);
> +  sqlExprListSetSortOrder(A,Z);
> +}
> +sortlist_with_autoinc(A) ::= expr(Y) sortorder(Z) autoinc(I). {
> +  if (I == 1) {
> +    if (pParse->create_table_def.has_autoinc) {
> +      diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table must feature "
> +               "at most one AUTOINCREMENT field");
> +      pParse->is_aborted = true;
> +      return;
> +    }
> +    pParse->create_table_def.has_autoinc = true;
> +    struct Expr *name = sqlExprSkipCollate(Y.pExpr);
> +    if (name->op != TK_ID) {
> +      diag_set(ClientError, ER_INDEX_DEF_UNSUPPORTED, "Expressions");
> +      pParse->is_aborted = true;
> +      return;
> +    }
> +    char *name_str = region_alloc(&pParse->region, strlen(name->u.zToken) + 1);
> +    strcpy(name_str, name->u.zToken);
> +    pParse->create_table_def.autoinc_field_name = name_str;
> +    assert(pParse->create_table_def.autoinc_field_id == 0);
> +  }

Consider my comments about using field's name and filling auto-increment
property in autoinc rule (or at least move it to auxiliary function) and
you will be able to reduce amount of code significantly.

> +  /* A-overwrites-Y. */
> +  A = sql_expr_list_append(pParse->db,NULL,Y.pExpr);
> +  sqlExprListSetSortOrder(A,Z);
> +}





More information about the Tarantool-patches mailing list