From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 3 Jul 2019 23:00:32 +0300 From: Vladimir Davydov Subject: Re: [PATCH 02/10] ddl: synchronize user cache with actual data state Message-ID: <20190703200032.hlbegritnbzbobdn@esperanza> References: <7d3e055e64ee3de2fb6486e4615849574b569bf4.1562181197.git.vdavydov.dev@gmail.com> <20190703194322.GJ17318@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190703194322.GJ17318@atlas> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Wed, Jul 03, 2019 at 10:43:22PM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [19/07/03 22:34]: > > To implement transactional DDL, we must make sure that in-memory schema > > is updated synchronously with system space updates, i.e. on_replace, not > > on_commit. > > > > See also commit 22bedebe715c ("ddl: synchronize privileges cache with > > actual data state"). > > Here's a problem with replacing before commit: > > Imagine you have a yielding transaction, which is in progress. > > You replace state in the metadata cache, it doesn't matter what > cache - user, view, space, trigger, or whatever. > > Then you yield on DDL. If we yield from a DDL transaction, we must invoke on_rollback triggers immediately. I don't see any problem with that. If we don't do that now, I'll change that soon enough. > > The yielding transaction kicks-in and does some work, seeing the > dirty state. But doesn't commit yet. If on_rollback triggers are run right upon a yield, nobody will see a dirty state. > > Then you roll back. > > Your yielding transaction isolation is violated. > > Despite this gap I think we should proceed along this path. But we > also need to make sure all caches are multi-versioned: when you > replace a value in the cache, only a new transaction should see > the new value, the old one shouldn't. > > This looks fairly straightforward to implement, by adding a > schema_version to each cacheable schema object and adding > schema_version to struct txn as well. > > Now that we have killed implicit transaction start and separated > txn from fiber, we can start txn whenever we want and assign it a > schema version. > > So the patch is basically LGTM, but please be aware that moving > the changes in the cache from on_commit to on_replace breaks > yielding transaction isolation. > > I just asked myself, why did I do it this way in the first place, > and recalled the above argument.