From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 3 Jul 2019 22:56:05 +0300 From: Vladimir Davydov Subject: Re: [PATCH 6/6] Replace schema lock with fine-grained locking Message-ID: <20190703195605.dhuoxv7xrxqzklug@esperanza> References: <20190703193541.GH17318@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190703193541.GH17318@atlas> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Wed, Jul 03, 2019 at 10:35:41PM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [19/07/01 10:04]: > > Now, as we don't need to take the schema lock for checkpointing, it is > > only used to synchronize concurrent space modifications (drop, truncate, > > alter). Actually, a global lock is a way too heavy means to achieve this > > goal, because we only care about forbidding concurrent modifications of > > the same space while concurrent modifications of different spaces should > > work just fine. So this patch replaces schema lock with a per-space > > flag. The flag is called is_in_alter and set by alter_space_new() and > > cleared by alter_space_delete(). If the flag is already set when > > alter_space_new() is called, an error is thrown. > > Uh-oh. > > Could you please do a bit more coding? > > There are inherent dangers in using a boolean flag rather than a > normal lock: > - lock life time is bound to object life time > - there is no way to put itself into a wait queue > - there is an implicit assumption that a fiber only takes one lock > and no way to inspect/free all locks of a fiber. > - deadlock detection is impossible. TBO I don't think you follow. It isn't a lock, actually. It's just a flag saying the space is busy building an index. If someone tries to do something with a space that is busy, they will fail. This is consistent with vinyl tx manager behavior. No deadlock is possible by design. I don't see any point implementing some kind of generic locking scheme at this point, because as I said, there are actually no locks. I think we should get to this once we start thinking about locking in the transaction manager, not now. > > Let's add a normal name-based locking for this: > > struct lock { > enum object_type object_type; > char *object_name; > enum { S, X } type; > struct lock *pending; > struct fiber *owner; // ideally it should be struct txn, or int txn_id, not struct fiber > }; > > hash metadata_locks; > > This could be a separate module in box.cc. The api should take > locks by name: > > struct lock *metadata_lock_get(enum object_type type, char > *object_name, enum lock_type type, int txn_id); > > and unlock by value or txn_id: > > void metadata_lock_free(struct lock *lock); > > void metadata_lock_free_all(int txn_id);