From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 8 Jul 2019 12:29:05 +0300 From: Konstantin Osipov Subject: Re: [PATCH 1/5] vinyl: don't sync WAL on space alter if not necessary Message-ID: <20190708092904.GB8512@atlas> References: <246584ed1dc422b65ce557f2170f89b372d87c71.1562357452.git.vdavydov.dev@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <246584ed1dc422b65ce557f2170f89b372d87c71.1562357452.git.vdavydov.dev@gmail.com> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [19/07/05 23:27]: > Changes done to an altered space while a new index is being built or > the format is being checked are propagated via an on_replace trigger. > The problem is there may be transactions that started before the alter > request. Their working set can't be checked so we simply abort them. > We can't abort transactions that have reached WAL so we also call > wal_sync() to flush all pending WAL requests. This is a yielding > operation and we call it even if there's no transactions that need > to be flushed. As a result, vinyl space alter yields unconditionally, > even if the space is empty and there is no pending transactions > affecting it. This prevents us from implementing transactional DDL. > Let's call wal_sync() only if there's actually at least one pending > transaction affecting the altered space and waiting for WAL. > --- > src/box/vinyl.c | 51 ++++++++++++++++++++++++++++++--------------------- > src/box/vy_tx.c | 11 ++++++++++- > src/box/vy_tx.h | 7 ++++++- > 3 files changed, 46 insertions(+), 23 deletions(-) > > diff --git a/src/box/vinyl.c b/src/box/vinyl.c > index 128b1199..e0de65d0 100644 > --- a/src/box/vinyl.c > +++ b/src/box/vinyl.c > @@ -1052,25 +1052,8 @@ vinyl_space_invalidate(struct space *space) > * soon as it's done reading disk, which will make the DML > * request bail out early, without dereferencing the space. > */ > - tx_manager_abort_writers_for_ddl(env->xm, space); > -} Please add a comment for vinyl_space_invalidate, explaining what it is used for and who's using it, just like in the comment that you removed: > - > -/** > - * This function is called after installing on_replace trigger > - * used for propagating changes done during DDL. It aborts all > - * rw transactions affecting the given space that began > - * before the trigger was installed so that DDL doesn't miss > - * their working set. > - */ > -static void > -vy_abort_writers_for_ddl(struct vy_env *env, struct space *space) Otherwise LGTM. -- Konstantin Osipov, Moscow, Russia