On Thu, Mar 13, 2025 at 08:59:51PM +0530, Meet Soni wrote:

On Wed, 12 Mar 2025 at 18:19, Patrick Steinhardt [off-list ref] wrote:

quoted

quoted

+     /*
+      * The current block is full, so we need to flush and reinitialize the
+      * writer to start writing the next block.
+      */
      arg->err = writer_flush_block(arg->w);
      if (arg->err < 0)
              goto done;

But there is another case further down where we do `block_writer_add()`
and then re-try in case the write fails. This one is a bit more curious:
if the write fails, we don't create a new block -- after all we have
just created one. Instead, we reset the record's offset length to zero
before retrying.

I _think_ that this is done because we know that when resetting the
offset we would write less data to the block, as can be seen in
`reftable_obj_record_encode()`. But I'm honestly not quite sure here as
I haven't yet done a deep dive into object records -- after all, we
don't even really use them in Git.

In any case, I think that this callsite also needs adjustment and
warrants a comment. And if so, all changes to `write_object_record()`
should probably go into a separate commit, as well.

Sorry for taking so long to respond.

Regarding the callsite in write_object_record() where we reset the
record's offset length to zero before retrying: my changes currently
follow the same principle.

    - If block_writer_add() returns an error other than
      REFTABLE_ENTRY_TOO_BIG_ERROR, we simply return.

    - For REFTABLE_ENTRY_TOO_BIG_ERROR, we flush the block and retry.

    - If that fails, we reset the record's offset length to zero and
      then retry.

It's this last step that also needs to be adapted to check for
REFTABLE_ENTRY_TOO_BIG_ERROR, because the intent here is the exact same:
if writing the object record failed even in a completely new block then
we reset the object's offset and try a third time. But same as for the
first time, we don't check whether we get REFTABLE_ENTRY_TOO_BIG_ERROR
here and thus we might be failing and retrying even in unintended cases.

I'm not sure what adjustments or additional comments you are referring to.
Could you please clarify what changes you expect at this callsite?

So overall we'd add the check to both callsites, like in the below
patch.

Patrick

diff --git a/reftable/writer.c b/reftable/writer.c
index f3ab1035d61..63629e00a37 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c

@@ -628,6 +628,8 @@ static void write_object_record(void *void_arg, void *key)
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
+	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
 
 	arg->err = writer_flush_block(arg->w);
 	if (arg->err < 0)

@@ -640,6 +642,8 @@ static void write_object_record(void *void_arg, void *key)
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
+	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
 
 	rec.u.obj.offset_len = 0;
 	arg->err = block_writer_add(arg->w->block_writer, &rec);