From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 3D1AF445320 for ; Wed, 15 Jul 2020 11:11:08 +0300 (MSK) References: <1594221263-6228-1-git-send-email-alyapunov@tarantool.org> <1594221263-6228-14-git-send-email-alyapunov@tarantool.org> <85786cb3-af03-03da-0405-b137a17ce929@tarantool.org> From: Aleksandr Lyapunov Message-ID: <340f6b5a-4c5d-6d26-1268-da9a7d995ac5@tarantool.org> Date: Wed, 15 Jul 2020 11:11:06 +0300 MIME-Version: 1.0 In-Reply-To: <85786cb3-af03-03da-0405-b137a17ce929@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH 13/16] tx: introduce txm_story List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Hi, thank for review! On 15.07.2020 02:46, Vladislav Shpilevoy wrote: > Thanks for the patch! > > See 3 comments below. > > > 1. What if index count of the space will change in a concurrect > DDL transaction? That's a good question. I know what should be - DDL must create a new space structure for new transaction while leaving old space structure for old transactions.. But I'm sure it does not work that way now. I'll write a test and file a ticket. > >> +} >> + >> +static struct txm_story * >> +txm_story_new_add_stmt(struct tuple *tuple, struct txn_stmt *stmt) > 2. Probably would be better to use the existing terms: old_tuple and new_tuple. > So the functions would be txm_story_add_new_stmt and txm_story_add_old_stmt. The > same in all the other places. 'add_stmt' -> 'new_stmt'. 'del_stmt' -> 'old_stmt'. I thought about it. I fear in terms of txm_story, when we speak about lifetime of a tuple it would be weird. add_stmt - is the statement that added the tuple. But what is new_stmt? new_psn? > >> +{ >> + struct txm_story *res = txm_story_new(tuple, stmt->space->index_count); >> + if (res == NULL) >> + return NULL; >> + res->add_stmt = stmt; >> + assert(stmt->add_story == NULL); >> + stmt->add_story = res; >> + return res; >> +} >> + >> +static struct txm_story * >> +txm_story_new_del_stmt(struct tuple *tuple, struct txn_stmt *stmt) >> +{ >> + struct txm_story *res = txm_story_new(tuple, stmt->space->index_count); >> + if (res == NULL) >> + return NULL; >> + res->del_stmt = stmt; >> + assert(stmt->del_story == NULL); >> + stmt->del_story = res; >> + return res; >> +} >> + >> +void >> +txm_history_prepare_stmt(struct txn_stmt *stmt) >> +{ >> + assert(stmt->txn->psn != 0); >> + >> + /* Move story to the past to prepared stories. */ >> + >> + struct txm_story *story = stmt->add_story; >> + uint32_t index_count = story == NULL ? 0 : story->index_count; >> + for (uint32_t i = 0; i < index_count; ) { >> ... >> ... >> ... >> + } >> + if (stmt->add_story != NULL) > 3. According to a check in the beginning of the function stmt can > be NULL. But then this will crash. No, index_count is initialized as 0 in that case and we will not enter the loop.