[tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine
v.shpilevoy at tarantool.org
v.shpilevoy at tarantool.org
Thu Mar 22 16:57:45 MSK 2018
See below 4 comments.
> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev at tarantool.org> написал(а):
>
> As far as new opcodes to operate on system spaces have been introduced,
> there is no more need to open cursors on such tables, when it comes to
> insert/delete operations. In addition, it allows to get rid of system
> spaces lookups from SQL data dictionary. Moreover, previously DDL
> relied on nested parse to make deletions from system space. But during
> nested parse it is impossible to convert space id to space pointer (i.e.
> emit OP_SIDtoPtr opcode) without any overhead or "hacks". So, nested
> parse has been replaced with hardcoded sequences of opcodes,
> implementing the same logic.
>
> Closes #3252
> ---
> src/box/sql/build.c | 322 ++++++++++++++++++++++---------------------
> src/box/sql/sqliteInt.h | 5 +
> src/box/sql/trigger.c | 27 ++--
> test/sql/transition.result | 11 +-
> test/sql/transition.test.lua | 8 +-
> 5 files changed, 190 insertions(+), 183 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 229c8b4d5..02d27dec6 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2238,9 +2182,11 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView)
1. sqlite3CodeDropTable now is used in build.c only, and can be declared as static (and removed from sqliteInt.h).
> v = sqlite3GetVdbe(pParse);
> assert(v != 0);
> sqlite3BeginWriteOperation(pParse, 1);
> -
> - /* Drop all triggers associated with the table being dropped. Code
> - * is generated to remove entries from _trigger space.
> + /*
> + * Drop all triggers associated with the table being
> + * dropped. Code is generated to remove entries from
> + * _trigger. OP_DropTrigger will remove it from internal
> + * SQL structures.
> */
> pTrigger = pTab->pTrigger;
> /* Do not account triggers deletion - they will be accounted
> @@ -2254,58 +2200,115 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView)
> -
> - /* Drop all _space and _index entries that refer to the
> - * table. The program loops through the _index & _space tables and deletes
> - * every row that refers to a table.
> - * Triggers are handled separately because a trigger can be
> - * created in the temp database that refers to a table in another
> - * database.
> + /* Delete entry by from _space_sequence. */
> + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1,
> + idx_rec_reg);
> + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID,
> + idx_rec_reg);
> + VdbeComment((v, "Delete entry from _space_sequence"));
> + /*
> + * Program below implement analogue of SQL statement:
> + * "DELETE FROM BOX_SEQUENCE WHERE name = zName;"
> + * However, we can't use nested parse for DDL,
> + * since pointers to spaces will expire.
> + */
2. Why do you delete by name, if you have struct space.sequence->def->id?
>
> + /*
> + * Drop all _space and _index entries that refer to the
> + * table.
> */
> - int space_id = SQLITE_PAGENO_TO_SPACEID(pTab->tnum);
> if (!isView) {
> - if (pTab->pIndex && pTab->pIndex->pNext) {
> - /* Remove all indexes, except for primary.
> - Tarantool won't allow remove primary when secondary exist. */
> - sqlite3NestedParse(pParse,
> - "DELETE FROM \"%s\" WHERE \"id\"=%d AND \"iid\">0",
> - TARANTOOL_SYS_INDEX_NAME, space_id);
> + struct space *space = space_by_id(space_id);
> + assert(space != NULL);
> + uint32_t index_count = space->index_count;
> + if (index_count > 1) {
> + /*
> + * Remove all indexes, except for primary.
> + * Tarantool won't allow remove primary when
> + * secondary exist.
> + */
> + uint32_t *iids =
> + (uint32_t *) sqlite3Malloc(sizeof(uint32_t) *
> + (index_count - 1));
3.1. Check for OOM;
3.2. For such short lived allocations you can use region_alloc - it allows to avoid multiple free() calls on each alloc, and free all temp allocations at once after a transaction is finished.
>
> @@ -2603,16 +2606,18 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
> sqlite3VdbeJumpHere(v, addr1);
> if (memRootPage < 0)
> sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0);
> - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum));
> - assert(space != NULL);
> int space_ptr_reg = ++pParse->nMem;
> - sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0,
> - ((int64_t) space));
> + /*
> + * OP_Clear can use truncate optimization
> + * (i.e. insert record into _truncate) and schema
> + * may change. Thus, use dynamic conversion from
> + * space id to ptr.
> + */
> + sqlite3VdbeAddOp2(v, OP_SIDtoPtr, SQLITE_PAGENO_TO_SPACEID(tnum),
> + space_ptr_reg);
> sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, space_ptr_reg,
> (char *)pKey, P4_KEYINFO);
> - sqlite3VdbeChangeP5(v,
> - OPFLAG_BULKCSR | ((memRootPage >= 0) ?
> - OPFLAG_P2ISREG : 0));
> + sqlite3VdbeChangeP5(v, OPFLAG_FRESH_PTR);
4. OPFLAG_FRESH_PTR seems to be unused. Why do you need it?
>
More information about the Tarantool-patches
mailing list