[tarantool-patches] Re: [PATCH 1/2] sql: allow key_def to be NULL for ephemeral create
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri May 18 13:50:49 MSK 2018
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 <kyukhin at tarantool.org>
> 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;
> }
>
More information about the Tarantool-patches
mailing list