From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 A4FA9469710 for ; Wed, 27 May 2020 23:10:09 +0300 (MSK) Date: Wed, 27 May 2020 20:10:08 +0000 From: Nikita Pettik Message-ID: <20200527201008.GD17325@tarantool.org> References: <0bdc1b5e-82b4-2e24-ad06-26587b007742@tarantool.org> <20200522024206.GA24698@tarantool.org> <6709ad16-8dae-bdc9-fb7b-b63cbb881e4a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <6709ad16-8dae-bdc9-fb7b-b63cbb881e4a@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 0/2] Validate result of upserts squash & skip invalid upserts which can't be applied List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 26 May 23:33, Vladislav Shpilevoy wrote: > Thanks for the patch, nice bug you found! > > > Author: Nikita Pettik > > Date: Fri May 22 04:38:45 2020 +0300 > > > > vinyl: handle invalid upserts in background squash process > > > > When L0 collects more than certain number of upserts (to be more precise > > VY_UPSERT_THRESHOLD == 128), background process is launched to squash > > them. During this squash, new statements could be inserted into L0. > > Until committed prepared statements feature lsn = MAX_LSN as a special > > marker. > > Did you mean 'until' -> 'while'? > > > It is assumed that all upserts will be successfully squashed. As > > a result statements with given key in L0 will be able to have only > > MAX_LSN lsn (except for the squash result). > > Honestly, I didn't understand anything from this. I got more info from > reading squashing code source. Probably this needs some rewording. For > example, in the second sentence say 'After squash it is expected all > newer statements on top of the squashed tuple have MAX_LSN, i.e. they > are prepared but not committed. Otherwise they would end up being squashed > too.' Ok, sorry, probably it was really messy one. Look at this version: vinyl: handle invalid upserts in background squash process When L0 collects more than certain number of upserts (to be more precise VY_UPSERT_THRESHOLD == 128), background process is launched to squash them. Squash process does not prevent from handling new statements being inserted in L0. Prepared (but still not committed) statements feature lsn >= MAX_LSN as a special marker. It is assumed (by mistake) that all upserts will be successfully squashed. According to this assumption, the previous statement to squash result is considered to have lsn >= MAX_LSN (as it is prepared but still not committed; if it has been already committed then it is affected by squash process). Obviously, it is not so if squashing fails. In this case upserts which are not squashed remain in L0 but have normal (i.e. < MAX_LSN) lsn. So the assertion which verifies that previous statements are prepared but not committed is wrong. Moreover, if we execute enough (more than threshold limit) invalid upserts, they will prevent from squashing valid upserts. So let's skip invalid upserts and don't account them while calculating number of upserts residing in L0. Follow-up #1622 > > > However, it is not so if at > > least one upsert is failed to be applied: it will get stack in L0 until > > stack -> stuck? The same in the code comments. Fixed. > > diff --git a/src/box/vinyl.c b/src/box/vinyl.c > > index 55a4c8fb7..b39e3b9d8 100644 > > --- a/src/box/vinyl.c > > +++ b/src/box/vinyl.c > > @@ -3539,10 +3539,21 @@ vy_squash_process(struct vy_squash *squash) > > if (vy_tuple_compare(result, mem_stmt, lsm->cmp_def) != 0 || > > vy_stmt_type(mem_stmt) != IPROTO_UPSERT) > > break; > > - assert(vy_stmt_lsn(mem_stmt) >= MAX_LSN); > > - vy_stmt_set_n_upserts((struct tuple *)mem_stmt, n_upserts); > > - if (n_upserts <= VY_UPSERT_THRESHOLD) > > - ++n_upserts; > > + /* > > + * Upsert was not applied in vy_history_apply(), > > + * but it still resides in tree memory. > > Could we remove these upserts from memory right away? The code below was not > simple beforehand, and now looks even more complex. I am looking for a way > to simplify it anyhow. > > Could vy_tx_write() fix help? If you would skip them there instead of writing > them to the tree anyway. Or is it something totally not related? These invalid upserts are already committed and residing in tree. The only way to delete them is to generate DELETE statement and insert it to the tree. I guess I can try this approach (I considered it while was preparing this patch) but code definitely won't become simpler (that's why I've choosen current one).