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 473E524E06 for ; Fri, 18 May 2018 02:57:15 -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 naHQsH_DX_HL for ; Fri, 18 May 2018 02:57:15 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 D3D9A24DE2 for ; Fri, 18 May 2018 02:57:14 -0400 (EDT) Date: Fri, 18 May 2018 09:57:12 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH 1/2] sql: allow key_def to be NULL for ephemeral create Message-ID: <20180518065712.aal76hawn3ofyzln@tarantool.org> References: <54fef8ed5144ab8ee384687caa3448a89ac62746.1526483543.git.kyukhin@tarantool.org> <20180517154939.ya64lgo4ifipmofk@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org Hi Vlad! Thanks for uyour inputs. My answers inlined, patch in the bottom. On 17 мая 19:47, Vladislav Shpilevoy wrote: > Thanks for review fixes! See 4 comments below. > > On 17/05/2018 18:49, Kirill Yukhin wrote: > > Hi Vlad, > > > > On 16 мая 18:24, Kirill Yukhin wrote: > > > There're many cases in SQL FE, where exact key def passed to > > > doesn't ephemeral table matter. It is used to store data only. > > 1. Can't read. Maybe 'where exact key def passed to ephemeral table doesn't matter' ? > > > > Only field count makes sense in such a case. Hoewever it is > > 2. However. > > > > passed separately (in P2). So, allow P4 field to be NULL for > > > OP_OpenTEphemeral. Update delte from routine from sql/delete.c > > 3. delete. > > > > accordingly. > > > > > > Part of #3235 > > Pasting updated patch from 1st round of review of 1/2 patch. > > > > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > > index 22130ef..1ad7469 100644 > > --- a/src/box/sql/insert.c > > +++ b/src/box/sql/insert.c > > @@ -566,13 +566,8 @@ 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); > > + 0, (char*)NULL, P4_KEYDEF); > > 4. In the OP_OpenTEphemeral code you check for P4_KEYDEF absence, so you can use > AddOp2 here. And in other places. Fixed around the code. -- Regards, Kirill Yukhin commit 9e5ed7bb5e0d01ff5b8f9d5c6af82e915135fcb0 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..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); } 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; }