From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 8 Jul 2019 12:50:42 +0300 From: Vladimir Davydov Subject: Re: [PATCH 1/5] vinyl: don't sync WAL on space alter if not necessary Message-ID: <20190708095042.ybjmcfh326qzgulk@esperanza> References: <246584ed1dc422b65ce557f2170f89b372d87c71.1562357452.git.vdavydov.dev@gmail.com> <20190708092904.GB8512@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190708092904.GB8512@atlas> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Mon, Jul 08, 2019 at 12:29:05PM +0300, Konstantin Osipov wrote: > * 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: I didn't remove any comments. I just moved a comment from vy_abort_writers_for_ddl() to vinyl_space_build_index() and vy_space_check_format(). Regarding vinyl_space_invalidate - I didn't touch it. And it seems to be pretty well commented as it is, see comments to vinyl_space_invalidate() and space_vtab.