From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3F46F46970E for ; Thu, 19 Dec 2019 12:06:50 +0300 (MSK) References: <20191209134851.14462-1-arkholga@tarantool.org> <20191209140728.GC8196@atlas> From: Olga Arkhangelskaia Message-ID: <4cfdd480-990b-9130-f614-9d14d1945c1b@tarantool.org> Date: Thu, 19 Dec 2019 12:06:48 +0300 MIME-Version: 1.0 In-Reply-To: <20191209140728.GC8196@atlas> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH rfc v2] memtx: fix out of memory handling for rtree List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , tarantool-patches@dev.tarantool.org Hello Konstantin, thanks for the review. It would be difficult to find right way without you. I have spent more time looking at the code and I still think that memtx_space_replace_no_keys is the best place for the check.  First of all there is no memtx_rtree_build_next, and secondly I am not sure that memtx_rtree_build_next should be implemented only with the check. On 09/12/2019 17:07, Konstantin Osipov wrote: > * Olga Arkhangelskaia [19/12/09 16:49]: >> When tarantool tries to recover rtree from a snapshot and memtx_memory value is >> lower than it has been when the snapshot was created, server suffers from >> segmentation fault. This happens because there is no out of memory error >> handling in rtree lib. In another words, we do not check the result of >> malloc operation. >> To prevent this behaviour we simply reserve memory before replace operation for >> rtree. And if there is not enough memory to be reserved - server will fail >> gently with the "Failed to allocate" error message. > It seems you're on track. You don't explain, however, why you had > to add an additional reserve() which is on the side of the main > execution flow (which is box_process1): during snapshot recovery > the secondary keys are built in batches, not using box_process1, > so the check sitting on the main execution track is not invoked. > > This begs the question: shouldn't you add the check to > memtx_*_build_next() instead? > >