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

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

On Mon, Jul 08, 2019 at 10:40:53AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/04 20:10]:
> 
> Come to think about this commit, it still raises a lot of
> questions.
> 
> What is it taken for entire alter_space_do while it protects alter_space_prepare
> only?

Simply because it's easier to write the code that way. Since we only
care about yields, the lock has no effect if no operation yields.
OTOH taking the lock early makes alter_space_do fail early in case the
space happens to be busy.

> 
> Why is it necessary to protect the yields from prepare, but not
> from WAL?

Once we submitted a request to WAL, we don't really care about
concurrent DDL modifying the space as we are done working with it.
We only care that the space doesn't go away until we run the commit
or rollback trigger. And it can't go away, because other DDL operations
will wait for WAL as well, which orders them to happen after us.

> 
> Why is not not taken when a new space is created? What happens
> when box.schema.space:create() is run concurrently from two
> fibers, would that work without a lock? Why?

We don't need to take the lock when a space is created or dropped,
because those operations never yield.

Concurrent requests are ordered by the WAL just like any other
concurrent DDL.

> 
> These questions should at least be addressed in the comments to
> the lock class. 

Urgh... Isn't it obvious?

> 
> And since the lock is wrapped around long yielding operations,
> perhaps it should be taken by objects which do these operations,
> like CheckSpaceFormat and CreateIndex (it is easy to grant the
> lock to a fiber which already owns it, so each object could be
> taken its own lock)?

I think of locks as protecting objects, not pieces of code. This
particular lock protects the space struct from getting modified by
a concurrent request. I don't see any benefit resulting from taking
it deeper in the call stack - it would only spread the logic between
different functions.