[tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine
n.pettik
korablev at tarantool.org
Fri Mar 23 19:33:24 MSK 2018
> On 22 Mar 2018, at 16:57, v.shpilevoy at tarantool.org wrote:
>
> See below 4 comments.
>
>> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev at tarantool.org <mailto: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).
Done:
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2306,7 +2250,7 @@ sqlite3ClearStatTables(Parse * pParse, /* The parsing context */
/*
* Generate code to drop a table.
*/
-void
+static void
sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView)
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3007,7 +3012,6 @@ int sqlite3ViewGetColumnNames(Parse *, Table *);
int sqlite3DbMaskAllZero(yDbMask);
#endif
void sqlite3DropTable(Parse *, SrcList *, int, int);
-void sqlite3CodeDropTable(Parse *, Table *, int);
>
>> 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?
To be honest, I forgot about such opportunity. Fetching id directly from sequence
allows to significantly reduce number of opcodes and simplify query logic:
+ /* Delete entry by id from _sequence. */
+ assert(space->sequence != NULL);
+ int sequence_id_reg = ++pParse->nMem;
+ sqlite3VdbeAddOp2(v, OP_Integer, space->sequence->def->id,
+ sequence_id_reg);
+ sqlite3VdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
+ idx_rec_reg);
+ sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg);
+ VdbeComment((v, "Delete entry from _sequence"));
+ }
>
>>
>> + /*
>> + * 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.
Ok, substituted with region_alloc:
+ size_t size = sizeof(uint32_t) * (index_count - 1);
+ uint32_t *iids =
+ (uint32_t *) region_alloc(&fiber()->gc, size);
+ if (iids == NULL) {
+ diag_set(OutOfMemory, size, "region_alloc",
+ "iids");
+ return;
+ }
>>
>> @@ -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?
I need it to indicate that pointer can’t become expired. Its usage is introduced in the last patch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180323/00961b88/attachment.html>
More information about the Tarantool-patches
mailing list