Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: introduce TRUNCATE TABLE operation
Date: Wed, 25 Jul 2018 15:58:10 +0300	[thread overview]
Message-ID: <28A6DE0C-68D4-4155-BB94-ACD742757291@tarantool.org> (raw)
In-Reply-To: <2ec56870-e8cb-6112-f12a-a654f59315dc@tarantool.org>

Except for comments below, also pls add to tarantool-doc bot request
notice concerning the fact that in our implementation TRUNCATE is DDL operation.


> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 1ec1538..705caf1 100644
> --- a/extra/mkkeywordhash.c
> +++ b/extra/mkkeywordhash.c
> @@ -281,6 +281,7 @@ static Keyword aKeywordTable[] = {
>   { "VARCHAR",                "TK_ID",          RESERVED,         true  },
>   { "WHENEVER",               "TK_STANDARD",    RESERVED,         true  },
>   { "WHILE",                  "TK_STANDARD",    RESERVED,         true  },
> +  { "TRUNCATE",               "TK_TRUNCATE",    ALWAYS,           true  },
> };
> 
> /* Number of keywords */
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 0681177..b016084 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -72,6 +72,62 @@ sql_materialize_view(struct Parse *parse, const char *name, struct Expr *where,
> }
> 
> void
> +sql_table_truncate(struct Parse *parse, struct SrcList *tab_list)
> +{
> +	struct sqlite3 *db = parse->db;
> +	if (parse->nErr || db->mallocFailed)
> +		goto cleanup;

Is this check reasonable?

> +	assert(tab_list->nSrc == 1);
> +
> +	struct Vdbe *v = sqlite3GetVdbe(parse);
> +	if (v == NULL)
> +		goto cleanup;
> +
> +	const char *tab_name = tab_list->a->zName;
> +	struct Table *table = sqlite3LocateTable(parse, LOCATE_NOERR, tab_name);

I guess here you can explicitly use space_by_id and box_space_id_by_name
to bypass table hash. If you do so, you will be able to get rid off 
if (table == NULL) branch.

> +	struct space_def *space_def = NULL;
> +	if (table == NULL) {
> +		/* Space created with LUA. */
> +		uint32_t space_id =
> +			box_space_id_by_name(tab_name, strlen(tab_name));
> +		if (space_id == BOX_ID_NIL) {
> +			diag_set(ClientError, ER_NO_SUCH_SPACE, tab_name);
> +			parse->rc = SQL_TARANTOOL_ERROR;
> +			parse->nErr++;

During exception handling you three times process
parse->rc = SQL_TARANTOOL_ERROR;
parse->nErr++;

Lets use separate label for this purpose.

> +			goto cleanup;
> +		}
> +		struct space *space = space_cache_find(space_id);
> +		assert(space != NULL);
> +		space_def = space->def;
> +	} else {
> +		space_def = table->def;
> +		if (sqlite3FkRequired(table, NULL) != 0) {

sqlite3FkRequired is too strict condition. I guess you are still
capable of truncating child (or referencing) table, since no
FK violations may take place in this case.

So, what you need is simple check:

sqlite3References() != NULL;

> +			const char *err_msg =
> +				tt_sprintf("cannot truncate %s because it has "
> +					   "foreign keys”);

Lets add ’space’ or ’table' word to message to avoid confusing:

“can no truncate table %s becase other objects depend on it”
OR
“can no truncate table %s becase other objects reference it”

> +			diag_set(ClientError, ER_SQL, err_msg);
> +			parse->rc = SQL_TARANTOOL_ERROR;
> +			parse->nErr++;
> +			goto cleanup;
> +		}
> +	}
> +	assert(space_def != NULL);
> +	if (space_def->opts.is_view) {
> +		const char *err_msg =
> +			tt_sprintf("cannot truncate %s because it is a view",
> +				   space_def->name);
> +		diag_set(ClientError, ER_SQL, err_msg);
> +		parse->rc = SQL_TARANTOOL_ERROR;
> +		parse->nErr++;
> +		goto cleanup;
> +	}
> +	sqlite3VdbeAddOp2(v, OP_Clear, space_def->id, true);
> +
> +cleanup:
> +	sqlite3SrcListDelete(parse->db, tab_list);
> +}
> +
> +void
> sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> 		      struct Expr *where)
> {
> 
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 07e7829..a90b10e 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -3710,11 +3710,22 @@ void sqlite3OpenTable(Parse *, int iCur, Table *, int);
>  * @param tab_list List of single element which table from which
>  * deletetion if performed.
>  * @param where The WHERE clause.  May be NULL.
> + * @param is_truncate use OP_Clear to truncate table.

This comment is remained from previous patch version, I guess.

> --- a/test/sql/delete.test.lua
> +++ b/test/sql/delete.test.lua
> @@ -37,3 +37,42 @@ box.sql.execute("CREATE TRIGGER t2 BEFORE INSERT ON t2 BEGIN DELETE FROM t1; END
> box.sql.execute("INSERT INTO t2 VALUES (0);")
> 
> box.sql.execute("DROP TABLE t2;")
> +
> +
> +--
> +-- gh-2201: TRUNCATE TABLE operation
> +--
> +
> +-- can't truncate system table

Pls start comments (even in tests) from capital letter and end with dot.

> +box.sql.execute("TRUNCATE TABLE \"_sql_stat1\";")
> +
> +box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY, a, b);”)

Specify types for all columns. Static typing is on the way.

  reply	other threads:[~2018-07-25 12:58 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-18  8:22 [tarantool-patches] " Kirill Shcherbatov
2018-07-18  8:31 ` [tarantool-patches] " Vladislav Shpilevoy
2018-07-18 13:01   ` Kirill Shcherbatov
2018-07-18 16:40     ` n.pettik
2018-07-19  9:01       ` Kirill Shcherbatov
2018-07-19 10:00       ` Kirill Shcherbatov
2018-07-20  2:16         ` n.pettik
2018-07-20  8:29           ` Kirill Shcherbatov
2018-07-20 16:33             ` n.pettik
2018-07-23 10:33               ` Kirill Shcherbatov
2018-07-25 12:58                 ` n.pettik [this message]
2018-07-25 16:59                   ` Kirill Shcherbatov
2018-07-26  9:10                     ` n.pettik
2018-07-26 20:40 ` Vladislav Shpilevoy
2018-07-27  7:09   ` Kirill Shcherbatov
2018-07-27  7:16 ` Kirill Yukhin

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=28A6DE0C-68D4-4155-BB94-ACD742757291@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v1 1/1] sql: introduce TRUNCATE TABLE operation' \
    /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