[tarantool-patches] [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE
imeevma at tarantool.org
imeevma at tarantool.org
Fri Jun 28 15:07:51 MSK 2019
Hi! Thank you for review! My answers and new patch below.
On 6/26/19 7:11 PM, n.pettik wrote:
>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 292168f..c75e1d8 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1066,14 +1066,21 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
>> sqlDbStrDup(db, ck_def->expr_str), P4_DYNAMIC);
>> sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 5,
>> ck_constraint_reg + 5);
>> - const char *error_msg =
>> + /* Check that there is no CHECK with such name. */
>> + const char *err =
>> tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS),
>> ck_def->name);
>> - if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
>> - ck_constraint_reg, 2,
>> - ER_CONSTRAINT_EXISTS, error_msg,
>> - false, OP_NoConflict) != 0)
>> - return;
>> + int cursor = parser->nTab++;
>> + vdbe_emit_open_cursor(parser, cursor, 0,
>> + space_by_id(BOX_CK_CONSTRAINT_ID));
>> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
>> + sqlVdbeAddOp4Int(v, OP_NoConflict, cursor, v->nOp + 3,
>> + ck_constraint_reg, 2);
>> + sqlVdbeAddOp4(v, OP_Error, ER_CONSTRAINT_EXISTS, v->nOp + 1, 0, err,
>> + P4_STATIC);
>> + sqlVdbeAddOp1(v, OP_Halt, -1);
>> + sqlVdbeAddOp1(v, OP_Close, cursor);
>
> I don’t like code duplication: this snippet looks almost the same as
> vdbe_emit_halt_with_presence_test(). Wouldn’t it be better if OP_Error
> was integrated to that function? Also, why not move error setting
> to OP_Error from other OP_Halt usages?
>
Fixed.
>>
>> +/* Opcode: Error P1 P2 * P4 *
>
> I’d call it OP_DiagError or OP_SetDiag.
>
Fixed. Changed to OP_SetDiag.
>> + *
>> + * Setting an error and an unconditional jump to address P2.
>
> Nit: -> Set an error and process unconditional jump …
> Or -> Set an error and unconditionally jump to
>
Fixed with new description.
>> + *
>> + * The P1 parameter is error code of the erroe. The P4 parameter
>
> -> is an error code to be set.
>
Fixed.
>> + * is a description of the error.
>
> -> is a text description.
>
Fixed.
>> + */
>> +case OP_Error: { /* jump */
>> + box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z);
>
> Why not simple diag_set() ?
>
I did it like this because using diag_set() would be a bit
troublesome if we decided to use an argument of a different type
or more than one argument.
New patch:
>From 55b8f0d0ac93ed6df10a538e8485d9309c12f2b7 Mon Sep 17 00:00:00 2001
Date: Thu, 27 Jun 2019 16:28:30 +0300
Subject: [PATCH] sql: add OP_SetDiag opcode in VDBE
This opcode is required to set an error using diag_set() without
halting VDBE. This will allow us to run destructors between error
setting and VDBE halting.
Needed for #4183
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 292168f..aab60dd 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3277,15 +3277,15 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
int cursor = parser->nTab++;
vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id));
sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
- int label = sqlVdbeCurrentAddr(v);
- sqlVdbeAddOp4Int(v, cond_opcode, cursor, label + 3, key_reg,
- key_len);
+ int jmp = sqlVdbeAddOp4Int(v, cond_opcode, cursor, 0, key_reg, key_len);
if (no_error) {
sqlVdbeAddOp0(v, OP_Halt);
} else {
- sqlVdbeAddOp4(v, OP_Halt, -1, 0, tarantool_error_code, error,
+ sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, error,
P4_DYNAMIC);
+ sqlVdbeAddOp1(v, OP_Halt, -1);
}
+ sqlVdbeJumpHere(v, jmp);
sqlVdbeAddOp1(v, OP_Close, cursor);
return 0;
}
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 898d16c..d549c6b 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4347,8 +4347,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
const char *err =
tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
pExpr->u.zToken);
- sqlVdbeAddOp4(v, OP_Halt, -1, pExpr->on_conflict_action,
- ER_SQL_EXECUTE, err, 0);
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
+ P4_STATIC);
+ sqlVdbeAddOp2(v, OP_Halt, -1,
+ pExpr->on_conflict_action);
}
break;
}
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 06c863a..4717677 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -287,8 +287,9 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
assert(incr_count == 1);
const char *err = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
"FOREIGN KEY constraint failed");
- sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err,
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
P4_STATIC);
+ sqlVdbeAddOp1(v, OP_Halt, -1);
} else {
sqlVdbeAddOp2(v, OP_FkCounter, fk_def->is_deferred,
incr_count);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b353148..b065de3 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -809,10 +809,10 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
int check_is_passed = sqlVdbeMakeLabel(v);
sqlExprIfTrue(parser, expr, check_is_passed, SQL_JUMPIFNULL);
const char *fmt = tnt_errcode_desc(ER_CK_CONSTRAINT_FAILED);
- const char *error_msg = tt_sprintf(fmt, ck_constraint_name, expr_str);
- sqlVdbeAddOp4(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT,
- 0, sqlDbStrDup(parser->db, error_msg), P4_DYNAMIC);
- sqlVdbeChangeP5(v, ER_CK_CONSTRAINT_FAILED);
+ const char *err = tt_sprintf(fmt, ck_constraint_name, expr_str);
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_CK_CONSTRAINT_FAILED, 0, 0,
+ sqlDbStrDup(parser->db, err), P4_DYNAMIC);
+ sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
VdbeNoopComment((v, "END: ck constraint %s test", ck_constraint_name));
sqlVdbeResolveLabel(v, check_is_passed);
}
@@ -863,8 +863,9 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
"failed: %s.%s", def->name,
def->fields[i].name));
addr = sqlVdbeAddOp1(v, OP_NotNull, new_tuple_reg + i);
- sqlVdbeAddOp4(v, OP_Halt, -1, on_conflict_nullable,
- ER_SQL_EXECUTE, err, P4_STATIC);
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
+ P4_STATIC);
+ sqlVdbeAddOp2(v, OP_Halt, -1, on_conflict_nullable);
sqlVdbeJumpHere(v, addr);
break;
case ON_CONFLICT_ACTION_IGNORE:
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index a257e72..faa7c34 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2092,8 +2092,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
"Only positive integers are "\
"allowed in the LIMIT clause");
sqlVdbeResolveLabel(v, halt_label);
- sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err,
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
P4_STATIC);
+ sqlVdbeAddOp1(v, OP_Halt, -1);
sqlVdbeResolveLabel(v, positive_limit_label);
VdbeCoverage(v);
@@ -2124,8 +2125,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
err = tt_sprintf(err, "Expression subquery "\
"could be limited only "\
"with 1");
- sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE,
- err, P4_STATIC);
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0,
+ 0, err, P4_STATIC);
+ sqlVdbeAddOp1(v, OP_Halt, -1);
sqlVdbeResolveLabel(v, no_err);
sqlReleaseTempReg(pParse, r1);
@@ -2150,8 +2152,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
"Only positive integers are allowed "\
"in the OFFSET clause");
sqlVdbeResolveLabel(v, offset_error_label);
- sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err,
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
P4_STATIC);
+ sqlVdbeAddOp1(v, OP_Halt, -1);
sqlVdbeResolveLabel(v, positive_offset_label);
sqlReleaseTempReg(pParse, r1);
@@ -5405,10 +5408,11 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
int r1 = sqlGetTempReg(parser);
sqlVdbeAddOp2(v, OP_Integer, 0, r1);
sqlVdbeAddOp3(v, OP_Ne, r1, end_mark, limit_reg);
- const char *error = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
- "Expression subquery returned more "\
- "than 1 row");
- sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, error, P4_STATIC);
+ const char *err = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
+ "Expression subquery returned more than "
+ "1 row");
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, P4_STATIC);
+ sqlVdbeAddOp1(v, OP_Halt, -1);
sqlReleaseTempReg(parser, r1);
}
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c8887f9..ffb0df4 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -885,6 +885,21 @@ case OP_Goto: { /* jump */
goto jump_to_p2;
}
+/* Opcode: SetDiag P1 P2 * P4 *
+ *
+ * Set an error. After that jump to address P2 if P2 is not 0,
+ * else go to the next instruction.
+ *
+ * The P1 parameter is an error code to be set. The P4 parameter
+ * is a text description.
+ */
+case OP_SetDiag: { /* jump */
+ box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z);
+ if (pOp->p2 == 0)
+ break;
+ goto jump_to_p2;
+}
+
/* Opcode: Gosub P1 P2 * * *
*
* Write the current address onto register P1
@@ -989,34 +1004,27 @@ case OP_Yield: { /* in1, jump */
break;
}
-/* Opcode: Halt P1 P2 P3 P4 *
+/* Opcode: Halt P1 P2 * * *
*
- * Exit immediately. All open cursors, etc are closed
+ * Exit immediately. All open cursors, etc are closed
* automatically.
*
- * P1 is the result code returned by sql_exec(),
- * sql_reset(), or sql_finalize(). For a normal halt,
- * this should be 0.
- * For errors, it can be some other value. If P1!=0 then P2 will
- * determine whether or not to rollback the current transaction.
- * Do not rollback if P2==ON_CONFLICT_ACTION_FAIL. Do the rollback
- * if P2==ON_CONFLICT_ACTION_ROLLBACK. If
- * P2==ON_CONFLICT_ACTION_ABORT, then back out all changes that
- * have occurred during this execution of the VDBE, but do not
- * rollback the transaction.
- *
- * If P4 is not null then it is an error message string.
+ * P1 is the result code. For a normal halt, this should be 0. For
+ * errors, it is -1. If P1 != 0 then P2 will determine whether or
+ * not to rollback the current transaction. Do not rollback if
+ * P2 is ON_CONFLICT_ACTION_FAIL. Do the rollback if P2 is
+ * ON_CONFLICT_ACTION_ROLLBACK. If P2 is ON_CONFLICT_ACTION_ABORT,
+ * then back out all changes that have occurred during this
+ * execution of the VDBE, but do not rollback the transaction.
*
- * If P1 is -1 then P3 is a ClientError code and P4 is error
- * message to set.
- *
- * 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 is the same as executing Halt.
+ * There is an implied "Halt 0 0" instruction inserted at the very
+ * end of every program. So a jump past the last instruction of
+ * the program is the same as executing Halt.
*/
case OP_Halt: {
VdbeFrame *pFrame;
int pcx;
+ assert(pOp->p1 == 0 || ! diag_is_empty(diag_get()));
pcx = (int)(pOp - aOp);
if (pOp->p1 == 0 && p->pFrame != NULL) {
@@ -1045,11 +1053,6 @@ case OP_Halt: {
p->is_aborted = true;
p->errorAction = (u8)pOp->p2;
p->pc = pcx;
- if (p->is_aborted) {
- if (pOp->p4.z != NULL)
- box_error_set(__FILE__, __LINE__, pOp->p3, pOp->p4.z);
- assert(! diag_is_empty(diag_get()));
- }
sqlVdbeHalt(p);
rc = p->is_aborted ? -1 : SQL_DONE;
goto vdbe_return;
More information about the Tarantool-patches
mailing list