From ed7d2f4770659be207c0d512bceb337aa18c5527 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 8 Aug 2024 16:06:19 +0200 Subject: reftable/stack: refactor function to gather table sizes Refactor the function that gathers table sizes to be more idiomatic. For one, use `REFTABLE_CALLOC_ARRAY()` instead of `reftable_calloc()`. Second, avoid using an integer to iterate through the tables in the reftable stack given that `stack_len` itself is using a `size_t`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/stack.c b/reftable/stack.c index 737591125e..ba8234b486 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1305,14 +1305,15 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n, static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st) { - uint64_t *sizes = - reftable_calloc(st->merged->stack_len, sizeof(*sizes)); int version = (st->opts.hash_id == GIT_SHA1_FORMAT_ID) ? 1 : 2; int overhead = header_size(version) - 1; - int i = 0; - for (i = 0; i < st->merged->stack_len; i++) { + uint64_t *sizes; + + REFTABLE_CALLOC_ARRAY(sizes, st->merged->stack_len); + + for (size_t i = 0; i < st->merged->stack_len; i++) sizes[i] = st->readers[i]->size - overhead; - } + return sizes; } -- cgit 1.2.3-korg From 5f0ed603a1653f2394c468814bde4b0dca2cff45 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 8 Aug 2024 16:06:34 +0200 Subject: reftable/stack: update stats on failed full compaction When auto-compaction fails due to a locking error, we update the statistics to indicate this failure. We're not doing the same when performing a full compaction. Fix this inconsistency by using `stack_compact_range_stats()`, which handles the stat update for us. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 14 +++++++------- reftable/stack_test.c | 3 +-- 2 files changed, 8 insertions(+), 9 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/stack.c b/reftable/stack.c index ba8234b486..e5959d2c76 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1205,13 +1205,6 @@ static int stack_compact_range(struct reftable_stack *st, return err; } -int reftable_stack_compact_all(struct reftable_stack *st, - struct reftable_log_expiry_config *config) -{ - return stack_compact_range(st, 0, st->merged->stack_len ? - st->merged->stack_len - 1 : 0, config); -} - static int stack_compact_range_stats(struct reftable_stack *st, size_t first, size_t last, struct reftable_log_expiry_config *config) @@ -1222,6 +1215,13 @@ static int stack_compact_range_stats(struct reftable_stack *st, return err; } +int reftable_stack_compact_all(struct reftable_stack *st, + struct reftable_log_expiry_config *config) +{ + size_t last = st->merged->stack_len ? st->merged->stack_len - 1 : 0; + return stack_compact_range_stats(st, 0, last, config); +} + static int segment_size(struct segment *s) { return s->end - s->start; diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 1d109933d3..3ed8e44924 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -1005,8 +1005,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void) */ err = reftable_stack_compact_all(st, NULL); EXPECT(err == REFTABLE_LOCK_ERROR); - /* TODO: this is wrong, we should get notified about the failure. */ - EXPECT(st->stats.failures == 0); + EXPECT(st->stats.failures == 1); EXPECT(st->merged->stack_len == 3); reftable_stack_destroy(st); -- cgit 1.2.3-korg From 558f6fbeb1f194116231218b3e7496ceef4b9618 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 8 Aug 2024 16:06:39 +0200 Subject: reftable/stack: simplify tracking of table locks When compacting tables, we store the locks of all tables we are about to compact in the `table_locks` array. As we currently only ever compact all tables in the user-provided range or none, we simply track those locks via the indices of the respective tables in the merged stack. This is about to change though, as we will introduce a mode where auto compaction gracefully handles the case of already-locked files. In this case, it may happen that we only compact a subset of the user-supplied range of tables. In this case, the indices will not necessarily match the lock indices anymore. Refactor the code such that we track the number of locks via a separate variable. The resulting code is expected to perform the same, but will make it easier to perform the described change. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/stack.c b/reftable/stack.c index e5959d2c76..07e7ffc6b9 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1016,7 +1016,7 @@ static int stack_compact_range(struct reftable_stack *st, struct lock_file *table_locks = NULL; struct tempfile *new_table = NULL; int is_empty_table = 0, err = 0; - size_t i; + size_t i, nlocks = 0; if (first > last || (!expiry && first == last)) { err = 0; @@ -1051,7 +1051,7 @@ static int stack_compact_range(struct reftable_stack *st, for (i = first; i <= last; i++) { stack_filename(&table_name, st, reader_name(st->readers[i])); - err = hold_lock_file_for_update(&table_locks[i - first], + err = hold_lock_file_for_update(&table_locks[nlocks], table_name.buf, LOCK_NO_DEREF); if (err < 0) { if (errno == EEXIST) @@ -1066,7 +1066,7 @@ static int stack_compact_range(struct reftable_stack *st, * run into file descriptor exhaustion when we compress a lot * of tables. */ - err = close_lock_file_gently(&table_locks[i - first]); + err = close_lock_file_gently(&table_locks[nlocks++]); if (err < 0) { err = REFTABLE_IO_ERROR; goto done; @@ -1183,8 +1183,8 @@ static int stack_compact_range(struct reftable_stack *st, * Delete the old tables. They may still be in use by concurrent * readers, so it is expected that unlinking tables may fail. */ - for (i = first; i <= last; i++) { - struct lock_file *table_lock = &table_locks[i - first]; + for (i = 0; i < nlocks; i++) { + struct lock_file *table_lock = &table_locks[i]; char *table_path = get_locked_file_path(table_lock); unlink(table_path); free(table_path); @@ -1192,8 +1192,8 @@ static int stack_compact_range(struct reftable_stack *st, done: rollback_lock_file(&tables_list_lock); - for (i = first; table_locks && i <= last; i++) - rollback_lock_file(&table_locks[i - first]); + for (i = 0; table_locks && i < nlocks; i++) + rollback_lock_file(&table_locks[i]); reftable_free(table_locks); delete_tempfile(&new_table); -- cgit 1.2.3-korg From 7ee307da1bfe3867f91fbd9a053494bc5fe61675 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 8 Aug 2024 16:06:44 +0200 Subject: reftable/stack: do not die when fsyncing lock file files We use `fsync_component_or_die()` when committing an addition to the "tables.list" lock file, which unsurprisingly dies in case the fsync fails. Given that this is part of the reftable library, we should never die and instead let callers handle the error. Adapt accordingly and use `fsync_component()` instead. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/stack.c b/reftable/stack.c index 07e7ffc6b9..9ca549294f 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -674,8 +674,11 @@ int reftable_addition_commit(struct reftable_addition *add) goto done; } - fsync_component_or_die(FSYNC_COMPONENT_REFERENCE, lock_file_fd, - get_tempfile_path(add->lock_file)); + err = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd); + if (err < 0) { + err = REFTABLE_IO_ERROR; + goto done; + } err = rename_tempfile(&add->lock_file, add->stack->list_file); if (err < 0) { -- cgit 1.2.3-korg From 128b9aa3e9d3dc0417f8f65a240aad736db97959 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 8 Aug 2024 16:06:49 +0200 Subject: reftable/stack: use lock_file when adding table to "tables.list" When modifying "tables.list", we need to lock the list before updating it to ensure that no concurrent writers modify the list at the same point in time. While we do this via the `lock_file` subsystem when compacting the stack, we manually handle the lock when adding a new table to it. While not wrong, it is at least inconsistent. Refactor the code to consistently lock "tables.list" via the `lock_file` subsytem. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/stack.c b/reftable/stack.c index 9ca549294f..54982e0f7d 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -567,7 +567,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max) } struct reftable_addition { - struct tempfile *lock_file; + struct lock_file tables_list_lock; struct reftable_stack *stack; char **new_tables; @@ -581,13 +581,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add, struct reftable_stack *st) { struct strbuf lock_file_name = STRBUF_INIT; - int err = 0; - add->stack = st; + int err; - strbuf_addf(&lock_file_name, "%s.lock", st->list_file); + add->stack = st; - add->lock_file = create_tempfile(lock_file_name.buf); - if (!add->lock_file) { + err = hold_lock_file_for_update(&add->tables_list_lock, st->list_file, + LOCK_NO_DEREF); + if (err < 0) { if (errno == EEXIST) { err = REFTABLE_LOCK_ERROR; } else { @@ -596,7 +596,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add, goto done; } if (st->opts.default_permissions) { - if (chmod(add->lock_file->filename.buf, st->opts.default_permissions) < 0) { + if (chmod(get_lock_file_path(&add->tables_list_lock), + st->opts.default_permissions) < 0) { err = REFTABLE_IO_ERROR; goto done; } @@ -635,7 +636,7 @@ static void reftable_addition_close(struct reftable_addition *add) add->new_tables_len = 0; add->new_tables_cap = 0; - delete_tempfile(&add->lock_file); + rollback_lock_file(&add->tables_list_lock); strbuf_release(&nm); } @@ -651,7 +652,7 @@ void reftable_addition_destroy(struct reftable_addition *add) int reftable_addition_commit(struct reftable_addition *add) { struct strbuf table_list = STRBUF_INIT; - int lock_file_fd = get_tempfile_fd(add->lock_file); + int lock_file_fd = get_lock_file_fd(&add->tables_list_lock); int err = 0; size_t i; @@ -680,7 +681,7 @@ int reftable_addition_commit(struct reftable_addition *add) goto done; } - err = rename_tempfile(&add->lock_file, add->stack->list_file); + err = commit_lock_file(&add->tables_list_lock); if (err < 0) { err = REFTABLE_IO_ERROR; goto done; -- cgit 1.2.3-korg From ed1ad6b44deaf5089c64e0fbcae43a37d5cf666a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 8 Aug 2024 16:06:53 +0200 Subject: reftable/stack: fix corruption on concurrent compaction The locking employed by compaction uses the following schema: 1. Lock "tables.list" and verify that it matches the version we have loaded in core. 2. Lock each of the tables in the user-supplied range of tables that we are supposed to compact. These locks prohibit any concurrent process to compact those tables while we are doing that. 3. Unlock "tables.list". This enables concurrent processes to add new tables to the stack, but also allows them to compact tables outside of the range of tables that we have locked. 4. Perform the compaction. 5. Lock "tables.list" again. 6. Move the compacted table into place. 7. Write the new order of tables, including the compacted table, into the lockfile. 8. Commit the lockfile into place. Letting concurrent processes modify the "tables.list" file while we are doing the compaction is very much part of the design and thus expected. After all, it may take some time to compact tables in the case where we are compacting a lot of very large tables. But there is a bug in the code. Suppose we have two processes which are compacting two slices of the table. Given that we lock each of the tables before compacting them, we know that the slices must be disjunct from each other. But regardless of that, compaction performed by one process will always impact what the other process needs to write to the "tables.list" file. Right now, we do not check whether the "tables.list" has been changed after we have locked it for the second time in (5). This has the consequence that we will always commit the old, cached in-core tables to disk without paying to respect what the other process has written. This scenario would then lead to data loss and corruption. This can even happen in the simpler case of one compacting process and one writing process. The newly-appended table by the writing process would get discarded by the compacting process because it never sees the new table. Fix this bug by re-checking whether our stack is still up to date after locking for the second time. If it isn't, then we adjust the indices of tables to replace in the updated stack. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 102 insertions(+), 5 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/stack.c b/reftable/stack.c index 54982e0f7d..3f13c3eb34 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1020,7 +1020,9 @@ static int stack_compact_range(struct reftable_stack *st, struct lock_file *table_locks = NULL; struct tempfile *new_table = NULL; int is_empty_table = 0, err = 0; + size_t first_to_replace, last_to_replace; size_t i, nlocks = 0; + char **names = NULL; if (first > last || (!expiry && first == last)) { err = 0; @@ -1123,6 +1125,100 @@ static int stack_compact_range(struct reftable_stack *st, } } + /* + * As we have unlocked the stack while compacting our slice of tables + * it may have happened that a concurrently running process has updated + * the stack while we were compacting. In that case, we need to check + * whether the tables that we have just compacted still exist in the + * stack in the exact same order as we have compacted them. + * + * If they do exist, then it is fine to continue and replace those + * tables with our compacted version. If they don't, then we need to + * abort. + */ + err = stack_uptodate(st); + if (err < 0) + goto done; + if (err > 0) { + ssize_t new_offset = -1; + int fd; + + fd = open(st->list_file, O_RDONLY); + if (fd < 0) { + err = REFTABLE_IO_ERROR; + goto done; + } + + err = fd_read_lines(fd, &names); + close(fd); + if (err < 0) + goto done; + + /* + * Search for the offset of the first table that we have + * compacted in the updated "tables.list" file. + */ + for (size_t i = 0; names[i]; i++) { + if (strcmp(names[i], st->readers[first]->name)) + continue; + + /* + * We have found the first entry. Verify that all the + * subsequent tables we have compacted still exist in + * the modified stack in the exact same order as we + * have compacted them. + */ + for (size_t j = 1; j < last - first + 1; j++) { + const char *old = first + j < st->merged->stack_len ? + st->readers[first + j]->name : NULL; + const char *new = names[i + j]; + + /* + * If some entries are missing or in case the tables + * have changed then we need to bail out. Again, this + * shouldn't ever happen because we have locked the + * tables we are compacting. + */ + if (!old || !new || strcmp(old, new)) { + err = REFTABLE_OUTDATED_ERROR; + goto done; + } + } + + new_offset = i; + break; + } + + /* + * In case we didn't find our compacted tables in the stack we + * need to bail out. In theory, this should have never happened + * because we locked the tables we are compacting. + */ + if (new_offset < 0) { + err = REFTABLE_OUTDATED_ERROR; + goto done; + } + + /* + * We have found the new range that we want to replace, so + * let's update the range of tables that we want to replace. + */ + first_to_replace = new_offset; + last_to_replace = last + (new_offset - first); + } else { + /* + * `fd_read_lines()` uses a `NULL` sentinel to indicate that + * the array is at its end. As we use `free_names()` to free + * the array, we need to include this sentinel value here and + * thus have to allocate `stack_len + 1` many entries. + */ + REFTABLE_CALLOC_ARRAY(names, st->merged->stack_len + 1); + for (size_t i = 0; i < st->merged->stack_len; i++) + names[i] = xstrdup(st->readers[i]->name); + first_to_replace = first; + last_to_replace = last; + } + /* * If the resulting compacted table is not empty, then we need to move * it into place now. @@ -1145,12 +1241,12 @@ static int stack_compact_range(struct reftable_stack *st, * have just written. In case the compacted table became empty we * simply skip writing it. */ - for (i = 0; i < first; i++) - strbuf_addf(&tables_list_buf, "%s\n", st->readers[i]->name); + for (i = 0; i < first_to_replace; i++) + strbuf_addf(&tables_list_buf, "%s\n", names[i]); if (!is_empty_table) strbuf_addf(&tables_list_buf, "%s\n", new_table_name.buf); - for (i = last + 1; i < st->merged->stack_len; i++) - strbuf_addf(&tables_list_buf, "%s\n", st->readers[i]->name); + for (i = last_to_replace + 1; names[i]; i++) + strbuf_addf(&tables_list_buf, "%s\n", names[i]); err = write_in_full(get_lock_file_fd(&tables_list_lock), tables_list_buf.buf, tables_list_buf.len); @@ -1203,9 +1299,10 @@ static int stack_compact_range(struct reftable_stack *st, delete_tempfile(&new_table); strbuf_release(&new_table_name); strbuf_release(&new_table_path); - strbuf_release(&tables_list_buf); strbuf_release(&table_name); + free_names(names); + return err; } -- cgit 1.2.3-korg From f234df07f66c8f67391cc8ded5ade43b8a4428b6 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 8 Aug 2024 16:06:58 +0200 Subject: reftable/stack: handle locked tables during auto-compaction When compacting tables, it may happen that we want to compact a set of tables which are already locked by a concurrent process that compacts them. In the case where we wanted to perform a full compaction of all tables it is sensible to bail out in this case, as we cannot fulfill the requested action. But when performing auto-compaction it isn't necessarily in our best interest of us to abort the whole operation. For example, due to the geometric compacting schema that we use, it may be that process A takes a lot of time to compact the bulk of all tables whereas process B appends a bunch of new tables to the stack. B would in this case also notice that it has to compact the tables that process A is compacting already and thus also try to compact the same range, probably including the new tables it has appended. But because those tables are locked already, it will fail and thus abort the complete auto-compaction. The consequence is that the stack will grow longer and longer while A isn't yet done with compaction, which will lead to a growing performance impact. Instead of aborting auto-compaction altogether, let's gracefully handle this situation by instead compacting tables which aren't locked. To do so, instead of locking from the beginning of the slice-to-be-compacted, we start locking tables from the end of the slice. Once we hit the first table that is locked already, we abort. If we succeeded to lock two or more tables, then we simply reduce the slice of tables that we're about to compact to those which we managed to lock. This ensures that we can at least make some progress for compaction in said scenario. It also helps in other scenarios, like for example when a process died and left a stale lockfile behind. In such a case we can at least ensure some compaction on a best-effort basis. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 59 ++++++++++++++++++++++++++++++++++++++-------- reftable/stack_test.c | 12 ++++++---- t/t0610-reftable-basics.sh | 21 +++++++++++------ 3 files changed, 70 insertions(+), 22 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/stack.c b/reftable/stack.c index 3f13c3eb34..2071e428a8 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -999,6 +999,15 @@ static int stack_write_compact(struct reftable_stack *st, return err; } +enum stack_compact_range_flags { + /* + * Perform a best-effort compaction. That is, even if we cannot lock + * all tables in the specified range, we will try to compact the + * remaining slice. + */ + STACK_COMPACT_RANGE_BEST_EFFORT = (1 << 0), +}; + /* * Compact all tables in the range `[first, last)` into a single new table. * @@ -1010,7 +1019,8 @@ static int stack_write_compact(struct reftable_stack *st, */ static int stack_compact_range(struct reftable_stack *st, size_t first, size_t last, - struct reftable_log_expiry_config *expiry) + struct reftable_log_expiry_config *expiry, + unsigned int flags) { struct strbuf tables_list_buf = STRBUF_INIT; struct strbuf new_table_name = STRBUF_INIT; @@ -1052,19 +1062,47 @@ static int stack_compact_range(struct reftable_stack *st, /* * Lock all tables in the user-provided range. This is the slice of our * stack which we'll compact. + * + * Note that we lock tables in reverse order from last to first. The + * intent behind this is to allow a newer process to perform best + * effort compaction of tables that it has added in the case where an + * older process is still busy compacting tables which are preexisting + * from the point of view of the newer process. */ REFTABLE_CALLOC_ARRAY(table_locks, last - first + 1); - for (i = first; i <= last; i++) { - stack_filename(&table_name, st, reader_name(st->readers[i])); + for (i = last + 1; i > first; i--) { + stack_filename(&table_name, st, reader_name(st->readers[i - 1])); err = hold_lock_file_for_update(&table_locks[nlocks], table_name.buf, LOCK_NO_DEREF); if (err < 0) { - if (errno == EEXIST) + /* + * When the table is locked already we may do a + * best-effort compaction and compact only the tables + * that we have managed to lock so far. This of course + * requires that we have been able to lock at least two + * tables, otherwise there would be nothing to compact. + * In that case, we return a lock error to our caller. + */ + if (errno == EEXIST && last - (i - 1) >= 2 && + flags & STACK_COMPACT_RANGE_BEST_EFFORT) { + err = 0; + /* + * The subtraction is to offset the index, the + * addition is to only compact up to the table + * of the preceding iteration. They obviously + * cancel each other out, but that may be + * non-obvious when it was omitted. + */ + first = (i - 1) + 1; + break; + } else if (errno == EEXIST) { err = REFTABLE_LOCK_ERROR; - else + goto done; + } else { err = REFTABLE_IO_ERROR; - goto done; + goto done; + } } /* @@ -1308,9 +1346,10 @@ static int stack_compact_range(struct reftable_stack *st, static int stack_compact_range_stats(struct reftable_stack *st, size_t first, size_t last, - struct reftable_log_expiry_config *config) + struct reftable_log_expiry_config *config, + unsigned int flags) { - int err = stack_compact_range(st, first, last, config); + int err = stack_compact_range(st, first, last, config, flags); if (err == REFTABLE_LOCK_ERROR) st->stats.failures++; return err; @@ -1320,7 +1359,7 @@ int reftable_stack_compact_all(struct reftable_stack *st, struct reftable_log_expiry_config *config) { size_t last = st->merged->stack_len ? st->merged->stack_len - 1 : 0; - return stack_compact_range_stats(st, 0, last, config); + return stack_compact_range_stats(st, 0, last, config, 0); } static int segment_size(struct segment *s) @@ -1427,7 +1466,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st) reftable_free(sizes); if (segment_size(&seg) > 0) return stack_compact_range_stats(st, seg.start, seg.end - 1, - NULL); + NULL, STACK_COMPACT_RANGE_BEST_EFFORT); return 0; } diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 3ed8e44924..8c36590ff0 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -917,13 +917,15 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void) write_file_buf(buf.buf, "", 0); /* - * Ideally, we'd handle the situation where any of the tables is locked - * gracefully. We don't (yet) do this though and thus fail. + * When parts of the stack are locked, then auto-compaction does a best + * effort compaction of those tables which aren't locked. So while this + * would in theory compact all tables, due to the preexisting lock we + * only compact the newest two tables. */ err = reftable_stack_auto_compact(st); - EXPECT(err == REFTABLE_LOCK_ERROR); - EXPECT(st->stats.failures == 1); - EXPECT(st->merged->stack_len == 5); + EXPECT_ERR(err); + EXPECT(st->stats.failures == 0); + EXPECT(st->merged->stack_len == 4); reftable_stack_destroy(st); strbuf_release(&buf); diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index b06c46999d..37510c2b2a 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -478,19 +478,26 @@ test_expect_success "$command: auto compaction" ' test_oid blob17_2 | git hash-object -w --stdin && - # Lock all tables write some refs. Auto-compaction will be - # unable to compact tables and thus fails gracefully, leaving - # the stack in a sub-optimal state. - ls .git/reftable/*.ref | + # Lock all tables, write some refs. Auto-compaction will be + # unable to compact tables and thus fails gracefully, + # compacting only those tables which are not locked. + ls .git/reftable/*.ref | sort | while read table do - touch "$table.lock" || exit 1 + touch "$table.lock" && + basename "$table" >>tables.expect || exit 1 done && + test_line_count = 2 .git/reftable/tables.list && git branch B && git branch C && - rm .git/reftable/*.lock && - test_line_count = 4 .git/reftable/tables.list && + # The new tables are auto-compacted, but the locked tables are + # left intact. + test_line_count = 3 .git/reftable/tables.list && + head -n 2 .git/reftable/tables.list >tables.head && + test_cmp tables.expect tables.head && + + rm .git/reftable/*.lock && git $command --auto && test_line_count = 1 .git/reftable/tables.list ) -- cgit 1.2.3-korg From 6631ed3ce706a82ee43aa70afb38a236bf69d645 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 22 Aug 2024 08:34:41 +0200 Subject: reftable/merged: rename `reftable_new_merged_table()` Rename `reftable_new_merged_table()` to `reftable_merged_table_new()` such that the name matches our coding style. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/merged.c | 2 +- reftable/reftable-merged.h | 9 +++++---- reftable/stack.c | 4 ++-- t/unit-tests/t-reftable-merged.c | 8 ++++---- 4 files changed, 12 insertions(+), 11 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/merged.c b/reftable/merged.c index 8d78b3da71..25d414ec41 100644 --- a/reftable/merged.c +++ b/reftable/merged.c @@ -192,7 +192,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it, it->ops = &merged_iter_vtable; } -int reftable_new_merged_table(struct reftable_merged_table **dest, +int reftable_merged_table_new(struct reftable_merged_table **dest, struct reftable_table *stack, size_t n, uint32_t hash_id) { diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h index 4deb0ad22e..72762483b9 100644 --- a/reftable/reftable-merged.h +++ b/reftable/reftable-merged.h @@ -29,10 +29,11 @@ struct reftable_merged_table; /* A generic reftable; see below. */ struct reftable_table; -/* reftable_new_merged_table creates a new merged table. It takes ownership of - the stack array. -*/ -int reftable_new_merged_table(struct reftable_merged_table **dest, +/* + * reftable_merged_table_new creates a new merged table. It takes ownership of + * the stack array. + */ +int reftable_merged_table_new(struct reftable_merged_table **dest, struct reftable_table *stack, size_t n, uint32_t hash_id); diff --git a/reftable/stack.c b/reftable/stack.c index 2071e428a8..64c7fdf8c4 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -272,7 +272,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, } /* success! */ - err = reftable_new_merged_table(&new_merged, new_tables, + err = reftable_merged_table_new(&new_merged, new_tables, new_readers_len, st->opts.hash_id); if (err < 0) goto done; @@ -924,7 +924,7 @@ static int stack_write_compact(struct reftable_stack *st, reftable_writer_set_limits(wr, st->readers[first]->min_update_index, st->readers[last]->max_update_index); - err = reftable_new_merged_table(&mt, subtabs, subtabs_len, + err = reftable_merged_table_new(&mt, subtabs, subtabs_len, st->opts.hash_id); if (err < 0) { reftable_free(subtabs); diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c index b6263ee8b5..210603e8c7 100644 --- a/t/unit-tests/t-reftable-merged.c +++ b/t/unit-tests/t-reftable-merged.c @@ -111,7 +111,7 @@ merged_table_from_records(struct reftable_ref_record **refs, reftable_table_from_reader(&tabs[i], (*readers)[i]); } - err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID); + err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID); check(!err); return mt; } @@ -289,7 +289,7 @@ merged_table_from_log_records(struct reftable_log_record **logs, reftable_table_from_reader(&tabs[i], (*readers)[i]); } - err = reftable_new_merged_table(&mt, tabs, n, GIT_SHA1_FORMAT_ID); + err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID); check(!err); return mt; } @@ -441,9 +441,9 @@ static void t_default_write_opts(void) check_int(hash_id, ==, GIT_SHA1_FORMAT_ID); reftable_table_from_reader(&tab[0], rd); - err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA256_FORMAT_ID); + err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA256_FORMAT_ID); check_int(err, ==, REFTABLE_FORMAT_ERROR); - err = reftable_new_merged_table(&merged, tab, 1, GIT_SHA1_FORMAT_ID); + err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA1_FORMAT_ID); check(!err); reftable_reader_free(rd); -- cgit 1.2.3-korg From b8ca235ca5fad0421766db7fe2c0f52c6707bf8e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 22 Aug 2024 08:34:44 +0200 Subject: reftable/merged: stop using generic tables in the merged table The merged table provides access to a reftable stack by merging the contents of those tables into a virtual table. These subtables are being tracked via `struct reftable_table`, which is a generic interface for accessing either a single reftable or a merged reftable. So in theory, it would be possible for the merged table to merge together other merged tables. This is somewhat nonsensical though: we only ever set up a merged table over normal reftables, and there is no reason to do otherwise. This generic interface thus makes the code way harder to follow and reason about than really necessary. The abstraction layer may also have an impact on performance, even though the extra set of vtable function calls probably doesn't really matter. Refactor the merged tables to use a `struct reftable_reader` for each of the subtables instead, which gives us direct access to the underlying tables. Adjust names accordingly. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/merged.c | 28 +++++++++++------------ reftable/merged.h | 4 ++-- reftable/reader.c | 6 ++--- reftable/reader.h | 4 ++++ reftable/reftable-merged.h | 7 +++--- reftable/stack.c | 48 +++++++++++++++------------------------- reftable/stack_test.c | 22 +++++++++--------- t/unit-tests/t-reftable-merged.c | 16 ++++---------- 8 files changed, 60 insertions(+), 75 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/merged.c b/reftable/merged.c index 25d414ec41..2e72eab306 100644 --- a/reftable/merged.c +++ b/reftable/merged.c @@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at #include "constants.h" #include "iter.h" #include "pq.h" +#include "reader.h" #include "record.h" #include "generic.h" #include "reftable-merged.h" @@ -25,7 +26,7 @@ struct merged_subiter { struct merged_iter { struct merged_subiter *subiters; struct merged_iter_pqueue pq; - size_t stack_len; + size_t subiters_len; int suppress_deletions; ssize_t advance_index; }; @@ -38,12 +39,12 @@ static void merged_iter_init(struct merged_iter *mi, mi->advance_index = -1; mi->suppress_deletions = mt->suppress_deletions; - REFTABLE_CALLOC_ARRAY(mi->subiters, mt->stack_len); - for (size_t i = 0; i < mt->stack_len; i++) { + REFTABLE_CALLOC_ARRAY(mi->subiters, mt->readers_len); + for (size_t i = 0; i < mt->readers_len; i++) { reftable_record_init(&mi->subiters[i].rec, typ); - table_init_iter(&mt->stack[i], &mi->subiters[i].iter, typ); + reader_init_iter(mt->readers[i], &mi->subiters[i].iter, typ); } - mi->stack_len = mt->stack_len; + mi->subiters_len = mt->readers_len; } static void merged_iter_close(void *p) @@ -51,7 +52,7 @@ static void merged_iter_close(void *p) struct merged_iter *mi = p; merged_iter_pqueue_release(&mi->pq); - for (size_t i = 0; i < mi->stack_len; i++) { + for (size_t i = 0; i < mi->subiters_len; i++) { reftable_iterator_destroy(&mi->subiters[i].iter); reftable_record_release(&mi->subiters[i].rec); } @@ -80,7 +81,7 @@ static int merged_iter_seek(struct merged_iter *mi, struct reftable_record *want mi->advance_index = -1; - for (size_t i = 0; i < mi->stack_len; i++) { + for (size_t i = 0; i < mi->subiters_len; i++) { err = iterator_seek(&mi->subiters[i].iter, want); if (err < 0) return err; @@ -193,7 +194,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it, } int reftable_merged_table_new(struct reftable_merged_table **dest, - struct reftable_table *stack, size_t n, + struct reftable_reader **readers, size_t n, uint32_t hash_id) { struct reftable_merged_table *m = NULL; @@ -201,10 +202,10 @@ int reftable_merged_table_new(struct reftable_merged_table **dest, uint64_t first_min = 0; for (size_t i = 0; i < n; i++) { - uint64_t min = reftable_table_min_update_index(&stack[i]); - uint64_t max = reftable_table_max_update_index(&stack[i]); + uint64_t min = reftable_reader_min_update_index(readers[i]); + uint64_t max = reftable_reader_max_update_index(readers[i]); - if (reftable_table_hash_id(&stack[i]) != hash_id) { + if (reftable_reader_hash_id(readers[i]) != hash_id) { return REFTABLE_FORMAT_ERROR; } if (i == 0 || min < first_min) { @@ -216,8 +217,8 @@ int reftable_merged_table_new(struct reftable_merged_table **dest, } REFTABLE_CALLOC_ARRAY(m, 1); - m->stack = stack; - m->stack_len = n; + m->readers = readers; + m->readers_len = n; m->min = first_min; m->max = last_max; m->hash_id = hash_id; @@ -229,7 +230,6 @@ void reftable_merged_table_free(struct reftable_merged_table *mt) { if (!mt) return; - FREE_AND_NULL(mt->stack); reftable_free(mt); } diff --git a/reftable/merged.h b/reftable/merged.h index 2efe571da6..de5fd33f01 100644 --- a/reftable/merged.h +++ b/reftable/merged.h @@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at #include "system.h" struct reftable_merged_table { - struct reftable_table *stack; - size_t stack_len; + struct reftable_reader **readers; + size_t readers_len; uint32_t hash_id; /* If unset, produce deletions. This is useful for compaction. For the diff --git a/reftable/reader.c b/reftable/reader.c index 29c99e2269..f7ae35da72 100644 --- a/reftable/reader.c +++ b/reftable/reader.c @@ -605,9 +605,9 @@ static void iterator_from_table_iter(struct reftable_iterator *it, it->ops = &table_iter_vtable; } -static void reader_init_iter(struct reftable_reader *r, - struct reftable_iterator *it, - uint8_t typ) +void reader_init_iter(struct reftable_reader *r, + struct reftable_iterator *it, + uint8_t typ) { struct reftable_reader_offsets *offs = reader_offsets_for(r, typ); diff --git a/reftable/reader.h b/reftable/reader.h index e869165f23..a2c204d523 100644 --- a/reftable/reader.h +++ b/reftable/reader.h @@ -57,6 +57,10 @@ int init_reader(struct reftable_reader *r, struct reftable_block_source *source, void reader_close(struct reftable_reader *r); const char *reader_name(struct reftable_reader *r); +void reader_init_iter(struct reftable_reader *r, + struct reftable_iterator *it, + uint8_t typ); + /* initialize a block reader to read from `r` */ int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br, uint64_t next_off, uint8_t want_typ); diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h index 72762483b9..03c2619c0f 100644 --- a/reftable/reftable-merged.h +++ b/reftable/reftable-merged.h @@ -28,13 +28,14 @@ struct reftable_merged_table; /* A generic reftable; see below. */ struct reftable_table; +struct reftable_reader; /* - * reftable_merged_table_new creates a new merged table. It takes ownership of - * the stack array. + * reftable_merged_table_new creates a new merged table. The readers must be + * kept alive as long as the merged table is still in use. */ int reftable_merged_table_new(struct reftable_merged_table **dest, - struct reftable_table *stack, size_t n, + struct reftable_reader **readers, size_t n, uint32_t hash_id); /* Initialize a merged table iterator for reading refs. */ diff --git a/reftable/stack.c b/reftable/stack.c index 64c7fdf8c4..7f4e267ea9 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -225,13 +225,11 @@ static int reftable_stack_reload_once(struct reftable_stack *st, const char **names, int reuse_open) { - size_t cur_len = !st->merged ? 0 : st->merged->stack_len; + size_t cur_len = !st->merged ? 0 : st->merged->readers_len; struct reftable_reader **cur = stack_copy_readers(st, cur_len); size_t names_len = names_length(names); struct reftable_reader **new_readers = reftable_calloc(names_len, sizeof(*new_readers)); - struct reftable_table *new_tables = - reftable_calloc(names_len, sizeof(*new_tables)); size_t new_readers_len = 0; struct reftable_merged_table *new_merged = NULL; struct strbuf table_path = STRBUF_INIT; @@ -267,17 +265,15 @@ static int reftable_stack_reload_once(struct reftable_stack *st, } new_readers[new_readers_len] = rd; - reftable_table_from_reader(&new_tables[new_readers_len], rd); new_readers_len++; } /* success! */ - err = reftable_merged_table_new(&new_merged, new_tables, + err = reftable_merged_table_new(&new_merged, new_readers, new_readers_len, st->opts.hash_id); if (err < 0) goto done; - new_tables = NULL; st->readers_len = new_readers_len; if (st->merged) reftable_merged_table_free(st->merged); @@ -309,7 +305,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, reftable_reader_free(new_readers[i]); } reftable_free(new_readers); - reftable_free(new_tables); reftable_free(cur); strbuf_release(&table_path); return err; @@ -520,7 +515,7 @@ static int stack_uptodate(struct reftable_stack *st) } } - if (names[st->merged->stack_len]) { + if (names[st->merged->readers_len]) { err = 1; goto done; } @@ -659,7 +654,7 @@ int reftable_addition_commit(struct reftable_addition *add) if (add->new_tables_len == 0) goto done; - for (i = 0; i < add->stack->merged->stack_len; i++) { + for (i = 0; i < add->stack->merged->readers_len; i++) { strbuf_addstr(&table_list, add->stack->readers[i]->name); strbuf_addstr(&table_list, "\n"); } @@ -839,7 +834,7 @@ int reftable_addition_add(struct reftable_addition *add, uint64_t reftable_stack_next_update_index(struct reftable_stack *st) { - int sz = st->merged->stack_len; + int sz = st->merged->readers_len; if (sz > 0) return reftable_reader_max_update_index(st->readers[sz - 1]) + 1; @@ -906,30 +901,23 @@ static int stack_write_compact(struct reftable_stack *st, size_t first, size_t last, struct reftable_log_expiry_config *config) { - size_t subtabs_len = last - first + 1; - struct reftable_table *subtabs = reftable_calloc( - last - first + 1, sizeof(*subtabs)); struct reftable_merged_table *mt = NULL; struct reftable_iterator it = { NULL }; struct reftable_ref_record ref = { NULL }; struct reftable_log_record log = { NULL }; + size_t subtabs_len = last - first + 1; uint64_t entries = 0; int err = 0; - for (size_t i = first, j = 0; i <= last; i++) { - struct reftable_reader *t = st->readers[i]; - reftable_table_from_reader(&subtabs[j++], t); - st->stats.bytes += t->size; - } + for (size_t i = first; i <= last; i++) + st->stats.bytes += st->readers[i]->size; reftable_writer_set_limits(wr, st->readers[first]->min_update_index, st->readers[last]->max_update_index); - err = reftable_merged_table_new(&mt, subtabs, subtabs_len, + err = reftable_merged_table_new(&mt, st->readers + first, subtabs_len, st->opts.hash_id); - if (err < 0) { - reftable_free(subtabs); + if (err < 0) goto done; - } merged_table_init_iter(mt, &it, BLOCK_TYPE_REF); err = reftable_iterator_seek_ref(&it, ""); @@ -1207,7 +1195,7 @@ static int stack_compact_range(struct reftable_stack *st, * have compacted them. */ for (size_t j = 1; j < last - first + 1; j++) { - const char *old = first + j < st->merged->stack_len ? + const char *old = first + j < st->merged->readers_len ? st->readers[first + j]->name : NULL; const char *new = names[i + j]; @@ -1248,10 +1236,10 @@ static int stack_compact_range(struct reftable_stack *st, * `fd_read_lines()` uses a `NULL` sentinel to indicate that * the array is at its end. As we use `free_names()` to free * the array, we need to include this sentinel value here and - * thus have to allocate `stack_len + 1` many entries. + * thus have to allocate `readers_len + 1` many entries. */ - REFTABLE_CALLOC_ARRAY(names, st->merged->stack_len + 1); - for (size_t i = 0; i < st->merged->stack_len; i++) + REFTABLE_CALLOC_ARRAY(names, st->merged->readers_len + 1); + for (size_t i = 0; i < st->merged->readers_len; i++) names[i] = xstrdup(st->readers[i]->name); first_to_replace = first; last_to_replace = last; @@ -1358,7 +1346,7 @@ static int stack_compact_range_stats(struct reftable_stack *st, int reftable_stack_compact_all(struct reftable_stack *st, struct reftable_log_expiry_config *config) { - size_t last = st->merged->stack_len ? st->merged->stack_len - 1 : 0; + size_t last = st->merged->readers_len ? st->merged->readers_len - 1 : 0; return stack_compact_range_stats(st, 0, last, config, 0); } @@ -1449,9 +1437,9 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st) int overhead = header_size(version) - 1; uint64_t *sizes; - REFTABLE_CALLOC_ARRAY(sizes, st->merged->stack_len); + REFTABLE_CALLOC_ARRAY(sizes, st->merged->readers_len); - for (size_t i = 0; i < st->merged->stack_len; i++) + for (size_t i = 0; i < st->merged->readers_len; i++) sizes[i] = st->readers[i]->size - overhead; return sizes; @@ -1461,7 +1449,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st) { uint64_t *sizes = stack_table_sizes_for_compaction(st); struct segment seg = - suggest_compaction_segment(sizes, st->merged->stack_len, + suggest_compaction_segment(sizes, st->merged->readers_len, st->opts.auto_compaction_factor); reftable_free(sizes); if (segment_size(&seg) > 0) diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 8c36590ff0..dbca9eaf4a 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -347,9 +347,9 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void) * all tables in the stack. */ if (i != n) - EXPECT(st->merged->stack_len == i + 1); + EXPECT(st->merged->readers_len == i + 1); else - EXPECT(st->merged->stack_len == 1); + EXPECT(st->merged->readers_len == 1); } reftable_stack_destroy(st); @@ -375,7 +375,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void) err = reftable_stack_add(st, write_test_ref, &ref); EXPECT_ERR(err); - EXPECT(st->merged->stack_len == 1); + EXPECT(st->merged->readers_len == 1); EXPECT(st->stats.attempts == 0); EXPECT(st->stats.failures == 0); @@ -390,7 +390,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void) ref.update_index = 2; err = reftable_stack_add(st, write_test_ref, &ref); EXPECT_ERR(err); - EXPECT(st->merged->stack_len == 2); + EXPECT(st->merged->readers_len == 2); EXPECT(st->stats.attempts == 1); EXPECT(st->stats.failures == 1); @@ -881,7 +881,7 @@ static void test_reftable_stack_auto_compaction(void) err = reftable_stack_auto_compact(st); EXPECT_ERR(err); - EXPECT(i < 3 || st->merged->stack_len < 2 * fastlog2(i)); + EXPECT(i < 3 || st->merged->readers_len < 2 * fastlog2(i)); } EXPECT(reftable_stack_compaction_stats(st)->entries_written < @@ -905,7 +905,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void) EXPECT_ERR(err); write_n_ref_tables(st, 5); - EXPECT(st->merged->stack_len == 5); + EXPECT(st->merged->readers_len == 5); /* * Given that all tables we have written should be roughly the same @@ -925,7 +925,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void) err = reftable_stack_auto_compact(st); EXPECT_ERR(err); EXPECT(st->stats.failures == 0); - EXPECT(st->merged->stack_len == 4); + EXPECT(st->merged->readers_len == 4); reftable_stack_destroy(st); strbuf_release(&buf); @@ -970,9 +970,9 @@ static void test_reftable_stack_add_performs_auto_compaction(void) * all tables in the stack. */ if (i != n) - EXPECT(st->merged->stack_len == i + 1); + EXPECT(st->merged->readers_len == i + 1); else - EXPECT(st->merged->stack_len == 1); + EXPECT(st->merged->readers_len == 1); } reftable_stack_destroy(st); @@ -994,7 +994,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void) EXPECT_ERR(err); write_n_ref_tables(st, 3); - EXPECT(st->merged->stack_len == 3); + EXPECT(st->merged->readers_len == 3); /* Lock one of the tables that we're about to compact. */ strbuf_reset(&buf); @@ -1008,7 +1008,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void) err = reftable_stack_compact_all(st, NULL); EXPECT(err == REFTABLE_LOCK_ERROR); EXPECT(st->stats.failures == 1); - EXPECT(st->merged->stack_len == 3); + EXPECT(st->merged->readers_len == 3); reftable_stack_destroy(st); strbuf_release(&buf); diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c index 210603e8c7..577b1a5be8 100644 --- a/t/unit-tests/t-reftable-merged.c +++ b/t/unit-tests/t-reftable-merged.c @@ -94,10 +94,8 @@ merged_table_from_records(struct reftable_ref_record **refs, struct strbuf *buf, const size_t n) { struct reftable_merged_table *mt = NULL; - struct reftable_table *tabs; int err; - REFTABLE_CALLOC_ARRAY(tabs, n); REFTABLE_CALLOC_ARRAY(*readers, n); REFTABLE_CALLOC_ARRAY(*source, n); @@ -108,10 +106,9 @@ merged_table_from_records(struct reftable_ref_record **refs, err = reftable_new_reader(&(*readers)[i], &(*source)[i], "name"); check(!err); - reftable_table_from_reader(&tabs[i], (*readers)[i]); } - err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID); + err = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID); check(!err); return mt; } @@ -272,10 +269,8 @@ merged_table_from_log_records(struct reftable_log_record **logs, struct strbuf *buf, const size_t n) { struct reftable_merged_table *mt = NULL; - struct reftable_table *tabs; int err; - REFTABLE_CALLOC_ARRAY(tabs, n); REFTABLE_CALLOC_ARRAY(*readers, n); REFTABLE_CALLOC_ARRAY(*source, n); @@ -286,10 +281,9 @@ merged_table_from_log_records(struct reftable_log_record **logs, err = reftable_new_reader(&(*readers)[i], &(*source)[i], "name"); check(!err); - reftable_table_from_reader(&tabs[i], (*readers)[i]); } - err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID); + err = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID); check(!err); return mt; } @@ -418,7 +412,6 @@ static void t_default_write_opts(void) }; int err; struct reftable_block_source source = { 0 }; - struct reftable_table *tab = reftable_calloc(1, sizeof(*tab)); uint32_t hash_id; struct reftable_reader *rd = NULL; struct reftable_merged_table *merged = NULL; @@ -440,10 +433,9 @@ static void t_default_write_opts(void) hash_id = reftable_reader_hash_id(rd); check_int(hash_id, ==, GIT_SHA1_FORMAT_ID); - reftable_table_from_reader(&tab[0], rd); - err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA256_FORMAT_ID); + err = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA256_FORMAT_ID); check_int(err, ==, REFTABLE_FORMAT_ERROR); - err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA1_FORMAT_ID); + err = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA1_FORMAT_ID); check(!err); reftable_reader_free(rd); -- cgit 1.2.3-korg From aef860265365052bd0a2aac4cc618be22446fc9d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 22 Aug 2024 08:34:48 +0200 Subject: reftable/stack: open-code reading refs To read a reference for the reftable stack, we first create a generic `reftable_table` from the merged table and then read the reference via a convenience function. We are about to remove these generic interfaces, so let's instead open-code the logic to prepare for this removal. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/stack.c b/reftable/stack.c index 7f4e267ea9..d08ec00959 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1468,9 +1468,28 @@ reftable_stack_compaction_stats(struct reftable_stack *st) int reftable_stack_read_ref(struct reftable_stack *st, const char *refname, struct reftable_ref_record *ref) { - struct reftable_table tab = { NULL }; - reftable_table_from_merged_table(&tab, reftable_stack_merged_table(st)); - return reftable_table_read_ref(&tab, refname, ref); + struct reftable_iterator it = { 0 }; + int ret; + + reftable_merged_table_init_ref_iterator(st->merged, &it); + ret = reftable_iterator_seek_ref(&it, refname); + if (ret) + goto out; + + ret = reftable_iterator_next_ref(&it, ref); + if (ret) + goto out; + + if (strcmp(ref->refname, refname) || + reftable_ref_record_is_deletion(ref)) { + reftable_ref_record_release(ref); + ret = 1; + goto out; + } + +out: + reftable_iterator_destroy(&it); + return ret; } int reftable_stack_read_log(struct reftable_stack *st, const char *refname, -- cgit 1.2.3-korg From ca74ef6ffb7388d862379d5016282340aff1f68b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 22 Aug 2024 08:35:12 +0200 Subject: t/helper: inline `reftable_stack_print_directory()` Move `reftable_stack_print_directory()` into the "dump-reftable" helper. This follows the same reasoning as the preceding commit. Note that this requires us to remove the tests for this functionality in `reftable/stack_test.c`. The test does not really add much anyway, because all it verifies is that we do not crash or run into an error, and it specifically doesn't check the outputted data. Also, as the code is now part of the test helper, it doesn't make much sense to have a unit test for it in the first place. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/reftable-stack.h | 3 --- reftable/stack.c | 20 -------------------- reftable/stack_test.c | 7 ------- t/helper/test-reftable.c | 23 ++++++++++++++++++++++- 4 files changed, 22 insertions(+), 31 deletions(-) (limited to 'reftable/stack.c') diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h index 09e97c9991..f4f8cabc7f 100644 --- a/reftable/reftable-stack.h +++ b/reftable/reftable-stack.h @@ -140,7 +140,4 @@ struct reftable_compaction_stats { struct reftable_compaction_stats * reftable_stack_compaction_stats(struct reftable_stack *st); -/* print the entire stack represented by the directory */ -int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id); - #endif diff --git a/reftable/stack.c b/reftable/stack.c index d08ec00959..bedd503e7e 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1603,23 +1603,3 @@ int reftable_stack_clean(struct reftable_stack *st) reftable_addition_destroy(add); return err; } - -int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id) -{ - struct reftable_stack *stack = NULL; - struct reftable_write_options opts = { .hash_id = hash_id }; - struct reftable_merged_table *merged = NULL; - struct reftable_table table = { NULL }; - - int err = reftable_new_stack(&stack, stackdir, &opts); - if (err < 0) - goto done; - - merged = reftable_stack_merged_table(stack); - reftable_table_from_merged_table(&table, merged); - err = reftable_table_print(&table); -done: - if (stack) - reftable_stack_destroy(stack); - return err; -} diff --git a/reftable/stack_test.c b/reftable/stack_test.c index dbca9eaf4a..42044ed8a3 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -179,13 +179,6 @@ static void test_reftable_stack_add_one(void) EXPECT(0 == strcmp("master", dest.value.symref)); EXPECT(st->readers_len > 0); - printf("testing print functionality:\n"); - err = reftable_stack_print_directory(dir, GIT_SHA1_FORMAT_ID); - EXPECT_ERR(err); - - err = reftable_stack_print_directory(dir, GIT_SHA256_FORMAT_ID); - EXPECT(err == REFTABLE_FORMAT_ERROR); - #ifndef GIT_WINDOWS_NATIVE strbuf_addstr(&scratch, dir); strbuf_addstr(&scratch, "/tables.list"); diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c index 19367c25f9..db62ea8dc3 100644 --- a/t/helper/test-reftable.c +++ b/t/helper/test-reftable.c @@ -1,6 +1,7 @@ #include "reftable/system.h" #include "reftable/reftable-error.h" #include "reftable/reftable-generic.h" +#include "reftable/reftable-merged.h" #include "reftable/reftable-reader.h" #include "reftable/reftable-stack.h" #include "reftable/reftable-tests.h" @@ -29,6 +30,26 @@ static void print_help(void) "\n"); } +static int dump_stack(const char *stackdir, uint32_t hash_id) +{ + struct reftable_stack *stack = NULL; + struct reftable_write_options opts = { .hash_id = hash_id }; + struct reftable_merged_table *merged = NULL; + struct reftable_table table = { NULL }; + + int err = reftable_new_stack(&stack, stackdir, &opts); + if (err < 0) + goto done; + + merged = reftable_stack_merged_table(stack); + reftable_table_from_merged_table(&table, merged); + err = reftable_table_print(&table); +done: + if (stack) + reftable_stack_destroy(stack); + return err; +} + static int dump_reftable(const char *tablename) { struct reftable_block_source src = { NULL }; @@ -87,7 +108,7 @@ int cmd__dump_reftable(int argc, const char **argv) } else if (opt_dump_table) { err = dump_reftable(arg); } else if (opt_dump_stack) { - err = reftable_stack_print_directory(arg, opt_hash_id); + err = dump_stack(arg, opt_hash_id); } if (err < 0) { -- cgit 1.2.3-korg From 6014639837a8f118513563e6d9e3ff404e470b31 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 22 Aug 2024 08:35:29 +0200 Subject: reftable/generic: drop interface The `reftable_table` interface provides a generic infrastructure that can abstract away whether the underlying table is a single table, or a merged table. This abstraction can make it rather hard to reason about the code. We didn't ever use it to implement the reftable backend, and with the preceding patches in this patch series we in fact don't use it at all anymore. Furthermore, it became somewhat useless with the recent refactorings that made it possible to seek reftable iterators multiple times, as these now provide generic access to tables for us. The interface is thus redundant and only brings unnecessary complexity with it. Remove the `struct reftable_table` interface and its associated functions. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- Makefile | 1 - reftable/generic.c | 77 ---------------------------------------- reftable/generic.h | 27 -------------- reftable/iter.c | 1 - reftable/iter.h | 1 - reftable/merged.c | 38 -------------------- reftable/reader.c | 41 --------------------- reftable/reftable-generic.h | 44 ----------------------- reftable/reftable-merged.h | 6 ---- reftable/reftable-reader.h | 7 ---- reftable/stack.c | 1 - t/unit-tests/t-reftable-merged.c | 1 - 12 files changed, 245 deletions(-) delete mode 100644 reftable/generic.c delete mode 100644 reftable/generic.h delete mode 100644 reftable/reftable-generic.h (limited to 'reftable/stack.c') diff --git a/Makefile b/Makefile index 343f19a488..41dfa0bad2 100644 --- a/Makefile +++ b/Makefile @@ -2674,7 +2674,6 @@ REFTABLE_OBJS += reftable/merged.o REFTABLE_OBJS += reftable/pq.o REFTABLE_OBJS += reftable/reader.o REFTABLE_OBJS += reftable/record.o -REFTABLE_OBJS += reftable/generic.o REFTABLE_OBJS += reftable/stack.o REFTABLE_OBJS += reftable/tree.o REFTABLE_OBJS += reftable/writer.o diff --git a/reftable/generic.c b/reftable/generic.c deleted file mode 100644 index 495ee9af6b..0000000000 --- a/reftable/generic.c +++ /dev/null @@ -1,77 +0,0 @@ -/* -Copyright 2020 Google LLC - -Use of this source code is governed by a BSD-style -license that can be found in the LICENSE file or at -https://developers.google.com/open-source/licenses/bsd -*/ - -#include "constants.h" -#include "record.h" -#include "generic.h" -#include "iter.h" -#include "reftable-iterator.h" -#include "reftable-generic.h" - -void table_init_iter(struct reftable_table *tab, - struct reftable_iterator *it, - uint8_t typ) -{ - - tab->ops->init_iter(tab->table_arg, it, typ); -} - -void reftable_table_init_ref_iter(struct reftable_table *tab, - struct reftable_iterator *it) -{ - table_init_iter(tab, it, BLOCK_TYPE_REF); -} - -void reftable_table_init_log_iter(struct reftable_table *tab, - struct reftable_iterator *it) -{ - table_init_iter(tab, it, BLOCK_TYPE_LOG); -} - -int reftable_table_read_ref(struct reftable_table *tab, const char *name, - struct reftable_ref_record *ref) -{ - struct reftable_iterator it = { NULL }; - int err; - - reftable_table_init_ref_iter(tab, &it); - - err = reftable_iterator_seek_ref(&it, name); - if (err) - goto done; - - err = reftable_iterator_next_ref(&it, ref); - if (err) - goto done; - - if (strcmp(ref->refname, name) || - reftable_ref_record_is_deletion(ref)) { - reftable_ref_record_release(ref); - err = 1; - goto done; - } - -done: - reftable_iterator_destroy(&it); - return err; -} - -uint64_t reftable_table_max_update_index(struct reftable_table *tab) -{ - return tab->ops->max_update_index(tab->table_arg); -} - -uint64_t reftable_table_min_update_index(struct reftable_table *tab) -{ - return tab->ops->min_update_index(tab->table_arg); -} - -uint32_t reftable_table_hash_id(struct reftable_table *tab) -{ - return tab->ops->hash_id(tab->table_arg); -} diff --git a/reftable/generic.h b/reftable/generic.h deleted file mode 100644 index 837fbb8df2..0000000000 --- a/reftable/generic.h +++ /dev/null @@ -1,27 +0,0 @@ -/* -Copyright 2020 Google LLC - -Use of this source code is governed by a BSD-style -license that can be found in the LICENSE file or at -https://developers.google.com/open-source/licenses/bsd -*/ - -#ifndef GENERIC_H -#define GENERIC_H - -#include "record.h" -#include "reftable-generic.h" - -/* generic interface to reftables */ -struct reftable_table_vtable { - void (*init_iter)(void *tab, struct reftable_iterator *it, uint8_t typ); - uint32_t (*hash_id)(void *tab); - uint64_t (*min_update_index)(void *tab); - uint64_t (*max_update_index)(void *tab); -}; - -void table_init_iter(struct reftable_table *tab, - struct reftable_iterator *it, - uint8_t typ); - -#endif diff --git a/reftable/iter.c b/reftable/iter.c index 225feb7871..97a4642ed5 100644 --- a/reftable/iter.c +++ b/reftable/iter.c @@ -11,7 +11,6 @@ license that can be found in the LICENSE file or at #include "system.h" #include "block.h" -#include "generic.h" #include "constants.h" #include "reader.h" #include "reftable-error.h" diff --git a/reftable/iter.h b/reftable/iter.h index 3b401f1259..befc4597df 100644 --- a/reftable/iter.h +++ b/reftable/iter.h @@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at #include "record.h" #include "reftable-iterator.h" -#include "reftable-generic.h" /* * The virtual function table for implementing generic reftable iterators. diff --git a/reftable/merged.c b/reftable/merged.c index 2e72eab306..128a810c55 100644 --- a/reftable/merged.c +++ b/reftable/merged.c @@ -13,7 +13,6 @@ license that can be found in the LICENSE file or at #include "pq.h" #include "reader.h" #include "record.h" -#include "generic.h" #include "reftable-merged.h" #include "reftable-error.h" #include "system.h" @@ -270,40 +269,3 @@ uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *mt) { return mt->hash_id; } - -static void reftable_merged_table_init_iter_void(void *tab, - struct reftable_iterator *it, - uint8_t typ) -{ - merged_table_init_iter(tab, it, typ); -} - -static uint32_t reftable_merged_table_hash_id_void(void *tab) -{ - return reftable_merged_table_hash_id(tab); -} - -static uint64_t reftable_merged_table_min_update_index_void(void *tab) -{ - return reftable_merged_table_min_update_index(tab); -} - -static uint64_t reftable_merged_table_max_update_index_void(void *tab) -{ - return reftable_merged_table_max_update_index(tab); -} - -static struct reftable_table_vtable merged_table_vtable = { - .init_iter = reftable_merged_table_init_iter_void, - .hash_id = reftable_merged_table_hash_id_void, - .min_update_index = reftable_merged_table_min_update_index_void, - .max_update_index = reftable_merged_table_max_update_index_void, -}; - -void reftable_table_from_merged_table(struct reftable_table *tab, - struct reftable_merged_table *merged) -{ - assert(!tab->ops); - tab->ops = &merged_table_vtable; - tab->table_arg = merged; -} diff --git a/reftable/reader.c b/reftable/reader.c index fbd93b88df..082cf00b60 100644 --- a/reftable/reader.c +++ b/reftable/reader.c @@ -11,11 +11,9 @@ license that can be found in the LICENSE file or at #include "system.h" #include "block.h" #include "constants.h" -#include "generic.h" #include "iter.h" #include "record.h" #include "reftable-error.h" -#include "reftable-generic.h" uint64_t block_source_size(struct reftable_block_source *source) { @@ -759,45 +757,6 @@ uint64_t reftable_reader_min_update_index(struct reftable_reader *r) return r->min_update_index; } -/* generic table interface. */ - -static void reftable_reader_init_iter_void(void *tab, - struct reftable_iterator *it, - uint8_t typ) -{ - reader_init_iter(tab, it, typ); -} - -static uint32_t reftable_reader_hash_id_void(void *tab) -{ - return reftable_reader_hash_id(tab); -} - -static uint64_t reftable_reader_min_update_index_void(void *tab) -{ - return reftable_reader_min_update_index(tab); -} - -static uint64_t reftable_reader_max_update_index_void(void *tab) -{ - return reftable_reader_max_update_index(tab); -} - -static struct reftable_table_vtable reader_vtable = { - .init_iter = reftable_reader_init_iter_void, - .hash_id = reftable_reader_hash_id_void, - .min_update_index = reftable_reader_min_update_index_void, - .max_update_index = reftable_reader_max_update_index_void, -}; - -void reftable_table_from_reader(struct reftable_table *tab, - struct reftable_reader *reader) -{ - assert(!tab->ops); - tab->ops = &reader_vtable; - tab->table_arg = reader; -} - int reftable_reader_print_blocks(const char *tablename) { struct { diff --git a/reftable/reftable-generic.h b/reftable/reftable-generic.h deleted file mode 100644 index b8b1323a33..0000000000 --- a/reftable/reftable-generic.h +++ /dev/null @@ -1,44 +0,0 @@ -/* -Copyright 2020 Google LLC - -Use of this source code is governed by a BSD-style -license that can be found in the LICENSE file or at -https://developers.google.com/open-source/licenses/bsd -*/ - -#ifndef REFTABLE_GENERIC_H -#define REFTABLE_GENERIC_H - -#include "reftable-iterator.h" - -struct reftable_table_vtable; - -/* - * Provides a unified API for reading tables, either merged tables, or single - * readers. */ -struct reftable_table { - struct reftable_table_vtable *ops; - void *table_arg; -}; - -void reftable_table_init_ref_iter(struct reftable_table *tab, - struct reftable_iterator *it); - -void reftable_table_init_log_iter(struct reftable_table *tab, - struct reftable_iterator *it); - -/* returns the hash ID from a generic reftable_table */ -uint32_t reftable_table_hash_id(struct reftable_table *tab); - -/* returns the max update_index covered by this table. */ -uint64_t reftable_table_max_update_index(struct reftable_table *tab); - -/* returns the min update_index covered by this table. */ -uint64_t reftable_table_min_update_index(struct reftable_table *tab); - -/* convenience function to read a single ref. Returns < 0 for error, 0 - for success, and 1 if ref not found. */ -int reftable_table_read_ref(struct reftable_table *tab, const char *name, - struct reftable_ref_record *ref); - -#endif diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h index 03c2619c0f..16d19f8df2 100644 --- a/reftable/reftable-merged.h +++ b/reftable/reftable-merged.h @@ -26,8 +26,6 @@ license that can be found in the LICENSE file or at /* A merged table is implements seeking/iterating over a stack of tables. */ struct reftable_merged_table; -/* A generic reftable; see below. */ -struct reftable_table; struct reftable_reader; /* @@ -60,8 +58,4 @@ void reftable_merged_table_free(struct reftable_merged_table *m); /* return the hash ID of the merged table. */ uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *m); -/* create a generic table from reftable_merged_table */ -void reftable_table_from_merged_table(struct reftable_table *tab, - struct reftable_merged_table *table); - #endif diff --git a/reftable/reftable-reader.h b/reftable/reftable-reader.h index 7c7d171651..69621c5b0f 100644 --- a/reftable/reftable-reader.h +++ b/reftable/reftable-reader.h @@ -23,9 +23,6 @@ /* The reader struct is a handle to an open reftable file. */ struct reftable_reader; -/* Generic table. */ -struct reftable_table; - /* reftable_new_reader opens a reftable for reading. If successful, * returns 0 code and sets pp. The name is used for creating a * stack. Typically, it is the basename of the file. The block source @@ -60,10 +57,6 @@ uint64_t reftable_reader_max_update_index(struct reftable_reader *r); /* return the min_update_index for a table */ uint64_t reftable_reader_min_update_index(struct reftable_reader *r); -/* creates a generic table from a file reader. */ -void reftable_table_from_reader(struct reftable_table *tab, - struct reftable_reader *reader); - /* print blocks onto stdout for debugging. */ int reftable_reader_print_blocks(const char *tablename); diff --git a/reftable/stack.c b/reftable/stack.c index bedd503e7e..d3a95d2f1d 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at #include "merged.h" #include "reader.h" #include "reftable-error.h" -#include "reftable-generic.h" #include "reftable-record.h" #include "reftable-merged.h" #include "writer.h" diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c index 577b1a5be8..93345c6c8b 100644 --- a/t/unit-tests/t-reftable-merged.c +++ b/t/unit-tests/t-reftable-merged.c @@ -12,7 +12,6 @@ license that can be found in the LICENSE file or at #include "reftable/merged.h" #include "reftable/reader.h" #include "reftable/reftable-error.h" -#include "reftable/reftable-generic.h" #include "reftable/reftable-merged.h" #include "reftable/reftable-writer.h" -- cgit 1.2.3-korg