[tarantool-patches] Re: [PATCH 1/2] Update lua space cache just after creation

[Georgy Kirichenko <georgy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

On Thursday, August 30, 2018 3:31:07 PM MSK Konstantin Osipov wrote:

> We can well use txn->on_commit trigger for this purpose, having
> set an on_commit trigger which pushes on_ctl events. This trigger
> is invoked *after* a record is written to wal in txn.cc. Why do
> you need to manipulate with the internal alter.cc triggers at all?
> Why is the order of invocation of these triggers important?
No, we can't, because generally there may be a space writes between space
 was created and space was commited. It is only current implementation issue  
that index creation acquires a ddl lock and stops applying.
> 
> Ideally, your on_ctl trigger should be invoked after all the alter
> triggers ahve been invoked. For that, all you need to do is to
> ensure that on_ctl trigger is added *after* the alter trigger -
> i.e. it has a lower priority/order than alter trigger. Is your
> problem with trigger ordering? Is it that you've been trying to
> add alter trigger *after* you added the on_ctl trigger, and, since
> triggers are normally added to the end of the list, when on_ctl
> trigger was invoked the space was not yet available in box.space?
My problem I can't see lua space object for a created space while on_replace 
fires because on_replace trigger fires at operation stage and lua cache updates 
only after commit - this is the issue. This can't be solved with trigger 
ordering cos triggers are running on different stages and the only way i see 
is to move space cache update trigger to the operation stage.
> 
> If it is the case, then I think we should simply add
> trigger_add_first() function, and use this function to add alter
> triggers to the beginning of the list.
First we should change stage of lua cache updates, that the patch does.
> 
> That would solve the problem until we have more strict ordering
> requirements, at which point we could add trigger->order member
> and trigger_add_with_order() method.
> 
> Finally, I don't see how the test case is testing the order. I
> believe the test case should pass both before and after the patch.
Test uses fiber scheduling mechanism, the first fiber creates the second one that 
creates a space and yields on wal. The first fiber checks that corresponding 
space objects is visible (the second fiber still waiting for a wal).
> 
> > @@ -1571,3 +1571,64 @@ fio = require('fio')
> > 
> >  box.space.test:drop()
> >  ---
> >  ...
> > 
> > +-- allocate a space id to prevent max space id update
> 
> Why is test description missing in the result file?
> 
> 
> Nitpick: please add leading -- and trailing -- to the test
> description, start it with a capital letter and end with a dot.
> 
> 
> Thanks for the patch,


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]