From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 9 Apr 2018 11:24:36 +0300 From: Vladimir Davydov Subject: Re: [PATCH 12/12] vinyl: allow to modify format of non-empty spaces Message-ID: <20180409082436.uhl7j2lvcxfbkuzk@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: kostja@tarantool.org Cc: tarantool-patches@freelists.org List-ID: On Sat, Apr 07, 2018 at 04:38:09PM +0300, Vladimir Davydov wrote: > static int > vinyl_space_check_format(struct space *space, struct tuple_format *format) > { > - (void)format; > struct vy_env *env = vy_env(space->engine); > - if (space->index_count == 0) > + > + /* > + * If this is local recovery, the space was checked before > + * restart so there's nothing we need to do. > + */ > + if (env->status == VINYL_INITIAL_RECOVERY_LOCAL || > + env->status == VINYL_FINAL_RECOVERY_LOCAL) > return 0; > + > + if (space->index_count == 0) > + return 0; /* space is empty, nothing to do */ > + > + /* > + * Iterate over all tuples stored in the given space and > + * check each of them for conformity to the new format. > + * Since read iterator may yield, we install an on_replace > + * trigger to check tuples inserted after we started the > + * iteration. > + */ > struct vy_lsm *pk = vy_lsm(space->index[0]); > - if (env->status != VINYL_ONLINE) > - return 0; > - if (pk->stat.disk.count.rows == 0 && pk->stat.memory.count.rows == 0) > - return 0; > - diag_set(ClientError, ER_UNSUPPORTED, "Vinyl", > - "changing format of a non-empty space"); > - return -1; > + > + struct tuple *key = vy_stmt_new_select(pk->env->key_format, NULL, 0); > + if (key == NULL) > + return -1; > + > + struct trigger on_replace; > + struct vy_check_format_ctx ctx; > + ctx.format = format; > + ctx.is_failed = false; > + diag_create(&ctx.diag); > + trigger_create(&on_replace, vy_check_format_on_replace, &ctx, NULL); > + trigger_add(&space->on_replace, &on_replace); > + > + struct vy_read_iterator itr; > + vy_read_iterator_open(&itr, pk, NULL, ITER_ALL, key, > + &env->xm->p_global_read_view); > + int rc; > + struct tuple *tuple; > + while ((rc = vy_read_iterator_next(&itr, &tuple)) == 0) { > + if (tuple == NULL) > + break; > + if (ctx.is_failed) { > + diag_move(&ctx.diag, diag_get()); > + rc = -1; > + break; > + } This check should be before the EOF check (tuple == NULL), because even in case read iterator returned NULL it may have yield, which might have opened a time window for a concurrent fiber to insert a new tuple that doesn't match the new format. Fixed on the branch. The incremental diff is in the end of this email. > + rc = tuple_validate(format, tuple); > + if (rc != 0) > + break; > + } > + vy_read_iterator_close(&itr); > + diag_destroy(&ctx.diag); > + trigger_clear(&on_replace); > + tuple_unref(key); > + return rc; > } diff --git a/src/box/vinyl.c b/src/box/vinyl.c index c2769e6d..6bed09da 100644 --- a/src/box/vinyl.c +++ b/src/box/vinyl.c @@ -1104,13 +1104,13 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format) int rc; struct tuple *tuple; while ((rc = vy_read_iterator_next(&itr, &tuple)) == 0) { - if (tuple == NULL) - break; if (ctx.is_failed) { diag_move(&ctx.diag, diag_get()); rc = -1; break; } + if (tuple == NULL) + break; rc = tuple_validate(format, tuple); if (rc != 0) break;