* [Tarantool-patches] [PATCH v4 0/5] Support column addition
@ 2020-10-09 13:45 Roman Khabibov
2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 1/5] sql: rename TK_COLUMN to TK_COLUMN_REF Roman Khabibov
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Roman Khabibov @ 2020-10-09 13:45 UTC (permalink / raw)
To: tarantool-patches
I alreday have LGTM from Vlad.
About non empty spaces: https://github.com/tarantool/tarantool/issues/5405
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3075-add-column-v2
Issues: https://github.com/tarantool/tarantool/issues/2349 and
https://github.com/tarantool/tarantool/issues/3075
@ChangeLog
* Support <ALTER TABLE ADD COLUMN> statement (gh-3075).
Roman Khabibov (5):
sql: rename TK_COLUMN to TK_COLUMN_REF
sql: refactor create_table_def and parse
schema: add box_space_field_MAX
sql: use parser's region of "index" array
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 | 617 +++++++++++++++++++++++----------
src/box/sql/expr.c | 48 +--
src/box/sql/fk_constraint.c | 2 +-
src/box/sql/parse.y | 48 ++-
src/box/sql/parse_def.h | 83 +++--
src/box/sql/prepare.c | 3 +-
src/box/sql/resolve.c | 10 +-
src/box/sql/select.c | 10 +-
src/box/sql/sqlInt.h | 59 +++-
src/box/sql/tokenize.c | 17 +-
src/box/sql/treeview.c | 2 +-
src/box/sql/where.c | 18 +-
src/box/sql/whereexpr.c | 12 +-
test/box/error.result | 1 +
test/sql/add-column.result | 529 ++++++++++++++++++++++++++++
test/sql/add-column.test.sql | 183 ++++++++++
test/sql/checks.result | 20 ++
test/sql/checks.test.lua | 9 +
test/sql/foreign-keys.result | 28 ++
test/sql/foreign-keys.test.lua | 11 +
24 files changed, 1426 insertions(+), 296 deletions(-)
create mode 100644 test/sql/add-column.result
create mode 100644 test/sql/add-column.test.sql
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 18+ messages in thread* [Tarantool-patches] [PATCH v4 1/5] sql: rename TK_COLUMN to TK_COLUMN_REF 2020-10-09 13:45 [Tarantool-patches] [PATCH v4 0/5] Support column addition Roman Khabibov @ 2020-10-09 13:45 ` Roman Khabibov 2020-11-05 22:17 ` Nikita Pettik 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse Roman Khabibov ` (3 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Roman Khabibov @ 2020-10-09 13:45 UTC (permalink / raw) To: tarantool-patches Rename TK_COLUMN used for tokens treated as a column name to TK_COLUMN_REF. It is needed to allow the typing of COLUMN keyword in <ALTER TABLE ADD COLUMN> statement. Needed for #3075 --- extra/addopcodes.sh | 2 +- extra/mkkeywordhash.c | 5 +--- src/box/sql/build.c | 2 +- src/box/sql/expr.c | 48 ++++++++++++++++++++----------------- 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, 61 insertions(+), 58 deletions(-) diff --git a/extra/addopcodes.sh b/extra/addopcodes.sh index cb6c84725..3f8cfdf02 100755 --- a/extra/addopcodes.sh +++ b/extra/addopcodes.sh @@ -39,7 +39,7 @@ extras=" \ END_OF_FILE \ UNCLOSED_STRING \ FUNCTION \ - COLUMN \ + COLUMN_REF \ AGG_FUNCTION \ AGG_COLUMN \ UMINUS \ diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c index dd42c8f5f..bacdad9b3 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_REF", "TK_COLUMN_REF", 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..d55c1cd71 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_REF) { 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..3772596d6 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_REF: 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_REF || 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_REF 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_REF: case TK_AGG_FUNCTION: case TK_AGG_COLUMN: testcase(pExpr->op == TK_ID); - testcase(pExpr->op == TK_COLUMN); + testcase(pExpr->op == TK_COLUMN_REF); 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_REF: 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_REF: /* 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_REF) return 0; assert(pRes->iTable == pSrc->a[0].iCursor); /* Not a correlated subquery */ } @@ -3707,10 +3707,13 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) } return target; } - /* Otherwise, fall thru into the TK_COLUMN case */ + /* + * Otherwise, fall thru into the + * TK_COLUMN_REF case. + */ FALLTHROUGH; } - case TK_COLUMN:{ + case TK_COLUMN_REF:{ int iTab = pExpr->iTable; int col = pExpr->iColumn; if (iTab < 0) { @@ -4102,7 +4105,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_REF || exprOp == TK_AGG_COLUMN) { assert(SQL_FUNC_LENGTH == OPFLAG_LENGTHARG); @@ -4319,7 +4322,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_REF); exprToRegister(&tempX, exprCodeVector(pParse, &tempX, ®Free1)); @@ -4344,11 +4347,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_REF); sqlExprIfFalse(pParse, pTest, nextCase, SQL_JUMPIFNULL); testcase(aListelem[i + 1].pExpr->op == - TK_COLUMN); + TK_COLUMN_REF); sqlExprCode(pParse, aListelem[i + 1].pExpr, target); sqlVdbeGoto(v, endLabel); @@ -5081,7 +5084,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_REF && 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 +5165,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_REF 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 +5213,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_REFs 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_REF || NEVER(pExpr->op == TK_AGG_COLUMN)) { int i; struct SrcCount *p = pWalker->u.pSrcCount; SrcList *pSrc = p->pSrc; @@ -5299,9 +5303,9 @@ analyzeAggregate(Walker * pWalker, Expr * pExpr) switch (pExpr->op) { case TK_AGG_COLUMN: - case TK_COLUMN:{ + case TK_COLUMN_REF:{ testcase(pExpr->op == TK_AGG_COLUMN); - testcase(pExpr->op == TK_COLUMN); + testcase(pExpr->op == TK_COLUMN_REF); /* Check to see if the column is in one of the tables in the FROM * clause of the aggregate query */ @@ -5370,7 +5374,7 @@ analyzeAggregate(Walker * pWalker, Expr * pExpr) if (pE-> op == - TK_COLUMN + TK_COLUMN_REF && pE-> iTable diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c index 482220a95..0dd10c420 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_REF); 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..11b6139e3 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_REF. * 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_REF); 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_REF); 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_REF 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_REF * 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..c638511ef 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_REF || 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_REF && 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_REF && 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_REF 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_REF; } } diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index adf90d824..5913d7614 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_REF 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_REF: 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_REF: 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_REF: 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..5f042ce8b 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_REF:{ 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..bb84cbadb 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_REF) { 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_REF && 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_REF && 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_REF && 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_REF || 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_REF && 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_REF) 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_REF) continue; if (pOBExpr->iTable != iCur) continue; diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c index d9b5c78f5..3811ef3cf 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_REF || 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_REF 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_REF) { *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_REF && 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_REF) { 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_REF); if (pColRef == NULL) { pParse->is_aborted = true; return; -- 2.24.3 (Apple Git-128) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 1/5] sql: rename TK_COLUMN to TK_COLUMN_REF 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 1/5] sql: rename TK_COLUMN to TK_COLUMN_REF Roman Khabibov @ 2020-11-05 22:17 ` Nikita Pettik 0 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2020-11-05 22:17 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 09 Oct 16:45, Roman Khabibov wrote: > Rename TK_COLUMN used for tokens treated as a column name to > TK_COLUMN_REF. It is needed to allow the typing of COLUMN keyword > in <ALTER TABLE ADD COLUMN> statement. > > Needed for #3075 > --- LGTM ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse 2020-10-09 13:45 [Tarantool-patches] [PATCH v4 0/5] Support column addition Roman Khabibov 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 1/5] sql: rename TK_COLUMN to TK_COLUMN_REF Roman Khabibov @ 2020-10-09 13:45 ` Roman Khabibov 2020-11-05 22:17 ` Nikita Pettik 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 3/5] schema: add box_space_field_MAX Roman Khabibov ` (2 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Roman Khabibov @ 2020-10-09 13:45 UTC (permalink / raw) To: tarantool-patches Move ck, fk constraint lists from struct create_table_def into new defs and autoincrement into struct Parse to make the code more reusable when implementing <ALTER TABLE ADD COLUMN>. Needed for #3075 --- src/box/sql/build.c | 192 ++++++++++++++++++++++------------------ src/box/sql/parse.y | 2 + src/box/sql/parse_def.h | 57 ++++++------ src/box/sql/prepare.c | 3 +- src/box/sql/sqlInt.h | 12 +++ 5 files changed, 152 insertions(+), 114 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index d55c1cd71..3b3c8099a 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -590,8 +590,8 @@ sql_create_check_contraint(struct Parse *parser) } } else { assert(! is_alter); - uint32_t ck_idx = ++parser->create_table_def.check_count; - name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, ck_idx); + uint32_t ck_idx = ++parser->create_checks_def.count; + name = tt_sprintf("ck_unnamed_%s_%u", space->def->name, ck_idx); } size_t name_len = strlen(name); @@ -652,7 +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, + rlist_add_entry(&parser->create_checks_def.checks, ck_parse, link); } } @@ -930,7 +930,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 +1147,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 +vdbe_emit_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->create_fkeys_def.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->create_checks_def.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 +1295,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); - } + vdbe_emit_create_constraints(pParse, reg_space_id); } void @@ -1893,7 +1909,8 @@ 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->create_fkeys_def.fkeys, + fk_parse, link); } struct Token *parent = create_fk_def->parent_name; assert(parent != NULL); @@ -1910,8 +1927,10 @@ 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 rlist *fkeys = + &parse_context->create_fkeys_def.fkeys; struct fk_constraint_parse *fk = - rlist_first_entry(&table_def->new_fkey, + rlist_first_entry(fkeys, struct fk_constraint_parse, link); fk->selfref_cols = parent_cols; @@ -1923,10 +1942,9 @@ sql_create_foreign_key(struct Parse *parse_context) } if (!is_alter) { if (create_def->name.n == 0) { - constraint_name = - sqlMPrintf(db, "fk_unnamed_%s_%d", - space->def->name, - ++table_def->fkey_count); + uint32_t idx = ++parse_context->create_fkeys_def.count; + constraint_name = sqlMPrintf(db, "fk_unnamed_%s_%u", + space->def->name, idx); } else { constraint_name = sql_name_from_token(db, &create_def->name); @@ -2041,9 +2059,10 @@ sql_create_foreign_key(struct Parse *parse_context) * maintain list of all FK constraints inside parser. */ if (!is_alter) { + struct rlist *fkeys = &parse_context->create_fkeys_def.fkeys; struct fk_constraint_parse *fk_parse = - rlist_first_entry(&table_def->new_fkey, - struct fk_constraint_parse, link); + rlist_first_entry(fkeys, struct fk_constraint_parse, + link); fk_parse->fk_def = fk_def; } else { vdbe_emit_fk_constraint_create(parse_context, fk_def, @@ -2065,12 +2084,11 @@ 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)) + struct rlist *fkeys = &parse_context->create_fkeys_def.fkeys; + if (parse_context->db->init.busy || rlist_empty(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(fkeys, struct fk_constraint_parse, + link)->fk_def->is_deferred = is_deferred; } /** @@ -3306,15 +3324,15 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id, int sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno) { - if (parse_context->create_table_def.has_autoinc) { + if (parse_context->has_autoinc) { diag_set(ClientError, ER_SQL_SYNTAX_WITH_POS, parse_context->line_count, parse_context->line_pos, "table must feature at most one AUTOINCREMENT field"); parse_context->is_aborted = true; return -1; } - parse_context->create_table_def.has_autoinc = true; - parse_context->create_table_def.autoinc_fieldno = fieldno; + parse_context->has_autoinc = true; + parse_context->autoinc_fieldno = fieldno; return 0; } diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 995875566..da4e2a3ae 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -181,6 +181,8 @@ cmd ::= ROLLBACK TO savepoint_opt nm(X). { cmd ::= create_table create_table_args with_opts create_table_end. create_table ::= createkw TABLE ifnotexists(E) nm(Y). { create_table_def_init(&pParse->create_table_def, &Y, E); + create_checks_def_init(&pParse->create_checks_def); + create_fkeys_def_init(&pParse->create_fkeys_def); pParse->create_table_def.new_space = sqlStartTable(pParse, &Y); pParse->initiateTTrans = true; } diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h index cb0ecd2fc..21829b6f0 100644 --- a/src/box/sql/parse_def.h +++ b/src/box/sql/parse_def.h @@ -205,26 +205,20 @@ 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_checks_def { + /** List of ck_constraint_parse objects. */ + struct rlist checks; + /** Count of ck_constraint_parse objects. */ + uint32_t count; +}; + +struct create_fkeys_def { + /** List of fk_constraint_parse objects. */ + struct rlist fkeys; + /** Count of fk_constraint_parse objects. */ + uint32_t count; }; struct create_view_def { @@ -482,9 +476,20 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name, { create_entity_def_init(&table_def->base, ENTITY_TYPE_TABLE, NULL, name, if_not_exists); - rlist_create(&table_def->new_fkey); - rlist_create(&table_def->new_check); - table_def->autoinc_fieldno = 0; +} + +static inline void +create_checks_def_init(struct create_checks_def *checks_def) +{ + rlist_create(&checks_def->checks); + checks_def->count = 0; +} + +static inline void +create_fkeys_def_init(struct create_fkeys_def *fkeys_def) +{ + rlist_create(&fkeys_def->fkeys); + fkeys_def->count = 0; } static inline void @@ -500,12 +505,12 @@ create_view_def_init(struct create_view_def *view_def, struct Token *name, } static inline void -create_table_def_destroy(struct create_table_def *table_def) +create_fkeys_def_destroy(struct create_fkeys_def *fkeys_def) { - if (table_def->new_space == NULL) + if (fkeys_def->count == 0) return; struct fk_constraint_parse *fk; - rlist_foreach_entry(fk, &table_def->new_fkey, link) + rlist_foreach_entry(fk, &fkeys_def->fkeys, link) sql_expr_list_delete(sql_get(), fk->selfref_cols); } diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index a5a258805..1a0cfb9c5 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -200,6 +200,7 @@ 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; + parser->has_autoinc = false; region_create(&parser->region, &cord()->slabc); } @@ -211,7 +212,7 @@ 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); + create_fkeys_def_destroy(&parser->create_fkeys_def); if (db != NULL) { assert(db->lookaside.bDisable >= parser->disableLookaside); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 5913d7614..3d54835d2 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2257,6 +2257,18 @@ struct Parse { * sqlEndTable() function). */ struct create_table_def create_table_def; + /* + * FK and CK constraints appeared in a <CREATE TABLE>. + */ + struct create_fkeys_def create_fkeys_def; + struct create_checks_def create_checks_def; + /* + * 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.24.3 (Apple Git-128) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse Roman Khabibov @ 2020-11-05 22:17 ` Nikita Pettik 2020-11-10 12:17 ` Roman Khabibov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-11-05 22:17 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 09 Oct 16:45, Roman Khabibov wrote: > Move ck, fk constraint lists from struct create_table_def into new > defs and autoincrement into struct Parse to make the code more > reusable when implementing <ALTER TABLE ADD COLUMN>. > > Needed for #3075 > --- > > +/** > + * Emit code to create sequences, indexes, check and foreign key > + * constraints appeared in <CREATE TABLE>. > + */ > +static void > +vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id) I'd move this refactoring in a separate patch. Up to you. > @@ -3306,15 +3324,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..21829b6f0 100644 > --- a/src/box/sql/parse_def.h > +++ b/src/box/sql/parse_def.h > @@ -205,26 +205,20 @@ 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; Did you consider adding create_column_def class? Imho it would fit better in parse hierarchy: it would derive from create_entity_def, has field_def, autoinc, ck, fk members. > +}; > + > +struct create_checks_def { Why not create_ck_constraint_def? This naming would be more consistent with existing ck_contraint_def etc. The same for create_fk_constraint_def. > + /** List of ck_constraint_parse objects. */ > + struct rlist checks; > + /** Count of ck_constraint_parse objects. */ > + uint32_t count; > +}; > + ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse 2020-11-05 22:17 ` Nikita Pettik @ 2020-11-10 12:17 ` Roman Khabibov 2020-11-17 20:23 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Roman Khabibov @ 2020-11-10 12:17 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches Hi! Thanks for the review. > On Nov 6, 2020, at 01:17, Nikita Pettik <korablev@tarantool.org> wrote: > > On 09 Oct 16:45, Roman Khabibov wrote: >> Move ck, fk constraint lists from struct create_table_def into new >> defs and autoincrement into struct Parse to make the code more >> reusable when implementing <ALTER TABLE ADD COLUMN>. >> >> Needed for #3075 >> --- >> >> +/** >> + * Emit code to create sequences, indexes, check and foreign key >> + * constraints appeared in <CREATE TABLE>. >> + */ >> +static void >> +vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id) > > I'd move this refactoring in a separate patch. Up to you. > >> @@ -3306,15 +3324,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..21829b6f0 100644 >> --- a/src/box/sql/parse_def.h >> +++ b/src/box/sql/parse_def.h >> @@ -205,26 +205,20 @@ 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; > > Did you consider adding create_column_def class? Imho it would > fit better in parse hierarchy: it would derive from create_entity_def, > has field_def, autoinc, ck, fk members. Yes. We discussed this a lot with Vlad. This class is implemented in the main, last patch of the patchset. Unfortunately, the check and fk constraints lists had to be removed separately from this class, because the check and fk descriptions can occur separately from the column descriptions in CREATE TABLE, e.g. " CREATE TABLE t (a INT PRIMARY KEY, CHECK (a > 0))”. To make the code reusable, it is easier to emit opcode for all constants after CREATE TABLE parsing. For the same reason, autoinc is separate. >> +}; >> + >> +struct create_checks_def { > > Why not create_ck_constraint_def? This naming would be more consistent > with existing ck_contraint_def etc. The same for create_fk_constraint_def. Because these are lists of constant defs. We decided to emphasize this. > >> + /** List of ck_constraint_parse objects. */ >> + struct rlist checks; >> + /** Count of ck_constraint_parse objects. */ >> + uint32_t count; >> +}; >> + ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse 2020-11-10 12:17 ` Roman Khabibov @ 2020-11-17 20:23 ` Nikita Pettik 2020-11-15 11:51 ` roman 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-11-17 20:23 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 10 Nov 15:17, Roman Khabibov wrote: > Hi! Thanks for the review. > > > On Nov 6, 2020, at 01:17, Nikita Pettik <korablev@tarantool.org> wrote: > > > > On 09 Oct 16:45, Roman Khabibov wrote: > >> +++ b/src/box/sql/parse_def.h > >> @@ -205,26 +205,20 @@ struct create_entity_def { > >> - 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; > > > > Did you consider adding create_column_def class? Imho it would > > fit better in parse hierarchy: it would derive from create_entity_def, > > has field_def, autoinc, ck, fk members. > Yes. We discussed this a lot with Vlad. This class is implemented in > the main, last patch of the patchset. Unfortunately, the check and > fk constraints lists had to be removed separately from this class, > because the check and fk descriptions can occur separately from the > column descriptions in CREATE TABLE, e.g. " CREATE TABLE t (a INT > PRIMARY KEY, CHECK (a > 0))”. To make the code reusable, it is easier > to emit opcode for all constants after CREATE TABLE parsing. For the What does 'constant' mean? IMHO it is not so 'reusable': anyway you have a lot of if (is_alter/is_alter_add_constr..) branches. But OK, since patch is ready, I believe this refactoring is not worth time required for that. > same reason, autoinc is separate. > > >> +}; > >> + > >> +struct create_checks_def { > > > > Why not create_ck_constraint_def? This naming would be more consistent > > with existing ck_contraint_def etc. The same for create_fk_constraint_def. > Because these are lists of constant defs. We decided to emphasize this. Do not get what's that supposed to mean. This is naming which is taken in our source code.. If you have any strong arguments from your discussion, please copy-paste them in the answer. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse 2020-11-17 20:23 ` Nikita Pettik @ 2020-11-15 11:51 ` roman 0 siblings, 0 replies; 18+ messages in thread From: roman @ 2020-11-15 11:51 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches Hi! Thanks for the review. On 17.11.2020 23:23, Nikita Pettik wrote: > On 10 Nov 15:17, Roman Khabibov wrote: >> Hi! Thanks for the review. >> >>> On Nov 6, 2020, at 01:17, Nikita Pettik <korablev@tarantool.org> wrote: >>> >>> On 09 Oct 16:45, Roman Khabibov wrote: >>>> +++ b/src/box/sql/parse_def.h >>>> @@ -205,26 +205,20 @@ struct create_entity_def { >>>> - 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; >>> Did you consider adding create_column_def class? Imho it would >>> fit better in parse hierarchy: it would derive from create_entity_def, >>> has field_def, autoinc, ck, fk members. >> Yes. We discussed this a lot with Vlad. This class is implemented in >> the main, last patch of the patchset. Unfortunately, the check and >> fk constraints lists had to be removed separately from this class, >> because the check and fk descriptions can occur separately from the >> column descriptions in CREATE TABLE, e.g. " CREATE TABLE t (a INT >> PRIMARY KEY, CHECK (a > 0))”. To make the code reusable, it is easier >> to emit opcode for all constants after CREATE TABLE parsing. For the > What does 'constant' mean? IMHO it is not so 'reusable': anyway you > have a lot of if (is_alter/is_alter_add_constr..) branches. But OK, > since patch is ready, I believe this refactoring is not worth time > required for that. Sorry for misprint, I meant 'constraint'. >> same reason, autoinc is separate. >> >>>> +}; >>>> + >>>> +struct create_checks_def { >>> Why not create_ck_constraint_def? This naming would be more consistent >>> with existing ck_contraint_def etc. The same for create_fk_constraint_def. >> Because these are lists of constant defs. We decided to emphasize this. > Do not get what's that supposed to mean. This is naming which is taken in > our source code.. If you have any strong arguments from your discussion, > please copy-paste them in the answer. Ok. Let's name it with _parse_ tag in order to avoid confusion. These structs contain lists of of ck_constraint_parse_def/fk_constraint_parse_def objects. +struct create_ck_constraint_parse_def { + /** List of ck_constraint_parse objects. */ + struct rlist checks; + /** Count of ck_constraint_parse objects. */ + uint32_t count; +}; + +struct create_fk_constraint_parse_def { + /** List of fk_constraint_parse objects. */ + struct rlist fkeys; + /** Count of fk_constraint_parse objects. */ + uint32_t count; }; commit f2b3145e1e5a3b2af2a76511c51e4db865099129 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Sat Aug 1 00:27:52 2020 +0300 sql: refactor create_table_def and parse Move ck, fk constraint lists from struct create_table_def into new defs and autoincrement into struct Parse to make the code more reusable when implementing <ALTER TABLE ADD COLUMN>. Needed for #3075 diff --git a/src/box/sql/build.c b/src/box/sql/build.c index d55c1cd..9d827d2 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -590,8 +590,9 @@ sql_create_check_contraint(struct Parse *parser) } } else { assert(! is_alter); - uint32_t ck_idx = ++parser->create_table_def.check_count; - name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, ck_idx); + uint32_t ck_idx = + ++parser->create_ck_constraint_parse_def.count; + name = tt_sprintf("ck_unnamed_%s_%u", space->def->name, ck_idx); } size_t name_len = strlen(name); @@ -652,8 +653,8 @@ 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->create_ck_constraint_parse_def.checks, + ck_parse, link); } } @@ -930,7 +931,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 +1148,92 @@ 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 +vdbe_emit_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->create_fk_constraint_parse_def.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->create_ck_constraint_parse_def.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 +1300,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); - } + vdbe_emit_create_constraints(pParse, reg_space_id); } void @@ -1893,7 +1914,9 @@ sql_create_foreign_key(struct Parse *parse_context) goto tnt_error; } memset(fk_parse, 0, sizeof(*fk_parse)); - rlist_add_entry(&table_def->new_fkey, fk_parse, link); + struct rlist *fkeys = + &parse_context->create_fk_constraint_parse_def.fkeys; + rlist_add_entry(fkeys, fk_parse, link); } struct Token *parent = create_fk_def->parent_name; assert(parent != NULL); @@ -1910,8 +1933,10 @@ 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 create_fk_constraint_parse_def *parse_def = + &parse_context->create_fk_constraint_parse_def; struct fk_constraint_parse *fk = - rlist_first_entry(&table_def->new_fkey, + rlist_first_entry(&parse_def->fkeys, struct fk_constraint_parse, link); fk->selfref_cols = parent_cols; @@ -1923,10 +1948,11 @@ sql_create_foreign_key(struct Parse *parse_context) } if (!is_alter) { if (create_def->name.n == 0) { - constraint_name = - sqlMPrintf(db, "fk_unnamed_%s_%d", - space->def->name, - ++table_def->fkey_count); + struct create_fk_constraint_parse_def *parse_def = + &parse_context->create_fk_constraint_parse_def; + uint32_t idx = ++parse_def->count; + constraint_name = sqlMPrintf(db, "fk_unnamed_%s_%u", + space->def->name, idx); } else { constraint_name = sql_name_from_token(db, &create_def->name); @@ -2041,9 +2067,11 @@ sql_create_foreign_key(struct Parse *parse_context) * maintain list of all FK constraints inside parser. */ if (!is_alter) { + struct rlist *fkeys = + &parse_context->create_fk_constraint_parse_def.fkeys; struct fk_constraint_parse *fk_parse = - rlist_first_entry(&table_def->new_fkey, - struct fk_constraint_parse, link); + rlist_first_entry(fkeys, struct fk_constraint_parse, + link); fk_parse->fk_def = fk_def; } else { vdbe_emit_fk_constraint_create(parse_context, fk_def, @@ -2065,12 +2093,12 @@ 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)) + struct rlist *fkeys = + &parse_context->create_fk_constraint_parse_def.fkeys; + if (parse_context->db->init.busy || rlist_empty(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(fkeys, struct fk_constraint_parse, + link)->fk_def->is_deferred = is_deferred; } /** @@ -3306,15 +3334,15 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id, int sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno) { - if (parse_context->create_table_def.has_autoinc) { + if (parse_context->has_autoinc) { diag_set(ClientError, ER_SQL_SYNTAX_WITH_POS, parse_context->line_count, parse_context->line_pos, "table must feature at most one AUTOINCREMENT field"); parse_context->is_aborted = true; return -1; } - parse_context->create_table_def.has_autoinc = true; - parse_context->create_table_def.autoinc_fieldno = fieldno; + parse_context->has_autoinc = true; + parse_context->autoinc_fieldno = fieldno; return 0; } diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 9958755..9e870c8 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -181,6 +181,8 @@ cmd ::= ROLLBACK TO savepoint_opt nm(X). { cmd ::= create_table create_table_args with_opts create_table_end. create_table ::= createkw TABLE ifnotexists(E) nm(Y). { create_table_def_init(&pParse->create_table_def, &Y, E); + create_ck_constraint_parse_def_init(&pParse->create_ck_constraint_parse_def); + create_fk_constraint_parse_def_init(&pParse->create_fk_constraint_parse_def); pParse->create_table_def.new_space = sqlStartTable(pParse, &Y); pParse->initiateTTrans = true; } diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h index cb0ecd2..7ad7496 100644 --- a/src/box/sql/parse_def.h +++ b/src/box/sql/parse_def.h @@ -205,26 +205,20 @@ 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_ck_constraint_parse_def { + /** List of ck_constraint_parse_def objects. */ + struct rlist checks; + /** Count of ck_constraint_parse_def objects. */ + uint32_t count; +}; + +struct create_fk_constraint_parse_def { + /** List of fk_constraint_parse_def objects. */ + struct rlist fkeys; + /** Count of fk_constraint_parse_def objects. */ + uint32_t count; }; struct create_view_def { @@ -482,9 +476,20 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name, { create_entity_def_init(&table_def->base, ENTITY_TYPE_TABLE, NULL, name, if_not_exists); - rlist_create(&table_def->new_fkey); - rlist_create(&table_def->new_check); - table_def->autoinc_fieldno = 0; +} + +static inline void +create_ck_constraint_parse_def_init(struct create_ck_constraint_parse_def *def) +{ + rlist_create(&def->checks); + def->count = 0; +} + +static inline void +create_fk_constraint_parse_def_init(struct create_fk_constraint_parse_def *def) +{ + rlist_create(&def->fkeys); + def->count = 0; } static inline void @@ -500,12 +505,12 @@ create_view_def_init(struct create_view_def *view_def, struct Token *name, } static inline void -create_table_def_destroy(struct create_table_def *table_def) +create_fk_constraint_parse_def_destroy(struct create_fk_constraint_parse_def *d) { - if (table_def->new_space == NULL) + if (d->count == 0) return; struct fk_constraint_parse *fk; - rlist_foreach_entry(fk, &table_def->new_fkey, link) + rlist_foreach_entry(fk, &d->fkeys, link) sql_expr_list_delete(sql_get(), fk->selfref_cols); } diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index a5a2588..d859a02 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -200,6 +200,7 @@ 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; + parser->has_autoinc = false; region_create(&parser->region, &cord()->slabc); } @@ -211,7 +212,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 create_fk_constraint_parse_def *create_fk_constraint_parse_def = + &parser->create_fk_constraint_parse_def; + create_fk_constraint_parse_def_destroy(create_fk_constraint_parse_def); if (db != NULL) { assert(db->lookaside.bDisable >= parser->disableLookaside); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index dedad09..6c79a86 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2257,6 +2257,18 @@ struct Parse { * sqlEndTable() function). */ struct create_table_def create_table_def; + /* + * FK and CK constraints appeared in a <CREATE TABLE>. + */ + struct create_fk_constraint_parse_def create_fk_constraint_parse_def; + struct create_ck_constraint_parse_def create_ck_constraint_parse_def; + /* + * 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; ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH v4 3/5] schema: add box_space_field_MAX 2020-10-09 13:45 [Tarantool-patches] [PATCH v4 0/5] Support column addition Roman Khabibov 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 1/5] sql: rename TK_COLUMN to TK_COLUMN_REF Roman Khabibov 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse Roman Khabibov @ 2020-10-09 13:45 ` Roman Khabibov 2020-11-05 22:18 ` Nikita Pettik 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 4/5] sql: use parser's region of "index" array Roman Khabibov [not found] ` <20201009134529.13212-6-roman.habibov@tarantool.org> 4 siblings, 1 reply; 18+ messages in thread From: Roman Khabibov @ 2020-10-09 13:45 UTC (permalink / raw) To: tarantool-patches 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.24.3 (Apple Git-128) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 3/5] schema: add box_space_field_MAX 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 3/5] schema: add box_space_field_MAX Roman Khabibov @ 2020-11-05 22:18 ` Nikita Pettik 0 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2020-11-05 22:18 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 09 Oct 16:45, Roman Khabibov wrote: > Just add box_space_field_MAX to the _space fields enum. > > Needed for #3075 IDK why it is needed for 3075 but patch is ok. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH v4 4/5] sql: use parser's region of "index" array 2020-10-09 13:45 [Tarantool-patches] [PATCH v4 0/5] Support column addition Roman Khabibov ` (2 preceding siblings ...) 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 3/5] schema: add box_space_field_MAX Roman Khabibov @ 2020-10-09 13:45 ` Roman Khabibov 2020-11-05 22:26 ` Nikita Pettik [not found] ` <20201009134529.13212-6-roman.habibov@tarantool.org> 4 siblings, 1 reply; 18+ messages in thread From: Roman Khabibov @ 2020-10-09 13:45 UTC (permalink / raw) To: tarantool-patches Allocate memory for the "index" array of ephemeral space on the parser's region instead of a heap as it was before. Fixed a memory leak that realloc() generated. Needed for #2349 --- src/box/sql/build.c | 48 +++++++++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 3b3c8099a..fd8e7ed1e 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2228,29 +2228,52 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id, } /** - * Add new index to table's indexes list. + * Add new index to ephemeral @a space's "index" array. Reallocate + * memory on @a parse's region if needed. + * * We follow convention that PK comes first in list. * + * @param parse Parsing structure. * @param space Space to which belongs given index. * @param index Index to be added to list. */ -static void -table_add_index(struct space *space, struct index *index) +static int +sql_space_add_index(struct Parse *parse, struct space *space, + struct index *index) { uint32_t idx_count = space->index_count; - size_t indexes_sz = sizeof(struct index *) * (idx_count + 1); - struct index **idx = (struct index **) realloc(space->index, - indexes_sz); - if (idx == NULL) { - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); - return; - } - space->index = idx; + size_t size = 0; + struct index **idx = NULL; + struct region *region = &parse->region; + if (idx_count == 0) { + idx = region_alloc_array(region, typeof(struct index *), 1, + &size); + if (idx == NULL) + goto alloc_error; + /* + * Reallocate each time the idx_count becomes equal to the + * power of two. + */ + } else if ((idx_count & (idx_count - 1)) == 0) { + idx = region_alloc_array(region, typeof(struct index *), + idx_count * 2, &size); + if (idx == NULL) + goto alloc_error; + memcpy(idx, space->index, idx_count * sizeof(struct index *)); + } + if (idx != NULL) + space->index = idx; /* Make sure that PK always comes as first member. */ if (index->def->iid == 0 && idx_count != 0) SWAP(space->index[0], index); space->index[space->index_count++] = index; space->index_id_max = MAX(space->index_id_max, index->def->iid);; + return 0; + +alloc_error: + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); + parse->is_aborted = true; + return -1; } int @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { sqlVdbeAddOp0(vdbe, OP_Expire); } - if (tbl_name != NULL) + if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) goto exit_create_index; - table_add_index(space, index); index = NULL; /* Clean up before exiting. */ -- 2.24.3 (Apple Git-128) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 4/5] sql: use parser's region of "index" array 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 4/5] sql: use parser's region of "index" array Roman Khabibov @ 2020-11-05 22:26 ` Nikita Pettik 2020-11-10 12:18 ` Roman Khabibov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-11-05 22:26 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 09 Oct 16:45, Roman Khabibov wrote: > Allocate memory for the "index" array of ephemeral space on the > parser's region instead of a heap as it was before. Fixed a memory > leak that realloc() generated. Would be nice pointing out where leak occured. Also realloc usage looks better to me (27 lines vs 8 with straight execution flaw). > Needed for #2349 > --- > src/box/sql/build.c | 48 +++++++++++++++++++++++++++++++++------------ > 1 file changed, 35 insertions(+), 13 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 3b3c8099a..fd8e7ed1e 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2228,29 +2228,52 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id, > } > > /** > - * Add new index to table's indexes list. > + * Add new index to ephemeral @a space's "index" array. Reallocate > + * memory on @a parse's region if needed. > + * > * We follow convention that PK comes first in list. > * > + * @param parse Parsing structure. > * @param space Space to which belongs given index. > * @param index Index to be added to list. > */ > -static void > -table_add_index(struct space *space, struct index *index) > +static int > +sql_space_add_index(struct Parse *parse, struct space *space, > + struct index *index) > { > uint32_t idx_count = space->index_count; > - size_t indexes_sz = sizeof(struct index *) * (idx_count + 1); > - struct index **idx = (struct index **) realloc(space->index, > - indexes_sz); > - if (idx == NULL) { > - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); > - return; > - } > - space->index = idx; > + size_t size = 0; > + struct index **idx = NULL; > + struct region *region = &parse->region; > + if (idx_count == 0) { > + idx = region_alloc_array(region, typeof(struct index *), 1, > + &size); > + if (idx == NULL) > + goto alloc_error; > + /* > + * Reallocate each time the idx_count becomes equal to the > + * power of two. > + */ > + } else if ((idx_count & (idx_count - 1)) == 0) { > + idx = region_alloc_array(region, typeof(struct index *), > + idx_count * 2, &size); > + if (idx == NULL) > + goto alloc_error; > + memcpy(idx, space->index, idx_count * sizeof(struct index *)); > + } > + if (idx != NULL) > + space->index = idx; > /* Make sure that PK always comes as first member. */ > if (index->def->iid == 0 && idx_count != 0) > SWAP(space->index[0], index); > space->index[space->index_count++] = index; > space->index_id_max = MAX(space->index_id_max, index->def->iid);; > + return 0; > + > +alloc_error: > + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); > + parse->is_aborted = true; > + return -1; > } > > int > @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { > sqlVdbeAddOp0(vdbe, OP_Expire); > } > > - if (tbl_name != NULL) > + if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) > goto exit_create_index; > - table_add_index(space, index); > index = NULL; > > /* Clean up before exiting. */ > -- > 2.24.3 (Apple Git-128) > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 4/5] sql: use parser's region of "index" array 2020-11-05 22:26 ` Nikita Pettik @ 2020-11-10 12:18 ` Roman Khabibov 2020-11-17 20:35 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Roman Khabibov @ 2020-11-10 12:18 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches Hi! Thanks for the review. > On Nov 6, 2020, at 01:26, Nikita Pettik <korablev@tarantool.org> wrote: > > On 09 Oct 16:45, Roman Khabibov wrote: >> Allocate memory for the "index" array of ephemeral space on the >> parser's region instead of a heap as it was before. Fixed a memory >> leak that realloc() generated. > > Would be nice pointing out where leak occured. Also realloc usage looks > better to me (27 lines vs 8 with straight execution flaw). Vlad said that main point of the region - speed, and it is also harder to leave a memory leak, because the region is destroyed in the end of parsing entirely. >> Needed for #2349 >> --- >> src/box/sql/build.c | 48 +++++++++++++++++++++++++++++++++------------ >> 1 file changed, 35 insertions(+), 13 deletions(-) >> >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 3b3c8099a..fd8e7ed1e 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -2228,29 +2228,52 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id, >> } >> >> /** >> - * Add new index to table's indexes list. >> + * Add new index to ephemeral @a space's "index" array. Reallocate >> + * memory on @a parse's region if needed. >> + * >> * We follow convention that PK comes first in list. >> * >> + * @param parse Parsing structure. >> * @param space Space to which belongs given index. >> * @param index Index to be added to list. >> */ >> -static void >> -table_add_index(struct space *space, struct index *index) >> +static int >> +sql_space_add_index(struct Parse *parse, struct space *space, >> + struct index *index) >> { >> uint32_t idx_count = space->index_count; >> - size_t indexes_sz = sizeof(struct index *) * (idx_count + 1); >> - struct index **idx = (struct index **) realloc(space->index, >> - indexes_sz); >> - if (idx == NULL) { >> - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); >> - return; >> - } >> - space->index = idx; >> + size_t size = 0; >> + struct index **idx = NULL; >> + struct region *region = &parse->region; >> + if (idx_count == 0) { >> + idx = region_alloc_array(region, typeof(struct index *), 1, >> + &size); >> + if (idx == NULL) >> + goto alloc_error; >> + /* >> + * Reallocate each time the idx_count becomes equal to the >> + * power of two. >> + */ >> + } else if ((idx_count & (idx_count - 1)) == 0) { >> + idx = region_alloc_array(region, typeof(struct index *), >> + idx_count * 2, &size); >> + if (idx == NULL) >> + goto alloc_error; >> + memcpy(idx, space->index, idx_count * sizeof(struct index *)); >> + } >> + if (idx != NULL) >> + space->index = idx; >> /* Make sure that PK always comes as first member. */ >> if (index->def->iid == 0 && idx_count != 0) >> SWAP(space->index[0], index); >> space->index[space->index_count++] = index; >> space->index_id_max = MAX(space->index_id_max, index->def->iid);; >> + return 0; >> + >> +alloc_error: >> + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); >> + parse->is_aborted = true; >> + return -1; >> } >> >> int >> @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { >> sqlVdbeAddOp0(vdbe, OP_Expire); >> } >> >> - if (tbl_name != NULL) >> + if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) >> goto exit_create_index; >> - table_add_index(space, index); >> index = NULL; >> >> /* Clean up before exiting. */ >> -- >> 2.24.3 (Apple Git-128) >> commit 459410f559a1c2f36a2da8925565a32a4caf4ae3 (HEAD) Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Thu Sep 3 14:26:47 2020 +0300 sql: use parser's region of "index" array Allocate memory for the "index" array of ephemeral space on the parser's region instead of a heap as it was before. Fixed a memory leak that realloc() generated for the index array of ephemeral space. The memory was never released. Needed for #2349 diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 3b3c8099a..fd8e7ed1e 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2228,29 +2228,52 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id, } /** - * Add new index to table's indexes list. + * Add new index to ephemeral @a space's "index" array. Reallocate + * memory on @a parse's region if needed. + * * We follow convention that PK comes first in list. * + * @param parse Parsing structure. * @param space Space to which belongs given index. * @param index Index to be added to list. */ -static void -table_add_index(struct space *space, struct index *index) +static int +sql_space_add_index(struct Parse *parse, struct space *space, + struct index *index) { uint32_t idx_count = space->index_count; - size_t indexes_sz = sizeof(struct index *) * (idx_count + 1); - struct index **idx = (struct index **) realloc(space->index, - indexes_sz); - if (idx == NULL) { - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); - return; - } - space->index = idx; + size_t size = 0; + struct index **idx = NULL; + struct region *region = &parse->region; + if (idx_count == 0) { + idx = region_alloc_array(region, typeof(struct index *), 1, + &size); + if (idx == NULL) + goto alloc_error; + /* + * Reallocate each time the idx_count becomes equal to the + * power of two. + */ + } else if ((idx_count & (idx_count - 1)) == 0) { + idx = region_alloc_array(region, typeof(struct index *), + idx_count * 2, &size); + if (idx == NULL) + goto alloc_error; + memcpy(idx, space->index, idx_count * sizeof(struct index *)); + } + if (idx != NULL) + space->index = idx; /* Make sure that PK always comes as first member. */ if (index->def->iid == 0 && idx_count != 0) SWAP(space->index[0], index); space->index[space->index_count++] = index; space->index_id_max = MAX(space->index_id_max, index->def->iid);; + return 0; + +alloc_error: + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); + parse->is_aborted = true; + return -1; } int @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { sqlVdbeAddOp0(vdbe, OP_Expire); } - if (tbl_name != NULL) + if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) goto exit_create_index; - table_add_index(space, index); index = NULL; /* Clean up before exiting. */ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 4/5] sql: use parser's region of "index" array 2020-11-10 12:18 ` Roman Khabibov @ 2020-11-17 20:35 ` Nikita Pettik 0 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2020-11-17 20:35 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 10 Nov 15:18, Roman Khabibov wrote: > Hi! Thanks for the review. > > > On Nov 6, 2020, at 01:26, Nikita Pettik <korablev@tarantool.org> wrote: > > > > On 09 Oct 16:45, Roman Khabibov wrote: > >> Allocate memory for the "index" array of ephemeral space on the > >> parser's region instead of a heap as it was before. Fixed a memory > >> leak that realloc() generated. > > > > Would be nice pointing out where leak occured. Also realloc usage looks > > better to me (27 lines vs 8 with straight execution flaw). > Vlad said that main point of the region - speed, and it is also harder > to leave a memory leak, because the region is destroyed in the end of > parsing entirely. Region vs malloc allocation performance benefit is not a point at all as we speak about parsing process (at least for DDL statements). Ok, as you wish, let's leave patch as is. > >> Needed for #2349 > >> --- > >> src/box/sql/build.c | 48 +++++++++++++++++++++++++++++++++------------ > >> 1 file changed, 35 insertions(+), 13 deletions(-) > >> > >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c > >> index 3b3c8099a..fd8e7ed1e 100644 > >> --- a/src/box/sql/build.c > >> +++ b/src/box/sql/build.c > >> @@ -2228,29 +2228,52 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id, > >> } > >> > >> /** > >> - * Add new index to table's indexes list. > >> + * Add new index to ephemeral @a space's "index" array. Reallocate > >> + * memory on @a parse's region if needed. > >> + * > >> * We follow convention that PK comes first in list. > >> * > >> + * @param parse Parsing structure. > >> * @param space Space to which belongs given index. > >> * @param index Index to be added to list. > >> */ > >> -static void > >> -table_add_index(struct space *space, struct index *index) > >> +static int > >> +sql_space_add_index(struct Parse *parse, struct space *space, > >> + struct index *index) > >> { > >> uint32_t idx_count = space->index_count; > >> - size_t indexes_sz = sizeof(struct index *) * (idx_count + 1); > >> - struct index **idx = (struct index **) realloc(space->index, > >> - indexes_sz); > >> - if (idx == NULL) { > >> - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); > >> - return; > >> - } > >> - space->index = idx; > >> + size_t size = 0; > >> + struct index **idx = NULL; > >> + struct region *region = &parse->region; > >> + if (idx_count == 0) { > >> + idx = region_alloc_array(region, typeof(struct index *), 1, > >> + &size); > >> + if (idx == NULL) > >> + goto alloc_error; > >> + /* > >> + * Reallocate each time the idx_count becomes equal to the > >> + * power of two. > >> + */ > >> + } else if ((idx_count & (idx_count - 1)) == 0) { > >> + idx = region_alloc_array(region, typeof(struct index *), > >> + idx_count * 2, &size); > >> + if (idx == NULL) > >> + goto alloc_error; > >> + memcpy(idx, space->index, idx_count * sizeof(struct index *)); > >> + } > >> + if (idx != NULL) > >> + space->index = idx; > >> /* Make sure that PK always comes as first member. */ > >> if (index->def->iid == 0 && idx_count != 0) > >> SWAP(space->index[0], index); > >> space->index[space->index_count++] = index; > >> space->index_id_max = MAX(space->index_id_max, index->def->iid);; > >> + return 0; > >> + > >> +alloc_error: > >> + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); > >> + parse->is_aborted = true; > >> + return -1; > >> } > >> > >> int > >> @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { > >> sqlVdbeAddOp0(vdbe, OP_Expire); > >> } > >> > >> - if (tbl_name != NULL) > >> + if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) > >> goto exit_create_index; > >> - table_add_index(space, index); > >> index = NULL; > >> > >> /* Clean up before exiting. */ > >> -- > >> 2.24.3 (Apple Git-128) > >> > > commit 459410f559a1c2f36a2da8925565a32a4caf4ae3 (HEAD) > Author: Roman Khabibov <roman.habibov@tarantool.org> > Date: Thu Sep 3 14:26:47 2020 +0300 > > sql: use parser's region of "index" array > > Allocate memory for the "index" array of ephemeral space on the > parser's region instead of a heap as it was before. Fixed a memory > leak that realloc() generated for the index array of ephemeral > space. The memory was never released. > > Needed for #2349 > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 3b3c8099a..fd8e7ed1e 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2228,29 +2228,52 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id, > } > > /** > - * Add new index to table's indexes list. > + * Add new index to ephemeral @a space's "index" array. Reallocate > + * memory on @a parse's region if needed. > + * > * We follow convention that PK comes first in list. > * > + * @param parse Parsing structure. > * @param space Space to which belongs given index. > * @param index Index to be added to list. > */ > -static void > -table_add_index(struct space *space, struct index *index) > +static int > +sql_space_add_index(struct Parse *parse, struct space *space, > + struct index *index) > { > uint32_t idx_count = space->index_count; > - size_t indexes_sz = sizeof(struct index *) * (idx_count + 1); > - struct index **idx = (struct index **) realloc(space->index, > - indexes_sz); > - if (idx == NULL) { > - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); > - return; > - } > - space->index = idx; > + size_t size = 0; > + struct index **idx = NULL; > + struct region *region = &parse->region; > + if (idx_count == 0) { > + idx = region_alloc_array(region, typeof(struct index *), 1, > + &size); > + if (idx == NULL) > + goto alloc_error; > + /* > + * Reallocate each time the idx_count becomes equal to the > + * power of two. > + */ > + } else if ((idx_count & (idx_count - 1)) == 0) { > + idx = region_alloc_array(region, typeof(struct index *), > + idx_count * 2, &size); > + if (idx == NULL) > + goto alloc_error; > + memcpy(idx, space->index, idx_count * sizeof(struct index *)); > + } > + if (idx != NULL) > + space->index = idx; > /* Make sure that PK always comes as first member. */ > if (index->def->iid == 0 && idx_count != 0) > SWAP(space->index[0], index); > space->index[space->index_count++] = index; > space->index_id_max = MAX(space->index_id_max, index->def->iid);; > + return 0; > + > +alloc_error: > + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); > + parse->is_aborted = true; > + return -1; > } > > int > @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { > sqlVdbeAddOp0(vdbe, OP_Expire); > } > > - if (tbl_name != NULL) > + if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) > goto exit_create_index; > - table_add_index(space, index); > index = NULL; > > /* Clean up before exiting. */ > > ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <20201009134529.13212-6-roman.habibov@tarantool.org>]
* Re: [Tarantool-patches] [PATCH v4 5/5] sql: support column addition [not found] ` <20201009134529.13212-6-roman.habibov@tarantool.org> @ 2020-11-06 15:57 ` Nikita Pettik 2020-11-10 12:18 ` Roman Khabibov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-11-06 15:57 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 09 Oct 16:45, Roman Khabibov wrote: > diff --git a/src/box/errcode.h b/src/box/errcode.h > index e6957d612..5a59f477f 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -273,6 +273,8 @@ struct errcode_record { > /*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \ > /*219 */_(ER_XLOG_GAP, "%s") \ > /*220 */_(ER_TOO_EARLY_SUBSCRIBE, "Can't subscribe non-anonymous replica %s until join is done") \ > + /*221 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add AUTOINCREMENT: space %s can't feature more than one AUTOINCREMENT field") \ > + Extra empty line. > /* > * !IMPORTANT! Please follow instructions at start of the file > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index fd8e7ed1e..597608bea 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -285,48 +285,110 @@ sql_field_retrieve(Parse *parser, struct space_def *space_def, uint32_t id) > return field; > } > > +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; > + } You don't need all indexes from space..What you only need from space - count of indexes (to generate name if it is required). The same for field defs. Mb it is worth avoiding copying space at all - just store in create_column_def list of ck/fk constraints, single field_def, index/field count - these objects seem to be enough. Please consider this suggestion. > + > + 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; > + > /* > - * 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; assert(is_alter); > + struct Vdbe *v = sqlGetVdbe(parse); > + assert(v != NULL); > + > + struct space *system_space = space_by_id(BOX_SPACE_ID); -> you can call it simply _space or s_space > + assert(system_space != NULL); > + int cursor = parse->nTab++; > + vdbe_emit_open_cursor(parse, cursor, 0, system_space); > + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); > + > 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; Let's use standard facilities: assert(def->field_count > 0); > if (space == NULL) > goto primary_key_exit; > if (sql_space_primary_key(space) != NULL) { > @@ -574,8 +707,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; Consider tiny refactoring: is_alter = space == NULL; if (is_alter) space = ... assert(space != NULL); > + if (space == NULL) > + space = parser->create_table_def.new_space; > + bool is_alter_add_constr = space == NULL; > > - assert(! is_alter); > + assert(!is_alter_add_constr); What's the point of this renaming? > uint32_t ck_idx = ++parser->create_checks_def.count; > + /* > @@ -1149,15 +1302,28 @@ 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 > vdbe_emit_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 AFAIK only one index (corresponding to unique constraint). > + * 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; > + } > @@ -1176,6 +1342,21 @@ vdbe_emit_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) Oh please leave !=0 cond on previous line. This way looks disgusting. > + return; > + } > sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_rec); > /* Do an insertion into _space_sequence. */ > int reg_space_seq_record = > + * CONSTRAINT> statement handling. > */ > - bool is_alter = space == NULL; > + bool is_alter_add_constr = space == NULL; Again, what's the point of this renaming? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 5/5] sql: support column addition 2020-11-06 15:57 ` [Tarantool-patches] [PATCH v4 5/5] sql: support column addition Nikita Pettik @ 2020-11-10 12:18 ` Roman Khabibov 2020-11-18 16:15 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Roman Khabibov @ 2020-11-10 12:18 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches Hi! Thanks for the review. > On Nov 6, 2020, at 18:57, Nikita Pettik <korablev@tarantool.org> wrote: > > On 09 Oct 16:45, Roman Khabibov wrote: >> diff --git a/src/box/errcode.h b/src/box/errcode.h >> index e6957d612..5a59f477f 100644 >> --- a/src/box/errcode.h >> +++ b/src/box/errcode.h >> @@ -273,6 +273,8 @@ struct errcode_record { >> /*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \ >> /*219 */_(ER_XLOG_GAP, "%s") \ >> /*220 */_(ER_TOO_EARLY_SUBSCRIBE, "Can't subscribe non-anonymous replica %s until join is done") \ >> + /*221 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add AUTOINCREMENT: space %s can't feature more than one AUTOINCREMENT field") \ >> + > > Extra empty line. Removed. >> /* >> * !IMPORTANT! Please follow instructions at start of the file >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index fd8e7ed1e..597608bea 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -285,48 +285,110 @@ sql_field_retrieve(Parse *parser, struct space_def *space_def, uint32_t id) >> return field; >> } >> >> +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; >> + } > > > You don't need all indexes from space..What you only need from space - > count of indexes (to generate name if it is required). The same for > field defs. Mb it is worth avoiding copying space at all - just > store in create_column_def list of ck/fk constraints, single > field_def, index/field count - these objects seem to be enough. > Please consider this suggestion. Yes, I understand that. But unfortunately, the current code, namely the functions for creating indexes, fk, and ck constraints, are written in such a way that they cannot be called without struct space (ephemeral or not). Vlad and I discussed this and came to the conclusion that it is better to go in the direction of reusing the code, rather than rewriting a significant part of build.c. https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018460.html https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019003.html >> + >> + 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; >> + >> /* >> - * 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; > > assert(is_alter); If you meant if (def->exact_field_count == 0) { assert(is_alter); def->exact_field_count = def->field_count; } This branch is not only for alter. >> + struct Vdbe *v = sqlGetVdbe(parse); >> + assert(v != NULL); >> + >> + struct space *system_space = space_by_id(BOX_SPACE_ID); > > -> you can call it simply _space or s_space Done. >> + assert(system_space != NULL); >> + int cursor = parse->nTab++; >> + vdbe_emit_open_cursor(parse, cursor, 0, system_space); >> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); >> + >> 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; > > Let's use standard facilities: assert(def->field_count > 0); Done. 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)) - return; - struct space_def *def = space->def; + assert(parser->create_column_def.space != NULL); + struct space_def *def = parser->create_column_def.space->def; + assert(def->field_count > 0); struct field_def *field = &def->fields[def->field_count - 1]; if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT && nullable_action != field->nullable_action) { >> if (space == NULL) >> goto primary_key_exit; >> if (sql_space_primary_key(space) != NULL) { >> @@ -574,8 +707,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; > > Consider tiny refactoring: > > is_alter = space == NULL; > if (is_alter) > space = ... > assert(space != NULL); In my case it will be: struct space *space = parser->create_column_def.space; bool is_alter_add_constr = space == NULL && parser->create_table_def.new_space == NULL; if (!is_alter_add_constr) space = parser->parser->create_table_def.new_space; This results in more rows than there are now. >> + if (space == NULL) >> + space = parser->create_table_def.new_space; >> + bool is_alter_add_constr = space == NULL; >> >> - assert(! is_alter); >> + assert(!is_alter_add_constr); > > What's the point of this renaming? I need to distinguish between <CREATE TABLE>, <ALTER TABLE ADD COLUMN> and <ALTER TABLE ADD CONSTRAINT> branches. > >> uint32_t ck_idx = ++parser->create_checks_def.count; >> + /* >> @@ -1149,15 +1302,28 @@ 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 >> vdbe_emit_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 > > AFAIK only one index (corresponding to unique constraint). I can write any number of named unique constraints. They should be created. ALTER TABLE t ADD COLUMN a INT CONSTRAINT c_1 UNIQUE, CONSTRAINT c_2 UNIQUE … > > >> + * 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; >> + } >> @@ -1176,6 +1342,21 @@ vdbe_emit_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) > > Oh please leave !=0 cond on previous line. This way looks disgusting. Done. >> + return; >> + } >> sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_rec); >> /* Do an insertion into _space_sequence. */ >> int reg_space_seq_record = >> + * CONSTRAINT> statement handling. >> */ >> - bool is_alter = space == NULL; >> + bool is_alter_add_constr = space == NULL; > > Again, what's the point of this renaming? commit d00ac8986bd37d5234e7d3889cb6f77216c1d33c Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Thu Jan 2 19:06:14 2020 +0300 sql: support column addition Enable to add column to existing space with <ALTER TABLE ADD [COLUMN]> statement. Column definition can be supplemented with the four types of constraints, <DEFAULT>, <COLLATE> clauses and <[NOT] NULL>, AUTOINCREMENT. Closes #2349, #3075 @TarantoolBot document Title: Add columns to existing tables in SQL Now, it is possible to add columns to existing empty spaces using <ALTER TABLE table_name ADD [COLUMN] column_name column_type ...> statement. The column definition is the same as in <CREATE TABLE> statement. * Space emptiness is Tarantool's restriction. Possibilty to add column to non empty space will be implemented later. 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: 1 ... ``` diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c index bacdad9b3..7480c0211 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_REF", "TK_COLUMN_REF", 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 e6957d612..cfea5bcf2 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -273,6 +273,7 @@ struct errcode_record { /*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \ /*219 */_(ER_XLOG_GAP, "%s") \ /*220 */_(ER_TOO_EARLY_SUBSCRIBE, "Can't subscribe non-anonymous replica %s until join is done") \ + /*221 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add AUTOINCREMENT: space %s can't feature more than one AUTOINCREMENT 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 fd8e7ed1e..35883aa0f 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -285,48 +285,110 @@ 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) + 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; +} + 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; + /* - * 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 +396,85 @@ 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 +vdbe_emit_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 *s_space = space_by_id(BOX_SPACE_ID); + assert(s_space != NULL); + int cursor = parse->nTab++; + vdbe_emit_open_cursor(parse, cursor, 0, s_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 *) s_space, P4_SPACEPTR); + sqlVdbeCountChanges(v); + sqlVdbeChangeP5(v, OPFLAG_NCHANGE); + sqlReleaseTempRange(parse, tuple_reg, box_space_field_MAX + 1); + vdbe_emit_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)) - return; - struct space_def *def = space->def; + assert(parser->create_column_def.space != NULL); + struct space_def *def = parser->create_column_def.space->def; + assert(def->field_count > 0); struct field_def *field = &def->fields[def->field_count - 1]; if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT && nullable_action != field->nullable_action) { @@ -364,20 +493,21 @@ 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 an <ALTER TABLE ADD COLUMN> + * statement. */ 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; @@ -447,6 +577,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 +706,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,8 +723,22 @@ sql_create_check_contraint(struct Parse *parser) return; } } else { - assert(! is_alter); + assert(!is_alter_add_constr); uint32_t ck_idx = ++parser->create_checks_def.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 ck_constraint *ck; + rlist_foreach_entry(ck, &original_space->ck_constraint, + link) + ck_idx++; + } name = tt_sprintf("ck_unnamed_%s_%u", space->def->name, ck_idx); } size_t name_len = strlen(name); @@ -634,7 +782,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) { @@ -664,9 +812,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); @@ -696,7 +843,6 @@ struct coll * sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id) { assert(def != NULL); - struct space *space = space_by_id(def->id); /* * It is not always possible to fetch collation directly * from struct space due to its absence in space cache. @@ -705,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; @@ -795,7 +941,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 { @@ -1033,18 +1180,23 @@ 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 <ALTER TABLE ADD COLUMN> + * statement, we don't have child id (we know it, but + * fk->child_id stores register because of code reuse in + * vdbe_emit_create_constraints()), but we know register + * where it will be stored. */ - if (parse_context->create_table_def.new_space != NULL) { + bool is_alter_add_constr = + parse_context->create_table_def.new_space == NULL && + parse_context->create_column_def.space == NULL; + if (!is_alter_add_constr) { sqlVdbeAddOp2(vdbe, OP_SCopy, fk->child_id, constr_tuple_reg + 1); } else { sqlVdbeAddOp2(vdbe, OP_Integer, fk->child_id, constr_tuple_reg + 1); } - if (parse_context->create_table_def.new_space != NULL && - fk_constraint_is_self_referenced(fk)) { + if (!is_alter_add_constr && fk_constraint_is_self_referenced(fk)) { sqlVdbeAddOp2(vdbe, OP_SCopy, fk->parent_id, constr_tuple_reg + 2); } else { @@ -1108,7 +1260,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); } @@ -1149,15 +1301,28 @@ 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 vdbe_emit_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); for (; i < space->index_count; ++i) { struct index *idx = space->index[i]; vdbe_emit_create_index(parse, space->def, idx->def, @@ -1176,6 +1341,20 @@ vdbe_emit_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 = @@ -1874,24 +2053,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) { @@ -1909,6 +2092,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->create_fkeys_def.fkeys, fk_parse, link); } @@ -1922,27 +2111,41 @@ 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 rlist *fkeys = - &parse_context->create_fkeys_def.fkeys; - struct fk_constraint_parse *fk = - rlist_first_entry(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 rlist *fkeys = &parse_context->create_fkeys_def.fkeys; + struct fk_constraint_parse *fk = + rlist_first_entry(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) { uint32_t idx = ++parse_context->create_fkeys_def.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; + struct fk_constraint *fk; + rlist_foreach_entry(fk, child_fk, + in_child_space) + idx++; + } constraint_name = sqlMPrintf(db, "fk_unnamed_%s_%u", space->def->name, idx); } else { @@ -2024,7 +2227,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); /* @@ -2053,12 +2256,14 @@ 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 - * maintain list of all FK constraints inside parser. + * In case of <СREATE TABLE> and <ALTER TABLE ADD COLUMN> + * processing, all foreign keys constraints must be + * created after space itself (or space altering), so let + * delay it until vdbe_emit_create_constraints() call and + * simply maintain list of all FK constraints inside + * parser. */ - if (!is_alter) { + if (!is_alter_add_constr) { struct rlist *fkeys = &parse_context->create_fkeys_def.fkeys; struct fk_constraint_parse *fk_parse = rlist_first_entry(fkeys, struct fk_constraint_parse, @@ -2435,7 +2640,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); @@ -2448,10 +2656,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; } struct space_def *def = space->def; @@ -2486,7 +2692,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) { @@ -2652,7 +2858,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; @@ -2740,7 +2946,8 @@ sql_create_index(struct Parse *parse) { sqlVdbeAddOp0(vdbe, OP_Expire); } - if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) + if (!is_create_table_or_add_col || + sql_space_add_index(parse, space, index) != 0) goto exit_create_index; index = NULL; diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index da4e2a3ae..d7c65bb4b 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -228,19 +228,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. @@ -283,9 +288,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 } @@ -1737,11 +1744,30 @@ 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); + create_checks_def_init(&pParse->create_checks_def); + create_fkeys_def_init(&pParse->create_fkeys_def); + 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 21829b6f0..07230a8be 100644 --- a/src/box/sql/parse_def.h +++ b/src/box/sql/parse_def.h @@ -107,9 +107,10 @@ struct fk_constraint_parse { */ struct fk_constraint_def *fk_def; /** - * If inside CREATE TABLE statement we want to declare - * self-referenced FK constraint, we must delay their - * resolution until the end of parsing of all columns. + * If inside <CREATE TABLE> or <ALTER TABLE ADD COLUMN> + * statement we want to declare self-referenced FK + * constraint, we must delay their resolution until the + * end of parsing of all columns. * E.g.: CREATE TABLE t1(id REFERENCES t1(b), b); */ struct ExprList *selfref_cols; @@ -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 copy. */ + struct space *space; + /** Column type. */ + struct type_def *type_def; +}; + struct create_checks_def { /** List of ck_constraint_parse objects. */ struct rlist checks; @@ -478,6 +488,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_checks_def_init(struct create_checks_def *checks_def) { diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 2e426600d..a54aa1d51 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2251,20 +2251,24 @@ 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 create_fkeys_def create_fkeys_def; struct create_checks_def create_checks_def; /* - * 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>. */ @@ -2858,15 +2862,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_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); + +/** + * 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/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 5404a78e9..0899cf23e 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -40,6 +40,7 @@ #include <unicode/uchar.h> #include "box/session.h" +#include "box/schema.h" #include "say.h" #include "sqlInt.h" #include "tarantoolInt.h" @@ -431,8 +432,8 @@ sql_token(const char *z, int *type, bool *is_reserved) /** * This function is called to release parsing artifacts - * during table creation. The only objects allocated using - * malloc are index defs and check constraints. + * during table creation or column addition. The only objects + * allocated using malloc are index defs. * Note that this functions can't be called on ordinary * space object. It's purpose is to clean-up parser->new_space. * @@ -445,7 +446,15 @@ parser_space_delete(struct sql *db, struct space *space) if (space == NULL || db == NULL) return; assert(space->def->opts.is_ephemeral); - for (uint32_t i = 0; i < space->index_count; ++i) + struct space *altered_space = space_by_name(space->def->name); + uint32_t i = 0; + /* + * Don't delete already existing defs and start from new + * ones. + */ + if (altered_space != NULL) + i = altered_space->index_count; + for (; i < space->index_count; ++i) index_def_delete(space->index[i]->def); } @@ -539,7 +548,7 @@ sqlRunParser(Parse * pParse, const char *zSql) sqlVdbeDelete(pParse->pVdbe); pParse->pVdbe = 0; } - parser_space_delete(db, pParse->create_table_def.new_space); + parser_space_delete(db, pParse->create_column_def.space); if (pParse->pWithToFree) sqlWithDelete(db, pParse->pWithToFree); diff --git a/test/box/error.result b/test/box/error.result index 4d85b9e55..2f6d7fb22 100644 --- a/test/box/error.result +++ b/test/box/error.result @@ -439,6 +439,7 @@ t; | 218: box.error.TUPLE_METADATA_IS_TOO_BIG | 219: box.error.XLOG_GAP | 220: box.error.TOO_EARLY_SUBSCRIBE + | 221: 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..924368ea2 --- /dev/null +++ b/test/sql/add-column.result @@ -0,0 +1,529 @@ +-- 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: 1 + | ... + +-- +-- 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 c INT; + | --- + | - null + | - 'Can''t modify space ''V'': view can not be altered' + | ... + +\set language lua + | --- + | - true + | ... +view = box.space._space.index[2]:select('V')[1]:totable() + | --- + | ... +view_format = view[7] + | --- + | ... +f = {type = 'string', nullable_action = 'none', name = 'C', is_nullable = true} + | --- + | ... +table.insert(view_format, f) + | --- + | ... +view[5] = 3 + | --- + | ... +view[7] = view_format + | --- + | ... +box.space._space:replace(view) + | --- + | - error: 'Can''t modify space ''V'': view can not be altered' + | ... +\set language sql + | --- + | - true + | ... + +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: 1 + | ... +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: 1 + | ... +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: 1 + | ... +INSERT INTO ck_check VALUES (1, 0); + | --- + | - null + | - 'Check constraint failed ''ck_unnamed_CK_CHECK_1'': b > 0' + | ... +INSERT INTO ck_check VALUES (1, 1); + | --- + | - row_count: 1 + | ... +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: 1 + | ... +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' + | ... +INSERT INTO t1 VALUES (1, 1); + | --- + | - row_count: 1 + | ... +INSERT INTO fk_check VALUES (2, 1); + | --- + | - row_count: 1 + | ... +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: 1 + | ... +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: 1 + | ... +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: space AUTOINC_CHECK can''t feature more than one AUTOINCREMENT + | 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: 1 + | ... +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: 1 + | ... +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: 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] + | ... +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: 1 + | ... +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 + | ... + +-- +-- 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: 1 + | ... +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: 1 + | ... +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..430c7c46a --- /dev/null +++ b/test/sql/add-column.test.sql @@ -0,0 +1,183 @@ +-- +-- 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 c INT; + +\set language lua +view = box.space._space.index[2]:select('V')[1]:totable() +view_format = view[7] +f = {type = 'string', nullable_action = 'none', name = 'C', is_nullable = true} +table.insert(view_format, f) +view[5] = 3 +view[7] = view_format +box.space._space:replace(view) +\set language sql + +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); +INSERT INTO ck_check VALUES (1, 1); +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); +INSERT INTO t1 VALUES (1, 1); +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); +SELECT * FROM null_check; +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); +INSERT INTO notnull_check VALUES (1, 'not null'); +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..3ddd52d42 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: 1 +... +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..9ba995579 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: 1 +... +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') ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 5/5] sql: support column addition 2020-11-10 12:18 ` Roman Khabibov @ 2020-11-18 16:15 ` Nikita Pettik 2020-11-15 11:51 ` roman 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-11-18 16:15 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 10 Nov 15:18, Roman Khabibov wrote: > >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c > >> index fd8e7ed1e..597608bea 100644 > >> --- a/src/box/sql/build.c > >> +++ b/src/box/sql/build.c > >> @@ -285,48 +285,110 @@ sql_field_retrieve(Parse *parser, struct space_def *space_def, uint32_t id) > >> return field; > >> } > >> > >> +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; > >> + } > > > > > > You don't need all indexes from space..What you only need from space - > > count of indexes (to generate name if it is required). The same for > > field defs. Mb it is worth avoiding copying space at all - just > > store in create_column_def list of ck/fk constraints, single > > field_def, index/field count - these objects seem to be enough. > > Please consider this suggestion. > Yes, I understand that. But unfortunately, the current code, namely > the functions for creating indexes, fk, and ck constraints, are written > in such a way that they cannot be called without struct space (ephemeral > or not). Vlad and I discussed this and came to the conclusion that it is > better to go in the direction of reusing the code, rather than rewriting > a significant part of build.c. > > https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018460.html > https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019003.html Oh, ok, I won't insist on this refactoring. But could you please copy-paste referenced anwers from mailing list? Those letters quite large so navigating through them takes a lot of time. > >> + /* > >> + * If it is an <ALTER TABLE ADD COLUMN>, then we have to > >> + * create all indexes added by this statement. These > > > > AFAIK only one index (corresponding to unique constraint). > I can write any number of named unique constraints. They should be > created. > ALTER TABLE t ADD COLUMN a INT CONSTRAINT c_1 UNIQUE, CONSTRAINT c_2 UNIQUE … Well, ok. Just wondering why somebody might want to create several unique constraints over the same field.. Otherwise LGTM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 5/5] sql: support column addition 2020-11-18 16:15 ` Nikita Pettik @ 2020-11-15 11:51 ` roman 0 siblings, 0 replies; 18+ messages in thread From: roman @ 2020-11-15 11:51 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches Hi! Thanks for the review. On 18.11.2020 19:15, Nikita Pettik wrote: > On 10 Nov 15:18, Roman Khabibov wrote: >>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >>>> index fd8e7ed1e..597608bea 100644 >>>> --- a/src/box/sql/build.c >>>> +++ b/src/box/sql/build.c >>>> @@ -285,48 +285,110 @@ sql_field_retrieve(Parse *parser, struct space_def *space_def, uint32_t id) >>>> return field; >>>> } >>>> >>>> +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; >>>> + } >>> >>> You don't need all indexes from space..What you only need from space - >>> count of indexes (to generate name if it is required). The same for >>> field defs. Mb it is worth avoiding copying space at all - just >>> store in create_column_def list of ck/fk constraints, single >>> field_def, index/field count - these objects seem to be enough. >>> Please consider this suggestion. >> Yes, I understand that. But unfortunately, the current code, namely >> the functions for creating indexes, fk, and ck constraints, are written >> in such a way that they cannot be called without struct space (ephemeral >> or not). Vlad and I discussed this and came to the conclusion that it is >> better to go in the direction of reusing the code, rather than rewriting >> a significant part of build.c. >> >> https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018460.html >> https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019003.html > Oh, ok, I won't insist on this refactoring. But could you please > copy-paste referenced anwers from mailing list? Those letters quite > large so navigating through them takes a lot of time. I'd rather write down the discussion. At first, we wanted to create just struct space_def with a new column and store it as a field in struct create_column_def. The modified struct space_def is needed in the index_file_def() function. We didn'twant to rewrite this function. Then it turned out that we need to count indexesandadd them to the array inside struct space. sql_create_index() modifies this array during parsing of the statement. We found it easier to create ephemeral space thanto change the code for this function. Well, put ifs -- less work. Then, I noticedthat the opcode generation for creating constants after CREATE TABLE andALTER TABLE is the same, and decided to move the code from sqlTableEnd()to a separate function, so there was a patch for refactoring. >>>> + /* >>>> + * If it is an <ALTER TABLE ADD COLUMN>, then we have to >>>> + * create all indexes added by this statement. These >>> AFAIK only one index (corresponding to unique constraint). >> I can write any number of named unique constraints. They should be >> created. >> ALTER TABLE t ADD COLUMN a INT CONSTRAINT c_1 UNIQUE, CONSTRAINT c_2 UNIQUE … > Well, ok. Just wondering why somebody might want to create several unique > constraints over the same field.. Ha ha, I agree, a strange case. But, unfortunately, it is possible. > Otherwise LGTM commit ace2be0b40775b0852160a6dcb0502e85e546e04 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Thu Jan 2 19:06:14 2020 +0300 sql: support column addition Enable to add column to existing space with <ALTER TABLE ADD [COLUMN]> statement. Column definition can be supplemented with the four types of constraints, <DEFAULT>, <COLLATE> clauses and <[NOT] NULL>, AUTOINCREMENT. Closes #2349, #3075 @TarantoolBot document Title: Add columns to existing tables in SQL Now, it is possible to add columns to existing empty spaces using <ALTER TABLE table_name ADD [COLUMN] column_name column_type ...> statement. The column definition is the same as in <CREATE TABLE> statement. * Space emptiness is Tarantool's restriction. Possibilty to add column to non empty space will be implemented later. 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: 1 ... ``` diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c index bacdad9..7480c02 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_REF", "TK_COLUMN_REF", 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 e6957d6..cfea5bc 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -273,6 +273,7 @@ struct errcode_record { /*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \ /*219 */_(ER_XLOG_GAP, "%s") \ /*220 */_(ER_TOO_EARLY_SUBSCRIBE, "Can't subscribe non-anonymous replica %s until join is done") \ + /*221 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add AUTOINCREMENT: space %s can't feature more than one AUTOINCREMENT 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 7c1e248..7e5ff0e 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -285,48 +285,110 @@ 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) + 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; +} + 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; + /* - * As sql_field_retrieve will allocate memory on region - * ensure that def is also temporal and would be dropped. + * Format can be set in Lua, then exact_field_count can be + * zero, but field_count is not. */ - assert(def->opts.is_ephemeral); - if (sql_field_retrieve(pParse, def, def->field_count) == NULL) - return; - struct region *region = &pParse->region; - z = sql_normalized_name_region_new(region, pName->z, pName->n); - if (z == NULL) { - pParse->is_aborted = true; + if (def->exact_field_count == 0) + def->exact_field_count = def->field_count; + if (sql_field_retrieve(parse, def, def->field_count) == NULL) return; - } + 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 +396,85 @@ 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 +vdbe_emit_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 *s_space = space_by_id(BOX_SPACE_ID); + assert(s_space != NULL); + int cursor = parse->nTab++; + vdbe_emit_open_cursor(parse, cursor, 0, s_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 *) s_space, P4_SPACEPTR); + sqlVdbeCountChanges(v); + sqlVdbeChangeP5(v, OPFLAG_NCHANGE); + sqlReleaseTempRange(parse, tuple_reg, box_space_field_MAX + 1); + vdbe_emit_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)) - return; - struct space_def *def = space->def; + assert(parser->create_column_def.space != NULL); + struct space_def *def = parser->create_column_def.space->def; + assert(def->field_count > 0); struct field_def *field = &def->fields[def->field_count - 1]; if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT && nullable_action != field->nullable_action) { @@ -364,20 +493,21 @@ 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 an <ALTER TABLE ADD COLUMN> + * statement. */ 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; @@ -448,6 +578,8 @@ sqlAddPrimaryKey(struct Parse *pParse) 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) { diag_set(ClientError, ER_CREATE_SPACE, space->def->name, @@ -574,8 +706,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 +723,23 @@ sql_create_check_contraint(struct Parse *parser) return; } } else { - assert(! is_alter); + assert(!is_alter_add_constr); uint32_t ck_idx = ++parser->create_ck_constraint_parse_def.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 ck_constraint *ck; + rlist_foreach_entry(ck, &original_space->ck_constraint, + link) + ck_idx++; + } name = tt_sprintf("ck_unnamed_%s_%u", space->def->name, ck_idx); } size_t name_len = strlen(name); @@ -635,7 +783,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) { @@ -665,9 +813,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); @@ -697,7 +844,6 @@ struct coll * sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id) { assert(def != NULL); - struct space *space = space_by_id(def->id); /* * It is not always possible to fetch collation directly * from struct space due to its absence in space cache. @@ -706,13 +852,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; @@ -796,7 +942,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 { @@ -1034,18 +1181,23 @@ 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 <ALTER TABLE ADD COLUMN> + * statement, we don't have child id (we know it, but + * fk->child_id stores register because of code reuse in + * vdbe_emit_create_constraints()), 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 { @@ -1109,7 +1261,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); } @@ -1150,15 +1302,28 @@ 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 vdbe_emit_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); for (; i < space->index_count; ++i) { struct index *idx = space->index[i]; vdbe_emit_create_index(parse, space->def, idx->def, @@ -1177,6 +1342,20 @@ vdbe_emit_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 = @@ -1879,24 +2058,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) { @@ -1914,6 +2097,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; struct rlist *fkeys = &parse_context->create_fk_constraint_parse_def.fkeys; rlist_add_entry(fkeys, fk_parse, link); @@ -1928,29 +2117,44 @@ 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 create_fk_constraint_parse_def *parse_def = - &parse_context->create_fk_constraint_parse_def; - struct fk_constraint_parse *fk = - rlist_first_entry(&parse_def->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 rlist *fkeys = + &parse_context->create_fk_constraint_parse_def.fkeys; + struct fk_constraint_parse *fk = + rlist_first_entry(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) { struct create_fk_constraint_parse_def *parse_def = &parse_context->create_fk_constraint_parse_def; uint32_t idx = ++parse_def->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; + struct fk_constraint *fk; + rlist_foreach_entry(fk, child_fk, + in_child_space) + idx++; + } constraint_name = sqlMPrintf(db, "fk_unnamed_%s_%u", space->def->name, idx); } else { @@ -2032,7 +2236,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); /* @@ -2061,12 +2265,14 @@ 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 - * maintain list of all FK constraints inside parser. + * In case of <СREATE TABLE> and <ALTER TABLE ADD COLUMN> + * processing, all foreign keys constraints must be + * created after space itself (or space altering), so let + * delay it until vdbe_emit_create_constraints() call and + * simply maintain list of all FK constraints inside + * parser. */ - if (!is_alter) { + if (!is_alter_add_constr) { struct rlist *fkeys = &parse_context->create_fk_constraint_parse_def.fkeys; struct fk_constraint_parse *fk_parse = @@ -2445,7 +2651,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); @@ -2458,10 +2667,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; } struct space_def *def = space->def; @@ -2496,7 +2703,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) { @@ -2662,7 +2869,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; @@ -2750,7 +2957,8 @@ sql_create_index(struct Parse *parse) { sqlVdbeAddOp0(vdbe, OP_Expire); } - if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) + if (!is_create_table_or_add_col || + sql_space_add_index(parse, space, index) != 0) goto exit_create_index; index = NULL; diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 9e870c8..abc3639 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -228,19 +228,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. @@ -283,9 +288,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 } @@ -1737,11 +1744,30 @@ 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); + create_ck_constraint_parse_def_init(&pParse->create_ck_constraint_parse_def); + create_fk_constraint_parse_def_init(&pParse->create_fk_constraint_parse_def); + 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 7ad7496..c5e0935 100644 --- a/src/box/sql/parse_def.h +++ b/src/box/sql/parse_def.h @@ -107,9 +107,10 @@ struct fk_constraint_parse { */ struct fk_constraint_def *fk_def; /** - * If inside CREATE TABLE statement we want to declare - * self-referenced FK constraint, we must delay their - * resolution until the end of parsing of all columns. + * If inside <CREATE TABLE> or <ALTER TABLE ADD COLUMN> + * statement we want to declare self-referenced FK + * constraint, we must delay their resolution until the + * end of parsing of all columns. * E.g.: CREATE TABLE t1(id REFERENCES t1(b), b); */ struct ExprList *selfref_cols; @@ -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 copy. */ + struct space *space; + /** Column type. */ + struct type_def *type_def; +}; + struct create_ck_constraint_parse_def { /** List of ck_constraint_parse_def objects. */ struct rlist checks; @@ -479,6 +489,16 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name, } 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_ck_constraint_parse_def_init(struct create_ck_constraint_parse_def *def) { rlist_create(&def->checks); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 6c79a86..95a6fed 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2251,20 +2251,24 @@ 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 create_fk_constraint_parse_def create_fk_constraint_parse_def; struct create_ck_constraint_parse_def create_ck_constraint_parse_def; /* - * 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>. */ @@ -2858,15 +2862,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_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); + +/** + * 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/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 5404a78..0899cf2 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -40,6 +40,7 @@ #include <unicode/uchar.h> #include "box/session.h" +#include "box/schema.h" #include "say.h" #include "sqlInt.h" #include "tarantoolInt.h" @@ -431,8 +432,8 @@ sql_token(const char *z, int *type, bool *is_reserved) /** * This function is called to release parsing artifacts - * during table creation. The only objects allocated using - * malloc are index defs and check constraints. + * during table creation or column addition. The only objects + * allocated using malloc are index defs. * Note that this functions can't be called on ordinary * space object. It's purpose is to clean-up parser->new_space. * @@ -445,7 +446,15 @@ parser_space_delete(struct sql *db, struct space *space) if (space == NULL || db == NULL) return; assert(space->def->opts.is_ephemeral); - for (uint32_t i = 0; i < space->index_count; ++i) + struct space *altered_space = space_by_name(space->def->name); + uint32_t i = 0; + /* + * Don't delete already existing defs and start from new + * ones. + */ + if (altered_space != NULL) + i = altered_space->index_count; + for (; i < space->index_count; ++i) index_def_delete(space->index[i]->def); } @@ -539,7 +548,7 @@ sqlRunParser(Parse * pParse, const char *zSql) sqlVdbeDelete(pParse->pVdbe); pParse->pVdbe = 0; } - parser_space_delete(db, pParse->create_table_def.new_space); + parser_space_delete(db, pParse->create_column_def.space); if (pParse->pWithToFree) sqlWithDelete(db, pParse->pWithToFree); diff --git a/test/box/error.result b/test/box/error.result index 4d85b9e..2f6d7fb 100644 --- a/test/box/error.result +++ b/test/box/error.result @@ -439,6 +439,7 @@ t; | 218: box.error.TUPLE_METADATA_IS_TOO_BIG | 219: box.error.XLOG_GAP | 220: box.error.TOO_EARLY_SUBSCRIBE + | 221: 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 0000000..924368e --- /dev/null +++ b/test/sql/add-column.result @@ -0,0 +1,529 @@ +-- 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: 1 + | ... + +-- +-- 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 c INT; + | --- + | - null + | - 'Can''t modify space ''V'': view can not be altered' + | ... + +\set language lua + | --- + | - true + | ... +view = box.space._space.index[2]:select('V')[1]:totable() + | --- + | ... +view_format = view[7] + | --- + | ... +f = {type = 'string', nullable_action = 'none', name = 'C', is_nullable = true} + | --- + | ... +table.insert(view_format, f) + | --- + | ... +view[5] = 3 + | --- + | ... +view[7] = view_format + | --- + | ... +box.space._space:replace(view) + | --- + | - error: 'Can''t modify space ''V'': view can not be altered' + | ... +\set language sql + | --- + | - true + | ... + +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: 1 + | ... +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: 1 + | ... +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: 1 + | ... +INSERT INTO ck_check VALUES (1, 0); + | --- + | - null + | - 'Check constraint failed ''ck_unnamed_CK_CHECK_1'': b > 0' + | ... +INSERT INTO ck_check VALUES (1, 1); + | --- + | - row_count: 1 + | ... +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: 1 + | ... +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' + | ... +INSERT INTO t1 VALUES (1, 1); + | --- + | - row_count: 1 + | ... +INSERT INTO fk_check VALUES (2, 1); + | --- + | - row_count: 1 + | ... +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: 1 + | ... +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: 1 + | ... +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: space AUTOINC_CHECK can''t feature more than one AUTOINCREMENT + | 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: 1 + | ... +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: 1 + | ... +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: 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] + | ... +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: 1 + | ... +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 + | ... + +-- +-- 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: 1 + | ... +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: 1 + | ... +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 0000000..430c7c4 --- /dev/null +++ b/test/sql/add-column.test.sql @@ -0,0 +1,183 @@ +-- +-- 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 c INT; + +\set language lua +view = box.space._space.index[2]:select('V')[1]:totable() +view_format = view[7] +f = {type = 'string', nullable_action = 'none', name = 'C', is_nullable = true} +table.insert(view_format, f) +view[5] = 3 +view[7] = view_format +box.space._space:replace(view) +\set language sql + +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); +INSERT INTO ck_check VALUES (1, 1); +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); +INSERT INTO t1 VALUES (1, 1); +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); +SELECT * FROM null_check; +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); +INSERT INTO notnull_check VALUES (1, 'not null'); +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 7b18e5d..3ddd52d 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: 1 +... +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 301f8ea..a791314 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 33689a0..9ba9955 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: 1 +... +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 d2dd88d..29918c5 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') ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-11-18 23:06 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09 13:45 [Tarantool-patches] [PATCH v4 0/5] Support column addition Roman Khabibov
2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 1/5] sql: rename TK_COLUMN to TK_COLUMN_REF Roman Khabibov
2020-11-05 22:17 ` Nikita Pettik
2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse Roman Khabibov
2020-11-05 22:17 ` Nikita Pettik
2020-11-10 12:17 ` Roman Khabibov
2020-11-17 20:23 ` Nikita Pettik
2020-11-15 11:51 ` roman
2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 3/5] schema: add box_space_field_MAX Roman Khabibov
2020-11-05 22:18 ` Nikita Pettik
2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 4/5] sql: use parser's region of "index" array Roman Khabibov
2020-11-05 22:26 ` Nikita Pettik
2020-11-10 12:18 ` Roman Khabibov
2020-11-17 20:35 ` Nikita Pettik
[not found] ` <20201009134529.13212-6-roman.habibov@tarantool.org>
2020-11-06 15:57 ` [Tarantool-patches] [PATCH v4 5/5] sql: support column addition Nikita Pettik
2020-11-10 12:18 ` Roman Khabibov
2020-11-18 16:15 ` Nikita Pettik
2020-11-15 11:51 ` roman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox