Re: [PATCH 00/13] reftable: prepare for re-seekable iterators

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3234 bytes --]

On Wed, May 08, 2024 at 04:42:11PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > To address this inefficiency, the patch series at hand refactors the
> > reftable library such that creation of iterators and seeking on an
> > iterator are separate steps. This refactoring prepares us for reusing
> > iterators to perform multiple seeks, which in turn will allow us to
> > reuse internal data structures for subsequent seeks.
> 
> ;-)
> 
> > Note: this series does not yet go all the way to re-seekable iterators,
> > and there are no users yet. The patch series is complex enough as-is
> > already, so I decided to defer that to the next iteration. Thus, the
> > whole refactoring here should essentially be a large no-op that prepares
> > the infrastructure for re-seekable iterators.
> >
> > The series depends on pks/reftable-write-optim at fa74f32291
> > (reftable/block: reuse compressed array, 2024-04-08).
> 
> There is another topic on reftable to make write options tweakable,
> whose addition of reftable/dump and reader_print_blocks interface
> needs to be adjusted to this change, I think.

True, I forgot to double check that one. Your proposed resolution isn't
quite correct as we now leak the `ti` pointer -- `table_iter_close()`
only closes the iterator and releases its underlying resources, but does
not free the iterator itself.

The below diff would be needed on top of what you currently have in
`seen`. In any case though, I can also resend this topic with
ps/reftable-write-options pulled in as a dependency. Please let me know
your preference.

Patrick

-- >8 --

diff --git a/reftable/reader.c b/reftable/reader.c
index 2d9630b7c2..83f6772e5d 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -841,8 +841,8 @@ int reftable_reader_print_blocks(const char *tablename)
 		},
 	};
 	struct reftable_block_source src = { 0 };
-	struct table_iter *ti = NULL;
 	struct reftable_reader *r = NULL;
+	struct table_iter ti = {0};
 	size_t i;
 	int err;
 
@@ -854,14 +854,13 @@ int reftable_reader_print_blocks(const char *tablename)
 	if (err < 0)
 		goto done;
 
-	REFTABLE_ALLOC_ARRAY(ti, 1);
-	table_iter_init(ti, r);
+	table_iter_init(&ti, r);
 
 	printf("header:\n");
 	printf("  block_size: %d\n", r->block_size);
 
 	for (i = 0; i < ARRAY_SIZE(sections); i++) {
-		err = table_iter_seek_start(ti, sections[i].type, 0);
+		err = table_iter_seek_start(&ti, sections[i].type, 0);
 		if (err < 0)
 			goto done;
 		if (err > 0)
@@ -870,10 +869,10 @@ int reftable_reader_print_blocks(const char *tablename)
 		printf("%s:\n", sections[i].name);
 
 		while (1) {
-			printf("  - length: %u\n", ti->br.block_len);
-			printf("    restarts: %u\n", ti->br.restart_count);
+			printf("  - length: %u\n", ti.br.block_len);
+			printf("    restarts: %u\n", ti.br.restart_count);
 
-			err = table_iter_next_block(ti);
+			err = table_iter_next_block(&ti);
 			if (err < 0)
 				goto done;
 			if (err > 0)
@@ -883,7 +882,6 @@ int reftable_reader_print_blocks(const char *tablename)
 
 done:
 	reftable_reader_free(r);
-	if (ti)
-		table_iter_close(ti);
+	table_iter_close(&ti);
 	return err;
 }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]