From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 39DB42A847 for ; Tue, 23 Apr 2019 17:06:43 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 22C-gukDVfIu for ; Tue, 23 Apr 2019 17:06:43 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E42032A4CD for ; Tue, 23 Apr 2019 17:06:42 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 9/9] sql: make accept only boolean References: <103cef3d31c59d1b869a7675a01ed2e6279a47ef.1555252410.git.korablev@tarantool.org> <6DC565F1-23AA-49CA-BB2C-AA60EE9E3593@tarantool.org> <09F94053-CCE0-49FD-8788-3F0749BF7ED8@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4978b03d-c40e-ed4d-8aac-8567327779c3@tarantool.org> Date: Wed, 24 Apr 2019 00:06:40 +0300 MIME-Version: 1.0 In-Reply-To: <09F94053-CCE0-49FD-8788-3F0749BF7ED8@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, "n.pettik" Thanks for the fixes! On 23/04/2019 22:59, n.pettik wrote: > >> On 18/04/2019 20:55, n.pettik wrote: >>> >>>>> is a predicate used as a part of WHERE and >>>>> JOIN clauses. ANSI SQL states that must >>>>> accept only boolean arguments. In our SQL it is implemented as >>>>> bytecode instruction OP_If which in turn carries out logic of >>>>> conditional jump. Since it can be involved in executing other routines >>>>> different from , >>>> >>>> 1. Which other routines? What is a valid case of OP_If with non-boolean >>>> value in check? >>> >>> For instance, to verify that register containing LIMIT value is > 0. >> >> Yes, and this is almost the only case. What is more, it happens only once >> per request, to check if LIMIT == 0 initially. Further it is decremented >> and checked via OP_IfNotZero and OP_DecrJumpZero. >> >>> It is quite hard to track values which come to this opcode, so we >>> can’t be sure that it always accepts booleans. >> >> It is hard, but without it >> >> 1) You can't be sure, that really all the search conditions >> are checked to be booleans; >> >> 2) It makes OP_If/IfNot slower, and they are called repeatedly in >> requests; > > One branching worth nothing. Below you suggest to split opcode > into two (fix me if I’m wrong), which in turn affects performance way much more. The places which I proposed to split are called only once per request. For example, OP_IfNot with iLimit was used for initial check that it is not zero. All the next work with iLimit was being done via special opcodes OP_DecrJumpZero and OP_IfNotZero. On the other hand, if we branch inside OP_IfNot, we branch repeatedly, in cycles, many times per request. > >> 3) It adds one more flag SQL_BOOLREQ, which looks very crutchy. > > IMHO it is matter of taste. Anyway, removed this flag. You are right, it would have been, if OP_IfNot/OP_If had already had some other flags. But you proposed to change the opcodes dramatically, to add a new argument just for some minor cases. And as a result it hid a bug with CASE-WHEN search condition. >> It violates the standard. "Information technology — >> Database languages — SQL — Part 2: Foundation (SQL/Foundation)", >> 2011, page 230. >> >> 'WHEN' is a search condition, but I've used '1', not 'true'. >> Also I tested it on PostgreSQL - they raise an error, so it is >> both standard and practically used way. >> >> Below are my fixes for LIMIT and a small obvious refactoring, >> but they are *not on the branch* - not all the tests pass when I >> start banning non-bools in OP_If/IfNot. > > I’ve fixed that. But you still set OPFLAG_BOOLREQ and SQL_BOOLREQ. Why? Also the commit message still describes this flag as a key change of the patch.