[Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails
Nikita Pettik
korablev at tarantool.org
Fri Apr 10 18:47:13 MSK 2020
On 10 Apr 17:13, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 2 comments below.
>
> > diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> > index 7a6a20627..f6e6ed4d2 100644
> > --- a/src/box/vy_write_iterator.c
> > +++ b/src/box/vy_write_iterator.c
> > @@ -961,8 +961,11 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
> > size_t used = region_used(region);
> > stream->rv_used_count = 0;
> > if (vy_write_iterator_build_history(stream, &raw_count,
> > - &is_first_insert) != 0)
> > + &is_first_insert) != 0) {
> > + for (int i = 0; i < stream->rv_count; ++i)
> > + stream->read_views[i].history = NULL;
> > goto error;
>
> 1. Kostja probably meant encapsulation of function
> vy_write_iterator_build_history(). Currently that function
> leaves garbage in case of a fail. Therefore it becomes not
> self sufficient. Better cleanup the views in the same place
> where they are created - inside build_history().
>
> Imagine, if build_history() would be called not in a single
> place. We would need to cleanup its garbage in each one.
Ah, of course I agree. Applied.
> Diff below and attached as a file.
> ====================
> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index f6e6ed4d2..7d8c60d73 100644
> --- a/src/box/vy_write_iterator.c
> +++ b/src/box/vy_write_iterator.c
> @@ -790,8 +790,11 @@ next_lsn:
> * statement around if this is major compaction, because
> * there's no tuple it could overwrite.
> */
> - if (rc == 0 && stream->is_last_level &&
> - stream->deferred_delete_stmt != NULL) {
> + if (rc != 0) {
> + for (int i = 0; i < stream->rv_count; ++i)
> + stream->read_views[i].history = NULL;
> + } else if (stream->is_last_level &&
> + stream->deferred_delete_stmt != NULL) {
> vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
> stream->deferred_delete_stmt = NULL;
> }
> @@ -961,11 +964,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
> size_t used = region_used(region);
> stream->rv_used_count = 0;
> if (vy_write_iterator_build_history(stream, &raw_count,
> - &is_first_insert) != 0) {
> - for (int i = 0; i < stream->rv_count; ++i)
> - stream->read_views[i].history = NULL;
> + &is_first_insert) != 0)
> goto error;
> - }
> if (raw_count == 0) {
> /* A key is fully optimized. */
> region_truncate(region, used);
> ====================
>
> 2. I rolled back the patch and run the test - it passed. What can be a reason?
Hm, it fails not each run, but like every second/third execution.
I guess it depends on state of memory which has been truncated
(position of particular slabs in area etc). Not sure if we can
affect it.
> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index f6e6ed4d2..7d8c60d73 100644
> --- a/src/box/vy_write_iterator.c
> +++ b/src/box/vy_write_iterator.c
> @@ -790,8 +790,11 @@ next_lsn:
> * statement around if this is major compaction, because
> * there's no tuple it could overwrite.
> */
> - if (rc == 0 && stream->is_last_level &&
> - stream->deferred_delete_stmt != NULL) {
> + if (rc != 0) {
> + for (int i = 0; i < stream->rv_count; ++i)
> + stream->read_views[i].history = NULL;
> + } else if (stream->is_last_level &&
> + stream->deferred_delete_stmt != NULL) {
> vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
> stream->deferred_delete_stmt = NULL;
> }
> @@ -961,11 +964,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
> size_t used = region_used(region);
> stream->rv_used_count = 0;
> if (vy_write_iterator_build_history(stream, &raw_count,
> - &is_first_insert) != 0) {
> - for (int i = 0; i < stream->rv_count; ++i)
> - stream->read_views[i].history = NULL;
> + &is_first_insert) != 0)
> goto error;
> - }
> if (raw_count == 0) {
> /* A key is fully optimized. */
> region_truncate(region, used);
More information about the Tarantool-patches
mailing list