From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 2E232469710 for ; Fri, 22 May 2020 06:04:45 +0300 (MSK) Date: Fri, 22 May 2020 03:04:43 +0000 From: Nikita Pettik Message-ID: <20200522030443.GB24698@tarantool.org> References: <7e71d3cd-1c50-c236-d222-95f13b24da5a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <7e71d3cd-1c50-c236-d222-95f13b24da5a@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] vinyl: fix assert in vy_tx_write() 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 21 May 16:12, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > On 21/05/2020 03:15, Nikita Pettik wrote: > > Assert in vy_tx_write() validates that upsert applied on the top of > > cached statement is always replace. In fact, it is not always so. If > > vy_apply_upsert() fails due to the fact that PK is modified, it returns > > old statement (i.e. statement which upsert is applied on). In this > > regard, if tuple cache contains insert and invalid upsert is executed, > > vy_apply_upsert() will return insert. As a result, assert will fire. > > Let's fix it and take into account that vy_apply_upsert() is able to > > return inserts as well. > > I looked at other places in vinyl, which apply an upsert, and found > vy_lsm_commit_upsert() in vy_lsm.c:1018. The result is also always > assumed to be REPLACE. Is it correct? No, it's not a case. If vy_apply_upsert() fails due to PK modification, it will return old_stmt. In turn old_stmt has different LSN, so flaw returns before this assert: 1030 if (upserted_lsn != lsn) { 1031 /** 1032 * This could only happen if the upsert completely 1033 * failed and the old tuple was returned. 1034 * In this case we shouldn't insert the same replace 1035 * again. 1036 */ 1037 assert(older.stmt == NULL || 1038 upserted_lsn == vy_stmt_lsn(older.stmt)); 1039 tuple_unref(upserted.stmt); 1040 return; 1041 } Note that they must have different LSN since they can't come in one tx set. Otherwise, during apply_upsert() in vy_tx_set_entry() upsert turns into old_stmt (insert) and simply skipped (due to is_overwritten flag) during statement commit.