From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 43A7E25597 for ; Fri, 18 May 2018 06:50:52 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Y1EGwckcU9Dd for ; Fri, 18 May 2018 06:50:52 -0400 (EDT) Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D1CD624650 for ; Fri, 18 May 2018 06:50:51 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 1/2] sql: allow key_def to be NULL for ephemeral create References: <54fef8ed5144ab8ee384687caa3448a89ac62746.1526483543.git.kyukhin@tarantool.org> <20180517154939.ya64lgo4ifipmofk@tarantool.org> <20180518065712.aal76hawn3ofyzln@tarantool.org> <6186d3a5-8cb6-6ce9-c1ee-d100e04fec96@tarantool.org> <20180518104847.xni3sj7halyqozqi@tarantool.org> From: Vladislav Shpilevoy Message-ID: <0dd409e1-3162-653a-6a1f-f7cf1c6d3072@tarantool.org> Date: Fri, 18 May 2018 13:50:49 +0300 MIME-Version: 1.0 In-Reply-To: <20180518104847.xni3sj7halyqozqi@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Kirill Yukhin Now LGTM. On 18/05/2018 13:48, Kirill Yukhin wrote: > On 18 мая 13:33, Vladislav Shpilevoy wrote: >> Hello. Thanks for fixes! >> >>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c >>> index 3455f52..2c1ce44 100644 >>> --- a/src/box/sql/delete.c >>> +++ b/src/box/sql/delete.c >>> @@ -386,14 +386,9 @@ sqlite3DeleteFrom(Parse * pParse, /* The parser context */ >>> iPk = pParse->nMem + 1; >>> pParse->nMem += nPk; >>> iEphCur = pParse->nTab++; >>> - struct key_def *def = key_def_new(nPk); >>> - if (def == NULL) { >>> - sqlite3OomFault(db); >>> - goto delete_from_cleanup; >>> - } >>> addrEphOpen = >>> sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iEphCur, >>> - nPk, 0, (char*)def, P4_KEYDEF); >>> + nPk); >> >> Compilation error. > Whoops, fixed. > > -- > Regards, Kirill Yukhin > > commit 8640d51f1fb60022b4b61b84fd5c6111c5dbe916 > Author: Kirill Yukhin > Date: Wed May 16 09:47:02 2018 +0300 > > sql: allow key_def to be NULL for ephemeral create > > There're many cases in SQL FE, where exact key def passed to > ephemeral table doesn't matter. It is used to store data only. > Only field count makes sense in such a case. However it is > passed separately (in P2). So, allow P4 field to be NULL for > OP_OpenTEphemeral. Update delete from routine from sql/delete.c > accordingly. > > Part of #3235 > > diff --git a/src/box/sql.c b/src/box/sql.c > index 195fdfb..6d2e0b9 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -370,8 +370,9 @@ int tarantoolSqlite3Count(BtCursor *pCur, i64 *pnEntry) > * > * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise. > */ > -int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count, > - struct key_def *def) > +int > +tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count, > + struct key_def *def) > { > assert(pCur); > assert(pCur->curFlags & BTCF_TEphemCursor); > @@ -381,7 +382,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count, > return SQL_TARANTOOL_ERROR; > for (uint32_t part = 0; part < field_count; ++part) { > struct coll *coll; > - if (part < def->part_count) > + if (def != NULL && part < def->part_count) > coll = def->parts[part].coll; > else > coll = NULL; > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 3455f52..1a2f3d0 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -386,14 +386,9 @@ sqlite3DeleteFrom(Parse * pParse, /* The parser context */ > iPk = pParse->nMem + 1; > pParse->nMem += nPk; > iEphCur = pParse->nTab++; > - struct key_def *def = key_def_new(nPk); > - if (def == NULL) { > - sqlite3OomFault(db); > - goto delete_from_cleanup; > - } > addrEphOpen = > - sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iEphCur, > - nPk, 0, (char*)def, P4_KEYDEF); > + sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEphCur, > + nPk); > } else { > pPk = sqlite3PrimaryKeyIndex(pTab); > assert(pPk != 0); > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 5ada87e..54a7e4a 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -566,13 +566,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > regRec = sqlite3GetTempReg(pParse); > regCopy = sqlite3GetTempRange(pParse, nColumn); > regTempId = sqlite3GetTempReg(pParse); > - struct key_def *def = key_def_new(nColumn + 1); > - if (def == NULL) { > - sqlite3OomFault(db); > - goto insert_cleanup; > - } > - sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, srcTab, nColumn+1, > - 0, (char*)def, P4_KEYDEF); > + sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, srcTab, nColumn+1); > /* Create counter for rowid */ > sqlite3VdbeAddOp4Dup8(v, OP_Int64, > 0 /* unused */, > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index bd895bc..de111d3 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -2215,13 +2215,7 @@ generateWithRecursiveQuery(Parse * pParse, /* Parsing context */ > VdbeComment((v, "Orderby table")); > destQueue.pOrderBy = pOrderBy; > } else { > - struct key_def *def = key_def_new(nCol + 1); > - if (def == NULL) { > - sqlite3OomFault(pParse->db); > - goto end_of_recursive_query; > - } > - sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iQueue, nCol + 1, 0, > - (char*)def, P4_KEYDEF); > + sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iQueue, nCol + 1); > VdbeComment((v, "Queue table")); > } > if (iDistinct) { > @@ -2422,13 +2416,7 @@ multiSelect(Parse * pParse, /* Parsing context */ > if (dest.eDest == SRT_EphemTab) { > assert(p->pEList); > int nCols = p->pEList->nExpr; > - struct key_def *def = key_def_new(nCols + 1); > - if (def == NULL) { > - sqlite3OomFault(db); > - goto multi_select_end; > - } > - sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, dest.iSDParm, nCols + 1, > - 0, (char*)def, P4_KEYDEF); > + sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, dest.iSDParm, nCols + 1); > VdbeComment((v, "Destination temp")); > dest.eDest = SRT_Table; > } > @@ -5608,11 +5596,8 @@ sqlite3Select(Parse * pParse, /* The parser context */ > /* If the output is destined for a temporary table, open that table. > */ > if (pDest->eDest == SRT_EphemTab) { > - struct key_def *def = key_def_new(pEList->nExpr + 1); > - if (def == NULL) > - sqlite3OomFault(db); > - sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, pDest->iSDParm, > - pEList->nExpr + 1, 0, (char*)def, P4_KEYDEF); > + sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, pDest->iSDParm, > + pEList->nExpr + 1); > > VdbeComment((v, "Output table")); > } > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index f3db92c..204adc2 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -358,13 +358,8 @@ sqlite3Update(Parse * pParse, /* The parser context */ > sqlite3VdbeAddOp2(v, OP_Null, 0, iPk); > > if (isView) { > - struct key_def *def = key_def_new(nKey); > - if (def == NULL) { > - sqlite3OomFault(db); > - goto update_cleanup; > - } > - addrOpen = sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iEph, > - nKey, 0, (char*)def, P4_KEYDEF); > + addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, > + nKey); > } else { > addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, nPk); > sql_vdbe_set_p4_key_def(pParse, pPk); > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index e27fe2f..455198d 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3225,9 +3225,9 @@ open_cursor_set_hints: > /** > * Opcode: OpenTEphemeral P1 P2 * P4 * > * Synopsis: > - * @param P1 index of new cursor to be created > - * @param P2 number of columns in a new table > - * @param P4 key def for new table > + * @param P1 index of new cursor to be created. > + * @param P2 number of columns in a new table. > + * @param P4 key def for new table, NULL is allowed. > * > * This opcode creates Tarantool's ephemeral table and sets cursor P1 to it. > */ > @@ -3236,21 +3236,20 @@ case OP_OpenTEphemeral: { > BtCursor *pBtCur; > assert(pOp->p1 >= 0); > assert(pOp->p2 > 0); > - assert(pOp->p4.key_def != NULL); > - assert(pOp->p4type == P4_KEYDEF); > + assert(pOp->p4type != P4_KEYDEF || pOp->p4.key_def != NULL); > > pCx = allocateCursor(p, pOp->p1, pOp->p2, CURTYPE_TARANTOOL); > if (pCx == 0) goto no_mem; > pCx->nullRow = 1; > > - pCx->key_def = pOp->p4.key_def; > pBtCur = pCx->uc.pCursor; > /* Ephemeral spaces don't have space_id */ > pBtCur->eState = CURSOR_INVALID; > pBtCur->curFlags = BTCF_TEphemCursor; > > rc = tarantoolSqlite3EphemeralCreate(pCx->uc.pCursor, pOp->p2, > - pCx->key_def); > + pOp->p4.key_def); > + pCx->key_def = pCx->uc.pCursor->index->def->key_def; > if (rc) goto abort_due_to_error; > break; > } >