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

[Konstantin Osipov <kostja@tarantool.org>]

* Georgy Kirichenko <georgy@tarantool.org> [18/08/28 19:20]:
> The lua space cache (box.space.*) should be valid just after space is
> created because space is ready to accept new records or does not exists
> before wal would be written. So invoke a space create/drop trigger after
> a space is changed and recall it in a case of rollback.

I understand what this patch is doing, but I don't understand why
you need it.

We need to alert the user about system events, such as newly
created spaces, including system spaces.

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? 

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?

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.

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.

> @@ -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,


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov