From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 27 Mar 2019 13:03:51 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] [PATCH v3 2/5] Synchronize lua schema update
 with space cache
Message-ID: <20190327100351.gpo2befo6mulqw2k@esperanza>
References: <cover.1553255718.git.georgy@tarantool.org>
 <2c27dc3efab7420737653d3362570136d885dbb4.1553255718.git.georgy@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2c27dc3efab7420737653d3362570136d885dbb4.1553255718.git.georgy@tarantool.org>
To: Georgy Kirichenko <georgy@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Fri, Mar 22, 2019 at 03:06:07PM +0300, Georgy Kirichenko wrote:
> Update lua schema as soon as space cache replace was done instead of
> doing this while on_commit trigger executes. In opposite then case
> schema changes would not be visible until commit was finished.
> 
> Needed for: #2798
> ---
>  src/box/alter.cc | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index daaa9cd57..275e39dd5 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -750,8 +750,6 @@ alter_space_commit(struct trigger *trigger, void *event)
>  		op->commit(alter, txn->signature);
>  	}
>  
> -	trigger_run_xc(&on_alter_space, alter->new_space);
> -
>  	alter->new_space = NULL; /* for alter_space_delete(). */
>  	/*
>  	 * Delete the old version of the space, we are not
> @@ -787,6 +785,8 @@ alter_space_rollback(struct trigger *trigger, void * /* event */)
>  	space_swap_triggers(alter->new_space, alter->old_space);
>  	space_swap_fk_constraints(alter->new_space, alter->old_space);
>  	space_cache_replace(alter->new_space, alter->old_space);
> +	trigger_run(&on_alter_space, alter->old_space);
> +
>  	alter_space_delete(alter);
>  }
>  
> @@ -888,6 +888,7 @@ alter_space_do(struct txn *txn, struct alter_space *alter)
>  	 * cache with it.
>  	 */
>  	space_cache_replace(alter->old_space, alter->new_space);
> +	trigger_run_xc(&on_alter_space, alter->new_space);

The comment right above says that this function must not throw an
exception here so I guess we should use _xc free variant. Not that it
really matters, as on_alter_space trigger can only fail on OOM, which
is hardly ever possible.

>  
>  	/*
>  	 * Install transaction commit/rollback triggers to either
> @@ -1388,7 +1389,6 @@ on_drop_space_commit(struct trigger *trigger, void *event)
>  {
>  	(void) event;
>  	struct space *space = (struct space *)trigger->data;
> -	trigger_run_xc(&on_alter_space, space);
>  	space_delete(space);
>  }
>  
> @@ -1403,6 +1403,7 @@ on_drop_space_rollback(struct trigger *trigger, void *event)
>  	(void) event;
>  	struct space *space = (struct space *)trigger->data;
>  	space_cache_replace(NULL, space);
> +	trigger_run(&on_alter_space, space);
>  }
>  
>  /**
> @@ -1412,8 +1413,7 @@ static void
>  on_create_space_commit(struct trigger *trigger, void *event)
>  {
>  	(void) event;
> -	struct space *space = (struct space *)trigger->data;
> -	trigger_run_xc(&on_alter_space, space);
> +	(void) trigger;
>  }

The function is not used now. I guess it's okay to leave it, just in
case we want to extend it in the future.

>  
>  /**
> @@ -1429,6 +1429,7 @@ on_create_space_rollback(struct trigger *trigger, void *event)
>  	(void) event;
>  	struct space *space = (struct space *)trigger->data;
>  	space_cache_replace(space, NULL);
> +	trigger_run(&on_alter_space, space);
>  	space_delete(space);
>  }
>  
> @@ -1672,6 +1673,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
>  		 * execution on a replica.
>  		 */
>  		space_cache_replace(NULL, space);
> +		trigger_run_xc(&on_alter_space, space);

Agagin, we must no throw exceptions here.

>  		/*
>  		 * Do not forget to update schema_version right after
>  		 * inserting the space to the space_cache, since no
> @@ -1764,6 +1766,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
>  		 * execution on a replica.
>  		 */
>  		space_cache_replace(old_space, NULL);
> +		trigger_run_xc(&on_alter_space, old_space);
>  		/*
>  		 * Do not forget to update schema_version right after
>  		 * deleting the space from the space_cache, since no

I think we should've fixed on_alter_sequence trigger in the scope of
this issue, too.

The patch has been pushed to master, 2.1, and 1.10 by Kostja.

I guess we can address the minor hitches I pointed out above in
a separate 'review-fix' patch.