* [Tarantool-patches] [PATCH v3 0/4] Support ALTER TABLE ADD COLUMN
@ 2020-08-11 0:33 Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 1/4] sql: rename TK_COLUMN to TK_COLUMN_NAME Roman Khabibov
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Roman Khabibov @ 2020-08-11 0:33 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
I split the last patch into three patches for convenience.
@ChangeLog
- Added <ALTER TABLE ADD COLUMN> statement support. Column description is the same as in <CREATE TABLE> statement (gh-2349, gh-3075).
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3075-add-column-v2
Issue: https://github.com/tarantool/tarantool/issues/3075
https://github.com/tarantool/tarantool/issues/2349
Roman Khabibov (4):
sql: rename TK_COLUMN to TK_COLUMN_NAME
sql: refactor create_table_def and parse
schema: add box_space_field_MAX
sql: support column addition
extra/addopcodes.sh | 2 +-
extra/mkkeywordhash.c | 7 +-
src/box/errcode.h | 2 +
src/box/schema_def.h | 1 +
src/box/sql/build.c | 602 ++++++++++++++++++++++-----------
src/box/sql/expr.c | 45 +--
src/box/sql/fk_constraint.c | 2 +-
src/box/sql/parse.y | 44 ++-
src/box/sql/parse_def.h | 53 ++-
src/box/sql/prepare.c | 7 +-
src/box/sql/resolve.c | 10 +-
src/box/sql/select.c | 10 +-
src/box/sql/sqlInt.h | 61 +++-
src/box/sql/treeview.c | 2 +-
src/box/sql/where.c | 18 +-
src/box/sql/whereexpr.c | 12 +-
test/box/error.result | 2 +
test/sql/add-column.result | 471 ++++++++++++++++++++++++++
test/sql/add-column.test.sql | 167 +++++++++
test/sql/checks.result | 20 ++
test/sql/checks.test.lua | 9 +
test/sql/foreign-keys.result | 28 ++
test/sql/foreign-keys.test.lua | 11 +
23 files changed, 1275 insertions(+), 311 deletions(-)
create mode 100644 test/sql/add-column.result
create mode 100644 test/sql/add-column.test.sql
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Tarantool-patches] [PATCH v3 1/4] sql: rename TK_COLUMN to TK_COLUMN_NAME
2020-08-11 0:33 [Tarantool-patches] [PATCH v3 0/4] Support ALTER TABLE ADD COLUMN Roman Khabibov
@ 2020-08-11 0:33 ` Roman Khabibov
2020-08-19 22:20 ` Vladislav Shpilevoy
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 2/4] sql: refactor create_table_def and parse Roman Khabibov
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Roman Khabibov @ 2020-08-11 0:33 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
Rename TK_COLUMN used for tokens treated as a column name to
TK_COLUMN_NAME. It is needed to allow the typing of COLUMN
keyword in <ALTER TABLE ADD COLUMN> statement.
Needed for #3075
---
| 2 +-
| 5 +----
src/box/sql/build.c | 2 +-
src/box/sql/expr.c | 45 +++++++++++++++++++------------------
src/box/sql/fk_constraint.c | 2 +-
src/box/sql/resolve.c | 10 ++++-----
src/box/sql/select.c | 10 ++++-----
src/box/sql/sqlInt.h | 8 +++----
src/box/sql/treeview.c | 2 +-
src/box/sql/where.c | 18 ++++++++-------
src/box/sql/whereexpr.c | 12 +++++-----
11 files changed, 58 insertions(+), 58 deletions(-)
--git a/extra/addopcodes.sh b/extra/addopcodes.sh
index cb6c84725..986c62683 100755
--- a/extra/addopcodes.sh
+++ b/extra/addopcodes.sh
@@ -39,7 +39,7 @@ extras=" \
END_OF_FILE \
UNCLOSED_STRING \
FUNCTION \
- COLUMN \
+ COLUMN_NAME \
AGG_FUNCTION \
AGG_COLUMN \
UMINUS \
--git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index dd42c8f5f..486b6b30d 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -75,10 +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_NAME", "TK_COLUMN_NAME", true },
{ "COLUMN", "TK_STANDARD", true },
{ "COMMIT", "TK_COMMIT", true },
{ "CONFLICT", "TK_CONFLICT", false },
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 8f6b403b9..619bbf8e3 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2295,7 +2295,7 @@ index_fill_def(struct Parse *parse, struct index *index,
goto cleanup;
struct Expr *column_expr = sqlExprSkipCollate(expr);
- if (column_expr->op != TK_COLUMN) {
+ if (column_expr->op != TK_COLUMN_NAME) {
diag_set(ClientError, ER_UNSUPPORTED, "Tarantool",
"functional indexes");
goto tnt_error;
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index bc2182446..d389b3daf 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -62,7 +62,7 @@ sql_expr_type(struct Expr *pExpr)
assert(!ExprHasProperty(pExpr, EP_IntValue));
return pExpr->type;
case TK_AGG_COLUMN:
- case TK_COLUMN:
+ case TK_COLUMN_NAME:
case TK_TRIGGER:
assert(pExpr->iColumn >= 0);
return pExpr->space_def->fields[pExpr->iColumn].type;
@@ -262,13 +262,13 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
*is_explicit_coll = true;
break;
}
- if ((op == TK_AGG_COLUMN || op == TK_COLUMN ||
+ if ((op == TK_AGG_COLUMN || op == TK_COLUMN_NAME ||
op == TK_REGISTER || op == TK_TRIGGER) &&
p->space_def != NULL) {
/*
* op==TK_REGISTER && p->space_def!=0
* happens when pExpr was originally
- * a TK_COLUMN but was previously
+ * a TK_COLUMN_NAME but was previously
* evaluated and cached in a register.
*/
int j = p->iColumn;
@@ -2061,11 +2061,11 @@ exprNodeIsConstant(Walker * pWalker, Expr * pExpr)
return WRC_Abort;
}
case TK_ID:
- case TK_COLUMN:
+ case TK_COLUMN_NAME:
case TK_AGG_FUNCTION:
case TK_AGG_COLUMN:
testcase(pExpr->op == TK_ID);
- testcase(pExpr->op == TK_COLUMN);
+ testcase(pExpr->op == TK_COLUMN_NAME);
testcase(pExpr->op == TK_AGG_FUNCTION);
testcase(pExpr->op == TK_AGG_COLUMN);
if (pWalker->eCode == 3 && pExpr->iTable == pWalker->u.iCur) {
@@ -2236,7 +2236,7 @@ sqlExprCanBeNull(const Expr * p)
case TK_FLOAT:
case TK_BLOB:
return 0;
- case TK_COLUMN:
+ case TK_COLUMN_NAME:
assert(p->space_def != 0);
return ExprHasProperty(p, EP_CanBeNull) ||
(p->iColumn >= 0
@@ -2267,7 +2267,7 @@ sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)
return type == FIELD_TYPE_STRING;
case TK_BLOB:
return type == FIELD_TYPE_VARBINARY;
- case TK_COLUMN:
+ case TK_COLUMN_NAME:
/* p cannot be part of a CHECK constraint. */
assert(p->iTable >= 0);
return p->iColumn < 0 && sql_type_is_numeric(type);
@@ -2324,7 +2324,7 @@ isCandidateForInOpt(Expr * pX)
/* All SELECT results must be columns. */
for (i = 0; i < pEList->nExpr; i++) {
Expr *pRes = pEList->a[i].pExpr;
- if (pRes->op != TK_COLUMN)
+ if (pRes->op != TK_COLUMN_NAME)
return 0;
assert(pRes->iTable == pSrc->a[0].iCursor); /* Not a correlated subquery */
}
@@ -3707,10 +3707,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
}
return target;
}
- /* Otherwise, fall thru into the TK_COLUMN case */
+ /* Otherwise, fall thru into the TK_COLUMN_NAME case */
FALLTHROUGH;
}
- case TK_COLUMN:{
+ case TK_COLUMN_NAME:{
int iTab = pExpr->iTable;
int col = pExpr->iColumn;
if (iTab < 0) {
@@ -4102,7 +4102,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
assert(nFarg == 1);
assert(pFarg->a[0].pExpr != 0);
exprOp = pFarg->a[0].pExpr->op;
- if (exprOp == TK_COLUMN
+ if (exprOp == TK_COLUMN_NAME
|| exprOp == TK_AGG_COLUMN) {
assert(SQL_FUNC_LENGTH ==
OPFLAG_LENGTHARG);
@@ -4319,7 +4319,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
endLabel = sqlVdbeMakeLabel(v);
if ((pX = pExpr->pLeft) != 0) {
tempX = *pX;
- testcase(pX->op == TK_COLUMN);
+ testcase(pX->op == TK_COLUMN_NAME);
exprToRegister(&tempX,
exprCodeVector(pParse, &tempX,
®Free1));
@@ -4344,11 +4344,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
pTest = aListelem[i].pExpr;
}
nextCase = sqlVdbeMakeLabel(v);
- testcase(pTest->op == TK_COLUMN);
+ testcase(pTest->op == TK_COLUMN_NAME);
sqlExprIfFalse(pParse, pTest, nextCase,
SQL_JUMPIFNULL);
testcase(aListelem[i + 1].pExpr->op ==
- TK_COLUMN);
+ TK_COLUMN_NAME);
sqlExprCode(pParse, aListelem[i + 1].pExpr,
target);
sqlVdbeGoto(v, endLabel);
@@ -5081,7 +5081,8 @@ sqlExprCompare(Expr * pA, Expr * pB, int iTab)
}
return 2;
}
- if (pA->op != TK_COLUMN && pA->op != TK_AGG_COLUMN && pA->u.zToken) {
+ if (pA->op != TK_COLUMN_NAME && pA->op != TK_AGG_COLUMN &&
+ pA->u.zToken) {
if (pA->op == TK_FUNCTION) {
if (sqlStrICmp(pA->u.zToken, pB->u.zToken) != 0)
return 2;
@@ -5161,8 +5162,8 @@ sqlExprListCompare(ExprList * pA, ExprList * pB, int iTab)
* pE1: x IS NULL pE2: x IS NOT NULL Result: false
* pE1: x IS ?2 pE2: x IS NOT NULL Reuslt: false
*
- * When comparing TK_COLUMN nodes between pE1 and pE2, if pE2 has
- * Expr.iTable<0 then assume a table number given by iTab.
+ * When comparing TK_COLUMN_NAME nodes between pE1 and pE2, if
+ * pE2 has Expr.iTable<0 then assume a table number given by iTab.
*
* When in doubt, return false. Returning true might give a performance
* improvement. Returning false might cause a performance reduction, but
@@ -5209,11 +5210,11 @@ exprSrcCount(Walker * pWalker, Expr * pExpr)
{
/* The NEVER() on the second term is because sqlFunctionUsesThisSrc()
* is always called before sqlExprAnalyzeAggregates() and so the
- * TK_COLUMNs have not yet been converted into TK_AGG_COLUMN. If
+ * TK_COLUMN_NAMEs have not yet been converted into TK_AGG_COLUMN. If
* sqlFunctionUsesThisSrc() is used differently in the future, the
* NEVER() will need to be removed.
*/
- if (pExpr->op == TK_COLUMN || NEVER(pExpr->op == TK_AGG_COLUMN)) {
+ if (pExpr->op == TK_COLUMN_NAME || NEVER(pExpr->op == TK_AGG_COLUMN)) {
int i;
struct SrcCount *p = pWalker->u.pSrcCount;
SrcList *pSrc = p->pSrc;
@@ -5299,9 +5300,9 @@ analyzeAggregate(Walker * pWalker, Expr * pExpr)
switch (pExpr->op) {
case TK_AGG_COLUMN:
- case TK_COLUMN:{
+ case TK_COLUMN_NAME:{
testcase(pExpr->op == TK_AGG_COLUMN);
- testcase(pExpr->op == TK_COLUMN);
+ testcase(pExpr->op == TK_COLUMN_NAME);
/* Check to see if the column is in one of the tables in the FROM
* clause of the aggregate query
*/
@@ -5370,7 +5371,7 @@ analyzeAggregate(Walker * pWalker, Expr * pExpr)
if (pE->
op
==
- TK_COLUMN
+ TK_COLUMN_NAME
&&
pE->
iTable
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 482220a95..3f3625ad5 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -338,7 +338,7 @@ static struct Expr *
sql_expr_new_column_by_cursor(struct sql *db, struct space_def *def,
int cursor, int column)
{
- struct Expr *expr = sql_expr_new_anon(db, TK_COLUMN);
+ struct Expr *expr = sql_expr_new_anon(db, TK_COLUMN_NAME);
if (expr == NULL)
return NULL;
expr->space_def = def;
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 6f625dc18..5fe8ee3f6 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -189,7 +189,7 @@ sqlMatchSpanName(const char *zSpan,
* pExpr->space_def Points to the space_def structure of X.Y
* (even if X and/or Y are implied.)
* pExpr->iColumn Set to the column number within the table.
- * pExpr->op Set to TK_COLUMN.
+ * pExpr->op Set to TK_COLUMN_NAME.
* pExpr->pLeft Any expression this points to is deleted
* pExpr->pRight Any expression this points to is deleted.
*
@@ -461,7 +461,7 @@ lookupName(Parse * pParse, /* The parsing context */
pExpr->pLeft = 0;
sql_expr_delete(db, pExpr->pRight, false);
pExpr->pRight = 0;
- pExpr->op = (isTrigger ? TK_TRIGGER : TK_COLUMN);
+ pExpr->op = (isTrigger ? TK_TRIGGER : TK_COLUMN_NAME);
lookupname_end:
if (cnt == 1) {
assert(pNC != 0);
@@ -485,7 +485,7 @@ struct Expr *
sql_expr_new_column(struct sql *db, struct SrcList *src_list, int src_idx,
int column)
{
- struct Expr *expr = sql_expr_new_anon(db, TK_COLUMN);
+ struct Expr *expr = sql_expr_new_anon(db, TK_COLUMN_NAME);
if (expr == NULL)
return NULL;
struct SrcList_item *item = &src_list->a[src_idx];
@@ -518,7 +518,7 @@ exprProbability(Expr * p)
/*
* This routine is callback for sqlWalkExpr().
*
- * Resolve symbolic names into TK_COLUMN operators for the current
+ * Resolve symbolic names into TK_COLUMN_NAME operators for the current
* node in the expression tree. Return 0 to continue the search down
* the tree or 2 to abort the tree walk.
*
@@ -1451,7 +1451,7 @@ resolveSelectStep(Walker * pWalker, Select * p)
*
* The node at the root of the subtree is modified as follows:
*
- * Expr.op Changed to TK_COLUMN
+ * Expr.op Changed to TK_COLUMN_NAME
* Expr.pTab Points to the Table object for X.Y
* Expr.iColumn The column index in X.Y. -1 for the rowid.
* Expr.iTable The VDBE cursor number for X.Y
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index b0554a172..80fbf69e0 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1817,7 +1817,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
vdbe_metadata_set_col_nullability(v, i, -1);
const char *colname = pEList->a[i].zName;
const char *span = pEList->a[i].zSpan;
- if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
+ if (p->op == TK_COLUMN_NAME || p->op == TK_AGG_COLUMN) {
char *zCol;
int iCol = p->iColumn;
for (j = 0; ALWAYS(j < pTabList->nSrc); j++) {
@@ -1939,7 +1939,7 @@ sqlColumnsFromExprList(Parse * parse, ExprList * expr_list,
pColExpr = pColExpr->pRight;
assert(pColExpr != 0);
}
- if (pColExpr->op == TK_COLUMN
+ if (pColExpr->op == TK_COLUMN_NAME
&& ALWAYS(pColExpr->space_def != NULL)) {
/* For columns use the column name name */
int iCol = pColExpr->iColumn;
@@ -3653,7 +3653,7 @@ substExpr(Parse * pParse, /* Report errors here */
sql *db = pParse->db;
if (pExpr == 0)
return 0;
- if (pExpr->op == TK_COLUMN && pExpr->iTable == iTable) {
+ if (pExpr->op == TK_COLUMN_NAME && pExpr->iTable == iTable) {
if (pExpr->iColumn < 0) {
pExpr->op = TK_NULL;
} else {
@@ -6044,7 +6044,7 @@ sqlSelect(Parse * pParse, /* The parser context */
/* Create a label to jump to when we want to abort the query */
addrEnd = sqlVdbeMakeLabel(v);
- /* Convert TK_COLUMN nodes into TK_AGG_COLUMN and make entries in
+ /* Convert TK_COLUMN_NAME nodes into TK_AGG_COLUMN and make entries in
* sAggInfo for all TK_AGG_FUNCTION nodes in expressions of the
* SELECT statement.
*/
@@ -6416,7 +6416,7 @@ sqlSelect(Parse * pParse, /* The parser context */
flag !=
WHERE_ORDERBY_MIN ? 1 : 0;
pMinMax->a[0].pExpr->op =
- TK_COLUMN;
+ TK_COLUMN_NAME;
}
}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index adf90d824..beb83ce95 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1517,7 +1517,7 @@ typedef int ynVar;
* valid.
*
* An expression of the form ID or ID.ID refers to a column in a table.
- * For such expressions, Expr.op is set to TK_COLUMN and Expr.iTable is
+ * For such expressions, Expr.op is set to TK_COLUMN_NAME and Expr.iTable is
* the integer cursor number of a VDBE cursor pointing to that table and
* Expr.iColumn is the column number for the specific column. If the
* expression is used as a result in an aggregate SELECT, then the
@@ -1587,20 +1587,20 @@ struct Expr {
#if SQL_MAX_EXPR_DEPTH>0
int nHeight; /* Height of the tree headed by this node */
#endif
- int iTable; /* TK_COLUMN: cursor number of table holding column
+ int iTable; /* TK_COLUMN_NAME: cursor number of table holding column
* TK_REGISTER: register number
* TK_TRIGGER: 1 -> new, 0 -> old
* EP_Unlikely: 134217728 times likelihood
* TK_SELECT: 1st register of result vector
*/
- ynVar iColumn; /* TK_COLUMN: column index.
+ ynVar iColumn; /* TK_COLUMN_NAME: column index.
* TK_VARIABLE: variable number (always >= 1).
* TK_SELECT_COLUMN: column of the result vector
*/
i16 iAgg; /* Which entry in pAggInfo->aCol[] or ->aFunc[] */
i16 iRightJoinTable; /* If EP_FromJoin, the right table of the join */
u8 op2; /* TK_REGISTER: original value of Expr.op
- * TK_COLUMN: the value of p5 for OP_Column
+ * TK_COLUMN_NAME: the value of p5 for OP_Column
* TK_AGG_FUNCTION: nesting depth
*/
AggInfo *pAggInfo; /* Used by TK_AGG_COLUMN and TK_AGG_FUNCTION */
diff --git a/src/box/sql/treeview.c b/src/box/sql/treeview.c
index a04597979..ea26fcf6d 100644
--- a/src/box/sql/treeview.c
+++ b/src/box/sql/treeview.c
@@ -327,7 +327,7 @@ sqlTreeViewExpr(TreeView * pView, const Expr * pExpr, u8 moreToFollow)
zFlgs);
break;
}
- case TK_COLUMN:{
+ case TK_COLUMN_NAME:{
if (pExpr->iTable < 0) {
/* This only happens when coding check constraints */
sqlTreeViewLine(pView, "COLUMN(%d)%s",
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index e9e936856..77f863b0e 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -276,7 +276,7 @@ whereScanNext(WhereScan * pScan)
sqlExprSkipCollate(pTerm->
pExpr->
pRight))->
- op == TK_COLUMN) {
+ op == TK_COLUMN_NAME) {
int j;
for (j = 0; j < pScan->nEquiv; j++) {
if (pScan->aiCur[j] == pX->iTable
@@ -319,7 +319,7 @@ whereScanNext(WhereScan * pScan)
}
}
if ((pTerm->eOperator & WO_EQ) != 0
- && (pX = pTerm->pExpr->pRight)->op == TK_COLUMN
+ && (pX = pTerm->pExpr->pRight)->op == TK_COLUMN_NAME
&& pX->iTable == pScan->aiCur[0]
&& pX->iColumn == pScan->aiColumn[0]) {
continue;
@@ -555,7 +555,7 @@ findIndexCol(Parse * pParse, /* Parse context */
struct key_part *part_to_match = &idx_def->key_def->parts[iCol];
for (int i = 0; i < pList->nExpr; i++) {
Expr *p = sqlExprSkipCollate(pList->a[i].pExpr);
- if (p->op == TK_COLUMN && p->iTable == iBase &&
+ if (p->op == TK_COLUMN_NAME && p->iTable == iBase &&
p->iColumn == (int) part_to_match->fieldno) {
bool is_found;
uint32_t id;
@@ -601,7 +601,8 @@ isDistinctRedundant(Parse * pParse, /* Parsing context */
*/
for (int i = 0; i < pDistinct->nExpr; i++) {
Expr *p = sqlExprSkipCollate(pDistinct->a[i].pExpr);
- if (p->op == TK_COLUMN && p->iTable == iBase && p->iColumn < 0)
+ if (p->op == TK_COLUMN_NAME && p->iTable == iBase &&
+ p->iColumn < 0)
return 1;
}
if (space == NULL)
@@ -2245,7 +2246,7 @@ whereRangeVectorLen(Parse * pParse, /* Parsing context */
* leftmost index column.
*/
struct key_part *parts = idx_def->key_def->parts;
- if (pLhs->op != TK_COLUMN || pLhs->iTable != iCur ||
+ if (pLhs->op != TK_COLUMN_NAME || pLhs->iTable != iCur ||
pLhs->iColumn != (int)parts[i + nEq].fieldno ||
parts[i + nEq].sort_order != parts[nEq].sort_order)
break;
@@ -2677,7 +2678,7 @@ indexMightHelpWithOrderBy(WhereLoopBuilder * pBuilder,
return 0;
for (ii = 0; ii < pOB->nExpr; ii++) {
Expr *pExpr = sqlExprSkipCollate(pOB->a[ii].pExpr);
- if (pExpr->op == TK_COLUMN && pExpr->iTable == iCursor) {
+ if (pExpr->op == TK_COLUMN_NAME && pExpr->iTable == iCursor) {
if (pExpr->iColumn < 0)
return 1;
for (jj = 0; jj < part_count; jj++) {
@@ -3213,7 +3214,7 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
if (MASKBIT(i) & obSat)
continue;
pOBExpr = sqlExprSkipCollate(pOrderBy->a[i].pExpr);
- if (pOBExpr->op != TK_COLUMN)
+ if (pOBExpr->op != TK_COLUMN_NAME)
continue;
if (pOBExpr->iTable != iCur)
continue;
@@ -3361,7 +3362,8 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
if ((wctrlFlags & (WHERE_GROUPBY | WHERE_DISTINCTBY)) == 0)
bOnce = 0;
if (iColumn >= (-1)) {
- if (pOBExpr->op != TK_COLUMN)
+ if (pOBExpr->op !=
+ TK_COLUMN_NAME)
continue;
if (pOBExpr->iTable != iCur)
continue;
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index d9b5c78f5..c2ce4339f 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -279,7 +279,7 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
pList = pExpr->x.pList;
pLeft = pList->a[1].pExpr;
/* Value might be numeric */
- if (pLeft->op != TK_COLUMN ||
+ if (pLeft->op != TK_COLUMN_NAME ||
sql_expr_type(pLeft) != FIELD_TYPE_STRING) {
/* IMP: R-02065-49465 The left-hand side of the
* LIKE operator must be the name of an indexed
@@ -928,7 +928,7 @@ exprSelectUsage(WhereMaskSet * pMaskSet, Select * pS)
* number of the table that is indexed and *piColumn to the column number
* of the column that is indexed.
*
- * If pExpr is a TK_COLUMN column reference, then this routine always returns
+ * If pExpr is a TK_COLUMN_NAME column reference, then this routine always returns
* true even if that particular column is not indexed, because the column
* might be added to an automatic index later.
*/
@@ -950,7 +950,7 @@ exprMightBeIndexed(int op, /* The specific comparison operator */
pExpr = pExpr->x.pList->a[0].pExpr;
}
- if (pExpr->op == TK_COLUMN) {
+ if (pExpr->op == TK_COLUMN_NAME) {
*piCur = pExpr->iTable;
*piColumn = pExpr->iColumn;
return 1;
@@ -1272,7 +1272,7 @@ exprAnalyze(SrcList * pSrc, /* the FROM clause */
* Note that the virtual term must be tagged with TERM_VNULL.
*/
if (pExpr->op == TK_NOTNULL
- && pExpr->pLeft->op == TK_COLUMN
+ && pExpr->pLeft->op == TK_COLUMN_NAME
&& pExpr->pLeft->iColumn >= 0) {
Expr *pNewExpr;
Expr *pLeft = pExpr->pLeft;
@@ -1397,7 +1397,7 @@ sqlWhereExprUsage(WhereMaskSet * pMaskSet, Expr * p)
Bitmask mask;
if (p == 0)
return 0;
- if (p->op == TK_COLUMN) {
+ if (p->op == TK_COLUMN_NAME) {
mask = sqlWhereGetMask(pMaskSet, p->iTable);
return mask;
}
@@ -1475,7 +1475,7 @@ sqlWhereTabFuncArgs(Parse * pParse, /* Parsing context */
* unused.
*/
assert(k < (int)space_def->field_count);
- pColRef = sql_expr_new_anon(pParse->db, TK_COLUMN);
+ pColRef = sql_expr_new_anon(pParse->db, TK_COLUMN_NAME);
if (pColRef == NULL) {
pParse->is_aborted = true;
return;
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Tarantool-patches] [PATCH v3 2/4] sql: refactor create_table_def and parse
2020-08-11 0:33 [Tarantool-patches] [PATCH v3 0/4] Support ALTER TABLE ADD COLUMN Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 1/4] sql: rename TK_COLUMN to TK_COLUMN_NAME Roman Khabibov
@ 2020-08-11 0:33 ` Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 3/4] schema: add box_space_field_MAX Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 4/4] sql: support column addition Roman Khabibov
3 siblings, 0 replies; 9+ messages in thread
From: Roman Khabibov @ 2020-08-11 0:33 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
Move ck, fk constraint lists and autoincrement info from
struct create_table_def to struct Parse to make the code more
reusable when implementing <ALTER TABLE ADD COLUMN>.
Needed for #3075
---
src/box/sql/build.c | 179 +++++++++++++++++++++-------------------
src/box/sql/parse_def.h | 33 --------
src/box/sql/prepare.c | 7 +-
src/box/sql/sqlInt.h | 14 ++++
4 files changed, 116 insertions(+), 117 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 619bbf8e3..9013bc86f 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -590,7 +590,7 @@ sql_create_check_contraint(struct Parse *parser)
}
} else {
assert(! is_alter);
- uint32_t ck_idx = ++parser->create_table_def.check_count;
+ uint32_t ck_idx = ++parser->check_count;
name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, ck_idx);
}
size_t name_len = strlen(name);
@@ -652,8 +652,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);
}
}
@@ -930,7 +929,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;
@@ -1147,6 +1146,88 @@ resolve_link(struct Parse *parse_context, const struct space_def *def,
return -1;
}
+/**
+ * Emit code to create sequences, indexes, check and foreign key
+ * constraints appeared in <CREATE TABLE>.
+ */
+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;
+ assert(space != NULL);
+ uint32_t i = 0;
+ 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_rec = emitNewSysSequenceRecord(parse, reg_seq_id,
+ space->def->name);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_rec);
+ /* 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 +1294,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
@@ -1893,7 +1908,7 @@ 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);
+ rlist_add_entry(&parse_context->fkeys, fk_parse, link);
}
struct Token *parent = create_fk_def->parent_name;
assert(parent != NULL);
@@ -1911,7 +1926,7 @@ sql_create_foreign_key(struct Parse *parse_context)
if (parent_space == NULL) {
if (is_self_referenced) {
struct fk_constraint_parse *fk =
- rlist_first_entry(&table_def->new_fkey,
+ rlist_first_entry(&parse_context->fkeys,
struct fk_constraint_parse,
link);
fk->selfref_cols = parent_cols;
@@ -1926,7 +1941,7 @@ sql_create_foreign_key(struct Parse *parse_context)
constraint_name =
sqlMPrintf(db, "fk_unnamed_%s_%d",
space->def->name,
- ++table_def->fkey_count);
+ ++parse_context->fkey_count);
} else {
constraint_name =
sql_name_from_token(db, &create_def->name);
@@ -2042,7 +2057,7 @@ sql_create_foreign_key(struct Parse *parse_context)
*/
if (!is_alter) {
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 +2080,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;
}
/**
@@ -3306,15 +3319,15 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
int
sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno)
{
- if (parse_context->create_table_def.has_autoinc) {
+ if (parse_context->has_autoinc) {
diag_set(ClientError, ER_SQL_SYNTAX_WITH_POS,
parse_context->line_count, parse_context->line_pos,
"table must feature at most one AUTOINCREMENT field");
parse_context->is_aborted = true;
return -1;
}
- parse_context->create_table_def.has_autoinc = true;
- parse_context->create_table_def.autoinc_fieldno = fieldno;
+ parse_context->has_autoinc = true;
+ parse_context->autoinc_fieldno = fieldno;
return 0;
}
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index cb0ecd2fc..1105fda6e 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -205,26 +205,6 @@ 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_view_def {
@@ -482,9 +462,6 @@ 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
@@ -499,14 +476,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..b78eb317f 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -200,6 +200,9 @@ sql_parser_create(struct Parse *parser, struct sql *db, uint32_t sql_flags)
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 +214,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 beb83ce95..fa87e7bd2 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2257,6 +2257,20 @@ struct Parse {
* sqlEndTable() function).
*/
struct create_table_def create_table_def;
+ /*
+ * FK and CK constraints appeared in a <CREATE TABLE>.
+ */
+ struct rlist fkeys;
+ struct rlist checks;
+ uint32_t fkey_count;
+ uint32_t check_count;
+ /*
+ * True, if column within a <CREATE TABLE> statement 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;
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Tarantool-patches] [PATCH v3 3/4] schema: add box_space_field_MAX
2020-08-11 0:33 [Tarantool-patches] [PATCH v3 0/4] Support ALTER TABLE ADD COLUMN Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 1/4] sql: rename TK_COLUMN to TK_COLUMN_NAME Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 2/4] sql: refactor create_table_def and parse Roman Khabibov
@ 2020-08-11 0:33 ` Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 4/4] sql: support column addition Roman Khabibov
3 siblings, 0 replies; 9+ messages in thread
From: Roman Khabibov @ 2020-08-11 0:33 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
Just add box_space_field_MAX to the _space fields enum.
Needed for #3075
---
src/box/schema_def.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index f86cd42f1..238d9f1e4 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. */
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Tarantool-patches] [PATCH v3 4/4] sql: support column addition
2020-08-11 0:33 [Tarantool-patches] [PATCH v3 0/4] Support ALTER TABLE ADD COLUMN Roman Khabibov
` (2 preceding siblings ...)
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 3/4] schema: add box_space_field_MAX Roman Khabibov
@ 2020-08-11 0:33 ` Roman Khabibov
2020-08-19 22:20 ` Vladislav Shpilevoy
3 siblings, 1 reply; 9+ messages in thread
From: Roman Khabibov @ 2020-08-11 0:33 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
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
> CHECK (LENGTH(b) > 1)
> NOT NULL
> DEFAULT ('aa')
> COLLATE "unicode_ci"
> ]])
---
- row_count: 0
...
```
---
| 2 +-
src/box/errcode.h | 2 +
src/box/sql/build.c | 431 +++++++++++++++++++++---------
src/box/sql/parse.y | 44 ++-
src/box/sql/parse_def.h | 20 ++
src/box/sql/sqlInt.h | 45 +++-
test/box/error.result | 2 +
test/sql/add-column.result | 471 +++++++++++++++++++++++++++++++++
test/sql/add-column.test.sql | 167 ++++++++++++
test/sql/checks.result | 20 ++
test/sql/checks.test.lua | 9 +
test/sql/foreign-keys.result | 28 ++
test/sql/foreign-keys.test.lua | 11 +
13 files changed, 1108 insertions(+), 144 deletions(-)
create mode 100644 test/sql/add-column.result
create mode 100644 test/sql/add-column.test.sql
--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 3c21375f5..cbcffb3a8 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -271,6 +271,8 @@ struct errcode_record {
/*216 */_(ER_SYNC_QUORUM_TIMEOUT, "Quorum collection for a synchronous transaction is timed out") \
/*217 */_(ER_SYNC_ROLLBACK, "A rollback for a synchronous transaction is received") \
/*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \
+ /*219 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW, "Can't add column '%s'. '%s' is a view") \
+ /*220 */_(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/sql/build.c b/src/box/sql/build.c
index 9013bc86f..6f3d2747d 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -285,48 +285,113 @@ 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 an existing space with
+ * <ALTER TABLE ADD COLUMN> statement. Copy space def and index
+ * array to create constraints appeared in the statement. The
+ * index array copy will be modified by adding new elements to it.
+ * It is necessary, because the statement may contain several
+ * index definitions (constraints).
*/
+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;
+ }
+ memcpy(ret->index, space->index, indexes_sz);
+ memcpy(ret->def, space->def, sizeof(struct space_def));
+ ret->def->opts.is_temporary = true;
+ ret->def->opts.is_ephemeral = true;
+ if (ret->def->field_count != 0) {
+ uint32_t fields_size = 0;
+ ret->def->fields =
+ region_alloc_array(&parse->region,
+ typeof(struct field_def),
+ ret->def->field_count, &fields_size);
+ if (ret->def->fields == NULL) {
+ diag_set(OutOfMemory, fields_size, "region_alloc",
+ "ret->def->fields");
+ free(ret->index);
+ 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 (is_alter && 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)
+ if (def->exact_field_count == 0)
+ def->exact_field_count = def->field_count;
+ if (sql_field_retrieve(parse, 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;
- return;
- }
+
struct field_def *column_def = &def->fields[def->field_count];
memcpy(column_def, &field_def_default, sizeof(field_def_default));
- column_def->name = z;
+ column_def->name = column_name;
/*
* Marker ON_CONFLICT_ACTION_DEFAULT is used to detect
* attempts to define NULL multiple time or to detect
@@ -334,18 +399,86 @@ 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 space *space = parse->create_column_def.space;
+ assert(space != NULL);
+ struct space_def *def = space->def;
+ struct field_def *field = &def->fields[def->field_count - 1];
+ if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) {
+ 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);
+
+ /*
+ * Clean up array allocated in sql_shallow_space_copy().
+ */
+ free(space->index);
}
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))
+ assert(parser->create_column_def.space != NULL);
+ struct space_def *def = parser->create_column_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];
if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
nullable_action != field->nullable_action) {
@@ -364,51 +497,42 @@ 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));
+ assert(pParse->create_column_def.space != NULL);
+ struct space_def *def = pParse->create_column_def.space->def;
+ struct field_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 {
+ 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 +571,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 +700,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 +717,23 @@ sql_create_check_contraint(struct Parse *parser)
return;
}
} else {
- assert(! is_alter);
- uint32_t ck_idx = ++parser->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 it is <ALTER TABLE ADD COLUMN> we should
+ * count the existing CHECK constraints in the
+ * space and form a name based on this.
+ */
+ 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,7 +776,7 @@ 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);
if (space == NULL) {
@@ -663,9 +805,8 @@ 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;
+ assert(space != NULL);
uint32_t i = space->def->field_count - 1;
sql *db = pParse->db;
char *coll_name = sql_name_from_token(db, pToken);
@@ -704,8 +845,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);
@@ -794,7 +934,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 {
@@ -1032,18 +1173,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 {
@@ -1107,7 +1251,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);
}
@@ -1148,15 +1292,21 @@ resolve_link(struct Parse *parse_context, const struct space_def *def,
/**
* Emit code to create sequences, indexes, check and foreign key
- * constraints appeared in <CREATE TABLE>.
+ * constraints appeared in <CREATE TABLE> or
+ * <ALTER TABLE ADD COLUMN>.
*/
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;
- assert(space != NULL);
+ 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,
@@ -1175,6 +1325,21 @@ sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id)
sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id);
int reg_seq_rec = emitNewSysSequenceRecord(parse, reg_seq_id,
space->def->name);
+ if (is_alter) {
+ int errcode = ER_SQL_CANT_ADD_AUTOINC;
+ const char *error_msg =
+ tt_sprintf(tnt_errcode_desc(errcode),
+ space->def->name);
+ if (vdbe_emit_halt_with_presence_test(parse,
+ BOX_SEQUENCE_ID,
+ 2,
+ reg_seq_rec + 3,
+ 1, errcode,
+ error_msg, false,
+ OP_NoConflict)
+ != 0)
+ return;
+ }
sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_rec);
/* Do an insertion into _space_sequence. */
int reg_space_seq_record =
@@ -1873,24 +2038,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) {
@@ -1908,6 +2077,8 @@ sql_create_foreign_key(struct Parse *parse_context)
goto tnt_error;
}
memset(fk_parse, 0, sizeof(*fk_parse));
+ 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;
@@ -1920,28 +2091,45 @@ 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(&parse_context->fkeys,
- 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,
- ++parse_context->fkey_count);
+ uint32_t idx = ++parse_context->fkey_count;
+ /*
+ * If it is <ALTER TABLE ADD COLUMN> we
+ * should count the existing FK
+ * constraints in the space and form a
+ * name based on this.
+ */
+ 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);
@@ -2001,7 +2189,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);
@@ -2021,7 +2210,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);
/*
@@ -2050,12 +2239,13 @@ 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(&parse_context->fkeys,
struct fk_constraint_parse, link);
@@ -2407,7 +2597,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);
@@ -2420,10 +2613,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;
@@ -2458,7 +2649,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) {
@@ -2624,7 +2815,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;
@@ -2712,7 +2903,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;
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 995875566..0c9887851 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -226,19 +226,24 @@ 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.
+
+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;
+ if (pParse->create_table_def.new_space == NULL)
+ 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 +286,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 }
@@ -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 1105fda6e..336914c57 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,
@@ -207,6 +209,14 @@ struct create_table_def {
struct space *new_space;
};
+struct create_column_def {
+ struct create_entity_def base;
+ /** Shallow space_def copy. */
+ struct space *space;
+ /** Column type. */
+ struct type_def *type_def;
+};
+
struct create_view_def {
struct create_entity_def base;
/**
@@ -464,6 +474,16 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
if_not_exists);
}
+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 fa87e7bd2..32142a871 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2251,22 +2251,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>.
+ * FK and CK constraints appeared in a <CREATE TABLE> or
+ * an <ALTER TABLE ADD COLUMN> statement.
*/
struct rlist fkeys;
struct rlist checks;
uint32_t fkey_count;
uint32_t check_count;
/*
- * True, if column within a <CREATE TABLE> statement to be
- * created has <AUTOINCREMENT>.
+ * True, if column in a <CREATE TABLE> or an
+ * <ALTER TABLE ADD COLUMN> statement to be created has
+ * <AUTOINCREMENT>.
*/
bool has_autoinc;
/* Id of field with <AUTOINCREMENT>. */
@@ -2860,15 +2864,30 @@ 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);
+
+/**
+ * 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 cdecdb221..6fc3cb99f 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -437,6 +437,8 @@ t;
| 216: box.error.SYNC_QUORUM_TIMEOUT
| 217: box.error.SYNC_ROLLBACK
| 218: box.error.TUPLE_METADATA_IS_TOO_BIG
+ | 219: box.error.SQL_CANT_ADD_COLUMN_TO_VIEW
+ | 220: 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..f86259105
--- /dev/null
+++ b/test/sql/add-column.result
@@ -0,0 +1,471 @@
+-- test-run result file version 2
+--
+-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
+--
+CREATE TABLE t1 (a INT PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- COLUMN keyword is optional. Check it here, but omit it below.
+--
+ALTER TABLE t1 ADD COLUMN b INT;
+ | ---
+ | - row_count: 0
+ | ...
+
+--
+-- A column with the same name already exists.
+--
+ALTER TABLE t1 ADD b SCALAR;
+ | ---
+ | - null
+ | - Space field 'B' is duplicate
+ | ...
+
+--
+-- Can't add column to a view.
+--
+CREATE VIEW v AS SELECT * FROM t1;
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE v ADD b INT;
+ | ---
+ | - null
+ | - Can't add column 'B'. 'V' is a view
+ | ...
+DROP VIEW v;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check PRIMARY KEY constraint works with an added column.
+--
+CREATE TABLE pk_check (a INT CONSTRAINT pk PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE pk_check DROP CONSTRAINT pk;
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE pk_check ADD b INT PRIMARY KEY;
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO pk_check VALUES (1, 1);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO pk_check VALUES (1, 1);
+ | ---
+ | - null
+ | - Duplicate key exists in unique index 'pk_unnamed_PK_CHECK_1' in space 'PK_CHECK'
+ | ...
+DROP TABLE pk_check;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check UNIQUE constraint works with an added column.
+--
+CREATE TABLE unique_check (a INT PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE unique_check ADD b INT UNIQUE;
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO unique_check VALUES (1, 1);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO unique_check VALUES (2, 1);
+ | ---
+ | - null
+ | - Duplicate key exists in unique index 'unique_unnamed_UNIQUE_CHECK_2' in space 'UNIQUE_CHECK'
+ | ...
+DROP TABLE unique_check;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check CHECK constraint works with an added column.
+--
+CREATE TABLE ck_check (a INT PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE ck_check ADD b INT CHECK (b > 0);
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO ck_check VALUES (1, 0);
+ | ---
+ | - null
+ | - 'Check constraint failed ''ck_unnamed_CK_CHECK_1'': b > 0'
+ | ...
+DROP TABLE ck_check;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check FOREIGN KEY constraint works with an added column.
+--
+CREATE TABLE fk_check (a INT PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE fk_check ADD b INT REFERENCES t1(a);
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO fk_check VALUES (0, 1);
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | ...
+INSERT INTO fk_check VALUES (2, 0);
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | ...
+INSERT INTO fk_check VALUES (2, 1);
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | ...
+DROP TABLE fk_check;
+ | ---
+ | - row_count: 1
+ | ...
+DROP TABLE t1;
+ | ---
+ | - row_count: 1
+ | ...
+--
+-- Check FOREIGN KEY (self-referenced) constraint works with an
+-- added column.
+--
+CREATE TABLE self (id INT PRIMARY KEY AUTOINCREMENT, a INT UNIQUE)
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE self ADD b INT REFERENCES self(a)
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO self(a,b) VALUES(1, 1);
+ | ---
+ | - autoincrement_ids:
+ | - 1
+ | row_count: 1
+ | ...
+UPDATE self SET a = 2, b = 2;
+ | ---
+ | - row_count: 1
+ | ...
+UPDATE self SET b = 3;
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | ...
+UPDATE self SET a = 3;
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | ...
+DROP TABLE self;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check AUTOINCREMENT works with an added column.
+--
+CREATE TABLE autoinc_check (a INT CONSTRAINT pk PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE autoinc_check DROP CONSTRAINT pk;
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE autoinc_check ADD b INT PRIMARY KEY AUTOINCREMENT;
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO autoinc_check(a) VALUES(1);
+ | ---
+ | - autoincrement_ids:
+ | - 1
+ | row_count: 1
+ | ...
+INSERT INTO autoinc_check(a) VALUES(1);
+ | ---
+ | - autoincrement_ids:
+ | - 2
+ | row_count: 1
+ | ...
+TRUNCATE TABLE autoinc_check;
+ | ---
+ | - row_count: 0
+ | ...
+
+--
+-- Can't add second column with AUTOINCREMENT.
+--
+ALTER TABLE autoinc_check ADD c INT AUTOINCREMENT;
+ | ---
+ | - null
+ | - 'Can''t add AUTOINCREMENT: the space ''AUTOINC_CHECK'' already has one auto incremented
+ | field'
+ | ...
+DROP TABLE autoinc_check;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check COLLATE clause works with an added column.
+--
+CREATE TABLE collate_check (a INT PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE collate_check ADD b TEXT COLLATE "unicode_ci";
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO collate_check VALUES (1, 'a');
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO collate_check VALUES (2, 'A');
+ | ---
+ | - row_count: 1
+ | ...
+SELECT * FROM collate_check WHERE b LIKE 'a';
+ | ---
+ | - metadata:
+ | - name: A
+ | type: integer
+ | - name: B
+ | type: string
+ | rows:
+ | - [1, 'a']
+ | - [2, 'A']
+ | ...
+DROP TABLE collate_check;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check DEFAULT clause works with an added column.
+--
+CREATE TABLE default_check (a INT PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE default_check ADD b TEXT DEFAULT ('a');
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO default_check(a) VALUES (1);
+ | ---
+ | - row_count: 1
+ | ...
+SELECT * FROM default_check;
+ | ---
+ | - metadata:
+ | - name: A
+ | type: integer
+ | - name: B
+ | type: string
+ | rows:
+ | - [1, 'a']
+ | ...
+DROP TABLE default_check;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check NULL constraint works with an added column.
+--
+CREATE TABLE null_check (a INT PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE null_check ADD b TEXT NULL;
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO null_check(a) VALUES (1);
+ | ---
+ | - row_count: 1
+ | ...
+DROP TABLE null_check;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Check NOT NULL constraint works with an added column.
+--
+CREATE TABLE notnull_check (a INT PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE notnull_check ADD b TEXT NOT NULL;
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO notnull_check(a) VALUES (1);
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: NOT NULL constraint failed: NOTNULL_CHECK.B'
+ | ...
+DROP TABLE notnull_check;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Can't add a column with DEAFULT or NULL to a non-empty space.
+-- This ability isn't implemented yet.
+--
+CREATE TABLE non_empty (a INT PRIMARY KEY);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO non_empty VALUES (1);
+ | ---
+ | - row_count: 1
+ | ...
+ALTER TABLE non_empty ADD b INT NULL;
+ | ---
+ | - null
+ | - Tuple field count 1 does not match space field count 2
+ | ...
+ALTER TABLE non_empty ADD b INT DEFAULT (1);
+ | ---
+ | - null
+ | - Tuple field count 1 does not match space field count 2
+ | ...
+DROP TABLE non_empty;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Add to a no-SQL adjusted space without format.
+--
+\set language lua
+ | ---
+ | - true
+ | ...
+_ = box.schema.space.create('WITHOUT_FORMAT')
+ | ---
+ | ...
+\set language sql
+ | ---
+ | - true
+ | ...
+ALTER TABLE without_format ADD a INT PRIMARY KEY;
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO without_format VALUES (1);
+ | ---
+ | - row_count: 1
+ | ...
+DROP TABLE without_format;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Add to a no-SQL adjusted space with format.
+--
+\set language lua
+ | ---
+ | - true
+ | ...
+with_format = box.schema.space.create('WITH_FORMAT')
+ | ---
+ | ...
+with_format:format{{name = 'A', type = 'unsigned'}}
+ | ---
+ | ...
+\set language sql
+ | ---
+ | - true
+ | ...
+ALTER TABLE with_format ADD b INT PRIMARY KEY;
+ | ---
+ | - row_count: 0
+ | ...
+INSERT INTO with_format VALUES (1, 1);
+ | ---
+ | - row_count: 1
+ | ...
+DROP TABLE with_format;
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Add multiple columns (with a constraint) inside a transaction.
+--
+CREATE TABLE t2 (a INT PRIMARY KEY)
+ | ---
+ | - row_count: 1
+ | ...
+\set language lua
+ | ---
+ | - true
+ | ...
+box.begin() \
+box.execute('ALTER TABLE t2 ADD b INT') \
+box.execute('ALTER TABLE t2 ADD c INT UNIQUE') \
+box.commit()
+ | ---
+ | ...
+\set language sql
+ | ---
+ | - true
+ | ...
+INSERT INTO t2 VALUES (1, 1, 1);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO t2 VALUES (2, 1, 1);
+ | ---
+ | - null
+ | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in space 'T2'
+ | ...
+SELECT * FROM t2;
+ | ---
+ | - metadata:
+ | - name: A
+ | type: integer
+ | - name: B
+ | type: integer
+ | - name: C
+ | type: integer
+ | rows:
+ | - [1, 1, 1]
+ | ...
+DROP TABLE t2;
+ | ---
+ | - row_count: 1
+ | ...
diff --git a/test/sql/add-column.test.sql b/test/sql/add-column.test.sql
new file mode 100644
index 000000000..f8ab3f756
--- /dev/null
+++ b/test/sql/add-column.test.sql
@@ -0,0 +1,167 @@
+--
+-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
+--
+CREATE TABLE t1 (a INT PRIMARY KEY);
+
+--
+-- COLUMN keyword is optional. Check it here, but omit it below.
+--
+ALTER TABLE t1 ADD COLUMN b INT;
+
+--
+-- A column with the same name already exists.
+--
+ALTER TABLE t1 ADD b SCALAR;
+
+--
+-- Can't add column to a view.
+--
+CREATE VIEW v AS SELECT * FROM t1;
+ALTER TABLE v ADD b INT;
+DROP VIEW v;
+
+--
+-- Check PRIMARY KEY constraint works with an added column.
+--
+CREATE TABLE pk_check (a INT CONSTRAINT pk PRIMARY KEY);
+ALTER TABLE pk_check DROP CONSTRAINT pk;
+ALTER TABLE pk_check ADD b INT PRIMARY KEY;
+INSERT INTO pk_check VALUES (1, 1);
+INSERT INTO pk_check VALUES (1, 1);
+DROP TABLE pk_check;
+
+--
+-- Check UNIQUE constraint works with an added column.
+--
+CREATE TABLE unique_check (a INT PRIMARY KEY);
+ALTER TABLE unique_check ADD b INT UNIQUE;
+INSERT INTO unique_check VALUES (1, 1);
+INSERT INTO unique_check VALUES (2, 1);
+DROP TABLE unique_check;
+
+--
+-- Check CHECK constraint works with an added column.
+--
+CREATE TABLE ck_check (a INT PRIMARY KEY);
+ALTER TABLE ck_check ADD b INT CHECK (b > 0);
+INSERT INTO ck_check VALUES (1, 0);
+DROP TABLE ck_check;
+
+--
+-- Check FOREIGN KEY constraint works with an added column.
+--
+CREATE TABLE fk_check (a INT PRIMARY KEY);
+ALTER TABLE fk_check ADD b INT REFERENCES t1(a);
+INSERT INTO fk_check VALUES (0, 1);
+INSERT INTO fk_check VALUES (2, 0);
+INSERT INTO fk_check VALUES (2, 1);
+DROP TABLE fk_check;
+DROP TABLE t1;
+--
+-- Check FOREIGN KEY (self-referenced) constraint works with an
+-- added column.
+--
+CREATE TABLE self (id INT PRIMARY KEY AUTOINCREMENT, a INT UNIQUE)
+ALTER TABLE self ADD b INT REFERENCES self(a)
+INSERT INTO self(a,b) VALUES(1, 1);
+UPDATE self SET a = 2, b = 2;
+UPDATE self SET b = 3;
+UPDATE self SET a = 3;
+DROP TABLE self;
+
+--
+-- Check AUTOINCREMENT works with an added column.
+--
+CREATE TABLE autoinc_check (a INT CONSTRAINT pk PRIMARY KEY);
+ALTER TABLE autoinc_check DROP CONSTRAINT pk;
+ALTER TABLE autoinc_check ADD b INT PRIMARY KEY AUTOINCREMENT;
+INSERT INTO autoinc_check(a) VALUES(1);
+INSERT INTO autoinc_check(a) VALUES(1);
+TRUNCATE TABLE autoinc_check;
+
+--
+-- Can't add second column with AUTOINCREMENT.
+--
+ALTER TABLE autoinc_check ADD c INT AUTOINCREMENT;
+DROP TABLE autoinc_check;
+
+--
+-- Check COLLATE clause works with an added column.
+--
+CREATE TABLE collate_check (a INT PRIMARY KEY);
+ALTER TABLE collate_check ADD b TEXT COLLATE "unicode_ci";
+INSERT INTO collate_check VALUES (1, 'a');
+INSERT INTO collate_check VALUES (2, 'A');
+SELECT * FROM collate_check WHERE b LIKE 'a';
+DROP TABLE collate_check;
+
+--
+-- Check DEFAULT clause works with an added column.
+--
+CREATE TABLE default_check (a INT PRIMARY KEY);
+ALTER TABLE default_check ADD b TEXT DEFAULT ('a');
+INSERT INTO default_check(a) VALUES (1);
+SELECT * FROM default_check;
+DROP TABLE default_check;
+
+--
+-- Check NULL constraint works with an added column.
+--
+CREATE TABLE null_check (a INT PRIMARY KEY);
+ALTER TABLE null_check ADD b TEXT NULL;
+INSERT INTO null_check(a) VALUES (1);
+DROP TABLE null_check;
+
+--
+-- Check NOT NULL constraint works with an added column.
+--
+CREATE TABLE notnull_check (a INT PRIMARY KEY);
+ALTER TABLE notnull_check ADD b TEXT NOT NULL;
+INSERT INTO notnull_check(a) VALUES (1);
+DROP TABLE notnull_check;
+
+--
+-- Can't add a column with DEAFULT or NULL to a non-empty space.
+-- This ability isn't implemented yet.
+--
+CREATE TABLE non_empty (a INT PRIMARY KEY);
+INSERT INTO non_empty VALUES (1);
+ALTER TABLE non_empty ADD b INT NULL;
+ALTER TABLE non_empty ADD b INT DEFAULT (1);
+DROP TABLE non_empty;
+
+--
+-- Add to a no-SQL adjusted space without format.
+--
+\set language lua
+_ = box.schema.space.create('WITHOUT_FORMAT')
+\set language sql
+ALTER TABLE without_format ADD a INT PRIMARY KEY;
+INSERT INTO without_format VALUES (1);
+DROP TABLE without_format;
+
+--
+-- Add to a no-SQL adjusted space with format.
+--
+\set language lua
+with_format = box.schema.space.create('WITH_FORMAT')
+with_format:format{{name = 'A', type = 'unsigned'}}
+\set language sql
+ALTER TABLE with_format ADD b INT PRIMARY KEY;
+INSERT INTO with_format VALUES (1, 1);
+DROP TABLE with_format;
+
+--
+-- Add multiple columns (with a constraint) inside a transaction.
+--
+CREATE TABLE t2 (a INT PRIMARY KEY)
+\set language lua
+box.begin() \
+box.execute('ALTER TABLE t2 ADD b INT') \
+box.execute('ALTER TABLE t2 ADD c INT UNIQUE') \
+box.commit()
+\set language sql
+INSERT INTO t2 VALUES (1, 1, 1);
+INSERT INTO t2 VALUES (2, 1, 1);
+SELECT * FROM t2;
+DROP TABLE t2;
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 7b18e5d6b..513ed1b62 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -856,6 +856,26 @@ box.execute("DROP TABLE t6")
---
- row_count: 1
...
+--
+-- gh-3075: Check the auto naming of CHECK constraints in
+-- <ALTER TABLE ADD COLUMN>.
+--
+box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY CHECK (a > 0))")
+---
+- row_count: 1
+...
+box.execute("ALTER TABLE check_naming ADD b INT CHECK (b > 0)")
+---
+- row_count: 0
+...
+box.execute("ALTER TABLE check_naming DROP CONSTRAINT \"ck_unnamed_CHECK_NAMING_2\"")
+---
+- row_count: 1
+...
+box.execute("DROP TABLE check_naming")
+---
+- row_count: 1
+...
test_run:cmd("clear filter")
---
- true
diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index 301f8ea69..a79131466 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -280,4 +280,13 @@ box.func.MYFUNC:drop()
box.execute("INSERT INTO t6 VALUES(11);");
box.execute("DROP TABLE t6")
+--
+-- gh-3075: Check the auto naming of CHECK constraints in
+-- <ALTER TABLE ADD COLUMN>.
+--
+box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY CHECK (a > 0))")
+box.execute("ALTER TABLE check_naming ADD b INT CHECK (b > 0)")
+box.execute("ALTER TABLE check_naming DROP CONSTRAINT \"ck_unnamed_CHECK_NAMING_2\"")
+box.execute("DROP TABLE check_naming")
+
test_run:cmd("clear filter")
diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result
index 33689a06e..de2a0c512 100644
--- a/test/sql/foreign-keys.result
+++ b/test/sql/foreign-keys.result
@@ -499,5 +499,33 @@ box.space.S:drop()
box.space.T:drop()
---
...
+--
+-- gh-3075: Check the auto naming of FOREIGN KEY constraints in
+-- <ALTER TABLE ADD COLUMN>.
+--
+box.execute("CREATE TABLE t1 (a INT PRIMARY KEY)")
+---
+- row_count: 1
+...
+box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY REFERENCES t1(a))")
+---
+- row_count: 1
+...
+box.execute("ALTER TABLE check_naming ADD b INT REFERENCES t1(a)")
+---
+- row_count: 0
+...
+box.execute("ALTER TABLE check_naming DROP CONSTRAINT \"fk_unnamed_CHECK_NAMING_2\"")
+---
+- row_count: 1
+...
+box.execute("DROP TABLE check_naming")
+---
+- row_count: 1
+...
+box.execute("DROP TABLE t1")
+---
+- row_count: 1
+...
--- Clean-up SQL DD hash.
-test_run:cmd('restart server default with cleanup=1')
diff --git a/test/sql/foreign-keys.test.lua b/test/sql/foreign-keys.test.lua
index d2dd88d28..29918c5d4 100644
--- a/test/sql/foreign-keys.test.lua
+++ b/test/sql/foreign-keys.test.lua
@@ -209,5 +209,16 @@ box.space.T:select()
box.space.S:drop()
box.space.T:drop()
+--
+-- gh-3075: Check the auto naming of FOREIGN KEY constraints in
+-- <ALTER TABLE ADD COLUMN>.
+--
+box.execute("CREATE TABLE t1 (a INT PRIMARY KEY)")
+box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY REFERENCES t1(a))")
+box.execute("ALTER TABLE check_naming ADD b INT REFERENCES t1(a)")
+box.execute("ALTER TABLE check_naming DROP CONSTRAINT \"fk_unnamed_CHECK_NAMING_2\"")
+box.execute("DROP TABLE check_naming")
+box.execute("DROP TABLE t1")
+
--- Clean-up SQL DD hash.
-test_run:cmd('restart server default with cleanup=1')
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/4] sql: rename TK_COLUMN to TK_COLUMN_NAME
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 1/4] sql: rename TK_COLUMN to TK_COLUMN_NAME Roman Khabibov
@ 2020-08-19 22:20 ` Vladislav Shpilevoy
2020-09-11 21:51 ` Roman Khabibov
0 siblings, 1 reply; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-08-19 22:20 UTC (permalink / raw)
To: Roman Khabibov, tarantool-patches
Thanks for the patch!
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index adf90d824..beb83ce95 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -1587,20 +1587,20 @@ struct Expr {
> #if SQL_MAX_EXPR_DEPTH>0
> int nHeight; /* Height of the tree headed by this node */
> #endif
> - int iTable; /* TK_COLUMN: cursor number of table holding column
> + int iTable; /* TK_COLUMN_NAME: cursor number of table holding column
> * TK_REGISTER: register number
> * TK_TRIGGER: 1 -> new, 0 -> old
> * EP_Unlikely: 134217728 times likelihood
> * TK_SELECT: 1st register of result vector
> */
> - ynVar iColumn; /* TK_COLUMN: column index.
> + ynVar iColumn; /* TK_COLUMN_NAME: column index.
Does not this look wrong to you? - 'COLUMN_NAME: column index'. In
some other places TK_COLUMN_NAME also designates presense of a column
index, not name. Probably a better name would be TK_COLUMN_REF.
> * TK_VARIABLE: variable number (always >= 1).
> * TK_SELECT_COLUMN: column of the result vector
> */
> i16 iAgg; /* Which entry in pAggInfo->aCol[] or ->aFunc[] */
> i16 iRightJoinTable; /* If EP_FromJoin, the right table of the join */
> u8 op2; /* TK_REGISTER: original value of Expr.op
> - * TK_COLUMN: the value of p5 for OP_Column
> + * TK_COLUMN_NAME: the value of p5 for OP_Column
> * TK_AGG_FUNCTION: nesting depth
> */
> AggInfo *pAggInfo; /* Used by TK_AGG_COLUMN and TK_AGG_FUNCTION */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 4/4] sql: support column addition
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 4/4] sql: support column addition Roman Khabibov
@ 2020-08-19 22:20 ` Vladislav Shpilevoy
2020-09-11 21:51 ` Roman Khabibov
0 siblings, 1 reply; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-08-19 22:20 UTC (permalink / raw)
To: Roman Khabibov, tarantool-patches
Thanks for the patch!
See 20 comments below.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 9013bc86f..6f3d2747d 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -285,48 +285,113 @@ 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 an existing space with
> + * <ALTER TABLE ADD COLUMN> statement. Copy space def and index
> + * array to create constraints appeared in the statement. The
> + * index array copy will be modified by adding new elements to it.
> + * It is necessary, because the statement may contain several
> + * index definitions (constraints).
> */
> +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);
1. Why can't you use parser's region?
> + if (ret->index == NULL) {
> + diag_set(OutOfMemory, indexes_sz, "realloc", "ret->index");
2. It is not realloc, it is malloc.
3. Seems you need to set parser->is_aborted = true in case of an error. As
far as I understand, it is a contract of each function taking Parse
argument.
> + return NULL;
> + }
> + memcpy(ret->index, space->index, indexes_sz);
> + memcpy(ret->def, space->def, sizeof(struct space_def));
> + ret->def->opts.is_temporary = true;
> + ret->def->opts.is_ephemeral = true;
> + if (ret->def->field_count != 0) {
> + uint32_t fields_size = 0;
> + ret->def->fields =
> + region_alloc_array(&parse->region,
> + typeof(struct field_def),
> + ret->def->field_count, &fields_size);
> + if (ret->def->fields == NULL) {
> + diag_set(OutOfMemory, fields_size, "region_alloc",
> + "ret->def->fields");
> + free(ret->index);
> + return NULL;
> + }
> + memcpy(ret->def->fields, space->def->fields, fields_size);
> + }
> +
> + return ret;
> +}
> @@ -334,18 +399,86 @@ sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
> +void
> +sql_create_column_end(struct Parse *parse)
> +{
> + struct space *space = parse->create_column_def.space;
> + assert(space != NULL);
> + struct space_def *def = space->def;
> + struct field_def *field = &def->fields[def->field_count - 1];
> + if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) {
> + 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);
4. You need to call sqlReleaseTempRange() somewhere.
> + 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);
> +
> + /*
> + * Clean up array allocated in sql_shallow_space_copy().
> + */
> + free(space->index);
5. It may happen that sql_create_column_end() is never called. For
example, if an error is encountered somewhere inside column definition,
after sql_create_column_start() is called.
> }
>
> 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))
> + assert(parser->create_column_def.space != NULL);
> + struct space_def *def = parser->create_column_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];
> if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
> nullable_action != field->nullable_action) {
> @@ -364,51 +497,42 @@ 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));
> + assert(pParse->create_column_def.space != NULL);
> + struct space_def *def = pParse->create_column_def.space->def;
> + struct field_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 {
> + 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);
6. Was it necessary to make so many changes? Wouldn't it work if you
would just replace
struct space *p = pParse->create_table_def.new_space;
with
struct space *p = pParse->create_column_def.space;
?
> }
>
> @@ -574,8 +700,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;
7. Why in some places you check create_table_def.new_space != NULL
first, and in some places create_column_def.space != NULL first? Is
the order important somehow?
> + bool is_alter_add_constr = space == NULL;
> /* Prepare payload for ck constraint definition. */
> struct region *region = &parser->region;> @@ -704,8 +845,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) {
8. space_by_id() above is not needed, if the definition is ephemeral now. It
can be moved below this condition so as not to call it when the result is
not going to be used anyway.
> assert(column < (uint32_t)def->field_count);
> *coll_id = def->fields[column].coll_id;
> struct coll_id *collation = coll_by_id(*coll_id);
> @@ -1148,15 +1292,21 @@ resolve_link(struct Parse *parse_context, const struct space_def *def,
>
> /**
> * Emit code to create sequences, indexes, check and foreign key
> - * constraints appeared in <CREATE TABLE>.
> + * constraints appeared in <CREATE TABLE> or
> + * <ALTER TABLE ADD COLUMN>.
> */
> 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;
> - assert(space != NULL);
> + 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;
9. Why do you need the original space for that? sql_shallow_space_copy()
copies index_count as well.
> + }
> + assert(space != NULL);
> for (; i < space->index_count; ++i) {
> struct index *idx = space->index[i];
> vdbe_emit_create_index(parse, space->def, idx->def,
> @@ -1908,6 +2077,8 @@ sql_create_foreign_key(struct Parse *parse_context)
> goto tnt_error;
> }
> memset(fk_parse, 0, sizeof(*fk_parse));
> + if (parse_context->create_column_def.space != NULL)
> + child_space = space;
10. Why?
> rlist_add_entry(&parse_context->fkeys, fk_parse, link);
> }
> struct Token *parent = create_fk_def->parent_name;
> @@ -1920,28 +2091,45 @@ sql_create_foreign_key(struct Parse *parse_context)
> 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(&parse_context->fkeys,
> - 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;
> }
^^^
11. This refactoring seems unnecessary. What changed here functionally?
> - 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,
> - ++parse_context->fkey_count);
> + uint32_t idx = ++parse_context->fkey_count;
> + /*
> + * If it is <ALTER TABLE ADD COLUMN> we
> + * should count the existing FK
> + * constraints in the space and form a
> + * name based on this.
> + */
> + 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)) {
12. You don't need to check for emptiness. rlist_foreach_entry() works fine
with an empty list.
> + 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);
> @@ -2001,7 +2189,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;
13. Why?
> 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);
> @@ -2420,10 +2613,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) {
14. Why not !is_create_table_or_add_col?
> + goto exit_create_index;
> }
> struct space_def *def = space->def;
>
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 995875566..0c9887851 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -281,9 +286,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.
> -//
> +/*
15. Out-of-function comments are started from /**.
> + * "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 }
> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
> index 1105fda6e..336914c57 100644
> --- a/src/box/sql/parse_def.h
> +++ b/src/box/sql/parse_def.h
> @@ -207,6 +209,14 @@ struct create_table_def {
> struct space *new_space;
> };
>
> +struct create_column_def {
> + struct create_entity_def base;
> + /** Shallow space_def copy. */
16. Not space_def.
> + struct space *space;
> + /** Column type. */
> + struct type_def *type_def;
> +};
> +
> struct create_view_def {
> struct create_entity_def base;
> /**
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index fa87e7bd2..32142a871 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -2860,15 +2864,30 @@ 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.
17. It fills create_column_def, not create_table_def.
> + */
> +void
> +sql_create_column_start(struct Parse *parse);
> +
> diff --git a/test/sql/add-column.result b/test/sql/add-column.result
> new file mode 100644
> index 000000000..f86259105
> --- /dev/null
> +++ b/test/sql/add-column.result
> @@ -0,0 +1,471 @@
> +-- test-run result file version 2
> +--
> +-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
> +--
> +CREATE TABLE t1 (a INT PRIMARY KEY);
> + | ---
> + | - row_count: 1
> + | ...
> +
> +--
> +-- COLUMN keyword is optional. Check it here, but omit it below.
> +--
> +ALTER TABLE t1 ADD COLUMN b INT;
> + | ---
> + | - row_count: 0
18. It seems you need to return row_count 1. To be consistent with
other ALTER TABLE expressions.
> + | ...
> +
> +--
> +-- A column with the same name already exists.
> +--
> +ALTER TABLE t1 ADD b SCALAR;
> + | ---
> + | - null
> + | - Space field 'B' is duplicate
> + | ...
> +
> +--
> +-- Can't add column to a view.
> +--
> +CREATE VIEW v AS SELECT * FROM t1;
> + | ---
> + | - row_count: 1
> + | ...
> +ALTER TABLE v ADD b INT;
> + | ---
> + | - null
> + | - Can't add column 'B'. 'V' is a view
19. What if I do the same via direct replace into _space in Lua?
> + | ...
> +DROP VIEW v;
> + | ---
> + | - row_count: 1
> + | ...
> +
> +--
> +-- Check PRIMARY KEY constraint works with an added column.
> +--
> +CREATE TABLE pk_check (a INT CONSTRAINT pk PRIMARY KEY);
> + | ---
> + | - row_count: 1
> + | ...
> +ALTER TABLE pk_check DROP CONSTRAINT pk;
> + | ---
> + | - row_count: 1
> + | ...
> +ALTER TABLE pk_check ADD b INT PRIMARY KEY;
> + | ---
> + | - row_count: 0
> + | ...
> +INSERT INTO pk_check VALUES (1, 1);
> + | ---
> + | - row_count: 1
> + | ...
> +INSERT INTO pk_check VALUES (1, 1);
> + | ---
> + | - null
> + | - Duplicate key exists in unique index 'pk_unnamed_PK_CHECK_1' in space 'PK_CHECK'
> + | ...
> +DROP TABLE pk_check;
> + | ---
> + | - row_count: 1
> + | ...
> +
> +--
> +-- Check UNIQUE constraint works with an added column.
> +--
> +CREATE TABLE unique_check (a INT PRIMARY KEY);
> + | ---
> + | - row_count: 1
> + | ...
> +ALTER TABLE unique_check ADD b INT UNIQUE;
> + | ---
> + | - row_count: 0
> + | ...
> +INSERT INTO unique_check VALUES (1, 1);
> + | ---
> + | - row_count: 1
> + | ...
> +INSERT INTO unique_check VALUES (2, 1);
> + | ---
> + | - null
> + | - Duplicate key exists in unique index 'unique_unnamed_UNIQUE_CHECK_2' in space 'UNIQUE_CHECK'
> + | ...
> +DROP TABLE unique_check;
> + | ---
> + | - row_count: 1
> + | ...
> +
> +--
> +-- Check CHECK constraint works with an added column.
> +--
> +CREATE TABLE ck_check (a INT PRIMARY KEY);
> + | ---
> + | - row_count: 1
> + | ...
> +ALTER TABLE ck_check ADD b INT CHECK (b > 0);
> + | ---
> + | - row_count: 0
> + | ...
> +INSERT INTO ck_check VALUES (1, 0);
> + | ---
> + | - null
> + | - 'Check constraint failed ''ck_unnamed_CK_CHECK_1'': b > 0'
> + | ...
> +DROP TABLE ck_check;
> + | ---
> + | - row_count: 1
> + | ...
> +
> +--
> +-- Check FOREIGN KEY constraint works with an added column.
> +--
> +CREATE TABLE fk_check (a INT PRIMARY KEY);
> + | ---
> + | - row_count: 1
> + | ...
> +ALTER TABLE fk_check ADD b INT REFERENCES t1(a);
> + | ---
> + | - row_count: 0
> + | ...
> +INSERT INTO fk_check VALUES (0, 1);
> + | ---
> + | - null
> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
> + | ...
> +INSERT INTO fk_check VALUES (2, 0);
> + | ---
> + | - null
> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
> + | ...
> +INSERT INTO fk_check VALUES (2, 1);
> + | ---
> + | - null
> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
20. It is worth adding one more test with a successfull insertion.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 4/4] sql: support column addition
2020-08-19 22:20 ` Vladislav Shpilevoy
@ 2020-09-11 21:51 ` Roman Khabibov
0 siblings, 0 replies; 9+ messages in thread
From: Roman Khabibov @ 2020-09-11 21:51 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Aug 20, 2020, at 01:20, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the patch!
>
> See 20 comments below.
>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 9013bc86f..6f3d2747d 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -285,48 +285,113 @@ 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 an existing space with
>> + * <ALTER TABLE ADD COLUMN> statement. Copy space def and index
>> + * array to create constraints appeared in the statement. The
>> + * index array copy will be modified by adding new elements to it.
>> + * It is necessary, because the statement may contain several
>> + * index definitions (constraints).
>> */
>> +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);
>
> 1. Why can't you use parser's region?
I added the patch. See it in the new patch set version.
>> + if (ret->index == NULL) {
>> + diag_set(OutOfMemory, indexes_sz, "realloc", "ret->index");
>
> 2. It is not realloc, it is malloc.
>
> 3. Seems you need to set parser->is_aborted = true in case of an error. As
> far as I understand, it is a contract of each function taking Parse
> argument.
Done.
+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)
+ goto error;
+ ret->index_count = space->index_count;
+ ret->index_id_max = space->index_id_max;
+ size_t size = 0;
+ ret->index = region_alloc_array(&parse->region, typeof(struct index *),
+ ret->index_count, &size);
+ if (ret->index == NULL) {
+ diag_set(OutOfMemory, size, "region_alloc_array", "ret->index");
+ goto error;
+ }
+ memcpy(ret->index, space->index, size);
+ memcpy(ret->def, space->def, sizeof(struct space_def));
+ ret->def->opts.is_temporary = true;
+ ret->def->opts.is_ephemeral = true;
+ if (ret->def->field_count != 0) {
+ uint32_t fields_size = 0;
+ ret->def->fields =
+ region_alloc_array(&parse->region,
+ typeof(struct field_def),
+ ret->def->field_count, &fields_size);
+ if (ret->def->fields == NULL) {
+ diag_set(OutOfMemory, fields_size, "region_alloc",
+ "ret->def->fields");
+ goto error;
+ }
+ memcpy(ret->def->fields, space->def->fields, fields_size);
+ }
+
+ return ret;
+error:
+ parse->is_aborted = true;
+ return NULL;
+}
>> + return NULL;
>> + }
>> + memcpy(ret->index, space->index, indexes_sz);
>> + memcpy(ret->def, space->def, sizeof(struct space_def));
>> + ret->def->opts.is_temporary = true;
>> + ret->def->opts.is_ephemeral = true;
>> + if (ret->def->field_count != 0) {
>> + uint32_t fields_size = 0;
>> + ret->def->fields =
>> + region_alloc_array(&parse->region,
>> + typeof(struct field_def),
>> + ret->def->field_count, &fields_size);
>> + if (ret->def->fields == NULL) {
>> + diag_set(OutOfMemory, fields_size, "region_alloc",
>> + "ret->def->fields");
>> + free(ret->index);
>> + return NULL;
>> + }
>> + memcpy(ret->def->fields, space->def->fields, fields_size);
>> + }
>> +
>> + return ret;
>> +}
>> @@ -334,18 +399,86 @@ sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
>> +void
>> +sql_create_column_end(struct Parse *parse)
>> +{
>> + struct space *space = parse->create_column_def.space;
>> + assert(space != NULL);
>> + struct space_def *def = space->def;
>> + struct field_def *field = &def->fields[def->field_count - 1];
>> + if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) {
>> + 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);
>
> 4. You need to call sqlReleaseTempRange() somewhere.
Done.
>> + 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);
>> +
>> + /*
>> + * Clean up array allocated in sql_shallow_space_copy().
>> + */
>> + free(space->index);
>
> 5. It may happen that sql_create_column_end() is never called. For
> example, if an error is encountered somewhere inside column definition,
> after sql_create_column_start() is called.
Removed with the region usage.
I also added the constants to AddOp functions.
+void
+sql_create_column_end(struct Parse *parse)
+{
+ struct space *space = parse->create_column_def.space;
+ assert(space != NULL);
+ struct space_def *def = space->def;
+ struct field_def *field = &def->fields[def->field_count - 1];
+ if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) {
+ 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 + BOX_SPACE_FIELD_FIELD_COUNT);
+ sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz,
+ tuple_reg + BOX_SPACE_FIELD_FORMAT,
+ 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);
+ sqlVdbeCountChanges(v);
+ sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
+ sqlReleaseTempRange(parse, tuple_reg, box_space_field_MAX + 1);
+ sql_vdbe_create_constraints(parse, key_reg);
}
>> }
>>
>> 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))
>> + assert(parser->create_column_def.space != NULL);
>> + struct space_def *def = parser->create_column_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];
>> if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
>> nullable_action != field->nullable_action) {
>> @@ -364,51 +497,42 @@ 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));
>> + assert(pParse->create_column_def.space != NULL);
>> + struct space_def *def = pParse->create_column_def.space->def;
>> + struct field_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 {
>> + 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);
>
> 6. Was it necessary to make so many changes? Wouldn't it work if you
> would just replace
>
> struct space *p = pParse->create_table_def.new_space;
>
> with
>
> struct space *p = pParse->create_column_def.space;
>
> ?
Yes. Done.
void
sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan)
{
sql *db = pParse->db;
- struct space *p = pParse->create_table_def.new_space;
+ struct space *p = pParse->create_column_def.space;
if (p != NULL) {
assert(p->def->opts.is_ephemeral);
struct space_def *def = p->def;
>> }
>>
>> @@ -574,8 +700,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;
>
> 7. Why in some places you check create_table_def.new_space != NULL
> first, and in some places create_column_def.space != NULL first? Is
> the order important somehow?
The order is not important.
>> + bool is_alter_add_constr = space == NULL;
>> /* Prepare payload for ck constraint definition. */
>> struct region *region = &parser->region;> @@ -704,8 +845,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) {
>
> 8. space_by_id() above is not needed, if the definition is ephemeral now. It
> can be moved below this condition so as not to call it when the result is
> not going to be used anyway.
Yes.
@@ -704,13 +851,13 @@ 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);
return collation != NULL ? collation->coll : NULL;
}
+ struct space *space = space_by_id(def->id);
struct tuple_field *field = tuple_format_field(space->format, column);
*coll_id = field->coll_id;
return field->coll;
>> assert(column < (uint32_t)def->field_count);
>> *coll_id = def->fields[column].coll_id;
>> struct coll_id *collation = coll_by_id(*coll_id);
>> @@ -1148,15 +1292,21 @@ resolve_link(struct Parse *parse_context, const struct space_def *def,
>>
>> /**
>> * Emit code to create sequences, indexes, check and foreign key
>> - * constraints appeared in <CREATE TABLE>.
>> + * constraints appeared in <CREATE TABLE> or
>> + * <ALTER TABLE ADD COLUMN>.
>> */
>> 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;
>> - assert(space != NULL);
>> + 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;
>
> 9. Why do you need the original space for that? sql_shallow_space_copy()
> copies index_count as well.
Now, explained this step in the comment.
/**
* Emit code to create sequences, indexes, check and foreign key
- * constraints appeared in <CREATE TABLE>.
+ * constraints appeared in <CREATE TABLE> or
+ * <ALTER TABLE ADD COLUMN>.
*/
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;
- assert(space != NULL);
+ bool is_alter = space == NULL;
uint32_t i = 0;
+ /*
+ * If it is an <ALTER TABLE ADD COLUMN>, then we have to
+ * create all indexes added by this statement. These
+ * indexes are in the array, starting with old index_count
+ * (inside space object) and ending with new index_count
+ * (inside ephemeral space).
+ */
+ if (is_alter) {
+ space = parse->create_column_def.space;
+ i = space_by_name(space->def->name)->index_count;
+ }
+ assert(space != NULL);
>> + }
>> + assert(space != NULL);
>> for (; i < space->index_count; ++i) {
>> struct index *idx = space->index[i];
>> vdbe_emit_create_index(parse, space->def, idx->def,
>> @@ -1908,6 +2077,8 @@ sql_create_foreign_key(struct Parse *parse_context)
>> goto tnt_error;
>> }
>> memset(fk_parse, 0, sizeof(*fk_parse));
>> + if (parse_context->create_column_def.space != NULL)
>> + child_space = space;
>
> 10. Why?
Fixed.
@@ -1908,6 +2091,12 @@ sql_create_foreign_key(struct Parse *parse_context)
goto tnt_error;
}
memset(fk_parse, 0, sizeof(*fk_parse));
+ /*
+ * Child space already exists if it is
+ * <ALTER TABLE ADD COLUMN>.
+ */
+ if (table_def->new_space == NULL)
+ child_space = space;
>> rlist_add_entry(&parse_context->fkeys, fk_parse, link);
>> }
>> struct Token *parent = create_fk_def->parent_name;
>> @@ -1920,28 +2091,45 @@ sql_create_foreign_key(struct Parse *parse_context)
>> 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(&parse_context->fkeys,
>> - 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;
>> }
>
> ^^^
> 11. This refactoring seems unnecessary. What changed here functionally?
No. I need to execute this branch, when parent_space != NULL (ADD COLUMN case).
Tests fail without this change.
+ if (is_self_referenced) {
+ struct fk_constraint_parse *fk =
+ rlist_first_entry(&parse_context->fkeys_def.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,
>> - ++parse_context->fkey_count);
>> + uint32_t idx = ++parse_context->fkey_count;
>> + /*
>> + * If it is <ALTER TABLE ADD COLUMN> we
>> + * should count the existing FK
>> + * constraints in the space and form a
>> + * name based on this.
>> + */
>> + 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)) {
>
> 12. You don't need to check for emptiness. rlist_foreach_entry() works fine
> with an empty list.
Ok. Removed.
>> + 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);
>> @@ -2001,7 +2189,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;
>
> 13. Why?
Removed.
>> 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);
>> @@ -2420,10 +2613,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) {
>
> 14. Why not !is_create_table_or_add_col?
Fixed.
@@ -2439,10 +2646,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 (!is_create_table_or_add_col) {
+ goto exit_create_index;
>> + goto exit_create_index;
>> }
>> struct space_def *def = space->def;
>>
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index 995875566..0c9887851 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -281,9 +286,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.
>> -//
>> +/*
>
> 15. Out-of-function comments are started from /**.
Fixed.
>> + * "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 }
>> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
>> index 1105fda6e..336914c57 100644
>> --- a/src/box/sql/parse_def.h
>> +++ b/src/box/sql/parse_def.h
>> @@ -207,6 +209,14 @@ struct create_table_def {
>> struct space *new_space;
>> };
>>
>> +struct create_column_def {
>> + struct create_entity_def base;
>> + /** Shallow space_def copy. */
>
> 16. Not space_def.
Fixed.
>> + struct space *space;
>> + /** Column type. */
>> + struct type_def *type_def;
>> +};
>> +
>> struct create_view_def {
>> struct create_entity_def base;
>> /**
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index fa87e7bd2..32142a871 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -2860,15 +2864,30 @@ 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.
>
> 17. It fills create_column_def, not create_table_def.
-void sqlAddColumn(Parse *, Token *, struct type_def *);
+
+/**
+ * Add new field to the format of ephemeral space in
+ * create_column_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);
>> + */
>> +void
>> +sql_create_column_start(struct Parse *parse);
>> +
>> diff --git a/test/sql/add-column.result b/test/sql/add-column.result
>> new file mode 100644
>> index 000000000..f86259105
>> --- /dev/null
>> +++ b/test/sql/add-column.result
>> @@ -0,0 +1,471 @@
>> +-- test-run result file version 2
>> +--
>> +-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
>> +--
>> +CREATE TABLE t1 (a INT PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- COLUMN keyword is optional. Check it here, but omit it below.
>> +--
>> +ALTER TABLE t1 ADD COLUMN b INT;
>> + | ---
>> + | - row_count: 0
>
> 18. It seems you need to return row_count 1. To be consistent with
> other ALTER TABLE expressions.
Done. I added opcode emission.
>> + | ...
>> +
>> +--
>> +-- A column with the same name already exists.
>> +--
>> +ALTER TABLE t1 ADD b SCALAR;
>> + | ---
>> + | - null
>> + | - Space field 'B' is duplicate
>> + | ...
>> +
>> +--
>> +-- Can't add column to a view.
>> +--
>> +CREATE VIEW v AS SELECT * FROM t1;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE v ADD b INT;
>> + | ---
>> + | - null
>> + | - Can't add column 'B'. 'V' is a view
>
> 19. What if I do the same via direct replace into _space in Lua?
See the new patch in the patch set.
>> + | ...
>> +DROP VIEW v;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- Check PRIMARY KEY constraint works with an added column.
>> +--
>> +CREATE TABLE pk_check (a INT CONSTRAINT pk PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE pk_check DROP CONSTRAINT pk;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE pk_check ADD b INT PRIMARY KEY;
>> + | ---
>> + | - row_count: 0
>> + | ...
>> +INSERT INTO pk_check VALUES (1, 1);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +INSERT INTO pk_check VALUES (1, 1);
>> + | ---
>> + | - null
>> + | - Duplicate key exists in unique index 'pk_unnamed_PK_CHECK_1' in space 'PK_CHECK'
>> + | ...
>> +DROP TABLE pk_check;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- Check UNIQUE constraint works with an added column.
>> +--
>> +CREATE TABLE unique_check (a INT PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE unique_check ADD b INT UNIQUE;
>> + | ---
>> + | - row_count: 0
>> + | ...
>> +INSERT INTO unique_check VALUES (1, 1);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +INSERT INTO unique_check VALUES (2, 1);
>> + | ---
>> + | - null
>> + | - Duplicate key exists in unique index 'unique_unnamed_UNIQUE_CHECK_2' in space 'UNIQUE_CHECK'
>> + | ...
>> +DROP TABLE unique_check;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- Check CHECK constraint works with an added column.
>> +--
>> +CREATE TABLE ck_check (a INT PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE ck_check ADD b INT CHECK (b > 0);
>> + | ---
>> + | - row_count: 0
>> + | ...
>> +INSERT INTO ck_check VALUES (1, 0);
>> + | ---
>> + | - null
>> + | - 'Check constraint failed ''ck_unnamed_CK_CHECK_1'': b > 0'
>> + | ...
>> +DROP TABLE ck_check;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- Check FOREIGN KEY constraint works with an added column.
>> +--
>> +CREATE TABLE fk_check (a INT PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE fk_check ADD b INT REFERENCES t1(a);
>> + | ---
>> + | - row_count: 0
>> + | ...
>> +INSERT INTO fk_check VALUES (0, 1);
>> + | ---
>> + | - null
>> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
>> + | ...
>> +INSERT INTO fk_check VALUES (2, 0);
>> + | ---
>> + | - null
>> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
>> + | ...
>> +INSERT INTO fk_check VALUES (2, 1);
>> + | ---
>> + | - null
>> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
>
> 20. It is worth adding one more test with a successfull insertion.
+INSERT INTO fk_check VALUES (2, 1);
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | ...
+INSERT INTO t1 VALUES (1, 1);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO fk_check VALUES (2, 1);
+ | ---
+ | - row_count: 1
+ | …
+INSERT INTO null_check(a) VALUES (1);
+ | ---
+ | - row_count: 1
+ | ...
+SELECT * FROM null_check;
+ | ---
+ | - metadata:
+ | - name: A
+ | type: integer
+ | - name: B
+ | type: string
+ | rows:
+ | - [1, null]
+ | …
+INSERT INTO notnull_check(a) VALUES (1);
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: NOT NULL constraint failed: NOTNULL_CHECK.B'
+ | ...
+INSERT INTO notnull_check VALUES (1, 'not null');
+ | ---
+ | - row_count: 1
+ | ...
+DROP TABLE notnull_check;
+ | ---
+ | - row_count: 1
+ | …
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/4] sql: rename TK_COLUMN to TK_COLUMN_NAME
2020-08-19 22:20 ` Vladislav Shpilevoy
@ 2020-09-11 21:51 ` Roman Khabibov
0 siblings, 0 replies; 9+ messages in thread
From: Roman Khabibov @ 2020-09-11 21:51 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Aug 20, 2020, at 01:20, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the patch!
>
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index adf90d824..beb83ce95 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -1587,20 +1587,20 @@ struct Expr {
>> #if SQL_MAX_EXPR_DEPTH>0
>> int nHeight; /* Height of the tree headed by this node */
>> #endif
>> - int iTable; /* TK_COLUMN: cursor number of table holding column
>> + int iTable; /* TK_COLUMN_NAME: cursor number of table holding column
>> * TK_REGISTER: register number
>> * TK_TRIGGER: 1 -> new, 0 -> old
>> * EP_Unlikely: 134217728 times likelihood
>> * TK_SELECT: 1st register of result vector
>> */
>> - ynVar iColumn; /* TK_COLUMN: column index.
>> + ynVar iColumn; /* TK_COLUMN_NAME: column index.
>
> Does not this look wrong to you? - 'COLUMN_NAME: column index'. In
> some other places TK_COLUMN_NAME also designates presense of a column
> index, not name. Probably a better name would be TK_COLUMN_REF.
>
Ok. Done.
>> * TK_VARIABLE: variable number (always >= 1).
>> * TK_SELECT_COLUMN: column of the result vector
>> */
>> i16 iAgg; /* Which entry in pAggInfo->aCol[] or ->aFunc[] */
>> i16 iRightJoinTable; /* If EP_FromJoin, the right table of the join */
>> u8 op2; /* TK_REGISTER: original value of Expr.op
>> - * TK_COLUMN: the value of p5 for OP_Column
>> + * TK_COLUMN_NAME: the value of p5 for OP_Column
>> * TK_AGG_FUNCTION: nesting depth
>> */
>> AggInfo *pAggInfo; /* Used by TK_AGG_COLUMN and TK_AGG_FUNCTION */
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-11 21:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-11 0:33 [Tarantool-patches] [PATCH v3 0/4] Support ALTER TABLE ADD COLUMN Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 1/4] sql: rename TK_COLUMN to TK_COLUMN_NAME Roman Khabibov
2020-08-19 22:20 ` Vladislav Shpilevoy
2020-09-11 21:51 ` Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 2/4] sql: refactor create_table_def and parse Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 3/4] schema: add box_space_field_MAX Roman Khabibov
2020-08-11 0:33 ` [Tarantool-patches] [PATCH v3 4/4] sql: support column addition Roman Khabibov
2020-08-19 22:20 ` Vladislav Shpilevoy
2020-09-11 21:51 ` Roman Khabibov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox