Re: [PATCH 6/6] Replace schema lock with fine-grained locking

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Wed, Jul 03, 2019 at 10:35:41PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [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<lock> 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);