[tarantool-patches] Re: [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT
Mergen Imeev
imeevma at tarantool.org
Sat Sep 21 10:22:41 MSK 2019
Hi! Thank you for review! My answers, diff and bew patch
below.
On Wed, Sep 18, 2019 at 08:22:27PM +0300, Nikita Pettik wrote:
> 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?
>
Removed.
> > + 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) ...
>
Not sure if that would be better. Moved this code to a new
function.
> > + 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.
>
This would be quite problematic, since we need to get the
field name by its identifier, and then get the field
identifier by its name. Moved this code to a new function.
> > +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 (/**/).
>
Fixed.
> > +%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.
>
I just copied the "sortlist" rule and changed it a bit.
Since after processing the AUTOINCREMENT keyword, it should
work the same as the "sortlist" rule, I did not change the
original arguments of the rule.
> > + 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.
>
I created a function. I think now it looks much better.
> > + /* A-overwrites-Y. */
> > + A = sql_expr_list_append(pParse->db,NULL,Y.pExpr);
> > + sqlExprListSetSortOrder(A,Z);
> > +}
>
Diff
commit 2052df2aa71e7cd93c97a89e27a6931e3b0a35e0
Author: Mergen Imeev <imeevma at gmail.com>
Date: Thu Sep 19 11:00:50 2019 +0300
Review fix
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index eb0263b..c0ca425 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -954,20 +954,13 @@ emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
fieldno = i;
}
- struct key_part *part;
+ struct key_part *part = NULL;
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");
- pParse->is_aborted = true;
- return -1;
- }
- if (part->path != NULL) {
+ if (i != idx_def->key_def->part_count && part->path != NULL) {
path = sqlDbStrNDup(pParse->db, part->path, part->path_len);
if (path == NULL)
return -1;
@@ -3240,3 +3233,33 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
sqlVdbeAddOp1(v, OP_Close, cursor);
return 0;
}
+
+int
+sql_add_autoincrement(struct Parse *parse_context, struct Expr *field_name,
+ uint32_t field_id)
+{
+ if (parse_context->create_table_def.has_autoinc) {
+ diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table "
+ "must feature at most one AUTOINCREMENT field");
+ parse_context->is_aborted = true;
+ return -1;
+ }
+ parse_context->create_table_def.has_autoinc = true;
+ if (field_name != NULL) {
+ struct Expr *name = sqlExprSkipCollate(field_name);
+ if (name->op != TK_ID) {
+ diag_set(ClientError, ER_INDEX_DEF_UNSUPPORTED,
+ "Expressions");
+ parse_context->is_aborted = true;
+ return -1;
+ }
+ char *name_str = region_alloc(&parse_context->region,
+ strlen(name->u.zToken) + 1);
+ strcpy(name_str, name->u.zToken);
+ parse_context->create_table_def.autoinc_field_name = name_str;
+ return 0;
+ }
+ parse_context->create_table_def.autoinc_field_id = field_id;
+ assert(parse_context->create_table_def.autoinc_field_name == NULL);
+ return 0;
+}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 6188814..65a1569 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -226,32 +226,14 @@ create_table_end ::= . { sqlEndTable(pParse); }
columnlist ::= columnlist COMMA tcons.
columnlist ::= columnlist COMMA 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);
- }
+ uint32_t field_id = pParse->create_table_def.new_space->def->field_count - 1;
+ if (I == 1 && sql_add_autoincrement(pParse, NULL, field_id) != 0)
+ return;
}
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);
- }
+ uint32_t field_id = pParse->create_table_def.new_space->def->field_count - 1;
+ if (I == 1 && sql_add_autoincrement(pParse, NULL, field_id) != 0)
+ return;
}
columnlist ::= tcons.
columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);}
@@ -784,61 +766,26 @@ 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.
-//
+/**
+ * The sortlist_with_autoinc rule works the same as the sortlist
+ * rule, except that it can work with the AUTOINCREMENT keyword.
+ */
%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). {
- 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) ::= sortlist_with_autoinc(A) COMMA expr(Y) sortorder(Z)
+ autoinc(I). {
+ if (I == 1 && sql_add_autoincrement(pParse, Y.pExpr, 0) != 0)
+ return;
+ 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);
- }
+ if (I == 1 && sql_add_autoincrement(pParse, Y.pExpr, 0) != 0)
+ return;
/* A-overwrites-Y. */
- A = sql_expr_list_append(pParse->db,NULL,Y.pExpr);
- sqlExprListSetSortOrder(A,Z);
+ A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
+ sqlExprListSetSortOrder(A, Z);
}
%type sortorder {int}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index fbb3987..d7976a3 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4419,4 +4419,22 @@ void
vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
const char *idx_name, const char *table_name);
+/**
+ * Add AUTOINCREMENT feature for one of INTEGER or UNSIGNED fields
+ * of PRIMARY KEY. If field_name is NULL, the field is identified
+ * by its identifier (field_id). Otherwise, field_id is ignored,
+ * and the field is identified by the name of the field that is
+ * contained in field_name.
+ *
+ * @param parse_context Parsing context.
+ * @param field_name Expr that contains field name.
+ * @param field_id Field identifier.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+sql_add_autoincrement(struct Parse *parse_context, struct Expr *field_name,
+ uint32_t field_id);
+
#endif /* sqlINT_H */
New patch:
>From 505a99087eaf247b39d68c3e928800344c8621ed 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
Prior to this patch, the auto-increment feature could only be set
in an INTEGER field of PRIMARY KEY if the PRIMARY KEY consisted of
a single field. It was not possible to use this feature if the
PRIMARY KEY consisted of more than one field. This patch defines
two ways to set AUTOINCREMENT for any INTEGER or UNSIGNED field of
PRIMARY KEY.
Closes #4217
@TarantoolBot document
Title: The auto-increment feature for multipart PK
The auto-increment feature can be set to any INTEGER or UNSIGNED
field of PRIMARY KEY using one of two ways:
1) AUTOINCREMENT in column definition:
CREATE TABLE t (i INT, a INT AUTOINCREMENT, PRIMARY KEY (i, a));
CREATE TABLE t (i INT AUTOINCREMENT, a INT, PRIMARY KEY (i, a));
2) AUTOINCREMENT in PRIMARY KEY definition:
CREATE TABLE t (i INT, a INT, PRIMARY KEY (i, a AUTOINCREMENT));
CREATE TABLE t (i INT, a INT, PRIMARY KEY (i AUTOINCREMENT, a));
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index b4f114e..c0ca425 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -521,12 +521,6 @@ sqlAddPrimaryKey(struct Parse *pParse)
sql_create_index(pParse);
if (db->mallocFailed)
goto primary_key_exit;
- } else if (pParse->create_table_def.has_autoinc) {
- diag_set(ClientError, ER_CREATE_SPACE, space->def->name,
- "AUTOINCREMENT is only allowed on an INTEGER PRIMARY "\
- "KEY or INT PRIMARY KEY");
- pParse->is_aborted = true;
- goto primary_key_exit;
} else {
sql_create_index(pParse);
pList = NULL;
@@ -943,12 +937,30 @@ 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;
- if (part->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 = NULL;
+ 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 && part->path != NULL) {
path = sqlDbStrNDup(pParse->db, part->path, part->path_len);
if (path == NULL)
return -1;
@@ -1249,9 +1261,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);
}
@@ -3219,3 +3233,33 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
sqlVdbeAddOp1(v, OP_Close, cursor);
return 0;
}
+
+int
+sql_add_autoincrement(struct Parse *parse_context, struct Expr *field_name,
+ uint32_t field_id)
+{
+ if (parse_context->create_table_def.has_autoinc) {
+ diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table "
+ "must feature at most one AUTOINCREMENT field");
+ parse_context->is_aborted = true;
+ return -1;
+ }
+ parse_context->create_table_def.has_autoinc = true;
+ if (field_name != NULL) {
+ struct Expr *name = sqlExprSkipCollate(field_name);
+ if (name->op != TK_ID) {
+ diag_set(ClientError, ER_INDEX_DEF_UNSUPPORTED,
+ "Expressions");
+ parse_context->is_aborted = true;
+ return -1;
+ }
+ char *name_str = region_alloc(&parse_context->region,
+ strlen(name->u.zToken) + 1);
+ strcpy(name_str, name->u.zToken);
+ parse_context->create_table_def.autoinc_field_name = name_str;
+ return 0;
+ }
+ parse_context->create_table_def.autoinc_field_id = field_id;
+ assert(parse_context->create_table_def.autoinc_field_name == NULL);
+ return 0;
+}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 643e025..65a1569 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -225,8 +225,16 @@ create_table_end ::= . { sqlEndTable(pParse); }
*/
columnlist ::= columnlist COMMA tcons.
-columnlist ::= columnlist COMMA columnname carglist.
-columnlist ::= columnname carglist.
+columnlist ::= columnlist COMMA columnname carglist autoinc(I). {
+ uint32_t field_id = pParse->create_table_def.new_space->def->field_count - 1;
+ if (I == 1 && sql_add_autoincrement(pParse, NULL, field_id) != 0)
+ return;
+}
+columnlist ::= columnname carglist autoinc(I). {
+ uint32_t field_id = pParse->create_table_def.new_space->def->field_count - 1;
+ if (I == 1 && sql_add_autoincrement(pParse, NULL, field_id) != 0)
+ return;
+}
columnlist ::= tcons.
columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);}
@@ -299,8 +307,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 +376,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 +766,28 @@ sortlist(A) ::= expr(Y) sortorder(Z). {
sqlExprListSetSortOrder(A,Z);
}
+/**
+ * The sortlist_with_autoinc rule works the same as the sortlist
+ * rule, except that it can work with the AUTOINCREMENT keyword.
+ */
+%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). {
+ if (I == 1 && sql_add_autoincrement(pParse, Y.pExpr, 0) != 0)
+ return;
+ 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 && sql_add_autoincrement(pParse, Y.pExpr, 0) != 0)
+ return;
+ /* A-overwrites-Y. */
+ A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
+ sqlExprListSetSortOrder(A, Z);
+}
+
%type sortorder {int}
sortorder(A) ::= ASC. {A = SORT_ORDER_ASC;}
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 557e415..e4c153a 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -214,6 +214,16 @@ struct create_table_def {
struct rlist new_check;
/** True, if table to be created has AUTOINCREMENT PK. */
bool has_autoinc;
+ /**
+ * Id of field with AUTOINCREMENT in case this keyword
+ * follow definition of the field.
+ */
+ uint32_t autoinc_field_id;
+ /**
+ * Name of field with AUTOINCREMENT in case this keyword
+ * defined in PK definition.
+ */
+ char *autoinc_field_name;
};
struct create_view_def {
@@ -461,6 +471,8 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
if_not_exists);
rlist_create(&table_def->new_fkey);
rlist_create(&table_def->new_check);
+ table_def->autoinc_field_id = 0;
+ table_def->autoinc_field_name = NULL;
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index fbb3987..d7976a3 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4419,4 +4419,22 @@ void
vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
const char *idx_name, const char *table_name);
+/**
+ * Add AUTOINCREMENT feature for one of INTEGER or UNSIGNED fields
+ * of PRIMARY KEY. If field_name is NULL, the field is identified
+ * by its identifier (field_id). Otherwise, field_id is ignored,
+ * and the field is identified by the name of the field that is
+ * contained in field_name.
+ *
+ * @param parse_context Parsing context.
+ * @param field_name Expr that contains field name.
+ * @param field_id Field identifier.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+sql_add_autoincrement(struct Parse *parse_context, struct Expr *field_name,
+ uint32_t field_id);
+
#endif /* sqlINT_H */
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index 1d0be8a..de466e6 100755
--- a/test/sql-tap/autoinc.test.lua
+++ b/test/sql-tap/autoinc.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(47)
+test:plan(55)
--!./tcltestrunner.lua
-- 2004 November 12
@@ -561,7 +561,7 @@ test:do_catchsql_test(
CREATE TABLE t8(x TEXT PRIMARY KEY AUTOINCREMENT);
]], {
-- <autoinc-7.2>
- 1, "Failed to create space 'T8': AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY or INT PRIMARY KEY"
+ 1, "Can't create or modify index 'pk_unnamed_T8_1' in space 'T8': sequence cannot be used with a non-integer key"
-- </autoinc-7.2>
})
@@ -814,4 +814,97 @@ test:do_catchsql_test(
-- </autoinc-gh-3670>
})
+--
+-- gh-4217: make sure that AUTOINCREMENT can be used for any
+-- INTEGER field of PRIMARY KEY.
+--
+
+test:do_execsql_test(
+ "autoinc-11.1",
+ [[
+ CREATE TABLE t11_1 (i INT AUTOINCREMENT, a INT, PRIMARY KEY(i, a));
+ INSERT INTO t11_1 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+ SELECT * FROM t11_1;
+ ]], {
+ -- <autoinc-11.1>
+ 1, 1, 2, 1, 3, 1
+ -- </autoinc-11.1>
+ })
+
+test:do_execsql_test(
+ "autoinc-11.2",
+ [[
+ CREATE TABLE t11_2 (i INT AUTOINCREMENT, a INT, PRIMARY KEY(a, i));
+ INSERT INTO t11_2 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+ SELECT * FROM t11_2;
+ ]], {
+ -- <autoinc-11.2>
+ 1, 1, 2, 1, 3, 1
+ -- </autoinc-11.2>
+ })
+
+test:do_execsql_test(
+ "autoinc-11.3",
+ [[
+ CREATE TABLE t11_3 (i INT, a INT, PRIMARY KEY(i AUTOINCREMENT, a));
+ INSERT INTO t11_3 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+ SELECT * FROM t11_3;
+ ]], {
+ -- <autoinc-11.3>
+ 1, 1, 2, 1, 3, 1
+ -- </autoinc-11.3>
+ })
+
+test:do_execsql_test(
+ "autoinc-11.4",
+ [[
+ CREATE TABLE t11_4 (i INT, a INT, PRIMARY KEY(a, i AUTOINCREMENT));
+ INSERT INTO t11_4 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+ SELECT * FROM t11_4;
+ ]], {
+ -- <autoinc-11.4>
+ 1, 1, 2, 1, 3, 1
+ -- </autoinc-11.4>
+ })
+
+test:do_catchsql_test(
+ "autoinc-11.5",
+ [[
+ CREATE TABLE t11_5 (i INT, a INT, PRIMARY KEY(a, i COLLATE "unicode_ci" AUTOINCREMENT));
+ ]], {
+ -- <autoinc-11.5>
+ 1, "Wrong index options (field 2): collation is reasonable only for string and scalar parts"
+ -- </autoinc-11.5>
+ })
+
+test:do_catchsql_test(
+ "autoinc-11.6",
+ [[
+ CREATE TABLE t11_6 (i INT, a INT, b INT AUTOINCREMENT, PRIMARY KEY(a, i));
+ ]], {
+ -- <autoinc-11.6>
+ 1, "Can't create or modify index 'pk_unnamed_T11_6_1' in space 'T11_6': sequence field must be a part of the index"
+ -- </autoinc-11.6>
+ })
+
+test:do_catchsql_test(
+ "autoinc-11.7",
+ [[
+ CREATE TABLE t11_7 (i INT AUTOINCREMENT, a INT AUTOINCREMENT, PRIMARY KEY(a, i));
+ ]], {
+ -- <autoinc-11.7>
+ 1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field"
+ -- </autoinc-11.7>
+ })
+
+test:do_catchsql_test(
+ "autoinc-11.8",
+ [[
+ CREATE TABLE t11_8 (i INT, a INT, PRIMARY KEY(a AUTOINCREMENT, i AUTOINCREMENT));
+ ]], {
+ -- <autoinc-11.8>
+ 1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field"
+ -- </autoinc-11.8>
+ })
+
test:finish_test()
diff --git a/test/sql-tap/sql-errors.test.lua b/test/sql-tap/sql-errors.test.lua
index 05788e1..cb89524 100755
--- a/test/sql-tap/sql-errors.test.lua
+++ b/test/sql-tap/sql-errors.test.lua
@@ -62,7 +62,7 @@ test:do_catchsql_test(
CREATE TABLE t5 (i TEXT PRIMARY KEY AUTOINCREMENT);
]], {
-- <sql-errors-1.5>
- 1,"Failed to create space 'T5': AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY or INT PRIMARY KEY"
+ 1,"Can't create or modify index 'pk_unnamed_T5_1' in space 'T5': sequence cannot be used with a non-integer key"
-- </sql-errors-1.5>
})
More information about the Tarantool-patches
mailing list