[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