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 9E58A2B642 for ; Mon, 1 Oct 2018 12:13:12 -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 A3hkKw5soBWA for ; Mon, 1 Oct 2018 12:13:12 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E00EE295FD for ; Mon, 1 Oct 2018 12:13:11 -0400 (EDT) Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1g70on-00058f-R5 for tarantool-patches@freelists.org; Mon, 01 Oct 2018 19:13:10 +0300 Subject: [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes References: <7BEF7461-8C36-472F-95D7-6E46CD99E956@tarantool.org> <9C1DDEBD-C37D-4B61-96AC-832BC4DDD12A@tarantool.org> From: Alex Khatskevich Message-ID: Date: Mon, 1 Oct 2018 19:13:09 +0300 MIME-Version: 1.0 In-Reply-To: <9C1DDEBD-C37D-4B61-96AC-832BC4DDD12A@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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 >> I have expected the same thing. >> However: >> 1. sqlite works in the same way. > It doesn’t mean anything. SQLite features a wide range of really strange things. > >> 2. implementing this just need adding (nChanges) to savepoint > Seems so, but it shouldn’t be complicated. I suppose it is not complicated. >> 3. I suppose that we should not implement this. > Why? I guess it is matter of discussion. I think that it is not important, because it is relatively useless counter. 1. No one need to count exact number of changes. 2. Changing the api require solid understanding of what total_changes is     and why we need to change this.     It strange for me that it do not do rollback on rollback, however, it meant to work this     way and it also makes sense. > Anyway, I propose to decide what to do with TOTAL_CHANGES within this patch, > since original issue sounds like: > > "Fix if easy, remove otherwise.” > > Adding initially wrong tests is likely to be huge mistake. This thing works. Tests proof that. I believe that those tests are the artefact of this issue. If you want change something, you need to file an issue and explain, why this change is important.