[tarantool-patches] Re: [PATCH v4 6/6] sql: VDBE tests for trigger existence
n.pettik
korablev at tarantool.org
Sun Jun 24 18:25:34 MSK 2018
> On 20 Jun 2018, at 13:46, Kirill Shcherbatov <kshcherbatov at tarantool.org> wrote:
>
> Trigger presence in system should be
Why? If you didn’t check it, you simply would get ‘duplicate key error’.
So, you did it to display more informative error..?
> tested on each VDBE
> execution attempt, not on Parser iteration.
And? After this sentence you should explain changes which you patch provides.
>
> Part of #3435, #3273
> ---
> src/box/sql/build.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> src/box/sql/main.c | 11 ++++++++---
> src/box/sql/sqliteInt.h | 24 ++++++++++++++++++++++++
> src/box/sql/trigger.c | 25 +++++++++++++------------
> src/box/sql/vdbe.c | 18 +++++++++++-------
> test/sql-tap/view.test.lua | 8 ++++----
> test/sql/view.result | 4 ++--
> 7 files changed, 104 insertions(+), 28 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index c8bfad7..fff7c19 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3971,4 +3971,46 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
> sqlite3DbFree(db, pWith);
> }
> }
> +
> +int
> +vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
I would call it vdbe_emit_halt_if_exists().
Or vdbe_emit_halt_on_duplication()...
> + int index_id, const char *name_src,
> + int tarantool_error_code,
> + const char *error_src, bool no_error)
> +{
> + struct Vdbe *v = sqlite3GetVdbe(parser);
> + assert(v != NULL);
> +
> + struct sqlite3 *db = parser->db;
> + char *name = sqlite3DbStrDup(db, name_src);
> + if (name == NULL) {
> + size_t size = strlen(name_src) + 1;
> + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name”);
In previous patch, you used sqlite3DbMalloc() function without using diag_set(),
since in case of fail it would call sqlite3OomFault(db);
Lets handle all usages of sqlite3DbMalloc() in the same way.
> + return -1;
> + }
> + char *error = sqlite3DbStrDup(db, error_src);
> + if (error == NULL) {
> + sqlite3DbFree(db, name);
> + size_t size = strlen(error_src) + 1;
> + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error”);
The same is here.
> + return -1;
> + }
> +
> + int cursor = parser->nTab++;
> + vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id));
> +
> + int name_reg = parser->nMem++;
> + int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
> + P4_DYNAMIC);
> + sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
> + if (no_error) {
> + sqlite3VdbeAddOp0(v, OP_Halt);
> + } else {
> + sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
> + ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
Why do you set ON_CONFLICT_ACTION_FAIL? You can just skip this arg with 0.
> + sqlite3VdbeChangeP5(v, tarantool_error_code);
> + }
You may refactor code this way (if you like it):
+++ b/src/box/sql/build.c
@@ -4003,11 +4003,10 @@ vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
P4_DYNAMIC);
sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
- if (no_error) {
- sqlite3VdbeAddOp0(v, OP_Halt);
- } else {
- sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
- ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
+ int op_halt_addr = sqlite3VdbeAddOp0(v, OP_Halt);
+ if (!no_error) {
+ sqlite3VdbeChangeP1(v, op_halt_addr, SQL_TARANTOOL_ERROR);
+ sqlite3VdbeChangeP4(v, op_halt_addr, error, P4_DYNAMIC);
sqlite3VdbeChangeP5(v, tarantool_error_code);
}
> + sqlite3VdbeAddOp1(v, OP_Close, cursor);
> + return 0;
> +}
> #endif /* !defined(SQLITE_OMIT_CTE) */
Why this func is under SQLITE_OMIT_CTE guard?
> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
> index 0acf7bc..e435c01 100644
> --- a/src/box/sql/main.c
> +++ b/src/box/sql/main.c
> @@ -1454,11 +1454,16 @@ sqlite3_errmsg(sqlite3 * db)
> z = sqlite3ErrStr(SQLITE_NOMEM_BKPT);
> } else {
> testcase(db->pErr == 0);
> - z = (char *)sqlite3_value_text(db->pErr);
> assert(!db->mallocFailed);
> - if (z == 0) {
> - z = sqlite3ErrStr(db->errCode);
> + if (db->errCode != SQL_TARANTOOL_ERROR) {
> + assert(!db->mallocFailed);
Why have you duplicated this assert? It occurs 2 lines above.
> + z = (char *)sqlite3_value_text(db->pErr);
> + if (z == NULL)
> + z = sqlite3ErrStr(db->errCode);
> + } else {
> + z = diag_last_error(diag_get())->errmsg;
> }
> + assert(z != NULL);
> }
> return z;
> }
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 72803fa..acda23d 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -4627,4 +4627,28 @@ extern int sqlite3InitDatabase(sqlite3 * db);
> enum on_conflict_action
> table_column_nullable_action(struct Table *tab, uint32_t column);
>
> +/**
> + * Generate VDBE code to halt execution with correct error if
> + * the object with specified name is already present in
> + * specified space.
> + * The function does not begin to hold the passed error pointer
> + * to memory.
Rephrase last sentence - I can’t parse it..
> + *
> + * @param parser Parsing context.
> + * @param space_id Space to lookup identifier.
> + * @param index_id Index identifier containing string primary key.
How can index id contain ’string primary key’?
> + * @param name_src Of object to test existence.
Name of object to test on existence.
> + * @param tarantool_error_code to set on halt.
> + * @param error_src string to notify on halt.
Error message to display on VDBE halt.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index c1df880..487b026 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -960,7 +960,9 @@ case OP_HaltIfNull: { /* in3 */
> *
> * If P4 is not null then it is an error message string.
> *
> - * P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
> + * If P1 is SQL_TARANTOOL_ERROR then P5 is a ClientError code and
> + * P4 is error message to set. Else P5 is a value between 0 and 4,
> + * inclusive, that modifies the P4 string.
> *
> * 0: (no change)
> * 1: NOT NULL contraint failed: P4
> @@ -968,8 +970,8 @@ case OP_HaltIfNull: { /* in3 */
> * 3: CHECK constraint failed: P4
> * 4: FOREIGN KEY constraint failed: P4
> *
> - * If P5 is not zero and P4 is NULL, then everything after the ":" is
> - * omitted.
> + * If P5 is not zero and P4 is NULL, then everything after the
> + * ":" is omitted.
Looks like redundant diff. If you wanted to make comments fit into 66 chars,
you would better do it for the whole comment.
> *
> * There is an implied "Halt 0 0 0" instruction inserted at the very end of
> * every program. So a jump past the last instruction of the program
> @@ -1005,9 +1007,11 @@ case OP_Halt: {
> p->rc = pOp->p1;
> p->errorAction = (u8)pOp->p2;
> p->pc = pcx;
> - assert(pOp->p5<=4);
> if (p->rc) {
> - if (pOp->p5) {
> + if (p->rc == SQL_TARANTOOL_ERROR) {
> + assert(pOp->p4.z != NULL);
> + box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
> + } else if (pOp->p5 != 0) {
> static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
> "FOREIGN KEY" };
> testcase( pOp->p5==1);
> @@ -2999,8 +3003,8 @@ case OP_CheckViewReferences: {
> struct space *space = space_by_id(space_id);
> assert(space != NULL);
> if (space->def->view_ref_count > 0) {
> - sqlite3VdbeError(p,"Can't drop table %s: other views depend "
> - "on this space", space->def->name);
> + diag_set(ClientError, ER_DROP_SPACE, space->def->name,
> + "other views depend on this space”);
Why did you provide this diff? AFAIK in this particular case these two calls
are *almost* equivalent.
More information about the Tarantool-patches
mailing list