From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Imeev Mergen <imeevma@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE Date: Mon, 15 Jul 2019 16:08:24 +0300 [thread overview] Message-ID: <4BC2F02A-F568-4061-A921-AFF7D01D8AC5@tarantool.org> (raw) In-Reply-To: <0a0f232c4c4c7068df8514230c149bc953a080a5.1562235700.git.imeevma@gmail.com> >>>>> + */ >>>>> +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. >> >> Do not understand what you mean. Could you provide >> some examples? >> > Sorry, I can't. But I have beed said so by Vlad: > > On 6/2/19 7:35 PM, Vladislav Shpilevoy wrote: >> There was a concrete reason, why it was possible - because different >> error codes have different arguments of various types, and the only way >> to set an error at parsing stage is to allow to set arbitrary error >> message to any error code. Without '...', va_arg etc. Besides, we could >> use it to set correct line number and function name in future. Now you >> use diag_set, which restricts us. >> >> So why do you need that patch? We will need to revert it when an error >> appears requiring more than one argument, or an argument of not >> const char * type. That will definitely happen. >> > > It can be ween here: > https://www.freelists.org/post/tarantool-patches/PATCH-v1-49-sql-rework-diag-set-in-OP-Halt,1 Ok, now it’s clear: it allows us to construct string containing error message during parsing stage, so that we avoid argument substitution in diag_set(). It is vital since "error codes have different arguments of various types”. >>> 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); >> >> Are these manipulations with labels related to patch? >> ‘label’ or ‘addr' are more appropriate names IMHO. >> > I think that these manipulations should be done, since in this > patch we should jump to different lengths depending on the value > of no_error. This will be even more severe after the next patch. Ok, now I see. This patch LGTM.
next prev parent reply other threads:[~2019-07-15 13:08 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-04 10:49 [tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed imeevma 2019-07-04 10:49 ` [tarantool-patches] [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE imeevma 2019-07-15 13:08 ` n.pettik [this message] 2019-07-04 10:49 ` [tarantool-patches] [PATCH v3 2/3] sql: clean-up in case constraint creation failed imeevma 2019-07-04 10:49 ` [tarantool-patches] [PATCH v3 3/3] sql: use common registers instead of temp. for constraints data imeevma 2019-07-15 13:13 ` [tarantool-patches] " n.pettik 2019-07-18 4:24 ` [tarantool-patches] Re: [PATCH v3 0/3] sql: clean-up in case constraint creation failed 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=4BC2F02A-F568-4061-A921-AFF7D01D8AC5@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE' \ /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