Tarantool development patches archive
 help / color / mirror / Atom feed
From: "v.shpilevoy@tarantool.org" <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Nikita Pettik <korablev@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine
Date: Thu, 22 Mar 2018 16:57:45 +0300	[thread overview]
Message-ID: <33D8E3FE-EA0B-4426-8CC0-BCCB448AD239@tarantool.org> (raw)
In-Reply-To: <71a9b8ac842931885bb91551c2466c670f238d10.1521583434.git.korablev@tarantool.org>

See below 4 comments.

> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@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?
> 

  reply	other threads:[~2018-03-22 13:57 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-20 23:48 [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime Nikita Pettik
2018-03-20 23:48 ` [tarantool-patches] [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite Nikita Pettik
2018-03-21 13:14   ` [tarantool-patches] " Kirill Yukhin
2018-03-22 10:07     ` n.pettik
2018-03-22 11:04       ` v.shpilevoy
2018-03-23 16:01         ` n.pettik
2018-03-20 23:48 ` [tarantool-patches] [PATCH 2/4] sql: introduce opcodes to operate on system spaces Nikita Pettik
2018-03-22 11:42   ` [tarantool-patches] " v.shpilevoy
2018-03-22 12:23     ` n.pettik
2018-03-22 13:09       ` v.shpilevoy
2018-03-23 16:20         ` n.pettik
2018-03-20 23:48 ` [tarantool-patches] [PATCH 3/4] sql: rework code generation for DDL routine Nikita Pettik
2018-03-22 13:57   ` v.shpilevoy [this message]
2018-03-23 16:33     ` [tarantool-patches] " n.pettik
2018-03-20 23:48 ` [tarantool-patches] [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead Nikita Pettik
2018-03-22 14:11   ` [tarantool-patches] " v.shpilevoy
2018-03-23 16:39     ` n.pettik
2018-03-24 12:37 ` [tarantool-patches] Re: [PATCH 0/4] Replace space id with space ptrs in VDBE runtime v.shpilevoy
2018-03-27 16:28   ` n.pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=33D8E3FE-EA0B-4426-8CC0-BCCB448AD239@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox