* [Tarantool-patches] [PATCH] sql: support column addition
@ 2020-01-31 15:05 Roman Khabibov
2020-03-09 15:08 ` Roman Khabibov
2020-03-14 17:17 ` Vladislav Shpilevoy
0 siblings, 2 replies; 5+ messages in thread
From: Roman Khabibov @ 2020-01-31 15:05 UTC (permalink / raw)
To: tarantool-patches
Enable to add column to existing space with
<ALTER TABLE ADD [COLUMN]> statement. Column definition can be
supplemented with <DEFAULT>, <COLLATE> clauses and <[NOT] NULL>
column constraint.
Closes #3075
@TarantoolBot document
Title: Add columns to existing tables in SQL
Now, it is possible to add columns to existing empty spaces using
<ALTER TABLE table_name ADD [COLUMN] column_name column_type ...>
statement. The column definition is the same as in <CREATE TABLE>
statement, except that table constraints (PRIMARY KEY, UNIQUE,
REFERENCES, CHECK) cannot be specified yet.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY
);]])
---
- row_count: 1
...
tarantool> box.execute([[ALTER TABLE test ADD COLUMN
b TEXT
NOT NULL
DEFAULT ('a')
COLLATE "unicode_ci"
;]])
---
- row_count: 0
...
---
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3075-add-column
Issue: https://github.com/tarantool/tarantool/issues/3075
P.S.: I didn't implement column constraints in ADD COLUMN statement,
because of the sql_create_index(), sql_create_foreign_key() and
sql_create_check_contraint() functions. These functions have
branching inside:
1) CREATE TABLE (parse->create_table_def.new_space != NULL)
We work with the ephemeral space. Use its def, indexes array,
ck and fk rlists for checks, opcode emitting, etc and modify it on
parsing level.
2) ALTER TABLE (parse->create_table_def.new_space == NULL)
We work with existing space = space_by_name(), but don't modify it.
This space will be modified after parsing by alter.cc triggers.
In the case of
ALTER TABLE test ADD a INT PRIMARY KEY CONSTRAINT c UNIQUE CONSTRAINT d UNIQUE ...
we need to modifiy the existing space, for example UNIQUE constraint
creation/opcode emitting requires new space def with new modified
fields array and new index_id_max. The same is for REFERENCES
constraint. With CHECK it seems easier, but I didn't implement it,
because I want to do patch in the futue: "Enable constraints in ADD COLUMN",
if it will be needed.
There is obvious way to implement constraints parsing during
ADD COLUMN. To do partial copy of space = space_by_name() on region,
to keep it in parse->create_column_>def and to work with it. Then
we will have to change the current code a bit, but it is necessary
to write API to create an ephemeral space based on the existing.
I would like to hear your opinion. Maybe I'm generally wrong in
thinking about all this, and it could be made easier.
P.P.S.: Could you, please, help me with COLUMN keyword support.
I couldn't add it as I wanted, i.e., the line in parse.y near the
line 1754:
column_name(N) ::= COLUMN nm(A). { N = A; }
because I had:
/Users/r.khabibov/tarantool/src/box/sql/parse.h:171:9: error: 'TK_COLUMN' macro redefined
#define TK_COLUMN
I don't understand why it is so? For example, the definition of
CONSTRAINT keyword in mkkeywordhash.c is the same, but there is no
error about this #define during compilation.
{ "COLUMN", "TK_COLUMN", true },
{ "CONSTRAINT", "TK_CONSTRAINT", true },
| 6 +-
src/box/errcode.h | 1 +
src/box/sql.c | 28 +++++---
src/box/sql/alter.c | 125 ++++++++++++++++++++++++++++++++++++
src/box/sql/build.c | 106 ++++++++++++++++++------------
src/box/sql/parse.y | 59 +++++++++++++++--
src/box/sql/parse_def.h | 23 +++++++
src/box/sql/sqlInt.h | 40 ++++++++++++
src/box/sql/tarantoolInt.h | 6 +-
test/box/misc.result | 1 +
test/sql-tap/alter.test.lua | 76 +++++++++++++++++++++-
11 files changed, 405 insertions(+), 66 deletions(-)
--git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 006285622..51c8cbb18 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -75,11 +75,7 @@ static Keyword aKeywordTable[] = {
{ "CAST", "TK_CAST", false },
{ "CHECK", "TK_CHECK", true },
{ "COLLATE", "TK_COLLATE", true },
- /* gh-3075: Reserved until ALTER ADD COLUMN is implemeneted.
- * Move it back to ALTER when done.
- */
- /* { "COLUMN", "TK_COLUMNKW", true }, */
- { "COLUMN", "TK_STANDARD", true },
+ { "COLUMN", "TK_COLUMN", true },
{ "COMMIT", "TK_COMMIT", true },
{ "CONFLICT", "TK_CONFLICT", false },
{ "CONSTRAINT", "TK_CONSTRAINT", true },
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 3065b1948..e076edf02 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -264,6 +264,7 @@ struct errcode_record {
/*209 */_(ER_SESSION_SETTING_INVALID_VALUE, "Session setting %s expected a value of type %s") \
/*210 */_(ER_SQL_PREPARE, "Failed to prepare SQL statement: %s") \
/*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with id %u does not exist") \
+ /*212 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW, "Can't add column '%s' to the view '%s'") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df856..7157cabe6 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -923,7 +923,8 @@ cursor_advance(BtCursor *pCur, int *pRes)
*/
char *
-sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
+sql_encode_table_format(struct region *region, struct field_def *fields,
+ uint32_t field_count, uint32_t *size)
{
size_t used = region_used(region);
struct mpstream stream;
@@ -931,12 +932,11 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
set_encode_error, &is_error);
- assert(def != NULL);
- uint32_t field_count = def->field_count;
+ assert(fields != NULL);
mpstream_encode_array(&stream, field_count);
for (uint32_t i = 0; i < field_count && !is_error; i++) {
- uint32_t cid = def->fields[i].coll_id;
- struct field_def *field = &def->fields[i];
+ struct field_def *field = &fields[i];
+ uint32_t cid = field->coll_id;
const char *default_str = field->default_value;
int base_len = 4;
if (cid != COLL_NONE)
@@ -947,16 +947,16 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
mpstream_encode_str(&stream, "name");
mpstream_encode_str(&stream, field->name);
mpstream_encode_str(&stream, "type");
- assert(def->fields[i].is_nullable ==
- action_is_nullable(def->fields[i].nullable_action));
+ assert(field->is_nullable ==
+ action_is_nullable(field->nullable_action));
mpstream_encode_str(&stream, field_type_strs[field->type]);
mpstream_encode_str(&stream, "is_nullable");
- mpstream_encode_bool(&stream, def->fields[i].is_nullable);
+ mpstream_encode_bool(&stream, field->is_nullable);
mpstream_encode_str(&stream, "nullable_action");
- assert(def->fields[i].nullable_action < on_conflict_action_MAX);
+ assert(field->nullable_action < on_conflict_action_MAX);
const char *action =
- on_conflict_action_strs[def->fields[i].nullable_action];
+ on_conflict_action_strs[field->nullable_action];
mpstream_encode_str(&stream, action);
if (cid != COLL_NONE) {
mpstream_encode_str(&stream, "collation");
@@ -980,6 +980,14 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
return raw;
}
+char *
+sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
+{
+ assert(def != NULL);
+ return sql_encode_table_format(region, def->fields, def->field_count,
+ size);
+}
+
char *
sql_encode_table_opts(struct region *region, struct space_def *def,
uint32_t *size)
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 973b420cf..c12d9373a 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -36,6 +36,7 @@
#include "sqlInt.h"
#include "box/box.h"
#include "box/schema.h"
+#include "tarantoolInt.h"
void
sql_alter_table_rename(struct Parse *parse)
@@ -209,3 +210,127 @@ rename_trigger(sql *db, char const *sql_stmt, char const *table_name,
table_name, tname.z + tname.n);
return new_sql_stmt;
}
+
+void
+sql_alter_add_column_start(struct Parse *parse)
+{
+ struct create_column_def *create_column_def = &parse->create_column_def;
+ struct alter_entity_def *alter_entity_def =
+ &create_column_def->base.base;
+ assert(alter_entity_def->entity_type == ENTITY_TYPE_COLUMN);
+ assert(alter_entity_def->alter_action == ALTER_ACTION_CREATE);
+
+ const char *space_name =
+ alter_entity_def->entity_name->a[0].zName;
+ struct space *space = space_by_name(space_name);
+ struct sql *db = parse->db;
+ if (space == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
+ goto tnt_error;
+ }
+
+ struct space_def *def = space->def;
+ create_column_def->space_def = def;
+ if (sql_space_def_check_format(def) != 0)
+ goto tnt_error;
+ uint32_t new_field_count = def->field_count + 1;
+ create_column_def->new_field_count = new_field_count;
+
+#if SQL_MAX_COLUMN
+ if ((int)new_field_count > db->aLimit[SQL_LIMIT_COLUMN]) {
+ diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, def->name,
+ new_field_count, db->aLimit[SQL_LIMIT_COLUMN]);
+ goto tnt_error;
+ }
+#endif
+ /*
+ * Create new fields array and add the new column's
+ * field_def to its end. During copying don't make
+ * duplicates of the non-scalar fields (name,
+ * default_value, default_value_expr), because they will
+ * be just copied and encoded to the msgpack array on sql
+ * allocator in sql_alter_add_column_end().
+ */
+ uint32_t size = sizeof(struct field_def) * new_field_count;
+ struct region *region = &parse->region;
+ struct field_def *new_fields = region_alloc(region, size);
+ if (new_fields == NULL) {
+ diag_set(OutOfMemory, size, "region_alloc", "new_fields");
+ goto tnt_error;
+ }
+ memcpy(new_fields, def->fields,
+ sizeof(struct field_def) * def->field_count);
+ create_column_def->new_fields = new_fields;
+ struct field_def *new_column_def = &new_fields[def->field_count];
+
+ memcpy(new_column_def, &field_def_default, sizeof(struct field_def));
+ struct Token *name = &create_column_def->base.name;
+ new_column_def->name =
+ sql_normalized_name_region_new(region, name->z, name->n);
+ if (new_column_def->name == NULL)
+ goto tnt_error;
+ if (def->opts.is_view) {
+ diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW,
+ new_column_def->name, def->name);
+ goto tnt_error;
+ }
+
+ new_column_def->type = create_column_def->type_def->type;
+
+exit_alter_add_column_start:
+ sqlSrcListDelete(db, alter_entity_def->entity_name);
+ return;
+tnt_error:
+ parse->is_aborted = true;
+ goto exit_alter_add_column_start;
+}
+
+void
+sql_alter_add_column_end(struct Parse *parse)
+{
+ struct create_column_def *create_column_def = &parse->create_column_def;
+ uint32_t new_field_count = create_column_def->new_field_count;
+ uint32_t table_stmt_sz = 0;
+ char *table_stmt =
+ sql_encode_table_format(&parse->region,
+ create_column_def->new_fields,
+ new_field_count, &table_stmt_sz);
+ if (table_stmt == NULL) {
+ parse->is_aborted = true;
+ return;
+ }
+ char *raw = sqlDbMallocRaw(parse->db, table_stmt_sz);
+ if (raw == NULL){
+ parse->is_aborted = true;
+ return;
+ }
+ memcpy(raw, table_stmt, table_stmt_sz);
+
+ struct space *system_space = space_by_id(BOX_SPACE_ID);
+ assert(system_space != NULL);
+ int cursor = parse->nTab++;
+ struct Vdbe *v = sqlGetVdbe(parse);
+ assert(v != NULL);
+ vdbe_emit_open_cursor(parse, cursor, 0, system_space);
+ sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
+
+ int key_reg = sqlGetTempReg(parse);
+ sqlVdbeAddOp2(v, OP_Integer, create_column_def->space_def->id, key_reg);
+ int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 1);
+ sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
+ sqlVdbeJumpHere(v, addr);
+
+ const int system_space_field_count = 7;
+ int tuple_reg = sqlGetTempRange(parse, system_space_field_count + 1);
+ for (int i = 0; i < system_space_field_count - 1; ++i)
+ sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
+ sqlVdbeAddOp1(v, OP_Close, cursor);
+
+ sqlVdbeAddOp2(v, OP_Integer, new_field_count, tuple_reg + 4);
+ sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6,
+ SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC);
+ sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, system_space_field_count,
+ tuple_reg + system_space_field_count);
+ sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + system_space_field_count, 0,
+ 0, (char *)system_space, P4_SPACEPTR);
+}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index bc50ecbfa..2f51b55df 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -368,11 +368,21 @@ void
sql_column_add_nullable_action(struct Parse *parser,
enum on_conflict_action nullable_action)
{
- struct space *space = parser->create_table_def.new_space;
- if (space == NULL || NEVER(space->def->field_count < 1))
+ struct space_def *def = NULL;
+ struct field_def *field = NULL;
+ if (parser->create_table_def.new_space != NULL) {
+ def = parser->create_table_def.new_space->def;
+ if (NEVER(def->field_count < 1))
+ return;
+ field = &def->fields[def->field_count - 1];
+ } else if (parser->create_column_def.space_def != NULL) {
+ def = parser->create_column_def.space_def;
+ uint32_t field_count =
+ parser->create_column_def.new_field_count;
+ field = &parser->create_column_def.new_fields[field_count - 1];
+ } else {
return;
- struct space_def *def = space->def;
- struct field_def *field = &def->fields[def->field_count - 1];
+ }
if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
nullable_action != field->nullable_action) {
/* Prevent defining nullable_action many times. */
@@ -390,51 +400,53 @@ sql_column_add_nullable_action(struct Parse *parser,
}
/*
- * The expression is the default value for the most recently added column
- * of the table currently under construction.
+ * The expression is the default value for the most recently added
+ * column.
*
* Default value expressions must be constant. Raise an exception if this
* is not the case.
*
* This routine is called by the parser while in the middle of
- * parsing a CREATE TABLE statement.
+ * parsing a <CREATE TABLE> or <ALTER TABLE ADD COLUMN> statement.
*/
void
sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan)
{
sql *db = pParse->db;
- struct space *p = pParse->create_table_def.new_space;
- if (p != NULL) {
- assert(p->def->opts.is_ephemeral);
- struct space_def *def = p->def;
- if (!sqlExprIsConstantOrFunction
- (pSpan->pExpr, db->init.busy)) {
- const char *column_name =
- def->fields[def->field_count - 1].name;
- diag_set(ClientError, ER_CREATE_SPACE, def->name,
- tt_sprintf("default value of column '%s' is "\
- "not constant", column_name));
+ struct space_def *def = NULL;
+ struct field_def *field = NULL;
+ if (pParse->create_table_def.new_space != NULL) {
+ def = pParse->create_table_def.new_space->def;
+ assert(def->opts.is_ephemeral);
+ field = &def->fields[def->field_count - 1];
+ } else if (pParse->create_column_def.space_def != NULL) {
+ def = pParse->create_column_def.space_def;
+ uint32_t field_count =
+ pParse->create_column_def.new_field_count;
+ field = &pParse->create_column_def.new_fields[field_count - 1];
+ } else {
+ goto add_default_value_exit;
+ }
+ if (!sqlExprIsConstantOrFunction(pSpan->pExpr, db->init.busy)) {
+ diag_set(ClientError, ER_CREATE_SPACE, def->name,
+ tt_sprintf("default value of column '%s' is not "
+ "constant", field->name));
+ pParse->is_aborted = true;
+ } else {
+ assert(def != NULL);
+ struct region *region = &pParse->region;
+ uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
+ field->default_value = region_alloc(region, default_length + 1);
+ if (field->default_value == NULL) {
+ diag_set(OutOfMemory, default_length + 1,
+ "region_alloc", "field->default_value");
pParse->is_aborted = true;
- } else {
- assert(def != NULL);
- struct field_def *field =
- &def->fields[def->field_count - 1];
- struct region *region = &pParse->region;
- uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
- field->default_value = region_alloc(region,
- default_length + 1);
- if (field->default_value == NULL) {
- diag_set(OutOfMemory, default_length + 1,
- "region_alloc",
- "field->default_value");
- pParse->is_aborted = true;
- return;
- }
- strncpy(field->default_value, pSpan->zStart,
- default_length);
- field->default_value[default_length] = '\0';
+ return;
}
+ strncpy(field->default_value, pSpan->zStart, default_length);
+ field->default_value[default_length] = '\0';
}
+add_default_value_exit:
sql_expr_delete(db, pSpan->pExpr, false);
}
@@ -686,17 +698,27 @@ void
sqlAddCollateType(Parse * pParse, Token * pToken)
{
struct space *space = pParse->create_table_def.new_space;
- if (space == NULL)
+ uint32_t *coll_id = NULL;
+ if (space != NULL) {
+ uint32_t i = space->def->field_count - 1;
+ coll_id = &space->def->fields[i].coll_id;
+ } else if (pParse->create_column_def.space_def != NULL) {
+ uint32_t field_count =
+ pParse->create_column_def.new_field_count;
+ struct field_def *field =
+ &pParse->create_column_def.new_fields[field_count - 1];
+ coll_id = &field->coll_id;
+ } else {
return;
- uint32_t i = space->def->field_count - 1;
+ }
sql *db = pParse->db;
char *coll_name = sql_name_from_token(db, pToken);
if (coll_name == NULL) {
pParse->is_aborted = true;
return;
}
- uint32_t *coll_id = &space->def->fields[i].coll_id;
- if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL) {
+ if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL &&
+ space != NULL) {
/* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
* then an index may have been created on this column before the
* collation type was added. Correct this if it is the case.
@@ -858,7 +880,9 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg,
if (table_opts_stmt == NULL)
goto error;
uint32_t table_stmt_sz = 0;
- char *table_stmt = sql_encode_table(region, space->def, &table_stmt_sz);
+ char *table_stmt = sql_encode_table_format(region, space->def->fields,
+ space->def->field_count,
+ &table_stmt_sz);
if (table_stmt == NULL)
goto error;
char *raw = sqlDbMallocRaw(pParse->db,
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..2eba72ac3 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -289,10 +289,14 @@ carglist ::= .
%type cconsname { struct Token }
cconsname(N) ::= CONSTRAINT nm(X). { N = X; }
cconsname(N) ::= . { N = Token_nil; }
-ccons ::= DEFAULT term(X). {sqlAddDefaultValue(pParse,&X);}
-ccons ::= DEFAULT LP expr(X) RP. {sqlAddDefaultValue(pParse,&X);}
-ccons ::= DEFAULT PLUS term(X). {sqlAddDefaultValue(pParse,&X);}
-ccons ::= DEFAULT MINUS(A) term(X). {
+ccons ::= default_clause_1.
+default_clause_1 ::= DEFAULT term(X). {sqlAddDefaultValue(pParse,&X);}
+ccons ::= default_clause_2.
+default_clause_2 ::= DEFAULT LP expr(X) RP. {sqlAddDefaultValue(pParse,&X);}
+ccons ::= default_clause_3.
+default_clause_3 ::= DEFAULT PLUS term(X). {sqlAddDefaultValue(pParse,&X);}
+ccons ::= default_clause_4.
+default_clause_4 ::= DEFAULT MINUS(A) term(X). {
ExprSpan v;
v.pExpr = sqlPExpr(pParse, TK_UMINUS, X.pExpr, 0);
v.zStart = A.z;
@@ -303,13 +307,18 @@ ccons ::= DEFAULT MINUS(A) term(X). {
// In addition to the type name, we also care about the primary key and
// UNIQUE constraints.
//
-ccons ::= NULL onconf(R). {
+ccons ::= null_cons.
+null_cons ::= NULL onconf(R). {
sql_column_add_nullable_action(pParse, ON_CONFLICT_ACTION_NONE);
/* Trigger nullability mismatch error if required. */
if (R != ON_CONFLICT_ACTION_ABORT)
sql_column_add_nullable_action(pParse, R);
}
-ccons ::= NOT NULL onconf(R). {sql_column_add_nullable_action(pParse, R);}
+ccons ::= not_null_cons.
+not_null_cons ::= NOT NULL onconf(R). {
+ sql_column_add_nullable_action(pParse, R);
+}
+
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);
@@ -335,7 +344,8 @@ ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R).
sql_create_foreign_key(pParse);
}
ccons ::= defer_subclause(D). {fk_constraint_change_defer_mode(pParse, D);}
-ccons ::= COLLATE id(C). {sqlAddCollateType(pParse, &C);}
+ccons ::= collate_clause.
+collate_clause ::= COLLATE id(C). {sqlAddCollateType(pParse, &C);}
// The optional AUTOINCREMENT keyword
%type autoinc {int}
@@ -1729,11 +1739,46 @@ alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; }
%type alter_add_constraint {struct alter_args}
alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). {
+ A.table_name = T;
+ A.name = N;
+ pParse->initiateTTrans = true;
+ }
+
+%type alter_add_column {struct alter_args}
+alter_add_column(A) ::= alter_table_start(T) ADD column_name(N). {
A.table_name = T;
A.name = N;
pParse->initiateTTrans = true;
}
+column_name(N) ::= nm(A). { N = A; }
+
+cmd ::= alter_column_def alter_carglist add_column_end.
+
+alter_column_def ::= alter_add_column(N) typedef(Y). {
+ create_column_def_init(&pParse->create_column_def, N.table_name, &N.name, &Y);
+ sql_alter_add_column_start(pParse);
+}
+
+/*
+ * "alter_carglist" is a list of additional constraints and
+ * clauses that come after the column name and column type in a
+ * ALTER TABLE ADD [COLUMN] statement.
+ */
+alter_carglist ::= alter_carglist alter_ccons.
+alter_carglist ::= .
+alter_ccons ::= default_clause_1.
+alter_ccons ::= default_clause_2.
+alter_ccons ::= default_clause_3.
+alter_ccons ::= default_clause_4.
+alter_ccons ::= null_cons.
+alter_ccons ::= not_null_cons.
+alter_ccons ::= collate_clause.
+
+add_column_end ::= . {
+ sql_alter_add_column_end(pParse);
+}
+
cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES
nm(T) eidlist_opt(TA) matcharg(M) refargs(R) defer_subclause_opt(D). {
create_fk_def_init(&pParse->create_fk_def, N.table_name, &N.name, FA, &T, TA,
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 2f433e4c0..56887efa7 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -154,6 +154,7 @@ enum sql_index_type {
enum entity_type {
ENTITY_TYPE_TABLE = 0,
+ ENTITY_TYPE_COLUMN,
ENTITY_TYPE_VIEW,
ENTITY_TYPE_INDEX,
ENTITY_TYPE_TRIGGER,
@@ -227,6 +228,18 @@ struct create_table_def {
uint32_t autoinc_fieldno;
};
+struct create_column_def {
+ struct create_entity_def base;
+ /** Column type. */
+ struct type_def *type_def;
+ /** Already existing space def. */
+ struct space_def *space_def;
+ /** New format array with the new column. */
+ struct field_def *new_fields;
+ /** Old field count + 1. */
+ uint32_t new_field_count;
+};
+
struct create_view_def {
struct create_entity_def base;
/**
@@ -486,6 +499,16 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
table_def->autoinc_fieldno = 0;
}
+static inline void
+create_column_def_init(struct create_column_def *column_def,
+ struct SrcList *table_name, struct Token *name,
+ struct type_def *type_def)
+{
+ create_entity_def_init(&column_def->base, ENTITY_TYPE_COLUMN,
+ table_name, name, false);
+ column_def->type_def = type_def;
+}
+
static inline void
create_view_def_init(struct create_view_def *view_def, struct Token *name,
struct Token *create, struct ExprList *aliases,
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d1fcf4761..46d3d6d2a 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2231,6 +2231,7 @@ struct Parse {
* from parse.y
*/
union {
+ struct create_column_def create_column_def;
struct create_ck_def create_ck_def;
struct create_fk_def create_fk_def;
struct create_index_def create_index_def;
@@ -2842,6 +2843,18 @@ struct space *
sqlStartTable(Parse *, Token *);
void sqlAddColumn(Parse *, Token *, struct type_def *);
+/**
+ * Set the is_nullable flag on the column @a field.
+ *
+ * @param field Column to set.
+ * @param nullable_action on_conflict_action value.
+ * @param space_name Name of the space to which the column
+ * belongs. Needed to set error.
+ *
+ * @retval 0 Success.
+ * @retval -1 nullable_action has been already set.
+ */
+
/**
* This routine is called by the parser while in the middle of
* parsing a CREATE TABLE statement. A "NOT NULL" constraint has
@@ -2868,6 +2881,19 @@ sqlAddPrimaryKey(struct Parse *parse);
void
sql_create_check_contraint(Parse *parser);
+/**
+ * Add default value to the column @a field.
+ *
+ * @param parse Parsing context.
+ * @param field Column to set.
+ * @param span Default expression.
+ * @param space_name Name of the space to which the column
+ * belongs. Needed to set error.
+ *
+ * @retval 0 Success.
+ * @retval -1 Error.
+ */
+
void sqlAddDefaultValue(Parse *, ExprSpan *);
void sqlAddCollateType(Parse *, Token *);
@@ -4031,6 +4057,20 @@ sql_alter_table_rename(struct Parse *parse);
void
sql_alter_ck_constraint_enable(struct Parse *parse);
+/**
+ * Process the statement: do the checks of space and column
+ * definition. Create new format array with a new column.
+ */
+void
+sql_alter_add_column_start(struct Parse *parse);
+
+/**
+ * Encode new format array and emit code to update an entry in
+ * _space.
+ */
+void
+sql_alter_add_column_end(struct Parse *parse);
+
/**
* Return the length (in bytes) of the token that begins at z[0].
* Store the token type in *type before returning.
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 1ded6c709..5fd433c60 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -118,14 +118,16 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id);
/**
* Encode format as entry to be inserted to _space on @region.
* @param region Region to use.
- * @param def Space definition to encode.
+ * @param fields Space format.
+ * @param field_count Number of fields.
* @param[out] size Size of result allocation.
*
* @retval NULL Error.
* @retval not NULL Pointer to msgpack on success.
*/
char *
-sql_encode_table(struct region *region, struct space_def *def, uint32_t *size);
+sql_encode_table_format(struct region *region, struct field_def *fields,
+ uint32_t field_count, uint32_t *size);
/**
* Encode "opts" dictionary for _space entry on @region.
diff --git a/test/box/misc.result b/test/box/misc.result
index 3ca4e31ac..2aaec6566 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -563,6 +563,7 @@ t;
209: box.error.SESSION_SETTING_INVALID_VALUE
210: box.error.SQL_PREPARE
211: box.error.WRONG_QUERY_ID
+ 212: box.error.SQL_CANT_ADD_COLUMN_TO_VIEW
...
test_run:cmd("setopt delimiter ''");
---
diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
index 615b9d8a6..283dc1759 100755
--- a/test/sql-tap/alter.test.lua
+++ b/test/sql-tap/alter.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(50)
+test:plan(57)
test:do_execsql_test(
"alter-1.1",
@@ -585,4 +585,78 @@ test:do_catchsql_test(
-- -- </alter-7.11>
-- })
+--
+-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
+--
+test:do_execsql_test(
+ "alter-9.1",
+ [[
+ CREATE TABLE test (a INTEGER PRIMARY KEY);
+ ALTER TABLE test ADD b INTEGER;
+ ]], {
+ -- <alter-9.1>
+ -- </alter-9.1>
+ })
+
+test:do_catchsql_test(
+ "alter-9.2",
+ [[
+ CREATE VIEW v AS SELECT * FROM test;
+ ALTER TABLE v ADD b INTEGER;
+ ]], {
+ -- <alter-9.2>
+ 1,"Can't add column 'B' to the view 'V'"
+ -- </alter-9.2>
+ })
+
+test:do_execsql_test(
+ "alter-9.3",
+ [[
+ ALTER TABLE test ADD c TEXT NOT NULL DEFAULT ('a') COLLATE "unicode_ci";
+ ]], {
+ -- <alter-9.3>
+ -- </alter-9.3>
+ })
+
+test:do_execsql_test(
+ "alter-9.4",
+ [[
+ INSERT INTO test(a, b) VALUES (1, 1);
+ SELECT * FROM test;
+ ]], {
+ -- <alter-9.4>
+ 1,1,"a"
+ -- </alter-9.4>
+ })
+
+test:do_catchsql_test(
+ "alter-9.5",
+ [[
+ INSERT INTO test VALUES (2, 2, NULL);
+ ]], {
+ -- <alter-9.5>
+ 1,"Failed to execute SQL statement: NOT NULL constraint failed: TEST.C"
+ -- </alter-9.5>
+ })
+
+test:do_execsql_test(
+ "alter-9.6",
+ [[
+ SELECT * FROM test WHERE c LIKE 'A';
+ ]], {
+ -- <alter-9.6>
+ 1,1,"a"
+ -- </alter-9.6>
+ })
+
+test:do_catchsql_test(
+ "alter-9.7",
+ [[
+ ALTER TABLE test ADD d INTEGER;
+ ]], {
+ -- <alter-9.7>
+ 1,"Tuple field count 3 does not match space field count 4"
+ -- </alter-9.7>
+ })
+
test:finish_test()
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: support column addition
2020-01-31 15:05 [Tarantool-patches] [PATCH] sql: support column addition Roman Khabibov
@ 2020-03-09 15:08 ` Roman Khabibov
2020-03-09 22:16 ` Vladislav Shpilevoy
2020-03-14 17:17 ` Vladislav Shpilevoy
1 sibling, 1 reply; 5+ messages in thread
From: Roman Khabibov @ 2020-03-09 15:08 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
Vlad, can you take it, please?
> On Jan 31, 2020, at 18:05, Roman Khabibov <roman.habibov@tarantool.org> wrote:
>
> Enable to add column to existing space with
> <ALTER TABLE ADD [COLUMN]> statement. Column definition can be
> supplemented with <DEFAULT>, <COLLATE> clauses and <[NOT] NULL>
> column constraint.
>
> Closes #3075
>
> @TarantoolBot document
> Title: Add columns to existing tables in SQL
> Now, it is possible to add columns to existing empty spaces using
> <ALTER TABLE table_name ADD [COLUMN] column_name column_type ...>
> statement. The column definition is the same as in <CREATE TABLE>
> statement, except that table constraints (PRIMARY KEY, UNIQUE,
> REFERENCES, CHECK) cannot be specified yet.
>
> For example:
>
> tarantool> box.execute([[CREATE TABLE test (
> a INTEGER PRIMARY KEY
> );]])
> ---
> - row_count: 1
> ...
>
> tarantool> box.execute([[ALTER TABLE test ADD COLUMN
> b TEXT
> NOT NULL
> DEFAULT ('a')
> COLLATE "unicode_ci"
> ;]])
> ---
> - row_count: 0
> ...
> ---
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3075-add-column
> Issue: https://github.com/tarantool/tarantool/issues/3075
>
> P.S.: I didn't implement column constraints in ADD COLUMN statement,
> because of the sql_create_index(), sql_create_foreign_key() and
> sql_create_check_contraint() functions. These functions have
> branching inside:
>
> 1) CREATE TABLE (parse->create_table_def.new_space != NULL)
> We work with the ephemeral space. Use its def, indexes array,
> ck and fk rlists for checks, opcode emitting, etc and modify it on
> parsing level.
>
> 2) ALTER TABLE (parse->create_table_def.new_space == NULL)
> We work with existing space = space_by_name(), but don't modify it.
> This space will be modified after parsing by alter.cc triggers.
>
> In the case of
> ALTER TABLE test ADD a INT PRIMARY KEY CONSTRAINT c UNIQUE CONSTRAINT d UNIQUE ...
> we need to modifiy the existing space, for example UNIQUE constraint
> creation/opcode emitting requires new space def with new modified
> fields array and new index_id_max. The same is for REFERENCES
> constraint. With CHECK it seems easier, but I didn't implement it,
> because I want to do patch in the futue: "Enable constraints in ADD COLUMN",
> if it will be needed.
>
> There is obvious way to implement constraints parsing during
> ADD COLUMN. To do partial copy of space = space_by_name() on region,
> to keep it in parse->create_column_>def and to work with it. Then
> we will have to change the current code a bit, but it is necessary
> to write API to create an ephemeral space based on the existing.
>
> I would like to hear your opinion. Maybe I'm generally wrong in
> thinking about all this, and it could be made easier.
>
> P.P.S.: Could you, please, help me with COLUMN keyword support.
> I couldn't add it as I wanted, i.e., the line in parse.y near the
> line 1754:
> column_name(N) ::= COLUMN nm(A). { N = A; }
> because I had:
> /Users/r.khabibov/tarantool/src/box/sql/parse.h:171:9: error: 'TK_COLUMN' macro redefined
> #define TK_COLUMN
>
> I don't understand why it is so? For example, the definition of
> CONSTRAINT keyword in mkkeywordhash.c is the same, but there is no
> error about this #define during compilation.
>
> { "COLUMN", "TK_COLUMN", true },
> { "CONSTRAINT", "TK_CONSTRAINT", true },
>
> extra/mkkeywordhash.c | 6 +-
> src/box/errcode.h | 1 +
> src/box/sql.c | 28 +++++---
> src/box/sql/alter.c | 125 ++++++++++++++++++++++++++++++++++++
> src/box/sql/build.c | 106 ++++++++++++++++++------------
> src/box/sql/parse.y | 59 +++++++++++++++--
> src/box/sql/parse_def.h | 23 +++++++
> src/box/sql/sqlInt.h | 40 ++++++++++++
> src/box/sql/tarantoolInt.h | 6 +-
> test/box/misc.result | 1 +
> test/sql-tap/alter.test.lua | 76 +++++++++++++++++++++-
> 11 files changed, 405 insertions(+), 66 deletions(-)
>
> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 006285622..51c8cbb18 100644
> --- a/extra/mkkeywordhash.c
> +++ b/extra/mkkeywordhash.c
> @@ -75,11 +75,7 @@ static Keyword aKeywordTable[] = {
> { "CAST", "TK_CAST", false },
> { "CHECK", "TK_CHECK", true },
> { "COLLATE", "TK_COLLATE", true },
> - /* gh-3075: Reserved until ALTER ADD COLUMN is implemeneted.
> - * Move it back to ALTER when done.
> - */
> - /* { "COLUMN", "TK_COLUMNKW", true }, */
> - { "COLUMN", "TK_STANDARD", true },
> + { "COLUMN", "TK_COLUMN", true },
> { "COMMIT", "TK_COMMIT", true },
> { "CONFLICT", "TK_CONFLICT", false },
> { "CONSTRAINT", "TK_CONSTRAINT", true },
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index 3065b1948..e076edf02 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -264,6 +264,7 @@ struct errcode_record {
> /*209 */_(ER_SESSION_SETTING_INVALID_VALUE, "Session setting %s expected a value of type %s") \
> /*210 */_(ER_SQL_PREPARE, "Failed to prepare SQL statement: %s") \
> /*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with id %u does not exist") \
> + /*212 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW, "Can't add column '%s' to the view '%s'") \
>
> /*
> * !IMPORTANT! Please follow instructions at start of the file
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 1256df856..7157cabe6 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -923,7 +923,8 @@ cursor_advance(BtCursor *pCur, int *pRes)
> */
>
> char *
> -sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
> +sql_encode_table_format(struct region *region, struct field_def *fields,
> + uint32_t field_count, uint32_t *size)
> {
> size_t used = region_used(region);
> struct mpstream stream;
> @@ -931,12 +932,11 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
> mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> set_encode_error, &is_error);
>
> - assert(def != NULL);
> - uint32_t field_count = def->field_count;
> + assert(fields != NULL);
> mpstream_encode_array(&stream, field_count);
> for (uint32_t i = 0; i < field_count && !is_error; i++) {
> - uint32_t cid = def->fields[i].coll_id;
> - struct field_def *field = &def->fields[i];
> + struct field_def *field = &fields[i];
> + uint32_t cid = field->coll_id;
> const char *default_str = field->default_value;
> int base_len = 4;
> if (cid != COLL_NONE)
> @@ -947,16 +947,16 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
> mpstream_encode_str(&stream, "name");
> mpstream_encode_str(&stream, field->name);
> mpstream_encode_str(&stream, "type");
> - assert(def->fields[i].is_nullable ==
> - action_is_nullable(def->fields[i].nullable_action));
> + assert(field->is_nullable ==
> + action_is_nullable(field->nullable_action));
> mpstream_encode_str(&stream, field_type_strs[field->type]);
> mpstream_encode_str(&stream, "is_nullable");
> - mpstream_encode_bool(&stream, def->fields[i].is_nullable);
> + mpstream_encode_bool(&stream, field->is_nullable);
> mpstream_encode_str(&stream, "nullable_action");
>
> - assert(def->fields[i].nullable_action < on_conflict_action_MAX);
> + assert(field->nullable_action < on_conflict_action_MAX);
> const char *action =
> - on_conflict_action_strs[def->fields[i].nullable_action];
> + on_conflict_action_strs[field->nullable_action];
> mpstream_encode_str(&stream, action);
> if (cid != COLL_NONE) {
> mpstream_encode_str(&stream, "collation");
> @@ -980,6 +980,14 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
> return raw;
> }
>
> +char *
> +sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
> +{
> + assert(def != NULL);
> + return sql_encode_table_format(region, def->fields, def->field_count,
> + size);
> +}
> +
> char *
> sql_encode_table_opts(struct region *region, struct space_def *def,
> uint32_t *size)
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 973b420cf..c12d9373a 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -36,6 +36,7 @@
> #include "sqlInt.h"
> #include "box/box.h"
> #include "box/schema.h"
> +#include "tarantoolInt.h"
>
> void
> sql_alter_table_rename(struct Parse *parse)
> @@ -209,3 +210,127 @@ rename_trigger(sql *db, char const *sql_stmt, char const *table_name,
> table_name, tname.z + tname.n);
> return new_sql_stmt;
> }
> +
> +void
> +sql_alter_add_column_start(struct Parse *parse)
> +{
> + struct create_column_def *create_column_def = &parse->create_column_def;
> + struct alter_entity_def *alter_entity_def =
> + &create_column_def->base.base;
> + assert(alter_entity_def->entity_type == ENTITY_TYPE_COLUMN);
> + assert(alter_entity_def->alter_action == ALTER_ACTION_CREATE);
> +
> + const char *space_name =
> + alter_entity_def->entity_name->a[0].zName;
> + struct space *space = space_by_name(space_name);
> + struct sql *db = parse->db;
> + if (space == NULL) {
> + diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
> + goto tnt_error;
> + }
> +
> + struct space_def *def = space->def;
> + create_column_def->space_def = def;
> + if (sql_space_def_check_format(def) != 0)
> + goto tnt_error;
> + uint32_t new_field_count = def->field_count + 1;
> + create_column_def->new_field_count = new_field_count;
> +
> +#if SQL_MAX_COLUMN
> + if ((int)new_field_count > db->aLimit[SQL_LIMIT_COLUMN]) {
> + diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, def->name,
> + new_field_count, db->aLimit[SQL_LIMIT_COLUMN]);
> + goto tnt_error;
> + }
> +#endif
> + /*
> + * Create new fields array and add the new column's
> + * field_def to its end. During copying don't make
> + * duplicates of the non-scalar fields (name,
> + * default_value, default_value_expr), because they will
> + * be just copied and encoded to the msgpack array on sql
> + * allocator in sql_alter_add_column_end().
> + */
> + uint32_t size = sizeof(struct field_def) * new_field_count;
> + struct region *region = &parse->region;
> + struct field_def *new_fields = region_alloc(region, size);
> + if (new_fields == NULL) {
> + diag_set(OutOfMemory, size, "region_alloc", "new_fields");
> + goto tnt_error;
> + }
> + memcpy(new_fields, def->fields,
> + sizeof(struct field_def) * def->field_count);
> + create_column_def->new_fields = new_fields;
> + struct field_def *new_column_def = &new_fields[def->field_count];
> +
> + memcpy(new_column_def, &field_def_default, sizeof(struct field_def));
> + struct Token *name = &create_column_def->base.name;
> + new_column_def->name =
> + sql_normalized_name_region_new(region, name->z, name->n);
> + if (new_column_def->name == NULL)
> + goto tnt_error;
> + if (def->opts.is_view) {
> + diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW,
> + new_column_def->name, def->name);
> + goto tnt_error;
> + }
> +
> + new_column_def->type = create_column_def->type_def->type;
> +
> +exit_alter_add_column_start:
> + sqlSrcListDelete(db, alter_entity_def->entity_name);
> + return;
> +tnt_error:
> + parse->is_aborted = true;
> + goto exit_alter_add_column_start;
> +}
> +
> +void
> +sql_alter_add_column_end(struct Parse *parse)
> +{
> + struct create_column_def *create_column_def = &parse->create_column_def;
> + uint32_t new_field_count = create_column_def->new_field_count;
> + uint32_t table_stmt_sz = 0;
> + char *table_stmt =
> + sql_encode_table_format(&parse->region,
> + create_column_def->new_fields,
> + new_field_count, &table_stmt_sz);
> + if (table_stmt == NULL) {
> + parse->is_aborted = true;
> + return;
> + }
> + char *raw = sqlDbMallocRaw(parse->db, table_stmt_sz);
> + if (raw == NULL){
> + parse->is_aborted = true;
> + return;
> + }
> + memcpy(raw, table_stmt, table_stmt_sz);
> +
> + struct space *system_space = space_by_id(BOX_SPACE_ID);
> + assert(system_space != NULL);
> + int cursor = parse->nTab++;
> + struct Vdbe *v = sqlGetVdbe(parse);
> + assert(v != NULL);
> + vdbe_emit_open_cursor(parse, cursor, 0, system_space);
> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
> +
> + int key_reg = sqlGetTempReg(parse);
> + sqlVdbeAddOp2(v, OP_Integer, create_column_def->space_def->id, key_reg);
> + int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 1);
> + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
> + sqlVdbeJumpHere(v, addr);
> +
> + const int system_space_field_count = 7;
> + int tuple_reg = sqlGetTempRange(parse, system_space_field_count + 1);
> + for (int i = 0; i < system_space_field_count - 1; ++i)
> + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
> + sqlVdbeAddOp1(v, OP_Close, cursor);
> +
> + sqlVdbeAddOp2(v, OP_Integer, new_field_count, tuple_reg + 4);
> + sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6,
> + SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC);
> + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, system_space_field_count,
> + tuple_reg + system_space_field_count);
> + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + system_space_field_count, 0,
> + 0, (char *)system_space, P4_SPACEPTR);
> +}
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index bc50ecbfa..2f51b55df 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -368,11 +368,21 @@ void
> sql_column_add_nullable_action(struct Parse *parser,
> enum on_conflict_action nullable_action)
> {
> - struct space *space = parser->create_table_def.new_space;
> - if (space == NULL || NEVER(space->def->field_count < 1))
> + struct space_def *def = NULL;
> + struct field_def *field = NULL;
> + if (parser->create_table_def.new_space != NULL) {
> + def = parser->create_table_def.new_space->def;
> + if (NEVER(def->field_count < 1))
> + return;
> + field = &def->fields[def->field_count - 1];
> + } else if (parser->create_column_def.space_def != NULL) {
> + def = parser->create_column_def.space_def;
> + uint32_t field_count =
> + parser->create_column_def.new_field_count;
> + field = &parser->create_column_def.new_fields[field_count - 1];
> + } else {
> return;
> - struct space_def *def = space->def;
> - struct field_def *field = &def->fields[def->field_count - 1];
> + }
> if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
> nullable_action != field->nullable_action) {
> /* Prevent defining nullable_action many times. */
> @@ -390,51 +400,53 @@ sql_column_add_nullable_action(struct Parse *parser,
> }
>
> /*
> - * The expression is the default value for the most recently added column
> - * of the table currently under construction.
> + * The expression is the default value for the most recently added
> + * column.
> *
> * Default value expressions must be constant. Raise an exception if this
> * is not the case.
> *
> * This routine is called by the parser while in the middle of
> - * parsing a CREATE TABLE statement.
> + * parsing a <CREATE TABLE> or <ALTER TABLE ADD COLUMN> statement.
> */
> void
> sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan)
> {
> sql *db = pParse->db;
> - struct space *p = pParse->create_table_def.new_space;
> - if (p != NULL) {
> - assert(p->def->opts.is_ephemeral);
> - struct space_def *def = p->def;
> - if (!sqlExprIsConstantOrFunction
> - (pSpan->pExpr, db->init.busy)) {
> - const char *column_name =
> - def->fields[def->field_count - 1].name;
> - diag_set(ClientError, ER_CREATE_SPACE, def->name,
> - tt_sprintf("default value of column '%s' is "\
> - "not constant", column_name));
> + struct space_def *def = NULL;
> + struct field_def *field = NULL;
> + if (pParse->create_table_def.new_space != NULL) {
> + def = pParse->create_table_def.new_space->def;
> + assert(def->opts.is_ephemeral);
> + field = &def->fields[def->field_count - 1];
> + } else if (pParse->create_column_def.space_def != NULL) {
> + def = pParse->create_column_def.space_def;
> + uint32_t field_count =
> + pParse->create_column_def.new_field_count;
> + field = &pParse->create_column_def.new_fields[field_count - 1];
> + } else {
> + goto add_default_value_exit;
> + }
> + if (!sqlExprIsConstantOrFunction(pSpan->pExpr, db->init.busy)) {
> + diag_set(ClientError, ER_CREATE_SPACE, def->name,
> + tt_sprintf("default value of column '%s' is not "
> + "constant", field->name));
> + pParse->is_aborted = true;
> + } else {
> + assert(def != NULL);
> + struct region *region = &pParse->region;
> + uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
> + field->default_value = region_alloc(region, default_length + 1);
> + if (field->default_value == NULL) {
> + diag_set(OutOfMemory, default_length + 1,
> + "region_alloc", "field->default_value");
> pParse->is_aborted = true;
> - } else {
> - assert(def != NULL);
> - struct field_def *field =
> - &def->fields[def->field_count - 1];
> - struct region *region = &pParse->region;
> - uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
> - field->default_value = region_alloc(region,
> - default_length + 1);
> - if (field->default_value == NULL) {
> - diag_set(OutOfMemory, default_length + 1,
> - "region_alloc",
> - "field->default_value");
> - pParse->is_aborted = true;
> - return;
> - }
> - strncpy(field->default_value, pSpan->zStart,
> - default_length);
> - field->default_value[default_length] = '\0';
> + return;
> }
> + strncpy(field->default_value, pSpan->zStart, default_length);
> + field->default_value[default_length] = '\0';
> }
> +add_default_value_exit:
> sql_expr_delete(db, pSpan->pExpr, false);
> }
>
> @@ -686,17 +698,27 @@ void
> sqlAddCollateType(Parse * pParse, Token * pToken)
> {
> struct space *space = pParse->create_table_def.new_space;
> - if (space == NULL)
> + uint32_t *coll_id = NULL;
> + if (space != NULL) {
> + uint32_t i = space->def->field_count - 1;
> + coll_id = &space->def->fields[i].coll_id;
> + } else if (pParse->create_column_def.space_def != NULL) {
> + uint32_t field_count =
> + pParse->create_column_def.new_field_count;
> + struct field_def *field =
> + &pParse->create_column_def.new_fields[field_count - 1];
> + coll_id = &field->coll_id;
> + } else {
> return;
> - uint32_t i = space->def->field_count - 1;
> + }
> sql *db = pParse->db;
> char *coll_name = sql_name_from_token(db, pToken);
> if (coll_name == NULL) {
> pParse->is_aborted = true;
> return;
> }
> - uint32_t *coll_id = &space->def->fields[i].coll_id;
> - if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL) {
> + if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL &&
> + space != NULL) {
> /* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
> * then an index may have been created on this column before the
> * collation type was added. Correct this if it is the case.
> @@ -858,7 +880,9 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg,
> if (table_opts_stmt == NULL)
> goto error;
> uint32_t table_stmt_sz = 0;
> - char *table_stmt = sql_encode_table(region, space->def, &table_stmt_sz);
> + char *table_stmt = sql_encode_table_format(region, space->def->fields,
> + space->def->field_count,
> + &table_stmt_sz);
> if (table_stmt == NULL)
> goto error;
> char *raw = sqlDbMallocRaw(pParse->db,
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index cfe1c0012..2eba72ac3 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -289,10 +289,14 @@ carglist ::= .
> %type cconsname { struct Token }
> cconsname(N) ::= CONSTRAINT nm(X). { N = X; }
> cconsname(N) ::= . { N = Token_nil; }
> -ccons ::= DEFAULT term(X). {sqlAddDefaultValue(pParse,&X);}
> -ccons ::= DEFAULT LP expr(X) RP. {sqlAddDefaultValue(pParse,&X);}
> -ccons ::= DEFAULT PLUS term(X). {sqlAddDefaultValue(pParse,&X);}
> -ccons ::= DEFAULT MINUS(A) term(X). {
> +ccons ::= default_clause_1.
> +default_clause_1 ::= DEFAULT term(X). {sqlAddDefaultValue(pParse,&X);}
> +ccons ::= default_clause_2.
> +default_clause_2 ::= DEFAULT LP expr(X) RP. {sqlAddDefaultValue(pParse,&X);}
> +ccons ::= default_clause_3.
> +default_clause_3 ::= DEFAULT PLUS term(X). {sqlAddDefaultValue(pParse,&X);}
> +ccons ::= default_clause_4.
> +default_clause_4 ::= DEFAULT MINUS(A) term(X). {
> ExprSpan v;
> v.pExpr = sqlPExpr(pParse, TK_UMINUS, X.pExpr, 0);
> v.zStart = A.z;
> @@ -303,13 +307,18 @@ ccons ::= DEFAULT MINUS(A) term(X). {
> // In addition to the type name, we also care about the primary key and
> // UNIQUE constraints.
> //
> -ccons ::= NULL onconf(R). {
> +ccons ::= null_cons.
> +null_cons ::= NULL onconf(R). {
> sql_column_add_nullable_action(pParse, ON_CONFLICT_ACTION_NONE);
> /* Trigger nullability mismatch error if required. */
> if (R != ON_CONFLICT_ACTION_ABORT)
> sql_column_add_nullable_action(pParse, R);
> }
> -ccons ::= NOT NULL onconf(R). {sql_column_add_nullable_action(pParse, R);}
> +ccons ::= not_null_cons.
> +not_null_cons ::= NOT NULL onconf(R). {
> + sql_column_add_nullable_action(pParse, R);
> +}
> +
> 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);
> @@ -335,7 +344,8 @@ ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R).
> sql_create_foreign_key(pParse);
> }
> ccons ::= defer_subclause(D). {fk_constraint_change_defer_mode(pParse, D);}
> -ccons ::= COLLATE id(C). {sqlAddCollateType(pParse, &C);}
> +ccons ::= collate_clause.
> +collate_clause ::= COLLATE id(C). {sqlAddCollateType(pParse, &C);}
>
> // The optional AUTOINCREMENT keyword
> %type autoinc {int}
> @@ -1729,11 +1739,46 @@ alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; }
>
> %type alter_add_constraint {struct alter_args}
> alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). {
> + A.table_name = T;
> + A.name = N;
> + pParse->initiateTTrans = true;
> + }
> +
> +%type alter_add_column {struct alter_args}
> +alter_add_column(A) ::= alter_table_start(T) ADD column_name(N). {
> A.table_name = T;
> A.name = N;
> pParse->initiateTTrans = true;
> }
>
> +column_name(N) ::= nm(A). { N = A; }
> +
> +cmd ::= alter_column_def alter_carglist add_column_end.
> +
> +alter_column_def ::= alter_add_column(N) typedef(Y). {
> + create_column_def_init(&pParse->create_column_def, N.table_name, &N.name, &Y);
> + sql_alter_add_column_start(pParse);
> +}
> +
> +/*
> + * "alter_carglist" is a list of additional constraints and
> + * clauses that come after the column name and column type in a
> + * ALTER TABLE ADD [COLUMN] statement.
> + */
> +alter_carglist ::= alter_carglist alter_ccons.
> +alter_carglist ::= .
> +alter_ccons ::= default_clause_1.
> +alter_ccons ::= default_clause_2.
> +alter_ccons ::= default_clause_3.
> +alter_ccons ::= default_clause_4.
> +alter_ccons ::= null_cons.
> +alter_ccons ::= not_null_cons.
> +alter_ccons ::= collate_clause.
> +
> +add_column_end ::= . {
> + sql_alter_add_column_end(pParse);
> +}
> +
> cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES
> nm(T) eidlist_opt(TA) matcharg(M) refargs(R) defer_subclause_opt(D). {
> create_fk_def_init(&pParse->create_fk_def, N.table_name, &N.name, FA, &T, TA,
> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
> index 2f433e4c0..56887efa7 100644
> --- a/src/box/sql/parse_def.h
> +++ b/src/box/sql/parse_def.h
> @@ -154,6 +154,7 @@ enum sql_index_type {
>
> enum entity_type {
> ENTITY_TYPE_TABLE = 0,
> + ENTITY_TYPE_COLUMN,
> ENTITY_TYPE_VIEW,
> ENTITY_TYPE_INDEX,
> ENTITY_TYPE_TRIGGER,
> @@ -227,6 +228,18 @@ struct create_table_def {
> uint32_t autoinc_fieldno;
> };
>
> +struct create_column_def {
> + struct create_entity_def base;
> + /** Column type. */
> + struct type_def *type_def;
> + /** Already existing space def. */
> + struct space_def *space_def;
> + /** New format array with the new column. */
> + struct field_def *new_fields;
> + /** Old field count + 1. */
> + uint32_t new_field_count;
> +};
> +
> struct create_view_def {
> struct create_entity_def base;
> /**
> @@ -486,6 +499,16 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
> table_def->autoinc_fieldno = 0;
> }
>
> +static inline void
> +create_column_def_init(struct create_column_def *column_def,
> + struct SrcList *table_name, struct Token *name,
> + struct type_def *type_def)
> +{
> + create_entity_def_init(&column_def->base, ENTITY_TYPE_COLUMN,
> + table_name, name, false);
> + column_def->type_def = type_def;
> +}
> +
> static inline void
> create_view_def_init(struct create_view_def *view_def, struct Token *name,
> struct Token *create, struct ExprList *aliases,
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index d1fcf4761..46d3d6d2a 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -2231,6 +2231,7 @@ struct Parse {
> * from parse.y
> */
> union {
> + struct create_column_def create_column_def;
> struct create_ck_def create_ck_def;
> struct create_fk_def create_fk_def;
> struct create_index_def create_index_def;
> @@ -2842,6 +2843,18 @@ struct space *
> sqlStartTable(Parse *, Token *);
> void sqlAddColumn(Parse *, Token *, struct type_def *);
>
> +/**
> + * Set the is_nullable flag on the column @a field.
> + *
> + * @param field Column to set.
> + * @param nullable_action on_conflict_action value.
> + * @param space_name Name of the space to which the column
> + * belongs. Needed to set error.
> + *
> + * @retval 0 Success.
> + * @retval -1 nullable_action has been already set.
> + */
> +
> /**
> * This routine is called by the parser while in the middle of
> * parsing a CREATE TABLE statement. A "NOT NULL" constraint has
> @@ -2868,6 +2881,19 @@ sqlAddPrimaryKey(struct Parse *parse);
> void
> sql_create_check_contraint(Parse *parser);
>
> +/**
> + * Add default value to the column @a field.
> + *
> + * @param parse Parsing context.
> + * @param field Column to set.
> + * @param span Default expression.
> + * @param space_name Name of the space to which the column
> + * belongs. Needed to set error.
> + *
> + * @retval 0 Success.
> + * @retval -1 Error.
> + */
> +
> void sqlAddDefaultValue(Parse *, ExprSpan *);
> void sqlAddCollateType(Parse *, Token *);
>
> @@ -4031,6 +4057,20 @@ sql_alter_table_rename(struct Parse *parse);
> void
> sql_alter_ck_constraint_enable(struct Parse *parse);
>
> +/**
> + * Process the statement: do the checks of space and column
> + * definition. Create new format array with a new column.
> + */
> +void
> +sql_alter_add_column_start(struct Parse *parse);
> +
> +/**
> + * Encode new format array and emit code to update an entry in
> + * _space.
> + */
> +void
> +sql_alter_add_column_end(struct Parse *parse);
> +
> /**
> * Return the length (in bytes) of the token that begins at z[0].
> * Store the token type in *type before returning.
> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
> index 1ded6c709..5fd433c60 100644
> --- a/src/box/sql/tarantoolInt.h
> +++ b/src/box/sql/tarantoolInt.h
> @@ -118,14 +118,16 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id);
> /**
> * Encode format as entry to be inserted to _space on @region.
> * @param region Region to use.
> - * @param def Space definition to encode.
> + * @param fields Space format.
> + * @param field_count Number of fields.
> * @param[out] size Size of result allocation.
> *
> * @retval NULL Error.
> * @retval not NULL Pointer to msgpack on success.
> */
> char *
> -sql_encode_table(struct region *region, struct space_def *def, uint32_t *size);
> +sql_encode_table_format(struct region *region, struct field_def *fields,
> + uint32_t field_count, uint32_t *size);
>
> /**
> * Encode "opts" dictionary for _space entry on @region.
> diff --git a/test/box/misc.result b/test/box/misc.result
> index 3ca4e31ac..2aaec6566 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -563,6 +563,7 @@ t;
> 209: box.error.SESSION_SETTING_INVALID_VALUE
> 210: box.error.SQL_PREPARE
> 211: box.error.WRONG_QUERY_ID
> + 212: box.error.SQL_CANT_ADD_COLUMN_TO_VIEW
> ...
> test_run:cmd("setopt delimiter ''");
> ---
> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
> index 615b9d8a6..283dc1759 100755
> --- a/test/sql-tap/alter.test.lua
> +++ b/test/sql-tap/alter.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(50)
> +test:plan(57)
>
> test:do_execsql_test(
> "alter-1.1",
> @@ -585,4 +585,78 @@ test:do_catchsql_test(
> -- -- </alter-7.11>
> -- })
>
> +--
> +-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
> +--
> +test:do_execsql_test(
> + "alter-9.1",
> + [[
> + CREATE TABLE test (a INTEGER PRIMARY KEY);
> + ALTER TABLE test ADD b INTEGER;
> + ]], {
> + -- <alter-9.1>
> + -- </alter-9.1>
> + })
> +
> +test:do_catchsql_test(
> + "alter-9.2",
> + [[
> + CREATE VIEW v AS SELECT * FROM test;
> + ALTER TABLE v ADD b INTEGER;
> + ]], {
> + -- <alter-9.2>
> + 1,"Can't add column 'B' to the view 'V'"
> + -- </alter-9.2>
> + })
> +
> +test:do_execsql_test(
> + "alter-9.3",
> + [[
> + ALTER TABLE test ADD c TEXT NOT NULL DEFAULT ('a') COLLATE "unicode_ci";
> + ]], {
> + -- <alter-9.3>
> + -- </alter-9.3>
> + })
> +
> +test:do_execsql_test(
> + "alter-9.4",
> + [[
> + INSERT INTO test(a, b) VALUES (1, 1);
> + SELECT * FROM test;
> + ]], {
> + -- <alter-9.4>
> + 1,1,"a"
> + -- </alter-9.4>
> + })
> +
> +test:do_catchsql_test(
> + "alter-9.5",
> + [[
> + INSERT INTO test VALUES (2, 2, NULL);
> + ]], {
> + -- <alter-9.5>
> + 1,"Failed to execute SQL statement: NOT NULL constraint failed: TEST.C"
> + -- </alter-9.5>
> + })
> +
> +test:do_execsql_test(
> + "alter-9.6",
> + [[
> + SELECT * FROM test WHERE c LIKE 'A';
> + ]], {
> + -- <alter-9.6>
> + 1,1,"a"
> + -- </alter-9.6>
> + })
> +
> +test:do_catchsql_test(
> + "alter-9.7",
> + [[
> + ALTER TABLE test ADD d INTEGER;
> + ]], {
> + -- <alter-9.7>
> + 1,"Tuple field count 3 does not match space field count 4"
> + -- </alter-9.7>
> + })
> +
> test:finish_test()
> --
> 2.21.0 (Apple Git-122)
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: support column addition
2020-03-09 15:08 ` Roman Khabibov
@ 2020-03-09 22:16 ` Vladislav Shpilevoy
0 siblings, 0 replies; 5+ messages in thread
From: Vladislav Shpilevoy @ 2020-03-09 22:16 UTC (permalink / raw)
To: Roman Khabibov, tarantool-patches
Hi! Sure.
On 09/03/2020 16:08, Roman Khabibov wrote:
> Vlad, can you take it, please?
>
>> On Jan 31, 2020, at 18:05, Roman Khabibov <roman.habibov@tarantool.org> wrote:
>>
>> Enable to add column to existing space with
>> <ALTER TABLE ADD [COLUMN]> statement. Column definition can be
>> supplemented with <DEFAULT>, <COLLATE> clauses and <[NOT] NULL>
>> column constraint.
>>
>> Closes #3075
>>
>> @TarantoolBot document
>> Title: Add columns to existing tables in SQL
>> Now, it is possible to add columns to existing empty spaces using
>> <ALTER TABLE table_name ADD [COLUMN] column_name column_type ...>
>> statement. The column definition is the same as in <CREATE TABLE>
>> statement, except that table constraints (PRIMARY KEY, UNIQUE,
>> REFERENCES, CHECK) cannot be specified yet.
>>
>> For example:
>>
>> tarantool> box.execute([[CREATE TABLE test (
>> a INTEGER PRIMARY KEY
>> );]])
>> ---
>> - row_count: 1
>> ...
>>
>> tarantool> box.execute([[ALTER TABLE test ADD COLUMN
>> b TEXT
>> NOT NULL
>> DEFAULT ('a')
>> COLLATE "unicode_ci"
>> ;]])
>> ---
>> - row_count: 0
>> ...
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3075-add-column
>> Issue: https://github.com/tarantool/tarantool/issues/3075
>>
>> P.S.: I didn't implement column constraints in ADD COLUMN statement,
>> because of the sql_create_index(), sql_create_foreign_key() and
>> sql_create_check_contraint() functions. These functions have
>> branching inside:
>>
>> 1) CREATE TABLE (parse->create_table_def.new_space != NULL)
>> We work with the ephemeral space. Use its def, indexes array,
>> ck and fk rlists for checks, opcode emitting, etc and modify it on
>> parsing level.
>>
>> 2) ALTER TABLE (parse->create_table_def.new_space == NULL)
>> We work with existing space = space_by_name(), but don't modify it.
>> This space will be modified after parsing by alter.cc triggers.
>>
>> In the case of
>> ALTER TABLE test ADD a INT PRIMARY KEY CONSTRAINT c UNIQUE CONSTRAINT d UNIQUE ...
>> we need to modifiy the existing space, for example UNIQUE constraint
>> creation/opcode emitting requires new space def with new modified
>> fields array and new index_id_max. The same is for REFERENCES
>> constraint. With CHECK it seems easier, but I didn't implement it,
>> because I want to do patch in the futue: "Enable constraints in ADD COLUMN",
>> if it will be needed.
>>
>> There is obvious way to implement constraints parsing during
>> ADD COLUMN. To do partial copy of space = space_by_name() on region,
>> to keep it in parse->create_column_>def and to work with it. Then
>> we will have to change the current code a bit, but it is necessary
>> to write API to create an ephemeral space based on the existing.
>>
>> I would like to hear your opinion. Maybe I'm generally wrong in
>> thinking about all this, and it could be made easier.
>>
>> P.P.S.: Could you, please, help me with COLUMN keyword support.
>> I couldn't add it as I wanted, i.e., the line in parse.y near the
>> line 1754:
>> column_name(N) ::= COLUMN nm(A). { N = A; }
>> because I had:
>> /Users/r.khabibov/tarantool/src/box/sql/parse.h:171:9: error: 'TK_COLUMN' macro redefined
>> #define TK_COLUMN
>>
>> I don't understand why it is so? For example, the definition of
>> CONSTRAINT keyword in mkkeywordhash.c is the same, but there is no
>> error about this #define during compilation.
>>
>> { "COLUMN", "TK_COLUMN", true },
>> { "CONSTRAINT", "TK_CONSTRAINT", true },
>>
>> extra/mkkeywordhash.c | 6 +-
>> src/box/errcode.h | 1 +
>> src/box/sql.c | 28 +++++---
>> src/box/sql/alter.c | 125 ++++++++++++++++++++++++++++++++++++
>> src/box/sql/build.c | 106 ++++++++++++++++++------------
>> src/box/sql/parse.y | 59 +++++++++++++++--
>> src/box/sql/parse_def.h | 23 +++++++
>> src/box/sql/sqlInt.h | 40 ++++++++++++
>> src/box/sql/tarantoolInt.h | 6 +-
>> test/box/misc.result | 1 +
>> test/sql-tap/alter.test.lua | 76 +++++++++++++++++++++-
>> 11 files changed, 405 insertions(+), 66 deletions(-)
>>
>> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
>> index 006285622..51c8cbb18 100644
>> --- a/extra/mkkeywordhash.c
>> +++ b/extra/mkkeywordhash.c
>> @@ -75,11 +75,7 @@ static Keyword aKeywordTable[] = {
>> { "CAST", "TK_CAST", false },
>> { "CHECK", "TK_CHECK", true },
>> { "COLLATE", "TK_COLLATE", true },
>> - /* gh-3075: Reserved until ALTER ADD COLUMN is implemeneted.
>> - * Move it back to ALTER when done.
>> - */
>> - /* { "COLUMN", "TK_COLUMNKW", true }, */
>> - { "COLUMN", "TK_STANDARD", true },
>> + { "COLUMN", "TK_COLUMN", true },
>> { "COMMIT", "TK_COMMIT", true },
>> { "CONFLICT", "TK_CONFLICT", false },
>> { "CONSTRAINT", "TK_CONSTRAINT", true },
>> diff --git a/src/box/errcode.h b/src/box/errcode.h
>> index 3065b1948..e076edf02 100644
>> --- a/src/box/errcode.h
>> +++ b/src/box/errcode.h
>> @@ -264,6 +264,7 @@ struct errcode_record {
>> /*209 */_(ER_SESSION_SETTING_INVALID_VALUE, "Session setting %s expected a value of type %s") \
>> /*210 */_(ER_SQL_PREPARE, "Failed to prepare SQL statement: %s") \
>> /*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with id %u does not exist") \
>> + /*212 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW, "Can't add column '%s' to the view '%s'") \
>>
>> /*
>> * !IMPORTANT! Please follow instructions at start of the file
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index 1256df856..7157cabe6 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -923,7 +923,8 @@ cursor_advance(BtCursor *pCur, int *pRes)
>> */
>>
>> char *
>> -sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> +sql_encode_table_format(struct region *region, struct field_def *fields,
>> + uint32_t field_count, uint32_t *size)
>> {
>> size_t used = region_used(region);
>> struct mpstream stream;
>> @@ -931,12 +932,11 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
>> set_encode_error, &is_error);
>>
>> - assert(def != NULL);
>> - uint32_t field_count = def->field_count;
>> + assert(fields != NULL);
>> mpstream_encode_array(&stream, field_count);
>> for (uint32_t i = 0; i < field_count && !is_error; i++) {
>> - uint32_t cid = def->fields[i].coll_id;
>> - struct field_def *field = &def->fields[i];
>> + struct field_def *field = &fields[i];
>> + uint32_t cid = field->coll_id;
>> const char *default_str = field->default_value;
>> int base_len = 4;
>> if (cid != COLL_NONE)
>> @@ -947,16 +947,16 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> mpstream_encode_str(&stream, "name");
>> mpstream_encode_str(&stream, field->name);
>> mpstream_encode_str(&stream, "type");
>> - assert(def->fields[i].is_nullable ==
>> - action_is_nullable(def->fields[i].nullable_action));
>> + assert(field->is_nullable ==
>> + action_is_nullable(field->nullable_action));
>> mpstream_encode_str(&stream, field_type_strs[field->type]);
>> mpstream_encode_str(&stream, "is_nullable");
>> - mpstream_encode_bool(&stream, def->fields[i].is_nullable);
>> + mpstream_encode_bool(&stream, field->is_nullable);
>> mpstream_encode_str(&stream, "nullable_action");
>>
>> - assert(def->fields[i].nullable_action < on_conflict_action_MAX);
>> + assert(field->nullable_action < on_conflict_action_MAX);
>> const char *action =
>> - on_conflict_action_strs[def->fields[i].nullable_action];
>> + on_conflict_action_strs[field->nullable_action];
>> mpstream_encode_str(&stream, action);
>> if (cid != COLL_NONE) {
>> mpstream_encode_str(&stream, "collation");
>> @@ -980,6 +980,14 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> return raw;
>> }
>>
>> +char *
>> +sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> +{
>> + assert(def != NULL);
>> + return sql_encode_table_format(region, def->fields, def->field_count,
>> + size);
>> +}
>> +
>> char *
>> sql_encode_table_opts(struct region *region, struct space_def *def,
>> uint32_t *size)
>> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
>> index 973b420cf..c12d9373a 100644
>> --- a/src/box/sql/alter.c
>> +++ b/src/box/sql/alter.c
>> @@ -36,6 +36,7 @@
>> #include "sqlInt.h"
>> #include "box/box.h"
>> #include "box/schema.h"
>> +#include "tarantoolInt.h"
>>
>> void
>> sql_alter_table_rename(struct Parse *parse)
>> @@ -209,3 +210,127 @@ rename_trigger(sql *db, char const *sql_stmt, char const *table_name,
>> table_name, tname.z + tname.n);
>> return new_sql_stmt;
>> }
>> +
>> +void
>> +sql_alter_add_column_start(struct Parse *parse)
>> +{
>> + struct create_column_def *create_column_def = &parse->create_column_def;
>> + struct alter_entity_def *alter_entity_def =
>> + &create_column_def->base.base;
>> + assert(alter_entity_def->entity_type == ENTITY_TYPE_COLUMN);
>> + assert(alter_entity_def->alter_action == ALTER_ACTION_CREATE);
>> +
>> + const char *space_name =
>> + alter_entity_def->entity_name->a[0].zName;
>> + struct space *space = space_by_name(space_name);
>> + struct sql *db = parse->db;
>> + if (space == NULL) {
>> + diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
>> + goto tnt_error;
>> + }
>> +
>> + struct space_def *def = space->def;
>> + create_column_def->space_def = def;
>> + if (sql_space_def_check_format(def) != 0)
>> + goto tnt_error;
>> + uint32_t new_field_count = def->field_count + 1;
>> + create_column_def->new_field_count = new_field_count;
>> +
>> +#if SQL_MAX_COLUMN
>> + if ((int)new_field_count > db->aLimit[SQL_LIMIT_COLUMN]) {
>> + diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, def->name,
>> + new_field_count, db->aLimit[SQL_LIMIT_COLUMN]);
>> + goto tnt_error;
>> + }
>> +#endif
>> + /*
>> + * Create new fields array and add the new column's
>> + * field_def to its end. During copying don't make
>> + * duplicates of the non-scalar fields (name,
>> + * default_value, default_value_expr), because they will
>> + * be just copied and encoded to the msgpack array on sql
>> + * allocator in sql_alter_add_column_end().
>> + */
>> + uint32_t size = sizeof(struct field_def) * new_field_count;
>> + struct region *region = &parse->region;
>> + struct field_def *new_fields = region_alloc(region, size);
>> + if (new_fields == NULL) {
>> + diag_set(OutOfMemory, size, "region_alloc", "new_fields");
>> + goto tnt_error;
>> + }
>> + memcpy(new_fields, def->fields,
>> + sizeof(struct field_def) * def->field_count);
>> + create_column_def->new_fields = new_fields;
>> + struct field_def *new_column_def = &new_fields[def->field_count];
>> +
>> + memcpy(new_column_def, &field_def_default, sizeof(struct field_def));
>> + struct Token *name = &create_column_def->base.name;
>> + new_column_def->name =
>> + sql_normalized_name_region_new(region, name->z, name->n);
>> + if (new_column_def->name == NULL)
>> + goto tnt_error;
>> + if (def->opts.is_view) {
>> + diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW,
>> + new_column_def->name, def->name);
>> + goto tnt_error;
>> + }
>> +
>> + new_column_def->type = create_column_def->type_def->type;
>> +
>> +exit_alter_add_column_start:
>> + sqlSrcListDelete(db, alter_entity_def->entity_name);
>> + return;
>> +tnt_error:
>> + parse->is_aborted = true;
>> + goto exit_alter_add_column_start;
>> +}
>> +
>> +void
>> +sql_alter_add_column_end(struct Parse *parse)
>> +{
>> + struct create_column_def *create_column_def = &parse->create_column_def;
>> + uint32_t new_field_count = create_column_def->new_field_count;
>> + uint32_t table_stmt_sz = 0;
>> + char *table_stmt =
>> + sql_encode_table_format(&parse->region,
>> + create_column_def->new_fields,
>> + new_field_count, &table_stmt_sz);
>> + if (table_stmt == NULL) {
>> + parse->is_aborted = true;
>> + return;
>> + }
>> + char *raw = sqlDbMallocRaw(parse->db, table_stmt_sz);
>> + if (raw == NULL){
>> + parse->is_aborted = true;
>> + return;
>> + }
>> + memcpy(raw, table_stmt, table_stmt_sz);
>> +
>> + struct space *system_space = space_by_id(BOX_SPACE_ID);
>> + assert(system_space != NULL);
>> + int cursor = parse->nTab++;
>> + struct Vdbe *v = sqlGetVdbe(parse);
>> + assert(v != NULL);
>> + vdbe_emit_open_cursor(parse, cursor, 0, system_space);
>> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
>> +
>> + int key_reg = sqlGetTempReg(parse);
>> + sqlVdbeAddOp2(v, OP_Integer, create_column_def->space_def->id, key_reg);
>> + int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 1);
>> + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
>> + sqlVdbeJumpHere(v, addr);
>> +
>> + const int system_space_field_count = 7;
>> + int tuple_reg = sqlGetTempRange(parse, system_space_field_count + 1);
>> + for (int i = 0; i < system_space_field_count - 1; ++i)
>> + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
>> + sqlVdbeAddOp1(v, OP_Close, cursor);
>> +
>> + sqlVdbeAddOp2(v, OP_Integer, new_field_count, tuple_reg + 4);
>> + sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6,
>> + SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC);
>> + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, system_space_field_count,
>> + tuple_reg + system_space_field_count);
>> + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + system_space_field_count, 0,
>> + 0, (char *)system_space, P4_SPACEPTR);
>> +}
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index bc50ecbfa..2f51b55df 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -368,11 +368,21 @@ void
>> sql_column_add_nullable_action(struct Parse *parser,
>> enum on_conflict_action nullable_action)
>> {
>> - struct space *space = parser->create_table_def.new_space;
>> - if (space == NULL || NEVER(space->def->field_count < 1))
>> + struct space_def *def = NULL;
>> + struct field_def *field = NULL;
>> + if (parser->create_table_def.new_space != NULL) {
>> + def = parser->create_table_def.new_space->def;
>> + if (NEVER(def->field_count < 1))
>> + return;
>> + field = &def->fields[def->field_count - 1];
>> + } else if (parser->create_column_def.space_def != NULL) {
>> + def = parser->create_column_def.space_def;
>> + uint32_t field_count =
>> + parser->create_column_def.new_field_count;
>> + field = &parser->create_column_def.new_fields[field_count - 1];
>> + } else {
>> return;
>> - struct space_def *def = space->def;
>> - struct field_def *field = &def->fields[def->field_count - 1];
>> + }
>> if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
>> nullable_action != field->nullable_action) {
>> /* Prevent defining nullable_action many times. */
>> @@ -390,51 +400,53 @@ sql_column_add_nullable_action(struct Parse *parser,
>> }
>>
>> /*
>> - * The expression is the default value for the most recently added column
>> - * of the table currently under construction.
>> + * The expression is the default value for the most recently added
>> + * column.
>> *
>> * Default value expressions must be constant. Raise an exception if this
>> * is not the case.
>> *
>> * This routine is called by the parser while in the middle of
>> - * parsing a CREATE TABLE statement.
>> + * parsing a <CREATE TABLE> or <ALTER TABLE ADD COLUMN> statement.
>> */
>> void
>> sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan)
>> {
>> sql *db = pParse->db;
>> - struct space *p = pParse->create_table_def.new_space;
>> - if (p != NULL) {
>> - assert(p->def->opts.is_ephemeral);
>> - struct space_def *def = p->def;
>> - if (!sqlExprIsConstantOrFunction
>> - (pSpan->pExpr, db->init.busy)) {
>> - const char *column_name =
>> - def->fields[def->field_count - 1].name;
>> - diag_set(ClientError, ER_CREATE_SPACE, def->name,
>> - tt_sprintf("default value of column '%s' is "\
>> - "not constant", column_name));
>> + struct space_def *def = NULL;
>> + struct field_def *field = NULL;
>> + if (pParse->create_table_def.new_space != NULL) {
>> + def = pParse->create_table_def.new_space->def;
>> + assert(def->opts.is_ephemeral);
>> + field = &def->fields[def->field_count - 1];
>> + } else if (pParse->create_column_def.space_def != NULL) {
>> + def = pParse->create_column_def.space_def;
>> + uint32_t field_count =
>> + pParse->create_column_def.new_field_count;
>> + field = &pParse->create_column_def.new_fields[field_count - 1];
>> + } else {
>> + goto add_default_value_exit;
>> + }
>> + if (!sqlExprIsConstantOrFunction(pSpan->pExpr, db->init.busy)) {
>> + diag_set(ClientError, ER_CREATE_SPACE, def->name,
>> + tt_sprintf("default value of column '%s' is not "
>> + "constant", field->name));
>> + pParse->is_aborted = true;
>> + } else {
>> + assert(def != NULL);
>> + struct region *region = &pParse->region;
>> + uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
>> + field->default_value = region_alloc(region, default_length + 1);
>> + if (field->default_value == NULL) {
>> + diag_set(OutOfMemory, default_length + 1,
>> + "region_alloc", "field->default_value");
>> pParse->is_aborted = true;
>> - } else {
>> - assert(def != NULL);
>> - struct field_def *field =
>> - &def->fields[def->field_count - 1];
>> - struct region *region = &pParse->region;
>> - uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
>> - field->default_value = region_alloc(region,
>> - default_length + 1);
>> - if (field->default_value == NULL) {
>> - diag_set(OutOfMemory, default_length + 1,
>> - "region_alloc",
>> - "field->default_value");
>> - pParse->is_aborted = true;
>> - return;
>> - }
>> - strncpy(field->default_value, pSpan->zStart,
>> - default_length);
>> - field->default_value[default_length] = '\0';
>> + return;
>> }
>> + strncpy(field->default_value, pSpan->zStart, default_length);
>> + field->default_value[default_length] = '\0';
>> }
>> +add_default_value_exit:
>> sql_expr_delete(db, pSpan->pExpr, false);
>> }
>>
>> @@ -686,17 +698,27 @@ void
>> sqlAddCollateType(Parse * pParse, Token * pToken)
>> {
>> struct space *space = pParse->create_table_def.new_space;
>> - if (space == NULL)
>> + uint32_t *coll_id = NULL;
>> + if (space != NULL) {
>> + uint32_t i = space->def->field_count - 1;
>> + coll_id = &space->def->fields[i].coll_id;
>> + } else if (pParse->create_column_def.space_def != NULL) {
>> + uint32_t field_count =
>> + pParse->create_column_def.new_field_count;
>> + struct field_def *field =
>> + &pParse->create_column_def.new_fields[field_count - 1];
>> + coll_id = &field->coll_id;
>> + } else {
>> return;
>> - uint32_t i = space->def->field_count - 1;
>> + }
>> sql *db = pParse->db;
>> char *coll_name = sql_name_from_token(db, pToken);
>> if (coll_name == NULL) {
>> pParse->is_aborted = true;
>> return;
>> }
>> - uint32_t *coll_id = &space->def->fields[i].coll_id;
>> - if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL) {
>> + if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL &&
>> + space != NULL) {
>> /* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
>> * then an index may have been created on this column before the
>> * collation type was added. Correct this if it is the case.
>> @@ -858,7 +880,9 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg,
>> if (table_opts_stmt == NULL)
>> goto error;
>> uint32_t table_stmt_sz = 0;
>> - char *table_stmt = sql_encode_table(region, space->def, &table_stmt_sz);
>> + char *table_stmt = sql_encode_table_format(region, space->def->fields,
>> + space->def->field_count,
>> + &table_stmt_sz);
>> if (table_stmt == NULL)
>> goto error;
>> char *raw = sqlDbMallocRaw(pParse->db,
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index cfe1c0012..2eba72ac3 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -289,10 +289,14 @@ carglist ::= .
>> %type cconsname { struct Token }
>> cconsname(N) ::= CONSTRAINT nm(X). { N = X; }
>> cconsname(N) ::= . { N = Token_nil; }
>> -ccons ::= DEFAULT term(X). {sqlAddDefaultValue(pParse,&X);}
>> -ccons ::= DEFAULT LP expr(X) RP. {sqlAddDefaultValue(pParse,&X);}
>> -ccons ::= DEFAULT PLUS term(X). {sqlAddDefaultValue(pParse,&X);}
>> -ccons ::= DEFAULT MINUS(A) term(X). {
>> +ccons ::= default_clause_1.
>> +default_clause_1 ::= DEFAULT term(X). {sqlAddDefaultValue(pParse,&X);}
>> +ccons ::= default_clause_2.
>> +default_clause_2 ::= DEFAULT LP expr(X) RP. {sqlAddDefaultValue(pParse,&X);}
>> +ccons ::= default_clause_3.
>> +default_clause_3 ::= DEFAULT PLUS term(X). {sqlAddDefaultValue(pParse,&X);}
>> +ccons ::= default_clause_4.
>> +default_clause_4 ::= DEFAULT MINUS(A) term(X). {
>> ExprSpan v;
>> v.pExpr = sqlPExpr(pParse, TK_UMINUS, X.pExpr, 0);
>> v.zStart = A.z;
>> @@ -303,13 +307,18 @@ ccons ::= DEFAULT MINUS(A) term(X). {
>> // In addition to the type name, we also care about the primary key and
>> // UNIQUE constraints.
>> //
>> -ccons ::= NULL onconf(R). {
>> +ccons ::= null_cons.
>> +null_cons ::= NULL onconf(R). {
>> sql_column_add_nullable_action(pParse, ON_CONFLICT_ACTION_NONE);
>> /* Trigger nullability mismatch error if required. */
>> if (R != ON_CONFLICT_ACTION_ABORT)
>> sql_column_add_nullable_action(pParse, R);
>> }
>> -ccons ::= NOT NULL onconf(R). {sql_column_add_nullable_action(pParse, R);}
>> +ccons ::= not_null_cons.
>> +not_null_cons ::= NOT NULL onconf(R). {
>> + sql_column_add_nullable_action(pParse, R);
>> +}
>> +
>> 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);
>> @@ -335,7 +344,8 @@ ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R).
>> sql_create_foreign_key(pParse);
>> }
>> ccons ::= defer_subclause(D). {fk_constraint_change_defer_mode(pParse, D);}
>> -ccons ::= COLLATE id(C). {sqlAddCollateType(pParse, &C);}
>> +ccons ::= collate_clause.
>> +collate_clause ::= COLLATE id(C). {sqlAddCollateType(pParse, &C);}
>>
>> // The optional AUTOINCREMENT keyword
>> %type autoinc {int}
>> @@ -1729,11 +1739,46 @@ alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; }
>>
>> %type alter_add_constraint {struct alter_args}
>> alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). {
>> + A.table_name = T;
>> + A.name = N;
>> + pParse->initiateTTrans = true;
>> + }
>> +
>> +%type alter_add_column {struct alter_args}
>> +alter_add_column(A) ::= alter_table_start(T) ADD column_name(N). {
>> A.table_name = T;
>> A.name = N;
>> pParse->initiateTTrans = true;
>> }
>>
>> +column_name(N) ::= nm(A). { N = A; }
>> +
>> +cmd ::= alter_column_def alter_carglist add_column_end.
>> +
>> +alter_column_def ::= alter_add_column(N) typedef(Y). {
>> + create_column_def_init(&pParse->create_column_def, N.table_name, &N.name, &Y);
>> + sql_alter_add_column_start(pParse);
>> +}
>> +
>> +/*
>> + * "alter_carglist" is a list of additional constraints and
>> + * clauses that come after the column name and column type in a
>> + * ALTER TABLE ADD [COLUMN] statement.
>> + */
>> +alter_carglist ::= alter_carglist alter_ccons.
>> +alter_carglist ::= .
>> +alter_ccons ::= default_clause_1.
>> +alter_ccons ::= default_clause_2.
>> +alter_ccons ::= default_clause_3.
>> +alter_ccons ::= default_clause_4.
>> +alter_ccons ::= null_cons.
>> +alter_ccons ::= not_null_cons.
>> +alter_ccons ::= collate_clause.
>> +
>> +add_column_end ::= . {
>> + sql_alter_add_column_end(pParse);
>> +}
>> +
>> cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES
>> nm(T) eidlist_opt(TA) matcharg(M) refargs(R) defer_subclause_opt(D). {
>> create_fk_def_init(&pParse->create_fk_def, N.table_name, &N.name, FA, &T, TA,
>> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
>> index 2f433e4c0..56887efa7 100644
>> --- a/src/box/sql/parse_def.h
>> +++ b/src/box/sql/parse_def.h
>> @@ -154,6 +154,7 @@ enum sql_index_type {
>>
>> enum entity_type {
>> ENTITY_TYPE_TABLE = 0,
>> + ENTITY_TYPE_COLUMN,
>> ENTITY_TYPE_VIEW,
>> ENTITY_TYPE_INDEX,
>> ENTITY_TYPE_TRIGGER,
>> @@ -227,6 +228,18 @@ struct create_table_def {
>> uint32_t autoinc_fieldno;
>> };
>>
>> +struct create_column_def {
>> + struct create_entity_def base;
>> + /** Column type. */
>> + struct type_def *type_def;
>> + /** Already existing space def. */
>> + struct space_def *space_def;
>> + /** New format array with the new column. */
>> + struct field_def *new_fields;
>> + /** Old field count + 1. */
>> + uint32_t new_field_count;
>> +};
>> +
>> struct create_view_def {
>> struct create_entity_def base;
>> /**
>> @@ -486,6 +499,16 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
>> table_def->autoinc_fieldno = 0;
>> }
>>
>> +static inline void
>> +create_column_def_init(struct create_column_def *column_def,
>> + struct SrcList *table_name, struct Token *name,
>> + struct type_def *type_def)
>> +{
>> + create_entity_def_init(&column_def->base, ENTITY_TYPE_COLUMN,
>> + table_name, name, false);
>> + column_def->type_def = type_def;
>> +}
>> +
>> static inline void
>> create_view_def_init(struct create_view_def *view_def, struct Token *name,
>> struct Token *create, struct ExprList *aliases,
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index d1fcf4761..46d3d6d2a 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -2231,6 +2231,7 @@ struct Parse {
>> * from parse.y
>> */
>> union {
>> + struct create_column_def create_column_def;
>> struct create_ck_def create_ck_def;
>> struct create_fk_def create_fk_def;
>> struct create_index_def create_index_def;
>> @@ -2842,6 +2843,18 @@ struct space *
>> sqlStartTable(Parse *, Token *);
>> void sqlAddColumn(Parse *, Token *, struct type_def *);
>>
>> +/**
>> + * Set the is_nullable flag on the column @a field.
>> + *
>> + * @param field Column to set.
>> + * @param nullable_action on_conflict_action value.
>> + * @param space_name Name of the space to which the column
>> + * belongs. Needed to set error.
>> + *
>> + * @retval 0 Success.
>> + * @retval -1 nullable_action has been already set.
>> + */
>> +
>> /**
>> * This routine is called by the parser while in the middle of
>> * parsing a CREATE TABLE statement. A "NOT NULL" constraint has
>> @@ -2868,6 +2881,19 @@ sqlAddPrimaryKey(struct Parse *parse);
>> void
>> sql_create_check_contraint(Parse *parser);
>>
>> +/**
>> + * Add default value to the column @a field.
>> + *
>> + * @param parse Parsing context.
>> + * @param field Column to set.
>> + * @param span Default expression.
>> + * @param space_name Name of the space to which the column
>> + * belongs. Needed to set error.
>> + *
>> + * @retval 0 Success.
>> + * @retval -1 Error.
>> + */
>> +
>> void sqlAddDefaultValue(Parse *, ExprSpan *);
>> void sqlAddCollateType(Parse *, Token *);
>>
>> @@ -4031,6 +4057,20 @@ sql_alter_table_rename(struct Parse *parse);
>> void
>> sql_alter_ck_constraint_enable(struct Parse *parse);
>>
>> +/**
>> + * Process the statement: do the checks of space and column
>> + * definition. Create new format array with a new column.
>> + */
>> +void
>> +sql_alter_add_column_start(struct Parse *parse);
>> +
>> +/**
>> + * Encode new format array and emit code to update an entry in
>> + * _space.
>> + */
>> +void
>> +sql_alter_add_column_end(struct Parse *parse);
>> +
>> /**
>> * Return the length (in bytes) of the token that begins at z[0].
>> * Store the token type in *type before returning.
>> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
>> index 1ded6c709..5fd433c60 100644
>> --- a/src/box/sql/tarantoolInt.h
>> +++ b/src/box/sql/tarantoolInt.h
>> @@ -118,14 +118,16 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id);
>> /**
>> * Encode format as entry to be inserted to _space on @region.
>> * @param region Region to use.
>> - * @param def Space definition to encode.
>> + * @param fields Space format.
>> + * @param field_count Number of fields.
>> * @param[out] size Size of result allocation.
>> *
>> * @retval NULL Error.
>> * @retval not NULL Pointer to msgpack on success.
>> */
>> char *
>> -sql_encode_table(struct region *region, struct space_def *def, uint32_t *size);
>> +sql_encode_table_format(struct region *region, struct field_def *fields,
>> + uint32_t field_count, uint32_t *size);
>>
>> /**
>> * Encode "opts" dictionary for _space entry on @region.
>> diff --git a/test/box/misc.result b/test/box/misc.result
>> index 3ca4e31ac..2aaec6566 100644
>> --- a/test/box/misc.result
>> +++ b/test/box/misc.result
>> @@ -563,6 +563,7 @@ t;
>> 209: box.error.SESSION_SETTING_INVALID_VALUE
>> 210: box.error.SQL_PREPARE
>> 211: box.error.WRONG_QUERY_ID
>> + 212: box.error.SQL_CANT_ADD_COLUMN_TO_VIEW
>> ...
>> test_run:cmd("setopt delimiter ''");
>> ---
>> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
>> index 615b9d8a6..283dc1759 100755
>> --- a/test/sql-tap/alter.test.lua
>> +++ b/test/sql-tap/alter.test.lua
>> @@ -1,6 +1,6 @@
>> #!/usr/bin/env tarantool
>> test = require("sqltester")
>> -test:plan(50)
>> +test:plan(57)
>>
>> test:do_execsql_test(
>> "alter-1.1",
>> @@ -585,4 +585,78 @@ test:do_catchsql_test(
>> -- -- </alter-7.11>
>> -- })
>>
>> +--
>> +-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
>> +--
>> +test:do_execsql_test(
>> + "alter-9.1",
>> + [[
>> + CREATE TABLE test (a INTEGER PRIMARY KEY);
>> + ALTER TABLE test ADD b INTEGER;
>> + ]], {
>> + -- <alter-9.1>
>> + -- </alter-9.1>
>> + })
>> +
>> +test:do_catchsql_test(
>> + "alter-9.2",
>> + [[
>> + CREATE VIEW v AS SELECT * FROM test;
>> + ALTER TABLE v ADD b INTEGER;
>> + ]], {
>> + -- <alter-9.2>
>> + 1,"Can't add column 'B' to the view 'V'"
>> + -- </alter-9.2>
>> + })
>> +
>> +test:do_execsql_test(
>> + "alter-9.3",
>> + [[
>> + ALTER TABLE test ADD c TEXT NOT NULL DEFAULT ('a') COLLATE "unicode_ci";
>> + ]], {
>> + -- <alter-9.3>
>> + -- </alter-9.3>
>> + })
>> +
>> +test:do_execsql_test(
>> + "alter-9.4",
>> + [[
>> + INSERT INTO test(a, b) VALUES (1, 1);
>> + SELECT * FROM test;
>> + ]], {
>> + -- <alter-9.4>
>> + 1,1,"a"
>> + -- </alter-9.4>
>> + })
>> +
>> +test:do_catchsql_test(
>> + "alter-9.5",
>> + [[
>> + INSERT INTO test VALUES (2, 2, NULL);
>> + ]], {
>> + -- <alter-9.5>
>> + 1,"Failed to execute SQL statement: NOT NULL constraint failed: TEST.C"
>> + -- </alter-9.5>
>> + })
>> +
>> +test:do_execsql_test(
>> + "alter-9.6",
>> + [[
>> + SELECT * FROM test WHERE c LIKE 'A';
>> + ]], {
>> + -- <alter-9.6>
>> + 1,1,"a"
>> + -- </alter-9.6>
>> + })
>> +
>> +test:do_catchsql_test(
>> + "alter-9.7",
>> + [[
>> + ALTER TABLE test ADD d INTEGER;
>> + ]], {
>> + -- <alter-9.7>
>> + 1,"Tuple field count 3 does not match space field count 4"
>> + -- </alter-9.7>
>> + })
>> +
>> test:finish_test()
>> --
>> 2.21.0 (Apple Git-122)
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: support column addition
2020-01-31 15:05 [Tarantool-patches] [PATCH] sql: support column addition Roman Khabibov
2020-03-09 15:08 ` Roman Khabibov
@ 2020-03-14 17:17 ` Vladislav Shpilevoy
2020-07-06 13:37 ` Roman Khabibov
1 sibling, 1 reply; 5+ messages in thread
From: Vladislav Shpilevoy @ 2020-03-14 17:17 UTC (permalink / raw)
To: Roman Khabibov, tarantool-patches
Hi! Thanks for the patch!
See 18 comments below.
On 31/01/2020 16:05, Roman Khabibov wrote:
> Enable to add column to existing space with
> <ALTER TABLE ADD [COLUMN]> statement. Column definition can be
> supplemented with <DEFAULT>, <COLLATE> clauses and <[NOT] NULL>
> column constraint.
>
> Closes #3075
>
> @TarantoolBot document
> Title: Add columns to existing tables in SQL
> Now, it is possible to add columns to existing empty spaces using
> <ALTER TABLE table_name ADD [COLUMN] column_name column_type ...>
1. Why do you require emptiness? At least for nullable columns it
is not necessary - nullable fields can be absent.
Moreover, someday we may even make possible to create a table
without a PK in SQL, and they allow to add a PK column as ADD COLUMN
instead of ADD CONSTRAINT like it works now (which works on a space
not having columns, btw).
> statement. The column definition is the same as in <CREATE TABLE>
> statement, except that table constraints (PRIMARY KEY, UNIQUE,
> REFERENCES, CHECK) cannot be specified yet.
>
> For example:
>
> tarantool> box.execute([[CREATE TABLE test (
> a INTEGER PRIMARY KEY
> );]])
> ---
> - row_count: 1
> ...
>
> tarantool> box.execute([[ALTER TABLE test ADD COLUMN
> b TEXT
> NOT NULL
> DEFAULT ('a')
> COLLATE "unicode_ci"
> ;]])
> ---
> - row_count: 0
> ...
> ---
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3075-add-column
> Issue: https://github.com/tarantool/tarantool/issues/3075
>
> P.S.: I didn't implement column constraints in ADD COLUMN statement,
> because of the sql_create_index(), sql_create_foreign_key() and
> sql_create_check_contraint() functions. These functions have
> branching inside:
2. Are constraints allowed by the standard to be used in ADD COLUMN?
> 1) CREATE TABLE (parse->create_table_def.new_space != NULL)
> We work with the ephemeral space. Use its def, indexes array,
> ck and fk rlists for checks, opcode emitting, etc and modify it on
> parsing level.
3. Column-related functions (null, collate, default) use create_table_def
only to extract a field_def. See my proposal in the comments far below
how to make their code more reusable.
Talking of constraints, I don't see at this moment how to make their
code completely resuable and intact. We will need to add some ifs.
For sql_create_index() you need to fill create_index_def in struct
Parse. However, that would collide with create_column_def stored
in the same union. So probably you should consider moving create_column_def
to a separate member of struct Parse. Or as a union with create_table_def
in case it will be possible. Although maybe it is not. The problem
is that sql_create_index() emits insertion into _index right away, if
it is not a CREATE TABLE, and to check that it uses create_table_def.
So you need to add a check that if it is ADD COLUMN, that don't generate
code now, and just keep the new index somewhere. You could save it into
create_column_def into a new member. Anyway you can't add more than one
index, so it should be simple. Then you generate the index insertion in
sql_alter_add_column_end().
For sql_create_foreign_key() and sql_create_check_contraint() it is the
same, from what I see.
> 2) ALTER TABLE (parse->create_table_def.new_space == NULL)
> We work with existing space = space_by_name(), but don't modify it.
> This space will be modified after parsing by alter.cc triggers.
>
> In the case of
> ALTER TABLE test ADD a INT PRIMARY KEY CONSTRAINT c UNIQUE CONSTRAINT d UNIQUE ...
> we need to modifiy the existing space, for example UNIQUE constraint
> creation/opcode emitting requires new space def with new modified
> fields array and new index_id_max. The same is for REFERENCES> constraint. With CHECK it seems easier, but I didn't implement it,
> because I want to do patch in the futue: "Enable constraints in ADD COLUMN",
> if it will be needed.
5. This should be a separate commit. But in scope of this
patchset, I think.
> There is obvious way to implement constraints parsing during
> ADD COLUMN. To do partial copy of space = space_by_name() on region,
> to keep it in parse->create_column_>def and to work with it. Then
6. Yes, you can create a copy of space_def. However it can be a
'shallow', light copy. Because constraint addition does not change
space_def. The only thing you need to add is the new column to the
field array so as constraint addition functions would see it. And it
seems to be fine to make a shallow copy of space_def for that, on a
region. You already create a field array copy. Just copy struct
space_def object too.
> we will have to change the current code a bit, but it is necessary
> to write API to create an ephemeral space based on the existing.
>
> I would like to hear your opinion. Maybe I'm generally wrong in
> thinking about all this, and it could be made easier.
7. It can't be done much easier. But a few simplifications
can be done. See my comments about setting NULL, COLLATE, and DEFAULT.
General idea seems to be fine, about trying to reuse the existing code.
> P.P.S.: Could you, please, help me with COLUMN keyword support.
> I couldn't add it as I wanted, i.e., the line in parse.y near the
> line 1754:
> column_name(N) ::= COLUMN nm(A). { N = A; }
> because I had:
> /Users/r.khabibov/tarantool/src/box/sql/parse.h:171:9: error: 'TK_COLUMN' macro redefined
> #define TK_COLUMN
>
> I don't understand why it is so? For example, the definition of
> CONSTRAINT keyword in mkkeywordhash.c is the same, but there is no
> error about this #define during compilation.
>
> { "COLUMN", "TK_COLUMN", true },
> { "CONSTRAINT", "TK_CONSTRAINT", true },
8. This is because of addopcodes.sh. It generates TK_COLUMN to
use for tokens treated as a column name. Seems like the only way
is to make a preparatory patch, which will change the old TK_COLUMN
to something else. For example, TK_COLUMN_NAME, it would be more
correct. Or TK_COLUMN_REF.
> extra/mkkeywordhash.c | 6 +-
> src/box/errcode.h | 1 +
> src/box/sql.c | 28 +++++---
> src/box/sql/alter.c | 125 ++++++++++++++++++++++++++++++++++++
> src/box/sql/build.c | 106 ++++++++++++++++++------------
> src/box/sql/parse.y | 59 +++++++++++++++--
> src/box/sql/parse_def.h | 23 +++++++
> src/box/sql/sqlInt.h | 40 ++++++++++++
> src/box/sql/tarantoolInt.h | 6 +-
> test/box/misc.result | 1 +
> test/sql-tap/alter.test.lua | 76 +++++++++++++++++++++-
> 11 files changed, 405 insertions(+), 66 deletions(-)
>
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 1256df856..7157cabe6 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -980,6 +980,14 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
> return raw;
> }
>
> +char *
> +sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
> +{
> + assert(def != NULL);
> + return sql_encode_table_format(region, def->fields, def->field_count,
> + size);
> +}
9. The function is unused.
> +
> char *
> sql_encode_table_opts(struct region *region, struct space_def *def,
> uint32_t *size)
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 973b420cf..c12d9373a 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -209,3 +210,127 @@ rename_trigger(sql *db, char const *sql_stmt, char const *table_name,
> table_name, tname.z + tname.n);
> return new_sql_stmt;
> }
> +
> +void
> +sql_alter_add_column_start(struct Parse *parse)
> +{
> + struct create_column_def *create_column_def = &parse->create_column_def;
> + struct alter_entity_def *alter_entity_def =
> + &create_column_def->base.base;
> + assert(alter_entity_def->entity_type == ENTITY_TYPE_COLUMN);
> + assert(alter_entity_def->alter_action == ALTER_ACTION_CREATE);
> +
> + const char *space_name =
> + alter_entity_def->entity_name->a[0].zName;
> + struct space *space = space_by_name(space_name);
> + struct sql *db = parse->db;
> + if (space == NULL) {
> + diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
> + goto tnt_error;
> + }
> +
> + struct space_def *def = space->def;
> + create_column_def->space_def = def;
> + if (sql_space_def_check_format(def) != 0)
> + goto tnt_error;
10. Why I can't add a field to a space not having any fields?
> + uint32_t new_field_count = def->field_count + 1;
> + create_column_def->new_field_count = new_field_count;
> +
> +#if SQL_MAX_COLUMN
> + if ((int)new_field_count > db->aLimit[SQL_LIMIT_COLUMN]) {
> + diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, def->name,
> + new_field_count, db->aLimit[SQL_LIMIT_COLUMN]);
> + goto tnt_error;
> + }
> +#endif
> + /*
> + * Create new fields array and add the new column's
> + * field_def to its end. During copying don't make
> + * duplicates of the non-scalar fields (name,
> + * default_value, default_value_expr), because they will
> + * be just copied and encoded to the msgpack array on sql
> + * allocator in sql_alter_add_column_end().
> + */
> + uint32_t size = sizeof(struct field_def) * new_field_count;
> + struct region *region = &parse->region;
> + struct field_def *new_fields = region_alloc(region, size);
> + if (new_fields == NULL) {
> + diag_set(OutOfMemory, size, "region_alloc", "new_fields");
> + goto tnt_error;
> + }
> + memcpy(new_fields, def->fields,
> + sizeof(struct field_def) * def->field_count);
> + create_column_def->new_fields = new_fields;
> + struct field_def *new_column_def = &new_fields[def->field_count];
> +
> + memcpy(new_column_def, &field_def_default, sizeof(struct field_def));
> + struct Token *name = &create_column_def->base.name;
> + new_column_def->name =
> + sql_normalized_name_region_new(region, name->z, name->n);
> + if (new_column_def->name == NULL)
> + goto tnt_error;
> + if (def->opts.is_view) {
> + diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW,
> + new_column_def->name, def->name);
> + goto tnt_error;
> + }
> +
> + new_column_def->type = create_column_def->type_def->type;
11. Seems like lots of duplication from sqlAddColumn(). You could reuse
it. Or reuse this function in sqlAddColumn(). Or try to extract their
common part. Because they are totally the same not counting how do they
get space_def. For example, you will be able to use sql_field_retrieve()
to realloc field array.
You could copy entire space_def on a region for ADD COLUMN. In that case
both functions will be even more the same.
> +
> +exit_alter_add_column_start:
12. The label is never used except from one goto below. Please,
move it to there and drop the label.
> + sqlSrcListDelete(db, alter_entity_def->entity_name);
> + return;
> +tnt_error:
> + parse->is_aborted = true;
> + goto exit_alter_add_column_start;
> +}
> +
> +void
> +sql_alter_add_column_end(struct Parse *parse)
> +{
> + struct create_column_def *create_column_def = &parse->create_column_def;
> + uint32_t new_field_count = create_column_def->new_field_count;
> + uint32_t table_stmt_sz = 0;
> + char *table_stmt =
> + sql_encode_table_format(&parse->region,
> + create_column_def->new_fields,
> + new_field_count, &table_stmt_sz);
> + if (table_stmt == NULL) {
> + parse->is_aborted = true;
> + return;
> + }
> + char *raw = sqlDbMallocRaw(parse->db, table_stmt_sz);
> + if (raw == NULL){
> + parse->is_aborted = true;
> + return;
> + }
> + memcpy(raw, table_stmt, table_stmt_sz);
> +
> + struct space *system_space = space_by_id(BOX_SPACE_ID);
> + assert(system_space != NULL);
> + int cursor = parse->nTab++;
> + struct Vdbe *v = sqlGetVdbe(parse);
> + assert(v != NULL);
> + vdbe_emit_open_cursor(parse, cursor, 0, system_space);
> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
> +
> + int key_reg = sqlGetTempReg(parse);
> + sqlVdbeAddOp2(v, OP_Integer, create_column_def->space_def->id, key_reg);
> + int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 1);
> + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
> + sqlVdbeJumpHere(v, addr);
> +
> + const int system_space_field_count = 7;
13. It does not look right to hardcode the constant here. Please, add a
new constant to schema_def.h, where all other _space fields are defined.
> + int tuple_reg = sqlGetTempRange(parse, system_space_field_count + 1);
> + for (int i = 0; i < system_space_field_count - 1; ++i)
> + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
> + sqlVdbeAddOp1(v, OP_Close, cursor);
> +
> + sqlVdbeAddOp2(v, OP_Integer, new_field_count, tuple_reg + 4);
> + sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6,
> + SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC);
> + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, system_space_field_count,
> + tuple_reg + system_space_field_count);
> + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + system_space_field_count, 0,
> + 0, (char *)system_space, P4_SPACEPTR);
> +}
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index bc50ecbfa..2f51b55df 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -368,11 +368,21 @@ void
> sql_column_add_nullable_action(struct Parse *parser,
> enum on_conflict_action nullable_action)
> {
> - struct space *space = parser->create_table_def.new_space;
> - if (space == NULL || NEVER(space->def->field_count < 1))
> + struct space_def *def = NULL;
> + struct field_def *field = NULL;
> + if (parser->create_table_def.new_space != NULL) {
> + def = parser->create_table_def.new_space->def;
> + if (NEVER(def->field_count < 1))
> + return;
> + field = &def->fields[def->field_count - 1];
> + } else if (parser->create_column_def.space_def != NULL) {
> + def = parser->create_column_def.space_def;
> + uint32_t field_count =
> + parser->create_column_def.new_field_count;
> + field = &parser->create_column_def.new_fields[field_count - 1];
> + } else {
14. I think you could try to use create_column_def both for CREATE TABLE
and for ADD COLUMN. Try storing the new field in create_column_def separately,
as a special attribute. CREATE TABLE will recreate that field everytime a new
column is scanned. ADD COLUMN will create it once and use in all such
functions.
The same hack can help to make sqlAddDefaultValue(), sqlAddCollateType() more
straightforward and reusable.
> return;
> - struct space_def *def = space->def;
> - struct field_def *field = &def->fields[def->field_count - 1];
> + }
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index cfe1c0012..2eba72ac3 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -335,7 +344,8 @@ ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R).
> +
> +/*
> + * "alter_carglist" is a list of additional constraints and
> + * clauses that come after the column name and column type in a
> + * ALTER TABLE ADD [COLUMN] statement.
> + */
> +alter_carglist ::= alter_carglist alter_ccons.
> +alter_carglist ::= .
> +alter_ccons ::= default_clause_1.
> +alter_ccons ::= default_clause_2.
> +alter_ccons ::= default_clause_3.
> +alter_ccons ::= default_clause_4.
> +alter_ccons ::= null_cons.
> +alter_ccons ::= not_null_cons.
> +alter_ccons ::= collate_clause.
15. Seems like you could just take ccons instead of inventing
alter_ccons. After you enable constraints in ADD COLUMN, right?
> +
> +add_column_end ::= . {
> + sql_alter_add_column_end(pParse);
> +}
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index d1fcf4761..46d3d6d2a 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -2842,6 +2843,18 @@ struct space *
> sqlStartTable(Parse *, Token *);
> void sqlAddColumn(Parse *, Token *, struct type_def *);
>
> +/**
> + * Set the is_nullable flag on the column @a field.
> + *
> + * @param field Column to set.
> + * @param nullable_action on_conflict_action value.
> + * @param space_name Name of the space to which the column
> + * belongs. Needed to set error.
> + *
> + * @retval 0 Success.
> + * @retval -1 nullable_action has been already set.
> + */
> +
16. The comment just hangs not belonging to any function. The
function below already has a comment.
> /**
> * This routine is called by the parser while in the middle of
> * parsing a CREATE TABLE statement. A "NOT NULL" constraint has
> @@ -2868,6 +2881,19 @@ sqlAddPrimaryKey(struct Parse *parse);
> void
> sql_create_check_contraint(Parse *parser);
>
> +/**
> + * Add default value to the column @a field.
> + *
> + * @param parse Parsing context.
> + * @param field Column to set.
> + * @param span Default expression.
> + * @param space_name Name of the space to which the column
> + * belongs. Needed to set error.
> + *
> + * @retval 0 Success.
> + * @retval -1 Error.
> + */
17. This comment also hangs, and contains not existing parameters.
> +
> void sqlAddDefaultValue(Parse *, ExprSpan *);
> void sqlAddCollateType(Parse *, Token *);
>
> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
> index 615b9d8a6..283dc1759 100755
> --- a/test/sql-tap/alter.test.lua
> +++ b/test/sql-tap/alter.test.lua
18. You will need to add much more tests. For example, ADD COLUMN
in a transaction, add multiple columns in a transaction, add to a
non-empty space a non-nullable field, add a nullable fields to a
non-empty space, add a column to a space not having any columns,
and so on.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: support column addition
2020-03-14 17:17 ` Vladislav Shpilevoy
@ 2020-07-06 13:37 ` Roman Khabibov
0 siblings, 0 replies; 5+ messages in thread
From: Roman Khabibov @ 2020-07-06 13:37 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Mar 14, 2020, at 20:17, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Hi! Thanks for the patch!
>
> See 18 comments below.
>
> On 31/01/2020 16:05, Roman Khabibov wrote:
>> Enable to add column to existing space with
>> <ALTER TABLE ADD [COLUMN]> statement. Column definition can be
>> supplemented with <DEFAULT>, <COLLATE> clauses and <[NOT] NULL>
>> column constraint.
>>
>> Closes #3075
>>
>> @TarantoolBot document
>> Title: Add columns to existing tables in SQL
>> Now, it is possible to add columns to existing empty spaces using
>> <ALTER TABLE table_name ADD [COLUMN] column_name column_type ...>
>
> 1. Why do you require emptiness? At least for nullable columns it
> is not necessary - nullable fields can be absent.
Because, I need to edit memtx code to allow this. Let’s do it in
separete patch/ticket.
tarantool> CREATE TABLE t1 (s1 INTEGER PRIMARY KEY)
---
- row_count: 1
...
tarantool> INSERT INTO t1 VALUES (1)
---
- row_count: 1
...
tarantool> ALTER TABLE t1 ADD s2 INT
---
- null
- Tuple field count 1 does not match space field count 2
...
> Moreover, someday we may even make possible to create a table
> without a PK in SQL, and they allow to add a PK column as ADD COLUMN
> instead of ADD CONSTRAINT like it works now (which works on a space
> not having columns, btw).
>
>> statement. The column definition is the same as in <CREATE TABLE>
>> statement, except that table constraints (PRIMARY KEY, UNIQUE,
>> REFERENCES, CHECK) cannot be specified yet.
>>
>> For example:
>>
>> tarantool> box.execute([[CREATE TABLE test (
>> a INTEGER PRIMARY KEY
>> );]])
>> ---
>> - row_count: 1
>> ...
>>
>> tarantool> box.execute([[ALTER TABLE test ADD COLUMN
>> b TEXT
>> NOT NULL
>> DEFAULT ('a')
>> COLLATE "unicode_ci"
>> ;]])
>> ---
>> - row_count: 0
>> ...
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3075-add-column
>> Issue: https://github.com/tarantool/tarantool/issues/3075
>>
>> P.S.: I didn't implement column constraints in ADD COLUMN statement,
>> because of the sql_create_index(), sql_create_foreign_key() and
>> sql_create_check_contraint() functions. These functions have
>> branching inside:
>
> 2. Are constraints allowed by the standard to be used in ADD COLUMN?
Yes. It is allowed by the standard. The rules of definition column in
CREATE TABLE and ALTER TABLE is the same.
>> 1) CREATE TABLE (parse->create_table_def.new_space != NULL)
>> We work with the ephemeral space. Use its def, indexes array,
>> ck and fk rlists for checks, opcode emitting, etc and modify it on
>> parsing level.
>
> 3. Column-related functions (null, collate, default) use create_table_def
> only to extract a field_def. See my proposal in the comments far below
> how to make their code more reusable.
>
> Talking of constraints, I don't see at this moment how to make their
> code completely resuable and intact. We will need to add some ifs.
>
> For sql_create_index() you need to fill create_index_def in struct
> Parse. However, that would collide with create_column_def stored
> in the same union. So probably you should consider moving create_column_def
> to a separate member of struct Parse. Or as a union with create_table_def
> in case it will be possible. Although maybe it is not. The problem
> is that sql_create_index() emits insertion into _index right away, if
> it is not a CREATE TABLE, and to check that it uses create_table_def.
>
> So you need to add a check that if it is ADD COLUMN, that don't generate
> code now, and just keep the new index somewhere. You could save it into
> create_column_def into a new member. Anyway you can't add more than one
> index, so it should be simple. Then you generate the index insertion in
> sql_alter_add_column_end().
>
> For sql_create_foreign_key() and sql_create_check_contraint() it is the
> same, from what I see.
See the code. A lot of changes.
>> 2) ALTER TABLE (parse->create_table_def.new_space == NULL)
>> We work with existing space = space_by_name(), but don't modify it.
>> This space will be modified after parsing by alter.cc triggers.
>>
>> In the case of
>> ALTER TABLE test ADD a INT PRIMARY KEY CONSTRAINT c UNIQUE CONSTRAINT d UNIQUE ...
>> we need to modifiy the existing space, for example UNIQUE constraint
>> creation/opcode emitting requires new space def with new modified
>> fields array and new index_id_max. The same is for REFERENCES> constraint. With CHECK it seems easier, but I didn't implement it,
>> because I want to do patch in the futue: "Enable constraints in ADD COLUMN",
>> if it will be needed.
>
> 5. This should be a separate commit. But in scope of this
> patchset, I think.
Done.
>> There is obvious way to implement constraints parsing during
>> ADD COLUMN. To do partial copy of space = space_by_name() on region,
>> to keep it in parse->create_column_>def and to work with it. Then
>
> 6. Yes, you can create a copy of space_def. However it can be a
> 'shallow', light copy. Because constraint addition does not change
> space_def. The only thing you need to add is the new column to the
> field array so as constraint addition functions would see it. And it
> seems to be fine to make a shallow copy of space_def for that, on a
> region. You already create a field array copy. Just copy struct
> space_def object too.
I need to take into account indexes from struct space, so that I do
a space shallow copy.
+ * Function is used to add a new column to the existing space with
+ * <ALTER TABLE ADD COLUMN>. Copy info about indexes and
+ * definition to create constraints appeared in the statement.
*/
+static struct space *
+sql_shallow_space_copy(struct Parse *parse, struct space *space)
+{
+ assert(space->def != NULL);
+ struct space *ret = sql_ephemeral_space_new(parse, space->def->name);
+ if (ret == NULL)
+ return NULL;
+ ret->index_count = space->index_count;
+ ret->index_id_max = space->index_id_max;
+ uint32_t indexes_sz = sizeof(struct index *) * (ret->index_count);
+ ret->index = (struct index **) malloc(indexes_sz);
+ if (ret->index == NULL) {
+ diag_set(OutOfMemory, indexes_sz, "realloc", "ret->index");
+ return NULL;
+ }
+ for (uint32_t i = 0; i < ret->index_count; i++)
+ ret->index[i] = space->index[i];
+ memcpy(ret->def, space->def, sizeof(struct space_def));
+ ret->def->opts.is_temporary = true;
+ ret->def->opts.is_ephemeral = true;
+ uint32_t fields_size = sizeof(struct field_def) * ret->def->field_count;
+ ret->def->fields = region_alloc(&parse->region, fields_size);
+ if (ret->def->fields == NULL) {
+ diag_set(OutOfMemory, fields_size, "region_alloc",
+ "ret->def->fields");
+ return NULL;
+ }
+ memcpy(ret->def->fields, space->def->fields, fields_size);
+
+ return ret;
+}
+
>> we will have to change the current code a bit, but it is necessary
>> to write API to create an ephemeral space based on the existing.
>>
>> I would like to hear your opinion. Maybe I'm generally wrong in
>> thinking about all this, and it could be made easier.
>
> 7. It can't be done much easier. But a few simplifications
> can be done. See my comments about setting NULL, COLLATE, and DEFAULT.
>
> General idea seems to be fine, about trying to reuse the existing code.
>
>> P.P.S.: Could you, please, help me with COLUMN keyword support.
>> I couldn't add it as I wanted, i.e., the line in parse.y near the
>> line 1754:
>> column_name(N) ::= COLUMN nm(A). { N = A; }
>> because I had:
>> /Users/r.khabibov/tarantool/src/box/sql/parse.h:171:9: error: 'TK_COLUMN' macro redefined
>> #define TK_COLUMN
>>
>> I don't understand why it is so? For example, the definition of
>> CONSTRAINT keyword in mkkeywordhash.c is the same, but there is no
>> error about this #define during compilation.
>>
>> { "COLUMN", "TK_COLUMN", true },
>> { "CONSTRAINT", "TK_CONSTRAINT", true },
>
> 8. This is because of addopcodes.sh. It generates TK_COLUMN to
> use for tokens treated as a column name. Seems like the only way
> is to make a preparatory patch, which will change the old TK_COLUMN
> to something else. For example, TK_COLUMN_NAME, it would be more
> correct. Or TK_COLUMN_REF.
See in the first patch of my patch set.
>> extra/mkkeywordhash.c | 6 +-
>> src/box/errcode.h | 1 +
>> src/box/sql.c | 28 +++++---
>> src/box/sql/alter.c | 125 ++++++++++++++++++++++++++++++++++++
>> src/box/sql/build.c | 106 ++++++++++++++++++------------
>> src/box/sql/parse.y | 59 +++++++++++++++--
>> src/box/sql/parse_def.h | 23 +++++++
>> src/box/sql/sqlInt.h | 40 ++++++++++++
>> src/box/sql/tarantoolInt.h | 6 +-
>> test/box/misc.result | 1 +
>> test/sql-tap/alter.test.lua | 76 +++++++++++++++++++++-
>> 11 files changed, 405 insertions(+), 66 deletions(-)
>>
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index 1256df856..7157cabe6 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -980,6 +980,14 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> return raw;
>> }
>>
>> +char *
>> +sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> +{
>> + assert(def != NULL);
>> + return sql_encode_table_format(region, def->fields, def->field_count,
>> + size);
>> +}
>
> 9. The function is unused.
Removed.
>> +
>> char *
>> sql_encode_table_opts(struct region *region, struct space_def *def,
>> uint32_t *size)
>> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
>> index 973b420cf..c12d9373a 100644
>> --- a/src/box/sql/alter.c
>> +++ b/src/box/sql/alter.c
>> @@ -209,3 +210,127 @@ rename_trigger(sql *db, char const *sql_stmt, char const *table_name,
>> table_name, tname.z + tname.n);
>> return new_sql_stmt;
>> }
>> +
>> +void
>> +sql_alter_add_column_start(struct Parse *parse)
>> +{
>> + struct create_column_def *create_column_def = &parse->create_column_def;
>> + struct alter_entity_def *alter_entity_def =
>> + &create_column_def->base.base;
>> + assert(alter_entity_def->entity_type == ENTITY_TYPE_COLUMN);
>> + assert(alter_entity_def->alter_action == ALTER_ACTION_CREATE);
>> +
>> + const char *space_name =
>> + alter_entity_def->entity_name->a[0].zName;
>> + struct space *space = space_by_name(space_name);
>> + struct sql *db = parse->db;
>> + if (space == NULL) {
>> + diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
>> + goto tnt_error;
>> + }
>> +
>> + struct space_def *def = space->def;
>> + create_column_def->space_def = def;
>> + if (sql_space_def_check_format(def) != 0)
>> + goto tnt_error;
>
> 10. Why I can't add a field to a space not having any fields?
Removed.
>> + uint32_t new_field_count = def->field_count + 1;
>> + create_column_def->new_field_count = new_field_count;
>> +
>> +#if SQL_MAX_COLUMN
>> + if ((int)new_field_count > db->aLimit[SQL_LIMIT_COLUMN]) {
>> + diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, def->name,
>> + new_field_count, db->aLimit[SQL_LIMIT_COLUMN]);
>> + goto tnt_error;
>> + }
>> +#endif
>> + /*
>> + * Create new fields array and add the new column's
>> + * field_def to its end. During copying don't make
>> + * duplicates of the non-scalar fields (name,
>> + * default_value, default_value_expr), because they will
>> + * be just copied and encoded to the msgpack array on sql
>> + * allocator in sql_alter_add_column_end().
>> + */
>> + uint32_t size = sizeof(struct field_def) * new_field_count;
>> + struct region *region = &parse->region;
>> + struct field_def *new_fields = region_alloc(region, size);
>> + if (new_fields == NULL) {
>> + diag_set(OutOfMemory, size, "region_alloc", "new_fields");
>> + goto tnt_error;
>> + }
>> + memcpy(new_fields, def->fields,
>> + sizeof(struct field_def) * def->field_count);
>> + create_column_def->new_fields = new_fields;
>> + struct field_def *new_column_def = &new_fields[def->field_count];
>> +
>> + memcpy(new_column_def, &field_def_default, sizeof(struct field_def));
>> + struct Token *name = &create_column_def->base.name;
>> + new_column_def->name =
>> + sql_normalized_name_region_new(region, name->z, name->n);
>> + if (new_column_def->name == NULL)
>> + goto tnt_error;
>> + if (def->opts.is_view) {
>> + diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW,
>> + new_column_def->name, def->name);
>> + goto tnt_error;
>> + }
>> +
>> + new_column_def->type = create_column_def->type_def->type;
>
> 11. Seems like lots of duplication from sqlAddColumn(). You could reuse
> it. Or reuse this function in sqlAddColumn(). Or try to extract their
> common part. Because they are totally the same not counting how do they
> get space_def. For example, you will be able to use sql_field_retrieve()
> to realloc field array.
See the code.
> You could copy entire space_def on a region for ADD COLUMN. In that case
> both functions will be even more the same.
>
>> +
>> +exit_alter_add_column_start:
>
> 12. The label is never used except from one goto below. Please,
> move it to there and drop the label.
Removed.
>> + sqlSrcListDelete(db, alter_entity_def->entity_name);
>> + return;
>> +tnt_error:
>> + parse->is_aborted = true;
>> + goto exit_alter_add_column_start;
>> +}
>> +
>> +void
>> +sql_alter_add_column_end(struct Parse *parse)
>> +{
>> + struct create_column_def *create_column_def = &parse->create_column_def;
>> + uint32_t new_field_count = create_column_def->new_field_count;
>> + uint32_t table_stmt_sz = 0;
>> + char *table_stmt =
>> + sql_encode_table_format(&parse->region,
>> + create_column_def->new_fields,
>> + new_field_count, &table_stmt_sz);
>> + if (table_stmt == NULL) {
>> + parse->is_aborted = true;
>> + return;
>> + }
>> + char *raw = sqlDbMallocRaw(parse->db, table_stmt_sz);
>> + if (raw == NULL){
>> + parse->is_aborted = true;
>> + return;
>> + }
>> + memcpy(raw, table_stmt, table_stmt_sz);
>> +
>> + struct space *system_space = space_by_id(BOX_SPACE_ID);
>> + assert(system_space != NULL);
>> + int cursor = parse->nTab++;
>> + struct Vdbe *v = sqlGetVdbe(parse);
>> + assert(v != NULL);
>> + vdbe_emit_open_cursor(parse, cursor, 0, system_space);
>> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
>> +
>> + int key_reg = sqlGetTempReg(parse);
>> + sqlVdbeAddOp2(v, OP_Integer, create_column_def->space_def->id, key_reg);
>> + int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 1);
>> + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
>> + sqlVdbeJumpHere(v, addr);
>> +
>> + const int system_space_field_count = 7;
>
> 13. It does not look right to hardcode the constant here. Please, add a
> new constant to schema_def.h, where all other _space fields are defined.
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index f86cd42f1..f2af9e23f 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -131,6 +131,7 @@ enum {
BOX_SPACE_FIELD_FIELD_COUNT = 4,
BOX_SPACE_FIELD_OPTS = 5,
BOX_SPACE_FIELD_FORMAT = 6,
+ box_space_field_MAX = 7
};
>> + int tuple_reg = sqlGetTempRange(parse, system_space_field_count + 1);
>> + for (int i = 0; i < system_space_field_count - 1; ++i)
>> + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
>> + sqlVdbeAddOp1(v, OP_Close, cursor);
>> +
>> + sqlVdbeAddOp2(v, OP_Integer, new_field_count, tuple_reg + 4);
>> + sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6,
>> + SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC);
>> + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, system_space_field_count,
>> + tuple_reg + system_space_field_count);
>> + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + system_space_field_count, 0,
>> + 0, (char *)system_space, P4_SPACEPTR);
>> +}
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index bc50ecbfa..2f51b55df 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -368,11 +368,21 @@ void
>> sql_column_add_nullable_action(struct Parse *parser,
>> enum on_conflict_action nullable_action)
>> {
>> - struct space *space = parser->create_table_def.new_space;
>> - if (space == NULL || NEVER(space->def->field_count < 1))
>> + struct space_def *def = NULL;
>> + struct field_def *field = NULL;
>> + if (parser->create_table_def.new_space != NULL) {
>> + def = parser->create_table_def.new_space->def;
>> + if (NEVER(def->field_count < 1))
>> + return;
>> + field = &def->fields[def->field_count - 1];
>> + } else if (parser->create_column_def.space_def != NULL) {
>> + def = parser->create_column_def.space_def;
>> + uint32_t field_count =
>> + parser->create_column_def.new_field_count;
>> + field = &parser->create_column_def.new_fields[field_count - 1];
>> + } else {
>
> 14. I think you could try to use create_column_def both for CREATE TABLE
> and for ADD COLUMN. Try storing the new field in create_column_def separately,
> as a special attribute. CREATE TABLE will recreate that field everytime a new
> column is scanned. ADD COLUMN will create it once and use in all such
> functions.
>
> The same hack can help to make sqlAddDefaultValue(), sqlAddCollateType() more
> straightforward and reusable.
>
>> return;
>> - struct space_def *def = space->def;
>> - struct field_def *field = &def->fields[def->field_count - 1];
>> + }
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index cfe1c0012..2eba72ac3 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -335,7 +344,8 @@ ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R).
>> +
>> +/*
>> + * "alter_carglist" is a list of additional constraints and
>> + * clauses that come after the column name and column type in a
>> + * ALTER TABLE ADD [COLUMN] statement.
>> + */
>> +alter_carglist ::= alter_carglist alter_ccons.
>> +alter_carglist ::= .
>> +alter_ccons ::= default_clause_1.
>> +alter_ccons ::= default_clause_2.
>> +alter_ccons ::= default_clause_3.
>> +alter_ccons ::= default_clause_4.
>> +alter_ccons ::= null_cons.
>> +alter_ccons ::= not_null_cons.
>> +alter_ccons ::= collate_clause.
>
> 15. Seems like you could just take ccons instead of inventing
> alter_ccons. After you enable constraints in ADD COLUMN, right?
Rewritten.
>> +
>> +add_column_end ::= . {
>> + sql_alter_add_column_end(pParse);
>> +}
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index d1fcf4761..46d3d6d2a 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -2842,6 +2843,18 @@ struct space *
>> sqlStartTable(Parse *, Token *);
>> void sqlAddColumn(Parse *, Token *, struct type_def *);
>>
>> +/**
>> + * Set the is_nullable flag on the column @a field.
>> + *
>> + * @param field Column to set.
>> + * @param nullable_action on_conflict_action value.
>> + * @param space_name Name of the space to which the column
>> + * belongs. Needed to set error.
>> + *
>> + * @retval 0 Success.
>> + * @retval -1 nullable_action has been already set.
>> + */
>> +
>
> 16. The comment just hangs not belonging to any function. The
> function below already has a comment.
Removed.
>> /**
>> * This routine is called by the parser while in the middle of
>> * parsing a CREATE TABLE statement. A "NOT NULL" constraint has
>> @@ -2868,6 +2881,19 @@ sqlAddPrimaryKey(struct Parse *parse);
>> void
>> sql_create_check_contraint(Parse *parser);
>>
>> +/**
>> + * Add default value to the column @a field.
>> + *
>> + * @param parse Parsing context.
>> + * @param field Column to set.
>> + * @param span Default expression.
>> + * @param space_name Name of the space to which the column
>> + * belongs. Needed to set error.
>> + *
>> + * @retval 0 Success.
>> + * @retval -1 Error.
>> + */
>
> 17. This comment also hangs, and contains not existing parameters.
Removed.
>> +
>> void sqlAddDefaultValue(Parse *, ExprSpan *);
>> void sqlAddCollateType(Parse *, Token *);
>>
>> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
>> index 615b9d8a6..283dc1759 100755
>> --- a/test/sql-tap/alter.test.lua
>> +++ b/test/sql-tap/alter.test.lua
>
> 18. You will need to add much more tests. For example, ADD COLUMN
> in a transaction, add multiple columns in a transaction, add to a
> non-empty space a non-nullable field, add a nullable fields to a
> non-empty space, add a column to a space not having any columns,
> and so on.
diff --git a/test/sql/add-column.test.lua b/test/sql/add-column.test.lua
new file mode 100644
index 000000000..f0ce1c44c
--- /dev/null
+++ b/test/sql/add-column.test.lua
@@ -0,0 +1,103 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+_ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}})
+
+--
+-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
+--
+box.execute('CREATE TABLE t1 (a INTEGER PRIMARY KEY);')
+--
+-- COLUMN keyword is optional. Check it here, but omit it below.
+--
+box.execute('ALTER TABLE t1 ADD COLUMN b INTEGER;')
+
+--
+-- Can't add column to a view.
+--
+box.execute('CREATE VIEW v AS SELECT * FROM t1;')
+box.execute('ALTER TABLE v ADD b INTEGER;')
+box.execute('DROP VIEW v;')
+
+--
+-- Check column constraints typing and work.
+--
+box.execute('CREATE TABLE t2 (a INTEGER CONSTRAINT pk_constr PRIMARY KEY);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT pk_constr')
+test_run:cmd("setopt delimiter ';'");
+box.execute([[ALTER TABLE t2 ADD b INTEGER CONSTRAINT pk_constr PRIMARY KEY
+ CHECK (b > 0)
+ REFERENCES t1(a)
+ CONSTRAINT u_constr UNIQUE]])
+test_run:cmd("setopt delimiter ''");
+box.execute('INSERT INTO t1 VALUES (1, 1);')
+box.execute('INSERT INTO t2 VALUES (1, 1);')
+box.execute('INSERT INTO t2 VALUES (1, 1);')
+
+box.execute('INSERT INTO t1 VALUES (0, 1);')
+box.execute('INSERT INTO t2 VALUES (2, 0);')
+
+box.execute('INSERT INTO t2 VALUES (2, 3);')
+
+box.execute('DROP TABLE t2;')
+
+--
+-- Check self-referenced FK creation.
+--
+box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY);')
+box.execute('ALTER TABLE t2 ADD b INT REFERENCES t1')
+
+box.execute('DROP TABLE t2;')
+
+--
+-- Check AUTOINCREMENT work.
+--
+box.execute("CREATE TABLE t2(a INTEGER CONSTRAINT pk PRIMARY KEY);")
+box.execute("ALTER TABLE t2 DROP CONSTRAINT pk;")
+box.execute("ALTER TABLE t2 ADD b INTEGER PRIMARY KEY AUTOINCREMENT;")
+box.execute("ALTER TABLE t2 ADD c INTEGER AUTOINCREMENT;")
+
+box.execute('DROP TABLE t2;')
+
+--
+-- Check clauses after column typing and work.
+--
+box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY, b INTEGER);')
+test_run:cmd("setopt delimiter ';'");
+box.execute([[ALTER TABLE t2 ADD c TEXT NOT NULL DEFAULT ('a')
+ COLLATE "unicode_ci";]]);
+test_run:cmd("setopt delimiter ''");
+box.execute('INSERT INTO t2(a, b) VALUES (1, 1);')
+box.execute('SELECT * FROM t2;')
+box.execute('INSERT INTO t2 VALUES (2, 2, NULL);')
+box.execute('SELECT * FROM t2 WHERE c LIKE \'A\';')
+
+--
+-- Try to add to a non-empty space a [non-]nullable field.
+--
+box.execute('ALTER TABLE t2 ADD d INTEGER;')
+box.execute('ALTER TABLE t2 ADD d TEXT NOT NULL');
+box.execute('ALTER TABLE t2 ADD e TEXT NULL');
+
+--
+-- Add to a space with no-SQL adjusted or without format.
+--
+_ = box.schema.space.create('WITHOUT')
+box.execute("ALTER TABLE WITHOUT ADD a INTEGER;")
+box.execute("DROP TABLE WITHOUT;")
+
+s = box.schema.space.create('NOSQL')
+s:format{{name = 'A', type = 'unsigned'}}
+box.execute("ALTER TABLE NOSQL ADD b INTEGER")
+
+box.execute('DROP TABLE t2;')
+--
+-- Add multiple columns inside a transaction.
+--
+box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY)')
+box.begin() \
+box.execute('ALTER TABLE t2 ADD b INT') \
+box.execute('ALTER TABLE t2 ADD c INT') \
+box.commit()
+
+box.execute('INSERT INTO t2 VALUES (1, 1, 1)')
+box.execute('SELECT * FROM t2;’)
commit 82c448dc66f6233faeb40dda353652c2fd5a3d29
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date: Thu Jan 2 19:06:14 2020 +0300
sql: support column addition
Enable to add column to existing space with
<ALTER TABLE ADD [COLUMN]> statement. Column definition can be
supplemented with the four types of constraints, <DEFAULT>,
<COLLATE> clauses and <[NOT] NULL>, AUTOINCREMENT.
Closes #2349, #3075
@TarantoolBot document
Title: Add columns to existing tables in SQL
Now, it is possible to add columns to existing empty spaces using
<ALTER TABLE table_name ADD [COLUMN] column_name column_type ...>
statement. The column definition is the same as in <CREATE TABLE>
statement.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY
);]])
---
- row_count: 1
...
tarantool> box.execute([[ALTER TABLE test ADD COLUMN
b TEXT
NOT NULL
DEFAULT ('a')
COLLATE "unicode_ci"
;]])
---
- row_count: 0
...
---
diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 486b6b30d..dea047241 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -76,7 +76,7 @@ static Keyword aKeywordTable[] = {
{ "CHECK", "TK_CHECK", true },
{ "COLLATE", "TK_COLLATE", true },
{ "COLUMN_NAME", "TK_COLUMN_NAME", true },
- { "COLUMN", "TK_STANDARD", true },
+ { "COLUMN", "TK_COLUMN", true },
{ "COMMIT", "TK_COMMIT", true },
{ "CONFLICT", "TK_CONFLICT", false },
{ "CONSTRAINT", "TK_CONSTRAINT", true },
diff --git a/src/box/errcode.h b/src/box/errcode.h
index d1e4d02a9..3e94bee7a 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -266,6 +266,8 @@ struct errcode_record {
/*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with id %u does not exist") \
/*212 */_(ER_SEQUENCE_NOT_STARTED, "Sequence '%s' is not started") \
/*213 */_(ER_NO_SUCH_SESSION_SETTING, "Session setting %s doesn't exist") \
+ /*214 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW, "Can't add column '%s'. '%s' is a view") \
+ /*215 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add AUTOINCREMENT: the space '%s' already has one auto-incremented field") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index f86cd42f1..f2af9e23f 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -131,6 +131,7 @@ enum {
BOX_SPACE_FIELD_FIELD_COUNT = 4,
BOX_SPACE_FIELD_OPTS = 5,
BOX_SPACE_FIELD_FORMAT = 6,
+ box_space_field_MAX = 7
};
/** _index fields. */
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 14f6c1a97..e2bf2b7e9 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -36,6 +36,7 @@
#include "sqlInt.h"
#include "box/box.h"
#include "box/schema.h"
+#include "tarantoolInt.h"
void
sql_alter_table_rename(struct Parse *parse)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ac42fe842..45fb90d38 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -285,48 +285,112 @@ sql_field_retrieve(Parse *parser, struct space_def *space_def, uint32_t id)
return field;
}
-/*
- * Add a new column to the table currently being constructed.
+/**
+ * Make shallow copy of @a space on region.
*
- * The parser calls this routine once for each column declaration
- * in a CREATE TABLE statement. sqlStartTable() gets called
- * first to get things going. Then this routine is called for each
- * column.
+ * Function is used to add a new column to the existing space with
+ * <ALTER TABLE ADD COLUMN>. Copy info about indexes and
+ * definition to create constraints appeared in the statement.
*/
+static struct space *
+sql_shallow_space_copy(struct Parse *parse, struct space *space)
+{
+ assert(space->def != NULL);
+ struct space *ret = sql_ephemeral_space_new(parse, space->def->name);
+ if (ret == NULL)
+ return NULL;
+ ret->index_count = space->index_count;
+ ret->index_id_max = space->index_id_max;
+ uint32_t indexes_sz = sizeof(struct index *) * (ret->index_count);
+ ret->index = (struct index **) malloc(indexes_sz);
+ if (ret->index == NULL) {
+ diag_set(OutOfMemory, indexes_sz, "realloc", "ret->index");
+ return NULL;
+ }
+ for (uint32_t i = 0; i < ret->index_count; i++)
+ ret->index[i] = space->index[i];
+ memcpy(ret->def, space->def, sizeof(struct space_def));
+ ret->def->opts.is_temporary = true;
+ ret->def->opts.is_ephemeral = true;
+ uint32_t fields_size = sizeof(struct field_def) * ret->def->field_count;
+ ret->def->fields = region_alloc(&parse->region, fields_size);
+ if (ret->def->fields == NULL) {
+ diag_set(OutOfMemory, fields_size, "region_alloc",
+ "ret->def->fields");
+ return NULL;
+ }
+ memcpy(ret->def->fields, space->def->fields, fields_size);
+
+ return ret;
+}
+
void
-sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
+sql_create_column_start(struct Parse *parse)
{
- assert(type_def != NULL);
- char *z;
- sql *db = pParse->db;
- if (pParse->create_table_def.new_space == NULL)
- return;
- struct space_def *def = pParse->create_table_def.new_space->def;
+ struct create_column_def *create_column_def = &parse->create_column_def;
+ struct alter_entity_def *alter_entity_def =
+ &create_column_def->base.base;
+ assert(alter_entity_def->entity_type == ENTITY_TYPE_COLUMN);
+ assert(alter_entity_def->alter_action == ALTER_ACTION_CREATE);
+ struct space *space = parse->create_table_def.new_space;
+ bool is_alter = space == NULL;
+ struct sql *db = parse->db;
+ if (is_alter) {
+ const char *space_name =
+ alter_entity_def->entity_name->a[0].zName;
+ space = space_by_name(space_name);
+ if (space == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
+ goto tnt_error;
+ }
+ space = sql_shallow_space_copy(parse, space);
+ if (space == NULL)
+ goto tnt_error;
+ }
+ create_column_def->space = space;
+ struct space_def *def = space->def;
+ assert(def->opts.is_ephemeral);
#if SQL_MAX_COLUMN
if ((int)def->field_count + 1 > db->aLimit[SQL_LIMIT_COLUMN]) {
diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, def->name,
def->field_count + 1, db->aLimit[SQL_LIMIT_COLUMN]);
- pParse->is_aborted = true;
- return;
+ goto tnt_error;
}
#endif
+
+ struct region *region = &parse->region;
+ struct Token *name = &create_column_def->base.name;
+ char *column_name =
+ sql_normalized_name_region_new(region, name->z, name->n);
+ if (column_name == NULL)
+ goto tnt_error;
+
+ if (parse->create_table_def.new_space == NULL && def->opts.is_view) {
+ diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW,
+ column_name, def->name);
+ goto tnt_error;
+ }
+
/*
- * As sql_field_retrieve will allocate memory on region
- * ensure that def is also temporal and would be dropped.
+ * Format can be set in Lua, then exact_field_count can be
+ * zero, but field_count is not.
*/
- assert(def->opts.is_ephemeral);
- if (sql_field_retrieve(pParse, def, def->field_count) == NULL)
- return;
- struct region *region = &pParse->region;
- z = sql_normalized_name_region_new(region, pName->z, pName->n);
- if (z == NULL) {
- pParse->is_aborted = true;
+ if (def->exact_field_count == 0)
+ def->exact_field_count = def->field_count;
+ if (sql_field_retrieve(parse, def, def->field_count) == NULL)
return;
+
+ for (uint32_t i = 0; i < def->field_count; i++) {
+ if (strcmp(column_name, def->fields[i].name) == 0) {
+ diag_set(ClientError, ER_SPACE_FIELD_IS_DUPLICATE,
+ column_name);
+ goto tnt_error;
+ }
}
struct field_def *column_def = &def->fields[def->field_count];
- memcpy(column_def, &field_def_default, sizeof(field_def_default));
- column_def->name = z;
+ memcpy(column_def, &field_def_default, sizeof(struct field_def));
+ column_def->name = column_name;
/*
* Marker ON_CONFLICT_ACTION_DEFAULT is used to detect
* attempts to define NULL multiple time or to detect
@@ -334,19 +398,100 @@ sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
*/
column_def->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
column_def->is_nullable = true;
- column_def->type = type_def->type;
+ column_def->type = create_column_def->type_def->type;
def->field_count++;
+
+ sqlSrcListDelete(db, alter_entity_def->entity_name);
+ return;
+tnt_error:
+ parse->is_aborted = true;
+ sqlSrcListDelete(db, alter_entity_def->entity_name);
+}
+
+static void
+sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id);
+
+void
+sql_create_column_end(struct Parse *parse)
+{
+ struct create_column_def *create_column_def = &parse->create_column_def;
+ struct space *space = parse->create_table_def.new_space;
+ bool is_alter = space == NULL;
+ space = create_column_def->space;
+ struct space_def *def = space->def;
+ if (is_alter) {
+ struct field_def *field = &def->fields[def->field_count - 1];
+ if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) {
+ if (create_column_def->is_pk) {
+ field->nullable_action =
+ ON_CONFLICT_ACTION_ABORT;
+ field->is_nullable = false;
+ } else {
+ field->nullable_action =
+ ON_CONFLICT_ACTION_NONE;
+ field->is_nullable = true;
+ }
+ }
+ /*
+ * Encode the format array and emit code to update _space.
+ */
+ uint32_t table_stmt_sz = 0;
+ struct region *region = &parse->region;
+ char *table_stmt = sql_encode_table(region, def,
+ &table_stmt_sz);
+ char *raw = sqlDbMallocRaw(parse->db, table_stmt_sz);
+ if (table_stmt == NULL || raw == NULL) {
+ parse->is_aborted = true;
+ return;
+ }
+ memcpy(raw, table_stmt, table_stmt_sz);
+
+ struct Vdbe *v = sqlGetVdbe(parse);
+ assert(v != NULL);
+
+ struct space *system_space = space_by_id(BOX_SPACE_ID);
+ assert(system_space != NULL);
+ int cursor = parse->nTab++;
+ vdbe_emit_open_cursor(parse, cursor, 0, system_space);
+ sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
+
+ int key_reg = ++parse->nMem;
+ sqlVdbeAddOp2(v, OP_Integer, def->id, key_reg);
+ int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 1);
+ sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
+ sqlVdbeJumpHere(v, addr);
+
+ int tuple_reg = sqlGetTempRange(parse, box_space_field_MAX + 1);
+ for (int i = 0; i < box_space_field_MAX - 1; ++i)
+ sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
+ sqlVdbeAddOp1(v, OP_Close, cursor);
+
+ sqlVdbeAddOp2(v, OP_Integer, def->field_count, tuple_reg + 4);
+ sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6,
+ SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC);
+ sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, box_space_field_MAX,
+ tuple_reg + box_space_field_MAX);
+ sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + box_space_field_MAX,
+ 0, 0, (char *) system_space, P4_SPACEPTR);
+ sql_vdbe_create_constraints(parse, key_reg);
+ }
+
+ memset(create_column_def, 0, sizeof(struct create_column_def));
+ create_column_def->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
}
void
sql_column_add_nullable_action(struct Parse *parser,
enum on_conflict_action nullable_action)
{
- struct space *space = parser->create_table_def.new_space;
- if (space == NULL || NEVER(space->def->field_count < 1))
+ struct space_def *def = NULL;
+ struct field_def *field = NULL;
+ struct space *space = parser->create_column_def.space;
+ assert (space != NULL);
+ def = space->def;
+ if (NEVER(def->field_count < 1))
return;
- struct space_def *def = space->def;
- struct field_def *field = &def->fields[def->field_count - 1];
+ field = &def->fields[def->field_count - 1];
if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
nullable_action != field->nullable_action) {
/* Prevent defining nullable_action many times. */
@@ -364,51 +509,46 @@ sql_column_add_nullable_action(struct Parse *parser,
}
/*
- * The expression is the default value for the most recently added column
- * of the table currently under construction.
+ * The expression is the default value for the most recently added
+ * column.
*
* Default value expressions must be constant. Raise an exception if this
* is not the case.
*
* This routine is called by the parser while in the middle of
- * parsing a CREATE TABLE statement.
+ * parsing a <CREATE TABLE> or a <ALTER TABLE ADD COLUMN>
+ * statement.
*/
void
sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan)
{
sql *db = pParse->db;
- struct space *p = pParse->create_table_def.new_space;
- if (p != NULL) {
- assert(p->def->opts.is_ephemeral);
- struct space_def *def = p->def;
- if (!sqlExprIsConstantOrFunction
- (pSpan->pExpr, db->init.busy)) {
- const char *column_name =
- def->fields[def->field_count - 1].name;
- diag_set(ClientError, ER_CREATE_SPACE, def->name,
- tt_sprintf("default value of column '%s' is "\
- "not constant", column_name));
+ struct space_def *def = NULL;
+ struct field_def *field = NULL;
+ struct space *space = pParse->create_column_def.space;
+ assert (space != NULL);
+ def = space->def;
+ field = &def->fields[def->field_count - 1];
+ if (!sqlExprIsConstantOrFunction(pSpan->pExpr, db->init.busy)) {
+ diag_set(ClientError, ER_CREATE_SPACE, def->name,
+ tt_sprintf("default value of column '%s' is not "
+ "constant", field->name));
+ pParse->is_aborted = true;
+ } else {
+ assert(def != NULL);
+ struct region *region = &pParse->region;
+ uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
+ field->default_value = region_alloc(region, default_length + 1);
+ if (field->default_value == NULL) {
+ diag_set(OutOfMemory, default_length + 1,
+ "region_alloc", "field->default_value");
pParse->is_aborted = true;
- } else {
- assert(def != NULL);
- struct field_def *field =
- &def->fields[def->field_count - 1];
- struct region *region = &pParse->region;
- uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
- field->default_value = region_alloc(region,
- default_length + 1);
- if (field->default_value == NULL) {
- diag_set(OutOfMemory, default_length + 1,
- "region_alloc",
- "field->default_value");
- pParse->is_aborted = true;
- return;
- }
- strncpy(field->default_value, pSpan->zStart,
- default_length);
- field->default_value[default_length] = '\0';
+ goto add_default_value_exit;
}
+ strncpy(field->default_value, pSpan->zStart, default_length);
+ field->default_value[default_length] = '\0';
}
+add_default_value_exit:
sql_expr_delete(db, pSpan->pExpr, false);
}
@@ -447,6 +587,8 @@ sqlAddPrimaryKey(struct Parse *pParse)
int nTerm;
struct ExprList *pList = pParse->create_index_def.cols;
struct space *space = pParse->create_table_def.new_space;
+ if (space == NULL)
+ space = pParse->create_column_def.space;
if (space == NULL)
goto primary_key_exit;
if (sql_space_primary_key(space) != NULL) {
@@ -574,8 +716,10 @@ sql_create_check_contraint(struct Parse *parser)
(struct alter_entity_def *) create_ck_def;
assert(alter_def->entity_type == ENTITY_TYPE_CK);
(void) alter_def;
- struct space *space = parser->create_table_def.new_space;
- bool is_alter = space == NULL;
+ struct space *space = parser->create_column_def.space;
+ if (space == NULL)
+ space = parser->create_table_def.new_space;
+ bool is_alter_add_constr = space == NULL;
/* Prepare payload for ck constraint definition. */
struct region *region = &parser->region;
@@ -589,9 +733,18 @@ sql_create_check_contraint(struct Parse *parser)
return;
}
} else {
- assert(! is_alter);
- uint32_t ck_idx = ++parser->create_table_def.check_count;
- name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, ck_idx);
+ assert(!is_alter_add_constr);
+ uint32_t idx = ++parser->check_count;
+ if (parser->create_table_def.new_space == NULL) {
+ struct space *original_space =
+ space_by_name(space->def->name);
+ assert(original_space != NULL);
+ struct rlist *checks = &original_space->ck_constraint;
+ struct ck_constraint *ck;
+ rlist_foreach_entry(ck, checks, link)
+ idx++;
+ }
+ name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, idx);
}
size_t name_len = strlen(name);
@@ -634,9 +787,9 @@ sql_create_check_contraint(struct Parse *parser)
trim_space_snprintf(ck_def->expr_str, expr_str, expr_str_len);
memcpy(ck_def->name, name, name_len);
ck_def->name[name_len] = '\0';
- if (is_alter) {
+ if (is_alter_add_constr) {
const char *space_name = alter_def->entity_name->a[0].zName;
- struct space *space = space_by_name(space_name);
+ space = space_by_name(space_name);
if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
parser->is_aborted = true;
@@ -652,8 +805,7 @@ sql_create_check_contraint(struct Parse *parser)
sqlVdbeCountChanges(v);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
} else {
- rlist_add_entry(&parser->create_table_def.new_check, ck_parse,
- link);
+ rlist_add_entry(&parser->checks, ck_parse, link);
}
}
@@ -664,18 +816,19 @@ sql_create_check_contraint(struct Parse *parser)
void
sqlAddCollateType(Parse * pParse, Token * pToken)
{
- struct space *space = pParse->create_table_def.new_space;
- if (space == NULL)
- return;
+ struct space *space = pParse->create_column_def.space;
+ uint32_t *coll_id = NULL;
+ assert(space != NULL);
uint32_t i = space->def->field_count - 1;
+ coll_id = &space->def->fields[i].coll_id;
sql *db = pParse->db;
char *coll_name = sql_name_from_token(db, pToken);
if (coll_name == NULL) {
pParse->is_aborted = true;
return;
}
- uint32_t *coll_id = &space->def->fields[i].coll_id;
- if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL) {
+ if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL &&
+ space != NULL) {
/* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
* then an index may have been created on this column before the
* collation type was added. Correct this if it is the case.
@@ -692,6 +845,7 @@ sqlAddCollateType(Parse * pParse, Token * pToken)
sqlDbFree(db, coll_name);
}
+
struct coll *
sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id)
{
@@ -705,8 +859,7 @@ sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id)
*
* In cases mentioned above collation is fetched by id.
*/
- if (space == NULL) {
- assert(def->opts.is_ephemeral);
+ if (def->opts.is_ephemeral) {
assert(column < (uint32_t)def->field_count);
*coll_id = def->fields[column].coll_id;
struct coll_id *collation = coll_by_id(*coll_id);
@@ -795,7 +948,8 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
memcpy(raw, index_parts, index_parts_sz);
index_parts = raw;
- if (parse->create_table_def.new_space != NULL) {
+ if (parse->create_table_def.new_space != NULL ||
+ parse->create_column_def.space != NULL) {
sqlVdbeAddOp2(v, OP_SCopy, space_id_reg, entry_reg);
sqlVdbeAddOp2(v, OP_Integer, idx_def->iid, entry_reg + 1);
} else {
@@ -930,7 +1084,7 @@ emitNewSysSequenceRecord(Parse *pParse, int reg_seq_id, const char *seq_name)
static int
emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id)
{
- uint32_t fieldno = pParse->create_table_def.autoinc_fieldno;
+ uint32_t fieldno = pParse->autoinc_fieldno;
Vdbe *v = sqlGetVdbe(pParse);
int first_col = pParse->nMem + 1;
@@ -1033,18 +1187,21 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
P4_DYNAMIC);
/*
* In case we are adding FK constraints during execution
- * of <CREATE TABLE ...> statement, we don't have child
- * id, but we know register where it will be stored.
+ * of <CREATE TABLE ...> or <ALER TABLE ADD COLUMN ...>
+ * statement, we don't have child id, but we know register
+ * where it will be stored.
*/
- if (parse_context->create_table_def.new_space != NULL) {
+ bool is_alter_add_constr =
+ parse_context->create_table_def.new_space == NULL &&
+ parse_context->create_column_def.space == NULL;
+ if (!is_alter_add_constr) {
sqlVdbeAddOp2(vdbe, OP_SCopy, fk->child_id,
constr_tuple_reg + 1);
} else {
sqlVdbeAddOp2(vdbe, OP_Integer, fk->child_id,
constr_tuple_reg + 1);
}
- if (parse_context->create_table_def.new_space != NULL &&
- fk_constraint_is_self_referenced(fk)) {
+ if (!is_alter_add_constr && fk_constraint_is_self_referenced(fk)) {
sqlVdbeAddOp2(vdbe, OP_SCopy, fk->parent_id,
constr_tuple_reg + 2);
} else {
@@ -1108,7 +1265,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
constr_tuple_reg + 9);
sqlVdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID,
constr_tuple_reg + 9);
- if (parse_context->create_table_def.new_space == NULL) {
+ if (is_alter_add_constr) {
sqlVdbeCountChanges(vdbe);
sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);
}
@@ -1147,6 +1304,105 @@ resolve_link(struct Parse *parse_context, const struct space_def *def,
return -1;
}
+/**
+ * Emit code to create sequences, indexes, check and foreign
+ * constraints appeared in <CREATE TABLE> or
+ * <ALTER TABLE ADD COLUMN> statement.
+ */
+static void
+sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id)
+{
+ assert(reg_space_id != 0);
+ struct space *space = parse->create_table_def.new_space;
+ bool is_alter = space == NULL;
+ uint32_t i = 0;
+ if (is_alter) {
+ space = parse->create_column_def.space;
+ i = space_by_name(space->def->name)->index_count;
+ }
+ assert(space != NULL);
+ 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);
+ }
+
+ /*
+ * Check to see if we need to create an _sequence table
+ * for keeping track of autoincrement keys.
+ */
+ if (parse->has_autoinc) {
+ /* Do an insertion into _sequence. */
+ int reg_seq_id = ++parse->nMem;
+ struct Vdbe *v = sqlGetVdbe(parse);
+ assert(v != NULL);
+ sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id);
+ int reg_seq_record =
+ emitNewSysSequenceRecord(parse, reg_seq_id,
+ space->def->name);
+ const char *error_msg =
+ tt_sprintf(tnt_errcode_desc(ER_SQL_CANT_ADD_AUTOINC),
+ space->def->name);
+ if (vdbe_emit_halt_with_presence_test(parse,
+ BOX_SEQUENCE_ID, 2,
+ reg_seq_record + 3, 1,
+ ER_SQL_CANT_ADD_AUTOINC,
+ error_msg, false,
+ OP_NoConflict) != 0)
+ return;
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
+ /* Do an insertion into _space_sequence. */
+ int reg_space_seq_record =
+ emitNewSysSpaceSequenceRecord(parse, reg_space_id,
+ reg_seq_id);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
+ reg_space_seq_record);
+ }
+
+ /* Code creation of FK constraints, if any. */
+ struct fk_constraint_parse *fk_parse;
+ rlist_foreach_entry(fk_parse, &parse->fkeys, link) {
+ struct fk_constraint_def *fk_def = fk_parse->fk_def;
+ if (fk_parse->selfref_cols != NULL) {
+ struct ExprList *cols = fk_parse->selfref_cols;
+ for (uint32_t i = 0; i < fk_def->field_count; ++i) {
+ if (resolve_link(parse, space->def,
+ cols->a[i].zName,
+ &fk_def->links[i].parent_field,
+ fk_def->name) != 0)
+ return;
+ }
+ fk_def->parent_id = reg_space_id;
+ } else if (fk_parse->is_self_referenced) {
+ struct key_def *pk_key_def =
+ sql_space_primary_key(space)->def->key_def;
+ if (pk_key_def->part_count != fk_def->field_count) {
+ diag_set(ClientError, ER_CREATE_FK_CONSTRAINT,
+ fk_def->name, "number of columns in "\
+ "foreign key does not match the "\
+ "number of columns in the primary "\
+ "index of referenced table");
+ parse->is_aborted = true;
+ return;
+ }
+ for (uint32_t i = 0; i < fk_def->field_count; ++i) {
+ fk_def->links[i].parent_field =
+ pk_key_def->parts[i].fieldno;
+ }
+ fk_def->parent_id = reg_space_id;
+ }
+ fk_def->child_id = reg_space_id;
+ vdbe_emit_fk_constraint_create(parse, fk_def, space->def->name);
+ }
+
+ /* Code creation of CK constraints, if any. */
+ struct ck_constraint_parse *ck_parse;
+ rlist_foreach_entry(ck_parse, &parse->checks, link) {
+ vdbe_emit_ck_constraint_create(parse, ck_parse->ck_def,
+ reg_space_id, space->def->name);
+ }
+}
+
/*
* This routine is called to report the final ")" that terminates
* a CREATE TABLE statement.
@@ -1213,73 +1469,7 @@ sqlEndTable(struct Parse *pParse)
int reg_space_id = getNewSpaceId(pParse);
vdbe_emit_space_create(pParse, reg_space_id, name_reg, new_space);
- for (uint32_t i = 0; i < new_space->index_count; ++i) {
- struct index *idx = new_space->index[i];
- vdbe_emit_create_index(pParse, new_space->def, idx->def,
- reg_space_id, idx->def->iid);
- }
-
- /*
- * Check to see if we need to create an _sequence table
- * for keeping track of autoincrement keys.
- */
- if (pParse->create_table_def.has_autoinc) {
- assert(reg_space_id != 0);
- /* Do an insertion into _sequence. */
- int reg_seq_id = ++pParse->nMem;
- sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id);
- int reg_seq_record =
- emitNewSysSequenceRecord(pParse, reg_seq_id,
- 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);
- sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
- reg_space_seq_record);
- }
- /* Code creation of FK constraints, if any. */
- struct fk_constraint_parse *fk_parse;
- rlist_foreach_entry(fk_parse, &pParse->create_table_def.new_fkey,
- link) {
- struct fk_constraint_def *fk_def = fk_parse->fk_def;
- if (fk_parse->selfref_cols != NULL) {
- struct ExprList *cols = fk_parse->selfref_cols;
- for (uint32_t i = 0; i < fk_def->field_count; ++i) {
- if (resolve_link(pParse, new_space->def,
- cols->a[i].zName,
- &fk_def->links[i].parent_field,
- fk_def->name) != 0)
- return;
- }
- fk_def->parent_id = reg_space_id;
- } else if (fk_parse->is_self_referenced) {
- struct index *pk = sql_space_primary_key(new_space);
- if (pk->def->key_def->part_count != fk_def->field_count) {
- diag_set(ClientError, ER_CREATE_FK_CONSTRAINT,
- fk_def->name, "number of columns in "\
- "foreign key does not match the "\
- "number of columns in the primary "\
- "index of referenced table");
- pParse->is_aborted = true;
- return;
- }
- for (uint32_t i = 0; i < fk_def->field_count; ++i) {
- fk_def->links[i].parent_field =
- pk->def->key_def->parts[i].fieldno;
- }
- fk_def->parent_id = reg_space_id;
- }
- fk_def->child_id = reg_space_id;
- vdbe_emit_fk_constraint_create(pParse, fk_def, space_name_copy);
- }
- struct ck_constraint_parse *ck_parse;
- rlist_foreach_entry(ck_parse, &pParse->create_table_def.new_check,
- link) {
- vdbe_emit_ck_constraint_create(pParse, ck_parse->ck_def,
- reg_space_id, space_name_copy);
- }
+ sql_vdbe_create_constraints(pParse, reg_space_id);
}
void
@@ -1858,24 +2048,28 @@ sql_create_foreign_key(struct Parse *parse_context)
char *parent_name = NULL;
char *constraint_name = NULL;
bool is_self_referenced = false;
+ struct space *space = parse_context->create_column_def.space;
struct create_table_def *table_def = &parse_context->create_table_def;
- struct space *space = table_def->new_space;
+ if (space == NULL)
+ space = table_def->new_space;
/*
- * Space under construction during CREATE TABLE
- * processing. NULL for ALTER TABLE statement handling.
+ * Space under construction during <CREATE TABLE>
+ * processing or shallow copy of space during <ALTER TABLE
+ * ... ADD COLUMN>. NULL for <ALTER TABLE ... ADD
+ * CONSTRAINT> statement handling.
*/
- bool is_alter = space == NULL;
+ bool is_alter_add_constr = space == NULL;
uint32_t child_cols_count;
struct ExprList *child_cols = create_fk_def->child_cols;
if (child_cols == NULL) {
- assert(!is_alter);
+ assert(!is_alter_add_constr);
child_cols_count = 1;
} else {
child_cols_count = child_cols->nExpr;
}
struct ExprList *parent_cols = create_fk_def->parent_cols;
struct space *child_space = NULL;
- if (is_alter) {
+ if (is_alter_add_constr) {
const char *child_name = alter_def->entity_name->a[0].zName;
child_space = space_by_name(child_name);
if (child_space == NULL) {
@@ -1893,7 +2087,9 @@ sql_create_foreign_key(struct Parse *parse_context)
goto tnt_error;
}
memset(fk_parse, 0, sizeof(*fk_parse));
- rlist_add_entry(&table_def->new_fkey, fk_parse, link);
+ if (parse_context->create_column_def.space != NULL)
+ child_space = space;
+ rlist_add_entry(&parse_context->fkeys, fk_parse, link);
}
struct Token *parent = create_fk_def->parent_name;
assert(parent != NULL);
@@ -1905,28 +2101,39 @@ sql_create_foreign_key(struct Parse *parse_context)
* self-referenced, but in this case parent (which is
* also child) table will definitely exist.
*/
- is_self_referenced = !is_alter &&
+ is_self_referenced = !is_alter_add_constr &&
strcmp(parent_name, space->def->name) == 0;
struct space *parent_space = space_by_name(parent_name);
- if (parent_space == NULL) {
- if (is_self_referenced) {
- struct fk_constraint_parse *fk =
- rlist_first_entry(&table_def->new_fkey,
- struct fk_constraint_parse,
- link);
- fk->selfref_cols = parent_cols;
- fk->is_self_referenced = true;
- } else {
- diag_set(ClientError, ER_NO_SUCH_SPACE, parent_name);;
- goto tnt_error;
- }
+ if (parent_space == NULL && !is_self_referenced) {
+ diag_set(ClientError, ER_NO_SUCH_SPACE, parent_name);;
+ goto tnt_error;
+ }
+ if (is_self_referenced) {
+ struct fk_constraint_parse *fk =
+ rlist_first_entry(&parse_context->fkeys,
+ struct fk_constraint_parse,
+ link);
+ fk->selfref_cols = parent_cols;
+ fk->is_self_referenced = true;
}
- if (!is_alter) {
+ if (!is_alter_add_constr) {
if (create_def->name.n == 0) {
- constraint_name =
- sqlMPrintf(db, "fk_unnamed_%s_%d",
- space->def->name,
- ++table_def->fkey_count);
+ uint32_t idx = ++parse_context->fkey_count;
+ if (table_def->new_space == NULL) {
+ struct space *original_space =
+ space_by_name(space->def->name);
+ assert(original_space != NULL);
+ struct rlist *child_fk =
+ &original_space->child_fk_constraint;
+ if (!rlist_empty(child_fk)) {
+ struct fk_constraint *fk;
+ rlist_foreach_entry(fk, child_fk,
+ in_child_space)
+ idx++;
+ }
+ }
+ constraint_name = sqlMPrintf(db, "fk_unnamed_%s_%d",
+ space->def->name, idx);
} else {
constraint_name =
sql_name_from_token(db, &create_def->name);
@@ -1986,7 +2193,8 @@ sql_create_foreign_key(struct Parse *parse_context)
}
int actions = create_fk_def->actions;
fk_def->field_count = child_cols_count;
- fk_def->child_id = child_space != NULL ? child_space->def->id : 0;
+ fk_def->child_id = table_def->new_space == NULL ?
+ child_space->def->id : 0;
fk_def->parent_id = parent_space != NULL ? parent_space->def->id : 0;
fk_def->is_deferred = create_constr_def->is_deferred;
fk_def->match = (enum fk_constraint_match) (create_fk_def->match);
@@ -2006,7 +2214,7 @@ sql_create_foreign_key(struct Parse *parse_context)
constraint_name) != 0) {
goto exit_create_fk;
}
- if (!is_alter) {
+ if (!is_alter_add_constr) {
if (child_cols == NULL) {
assert(i == 0);
/*
@@ -2035,14 +2243,15 @@ sql_create_foreign_key(struct Parse *parse_context)
memcpy(fk_def->name, constraint_name, name_len);
fk_def->name[name_len] = '\0';
/*
- * In case of CREATE TABLE processing, all foreign keys
- * constraints must be created after space itself, so
- * lets delay it until sqlEndTable() call and simply
+ * In case of <CREATE TABLE> or <ALTER TABLE ... ADD
+ * COLUMN> processing, all foreign keys constraints must
+ * be created after space itself, so lets delay it until
+ * sqlEndTable() or sql_add_column_end() call and simply
* maintain list of all FK constraints inside parser.
*/
- if (!is_alter) {
+ if (!is_alter_add_constr) {
struct fk_constraint_parse *fk_parse =
- rlist_first_entry(&table_def->new_fkey,
+ rlist_first_entry(&parse_context->fkeys,
struct fk_constraint_parse, link);
fk_parse->fk_def = fk_def;
} else {
@@ -2065,12 +2274,10 @@ tnt_error:
void
fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
{
- if (parse_context->db->init.busy ||
- rlist_empty(&parse_context->create_table_def.new_fkey))
+ if (parse_context->db->init.busy || rlist_empty(&parse_context->fkeys))
return;
- rlist_first_entry(&parse_context->create_table_def.new_fkey,
- struct fk_constraint_parse, link)->fk_def->is_deferred =
- is_deferred;
+ rlist_first_entry(&parse_context->fkeys, struct fk_constraint_parse,
+ link)->fk_def->is_deferred = is_deferred;
}
/**
@@ -2394,7 +2601,10 @@ sql_create_index(struct Parse *parse) {
* Find the table that is to be indexed.
* Return early if not found.
*/
- struct space *space = NULL;
+ struct space *space = parse->create_table_def.new_space;
+ if (space == NULL)
+ space = parse->create_column_def.space;
+ bool is_create_table_or_add_col = space != NULL;
struct Token token = create_entity_def->name;
if (tbl_name != NULL) {
assert(token.n > 0 && token.z != NULL);
@@ -2407,10 +2617,8 @@ sql_create_index(struct Parse *parse) {
}
goto exit_create_index;
}
- } else {
- if (parse->create_table_def.new_space == NULL)
- goto exit_create_index;
- space = parse->create_table_def.new_space;
+ } else if (space == NULL) {
+ goto exit_create_index;
}
struct space_def *def = space->def;
@@ -2445,7 +2653,7 @@ sql_create_index(struct Parse *parse) {
* 2) UNIQUE constraint is non-named and standard
* auto-index name will be generated.
*/
- if (parse->create_table_def.new_space == NULL) {
+ if (!is_create_table_or_add_col) {
assert(token.z != NULL);
name = sql_name_from_token(db, &token);
if (name == NULL) {
@@ -2611,7 +2819,7 @@ sql_create_index(struct Parse *parse) {
* constraint, but has different onError (behavior on
* constraint violation), then an error is raised.
*/
- if (parse->create_table_def.new_space != NULL) {
+ if (is_create_table_or_add_col) {
for (uint32_t i = 0; i < space->index_count; ++i) {
struct index *existing_idx = space->index[i];
uint32_t iid = existing_idx->def->iid;
@@ -2699,7 +2907,7 @@ sql_create_index(struct Parse *parse) {
sqlVdbeAddOp0(vdbe, OP_Expire);
}
- if (tbl_name != NULL)
+ if (!is_create_table_or_add_col)
goto exit_create_index;
table_add_index(space, index);
index = NULL;
@@ -3306,15 +3514,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.y b/src/box/sql/parse.y
index 995875566..5904414a3 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -226,19 +226,23 @@ create_table_end ::= . { sqlEndTable(pParse); }
*/
columnlist ::= columnlist COMMA tcons.
-columnlist ::= columnlist COMMA columnname carglist autoinc(I). {
- uint32_t fieldno = pParse->create_table_def.new_space->def->field_count - 1;
- if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 0)
- return;
+columnlist ::= columnlist COMMA column_def create_column_end.
+columnlist ::= column_def create_column_end.
+columnlist ::= tcons.
+
+column_def ::= column_name_and_type carglist.
+
+column_name_and_type ::= nm(A) typedef(Y). {
+ create_column_def_init(&pParse->create_column_def, NULL, &A, &Y);
+ sql_create_column_start(pParse);
}
-columnlist ::= columnname carglist autoinc(I). {
- uint32_t fieldno = pParse->create_table_def.new_space->def->field_count - 1;
+create_column_end ::= autoinc(I). {
+ uint32_t fieldno = pParse->create_column_def.space->def->field_count - 1;
if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 0)
return;
+ sql_create_column_end(pParse);
}
-columnlist ::= tcons.
-columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);}
// An IDENTIFIER can be a generic identifier, or one of several
// keywords. Any non-standard keyword can also be an identifier.
@@ -281,9 +285,11 @@ nm(A) ::= id(A). {
}
}
-// "carglist" is a list of additional constraints that come after the
-// column name and column type in a CREATE TABLE statement.
-//
+/*
+ * "carglist" is a list of additional constraints and clauses that
+ * come after the column name and column type in a <CREATE TABLE>
+ * or <ALTER TABLE ADD COLUMN> statement.
+ */
carglist ::= carglist ccons.
carglist ::= .
%type cconsname { struct Token }
@@ -314,6 +320,7 @@ 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);
+ pParse->create_column_def.is_pk = true;
}
ccons ::= cconsname(N) UNIQUE. {
create_index_def_init(&pParse->create_index_def, NULL, &N, NULL,
@@ -1735,11 +1742,28 @@ alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; }
%type alter_add_constraint {struct alter_args}
alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). {
+ A.table_name = T;
+ A.name = N;
+ pParse->initiateTTrans = true;
+ }
+
+%type alter_add_column {struct alter_args}
+alter_add_column(A) ::= alter_table_start(T) ADD column_name(N). {
A.table_name = T;
A.name = N;
pParse->initiateTTrans = true;
}
+column_name(N) ::= COLUMN nm(A). { N = A; }
+column_name(N) ::= nm(A). { N = A; }
+
+cmd ::= alter_column_def carglist create_column_end.
+
+alter_column_def ::= alter_add_column(N) typedef(Y). {
+ create_column_def_init(&pParse->create_column_def, N.table_name, &N.name, &Y);
+ sql_create_column_start(pParse);
+}
+
cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES
nm(T) eidlist_opt(TA) matcharg(M) refargs(R) defer_subclause_opt(D). {
create_fk_def_init(&pParse->create_fk_def, N.table_name, &N.name, FA, &T, TA,
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index cb0ecd2fc..91f9affa2 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -35,6 +35,7 @@
#include "box/fk_constraint.h"
#include "box/key_def.h"
#include "box/sql.h"
+#include "box/constraint_id.h"
/**
* This file contains auxiliary structures and functions which
@@ -154,6 +155,7 @@ enum sql_index_type {
enum entity_type {
ENTITY_TYPE_TABLE = 0,
+ ENTITY_TYPE_COLUMN,
ENTITY_TYPE_VIEW,
ENTITY_TYPE_INDEX,
ENTITY_TYPE_TRIGGER,
@@ -205,26 +207,22 @@ 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_column_def {
+ struct create_entity_def base;
+ /** Shallow space_def copy. */
+ struct space *space;
+ /* True if this column has <PRIMARY KEY> constraint. */
+ bool is_pk;
+ /** Column type. */
+ struct type_def *type_def;
+ /** Token from <COLLATE> clause. */
+ struct Token *collate;
+ /** Action to perform if NULL constraint failed. */
+ enum on_conflict_action nullable_action;
+ /** String from <DEFAULT> clause. */
+ struct ExprSpan *default_clause;
};
struct create_view_def {
@@ -482,9 +480,17 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
{
create_entity_def_init(&table_def->base, ENTITY_TYPE_TABLE, NULL, name,
if_not_exists);
- rlist_create(&table_def->new_fkey);
- rlist_create(&table_def->new_check);
- table_def->autoinc_fieldno = 0;
+}
+
+static inline void
+create_column_def_init(struct create_column_def *column_def,
+ struct SrcList *table_name, struct Token *name,
+ struct type_def *type_def)
+{
+ create_entity_def_init(&column_def->base, ENTITY_TYPE_COLUMN,
+ table_name, name, false);
+ column_def->type_def = type_def;
+ column_def->is_pk = false;
}
static inline void
@@ -499,14 +505,4 @@ create_view_def_init(struct create_view_def *view_def, struct Token *name,
view_def->aliases = aliases;
}
-static inline void
-create_table_def_destroy(struct create_table_def *table_def)
-{
- if (table_def->new_space == NULL)
- return;
- struct fk_constraint_parse *fk;
- rlist_foreach_entry(fk, &table_def->new_fkey, link)
- sql_expr_list_delete(sql_get(), fk->selfref_cols);
-}
-
#endif /* TARANTOOL_BOX_SQL_PARSE_DEF_H_INCLUDED */
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index a5a258805..b31bac437 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -197,9 +197,13 @@ sql_parser_create(struct Parse *parser, struct sql *db, uint32_t sql_flags)
{
memset(parser, 0, sizeof(struct Parse));
parser->db = db;
+ parser->create_column_def.nullable_action = ON_CONFLICT_ACTION_DEFAULT;
parser->sql_flags = sql_flags;
parser->line_count = 1;
parser->line_pos = 1;
+ rlist_create(&parser->fkeys);
+ rlist_create(&parser->checks);
+ parser->has_autoinc = false;
region_create(&parser->region, &cord()->slabc);
}
@@ -211,7 +215,9 @@ sql_parser_destroy(Parse *parser)
sql *db = parser->db;
sqlDbFree(db, parser->aLabel);
sql_expr_list_delete(db, parser->pConstExpr);
- create_table_def_destroy(&parser->create_table_def);
+ struct fk_constraint_parse *fk;
+ rlist_foreach_entry(fk, &parser->fkeys, link)
+ sql_expr_list_delete(sql_get(), fk->selfref_cols);
if (db != NULL) {
assert(db->lookaside.bDisable >=
parser->disableLookaside);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index aa6a470f8..3143ec521 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2249,12 +2249,26 @@ struct Parse {
struct enable_entity_def enable_entity_def;
};
/**
- * Table def is not part of union since information
- * being held must survive till the end of parsing of
- * whole CREATE TABLE statement (to pass it to
- * sqlEndTable() function).
+ * Table def or column def is not part of union since
+ * information being held must survive till the end of
+ * parsing of whole <CREATE TABLE> or
+ * <ALTER TABLE ADD COLUMN> statement (to pass it to
+ * sqlEndTable() sql_create_column_end() function).
*/
struct create_table_def create_table_def;
+ struct create_column_def create_column_def;
+ /**
+ * FK and CK constraints appeared in a <CREATE TABLE> or
+ * a <ALTER TABLE ADD COLUMN> statement.
+ */
+ struct rlist fkeys;
+ struct rlist checks;
+ uint32_t fkey_count;
+ uint32_t check_count;
+ /** True, if column to be created has <AUTOINCREMENT>. */
+ bool has_autoinc;
+ /** Id of field with <AUTOINCREMENT>. */
+ uint32_t autoinc_fieldno;
bool initiateTTrans; /* Initiate Tarantool transaction */
/** If set - do not emit byte code at all, just parse. */
bool parse_only;
@@ -2844,15 +2858,31 @@ struct space *sqlResultSetOfSelect(Parse *, Select *);
struct space *
sqlStartTable(Parse *, Token *);
-void sqlAddColumn(Parse *, Token *, struct type_def *);
+
+/**
+ * Add new field to the format of ephemeral space in
+ * create_table_def. If it is <ALTER TABLE> create shallow copy of
+ * the existing space and add field to its format.
+ */
+void
+sql_create_column_start(struct Parse *parse);
+
+/**
+ * Nullify create_column_def. If it is <ALTER TABLE> emit code to
+ * update entry in _space and code to create constraints (entries
+ * in _index, _ck_constraint, _fk_constraint) described with this
+ * column.
+ */
+void
+sql_create_column_end(struct Parse *parse);
/**
* This routine is called by the parser while in the middle of
- * parsing a CREATE TABLE statement. A "NOT NULL" constraint has
- * been seen on a column. This routine sets the is_nullable flag
- * on the column currently under construction.
- * If nullable_action has been already set, this function raises
- * an error.
+ * parsing a <CREATE TABLE> or a <ALTER TABLE ADD COLUMN>
+ * statement. A "NOT NULL" constraint has been seen on a column.
+ * This routine sets the is_nullable flag on the column currently
+ * under construction. If nullable_action has been already set,
+ * this function raises an error.
*
* @param parser SQL Parser object.
* @param nullable_action on_conflict_action value.
diff --git a/test/box/error.result b/test/box/error.result
index 2196fa541..eb55f9cdf 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -432,6 +432,8 @@ t;
| 211: box.error.WRONG_QUERY_ID
| 212: box.error.SEQUENCE_NOT_STARTED
| 213: box.error.NO_SUCH_SESSION_SETTING
+ | 214: box.error.SQL_CANT_ADD_COLUMN_TO_VIEW
+ | 215: box.error.SQL_CANT_ADD_AUTOINC
| ...
test_run:cmd("setopt delimiter ''");
diff --git a/test/sql/add-column.result b/test/sql/add-column.result
new file mode 100644
index 000000000..06c95facb
--- /dev/null
+++ b/test/sql/add-column.result
@@ -0,0 +1,276 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+_ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}})
+ | ---
+ | ...
+
+--
+-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
+--
+box.execute('CREATE TABLE t1 (a INTEGER PRIMARY KEY);')
+ | ---
+ | - row_count: 1
+ | ...
+--
+-- COLUMN keyword is optional. Check it here, but omit it below.
+--
+box.execute('ALTER TABLE t1 ADD COLUMN b INTEGER;')
+ | ---
+ | - row_count: 0
+ | ...
+
+--
+-- Can't add column to a view.
+--
+box.execute('CREATE VIEW v AS SELECT * FROM t1;')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE v ADD b INTEGER;')
+ | ---
+ | - null
+ | - Can't add column 'B'. 'V' is a view
+ | ...
+box.execute('DROP VIEW v;')
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check column constraints typing and work.
+--
+box.execute('CREATE TABLE t2 (a INTEGER CONSTRAINT pk_constr PRIMARY KEY);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT pk_constr')
+ | ---
+ | - row_count: 1
+ | ...
+test_run:cmd("setopt delimiter ';'");
+ | ---
+ | - true
+ | ...
+box.execute([[ALTER TABLE t2 ADD b INTEGER CONSTRAINT pk_constr PRIMARY KEY
+ CHECK (b > 0)
+ REFERENCES t1(a)
+ CONSTRAINT u_constr UNIQUE]])
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+box.execute('INSERT INTO t1 VALUES (1, 1);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('INSERT INTO t2 VALUES (1, 1);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('INSERT INTO t2 VALUES (1, 1);')
+ | ---
+ | - null
+ | - Duplicate key exists in unique index 'PK_CONSTR' in space 'T2'
+ | ...
+
+box.execute('INSERT INTO t1 VALUES (0, 1);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('INSERT INTO t2 VALUES (2, 0);')
+ | ---
+ | - null
+ | - 'Check constraint failed ''ck_unnamed_T2_1'': b > 0'
+ | ...
+
+box.execute('INSERT INTO t2 VALUES (2, 3);')
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | ...
+
+box.execute('DROP TABLE t2;')
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check self-referenced FK creation.
+--
+box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 ADD b INT REFERENCES t1')
+ | ---
+ | - row_count: 0
+ | ...
+
+box.execute('DROP TABLE t2;')
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check AUTOINCREMENT work.
+--
+box.execute("CREATE TABLE t2(a INTEGER CONSTRAINT pk PRIMARY KEY);")
+ | ---
+ | - row_count: 1
+ | ...
+box.execute("ALTER TABLE t2 DROP CONSTRAINT pk;")
+ | ---
+ | - row_count: 1
+ | ...
+box.execute("ALTER TABLE t2 ADD b INTEGER PRIMARY KEY AUTOINCREMENT;")
+ | ---
+ | - row_count: 0
+ | ...
+box.execute("ALTER TABLE t2 ADD c INTEGER AUTOINCREMENT;")
+ | ---
+ | - null
+ | - 'Can''t add AUTOINCREMENT: the space ''T2'' already has one auto-incremented field'
+ | ...
+
+box.execute('DROP TABLE t2;')
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check clauses after column typing and work.
+--
+box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY, b INTEGER);')
+ | ---
+ | - row_count: 1
+ | ...
+test_run:cmd("setopt delimiter ';'");
+ | ---
+ | - true
+ | ...
+box.execute([[ALTER TABLE t2 ADD c TEXT NOT NULL DEFAULT ('a')
+ COLLATE "unicode_ci";]]);
+ | ---
+ | - row_count: 0
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+box.execute('INSERT INTO t2(a, b) VALUES (1, 1);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('SELECT * FROM t2;')
+ | ---
+ | - metadata:
+ | - name: A
+ | type: integer
+ | - name: B
+ | type: integer
+ | - name: C
+ | type: string
+ | rows:
+ | - [1, 1, 'a']
+ | ...
+box.execute('INSERT INTO t2 VALUES (2, 2, NULL);')
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: NOT NULL constraint failed: T2.C'
+ | ...
+box.execute('SELECT * FROM t2 WHERE c LIKE \'A\';')
+ | ---
+ | - metadata:
+ | - name: A
+ | type: integer
+ | - name: B
+ | type: integer
+ | - name: C
+ | type: string
+ | rows:
+ | - [1, 1, 'a']
+ | ...
+
+--
+-- Try to add to a non-empty space a [non-]nullable field.
+--
+box.execute('ALTER TABLE t2 ADD d INTEGER;')
+ | ---
+ | - null
+ | - Tuple field count 3 does not match space field count 4
+ | ...
+box.execute('ALTER TABLE t2 ADD d TEXT NOT NULL');
+ | ---
+ | - null
+ | - Tuple field count 3 does not match space field count 4
+ | ...
+box.execute('ALTER TABLE t2 ADD e TEXT NULL');
+ | ---
+ | - null
+ | - Tuple field count 3 does not match space field count 4
+ | ...
+
+--
+-- Add to a space with no-SQL adjusted or without format.
+--
+_ = box.schema.space.create('WITHOUT')
+ | ---
+ | ...
+box.execute("ALTER TABLE WITHOUT ADD a INTEGER;")
+ | ---
+ | - row_count: 0
+ | ...
+box.execute("DROP TABLE WITHOUT;")
+ | ---
+ | - row_count: 1
+ | ...
+
+s = box.schema.space.create('NOSQL')
+ | ---
+ | ...
+s:format{{name = 'A', type = 'unsigned'}}
+ | ---
+ | ...
+box.execute("ALTER TABLE NOSQL ADD b INTEGER")
+ | ---
+ | - row_count: 0
+ | ...
+
+box.execute('DROP TABLE t2;')
+ | ---
+ | - row_count: 1
+ | ...
+--
+-- Add multiple columns inside a transaction.
+--
+box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY)')
+ | ---
+ | - row_count: 1
+ | ...
+box.begin() \
+box.execute('ALTER TABLE t2 ADD b INT') \
+box.execute('ALTER TABLE t2 ADD c INT') \
+box.commit()
+ | ---
+ | ...
+
+box.execute('INSERT INTO t2 VALUES (1, 1, 1)')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('SELECT * FROM t2;')
+ | ---
+ | - metadata:
+ | - name: A
+ | type: integer
+ | - name: B
+ | type: integer
+ | - name: C
+ | type: integer
+ | rows:
+ | - [1, 1, 1]
+ | ...
diff --git a/test/sql/add-column.test.lua b/test/sql/add-column.test.lua
new file mode 100644
index 000000000..f0ce1c44c
--- /dev/null
+++ b/test/sql/add-column.test.lua
@@ -0,0 +1,103 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+_ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}})
+
+--
+-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
+--
+box.execute('CREATE TABLE t1 (a INTEGER PRIMARY KEY);')
+--
+-- COLUMN keyword is optional. Check it here, but omit it below.
+--
+box.execute('ALTER TABLE t1 ADD COLUMN b INTEGER;')
+
+--
+-- Can't add column to a view.
+--
+box.execute('CREATE VIEW v AS SELECT * FROM t1;')
+box.execute('ALTER TABLE v ADD b INTEGER;')
+box.execute('DROP VIEW v;')
+
+--
+-- Check column constraints typing and work.
+--
+box.execute('CREATE TABLE t2 (a INTEGER CONSTRAINT pk_constr PRIMARY KEY);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT pk_constr')
+test_run:cmd("setopt delimiter ';'");
+box.execute([[ALTER TABLE t2 ADD b INTEGER CONSTRAINT pk_constr PRIMARY KEY
+ CHECK (b > 0)
+ REFERENCES t1(a)
+ CONSTRAINT u_constr UNIQUE]])
+test_run:cmd("setopt delimiter ''");
+box.execute('INSERT INTO t1 VALUES (1, 1);')
+box.execute('INSERT INTO t2 VALUES (1, 1);')
+box.execute('INSERT INTO t2 VALUES (1, 1);')
+
+box.execute('INSERT INTO t1 VALUES (0, 1);')
+box.execute('INSERT INTO t2 VALUES (2, 0);')
+
+box.execute('INSERT INTO t2 VALUES (2, 3);')
+
+box.execute('DROP TABLE t2;')
+
+--
+-- Check self-referenced FK creation.
+--
+box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY);')
+box.execute('ALTER TABLE t2 ADD b INT REFERENCES t1')
+
+box.execute('DROP TABLE t2;')
+
+--
+-- Check AUTOINCREMENT work.
+--
+box.execute("CREATE TABLE t2(a INTEGER CONSTRAINT pk PRIMARY KEY);")
+box.execute("ALTER TABLE t2 DROP CONSTRAINT pk;")
+box.execute("ALTER TABLE t2 ADD b INTEGER PRIMARY KEY AUTOINCREMENT;")
+box.execute("ALTER TABLE t2 ADD c INTEGER AUTOINCREMENT;")
+
+box.execute('DROP TABLE t2;')
+
+--
+-- Check clauses after column typing and work.
+--
+box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY, b INTEGER);')
+test_run:cmd("setopt delimiter ';'");
+box.execute([[ALTER TABLE t2 ADD c TEXT NOT NULL DEFAULT ('a')
+ COLLATE "unicode_ci";]]);
+test_run:cmd("setopt delimiter ''");
+box.execute('INSERT INTO t2(a, b) VALUES (1, 1);')
+box.execute('SELECT * FROM t2;')
+box.execute('INSERT INTO t2 VALUES (2, 2, NULL);')
+box.execute('SELECT * FROM t2 WHERE c LIKE \'A\';')
+
+--
+-- Try to add to a non-empty space a [non-]nullable field.
+--
+box.execute('ALTER TABLE t2 ADD d INTEGER;')
+box.execute('ALTER TABLE t2 ADD d TEXT NOT NULL');
+box.execute('ALTER TABLE t2 ADD e TEXT NULL');
+
+--
+-- Add to a space with no-SQL adjusted or without format.
+--
+_ = box.schema.space.create('WITHOUT')
+box.execute("ALTER TABLE WITHOUT ADD a INTEGER;")
+box.execute("DROP TABLE WITHOUT;")
+
+s = box.schema.space.create('NOSQL')
+s:format{{name = 'A', type = 'unsigned'}}
+box.execute("ALTER TABLE NOSQL ADD b INTEGER")
+
+box.execute('DROP TABLE t2;')
+--
+-- Add multiple columns inside a transaction.
+--
+box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY)')
+box.begin() \
+box.execute('ALTER TABLE t2 ADD b INT') \
+box.execute('ALTER TABLE t2 ADD c INT') \
+box.commit()
+
+box.execute('INSERT INTO t2 VALUES (1, 1, 1)')
+box.execute('SELECT * FROM t2;')
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-06 13:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-31 15:05 [Tarantool-patches] [PATCH] sql: support column addition Roman Khabibov
2020-03-09 15:08 ` Roman Khabibov
2020-03-09 22:16 ` Vladislav Shpilevoy
2020-03-14 17:17 ` Vladislav Shpilevoy
2020-07-06 13:37 ` Roman Khabibov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox