| Message ID | 37a18b91ca998ca5ad27d17e86d286040b6e6ee1.1710706118.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: introduce `--auto` to pack refs as needed | expand |
Patrick Steinhardt <ps@pks.im> writes: > Whenever we commit a new table to the reftable stack we will end up > invoking auto-compaction of the stack to keep the total number of tables > at bay. This auto-compaction may fail though in case at least one of the > tables which we are about to compact is locked. This is indicated by the > compaction function returning a positive value. We do not handle this We no longer return a positive value, do we? > case though, and thus bubble that return value up the calling chain, > which will ultimately cause a failure. > > Fix this bug by handling positive return values. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > reftable/stack.c | 13 ++++++++++++- > t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++ > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/reftable/stack.c b/reftable/stack.c > index ba15c48ddd..dd5160d751 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add) > if (err) > goto done; > > - if (!add->stack->disable_auto_compact) > + if (!add->stack->disable_auto_compact) { > + /* > + * Auto-compact the stack to keep the number of tables in > + * control. Note that we explicitly ignore locking issues which > + * may indicate that a concurrent process is already trying to > + * compact tables. This is fine, so we simply ignore that error > + * condition. > + */ > Nit: The last two sentences are somewhat the same, it'd be perhaps nicer to explain why "this is fine". > err = reftable_stack_auto_compact(add->stack); > + if (err < 0 && err != REFTABLE_LOCK_ERROR) > + goto done; > + err = 0; > + } > > done: > reftable_addition_close(add); > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > index 686781192e..5f2f9baa9b 100755 > --- a/t/t0610-reftable-basics.sh > +++ b/t/t0610-reftable-basics.sh > @@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' ' > EOF > ' > > +test_expect_success 'ref transaction: fails gracefully when auto compaction fails' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + > + test_commit A && > + for i in $(test_seq 10) > + do > + git branch branch-$i && > + for table in .git/reftable/*.ref > + do > + touch "$table.lock" || exit 1 > + done || > + exit 1 > + done && > + test_line_count = 13 .git/reftable/tables.list > + ) > +' I'm not sure about the testing setup for reftables, but maybe if we moved this to the unit tests, we could've also checked the `reftable_stack_compaction_stats(st)->failures` value. This is fine, but it doesn't really tell us if the compaction was even attempted.
On Wed, Mar 20, 2024 at 03:22:55PM -0700, Karthik Nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Whenever we commit a new table to the reftable stack we will end up > > invoking auto-compaction of the stack to keep the total number of tables > > at bay. This auto-compaction may fail though in case at least one of the > > tables which we are about to compact is locked. This is indicated by the > > compaction function returning a positive value. We do not handle this > > We no longer return a positive value, do we? Yup, this is stale. > > case though, and thus bubble that return value up the calling chain, > > which will ultimately cause a failure. > > > > Fix this bug by handling positive return values. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > reftable/stack.c | 13 ++++++++++++- > > t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++ > > 2 files changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/reftable/stack.c b/reftable/stack.c > > index ba15c48ddd..dd5160d751 100644 > > --- a/reftable/stack.c > > +++ b/reftable/stack.c > > @@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add) > > if (err) > > goto done; > > > > - if (!add->stack->disable_auto_compact) > > + if (!add->stack->disable_auto_compact) { > > + /* > > + * Auto-compact the stack to keep the number of tables in > > + * control. Note that we explicitly ignore locking issues which > > + * may indicate that a concurrent process is already trying to > > + * compact tables. This is fine, so we simply ignore that error > > + * condition. > > + */ > > > > Nit: The last two sentences are somewhat the same, it'd be perhaps nicer > to explain why "this is fine". Fair enough. > > err = reftable_stack_auto_compact(add->stack); > > + if (err < 0 && err != REFTABLE_LOCK_ERROR) > > + goto done; > > + err = 0; > > + } > > > > done: > > reftable_addition_close(add); > > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > > index 686781192e..5f2f9baa9b 100755 > > --- a/t/t0610-reftable-basics.sh > > +++ b/t/t0610-reftable-basics.sh > > @@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' ' > > EOF > > ' > > > > +test_expect_success 'ref transaction: fails gracefully when auto compaction fails' ' > > + test_when_finished "rm -rf repo" && > > + git init repo && > > + ( > > + cd repo && > > + > > + test_commit A && > > + for i in $(test_seq 10) > > + do > > + git branch branch-$i && > > + for table in .git/reftable/*.ref > > + do > > + touch "$table.lock" || exit 1 > > + done || > > + exit 1 > > + done && > > + test_line_count = 13 .git/reftable/tables.list > > + ) > > +' > > I'm not sure about the testing setup for reftables, but maybe if we > moved this to the unit tests, we could've also checked the > `reftable_stack_compaction_stats(st)->failures` value. This is fine, but > it doesn't really tell us if the compaction was even attempted. Both tests have their merit, I'd say. Let's thus just add both. Patrick
diff --git a/reftable/stack.c b/reftable/stack.c index ba15c48ddd..dd5160d751 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add) if (err) goto done; - if (!add->stack->disable_auto_compact) + if (!add->stack->disable_auto_compact) { + /* + * Auto-compact the stack to keep the number of tables in + * control. Note that we explicitly ignore locking issues which + * may indicate that a concurrent process is already trying to + * compact tables. This is fine, so we simply ignore that error + * condition. + */ err = reftable_stack_auto_compact(add->stack); + if (err < 0 && err != REFTABLE_LOCK_ERROR) + goto done; + err = 0; + } done: reftable_addition_close(add); diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 686781192e..5f2f9baa9b 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' ' EOF ' +test_expect_success 'ref transaction: fails gracefully when auto compaction fails' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + + test_commit A && + for i in $(test_seq 10) + do + git branch branch-$i && + for table in .git/reftable/*.ref + do + touch "$table.lock" || exit 1 + done || + exit 1 + done && + test_line_count = 13 .git/reftable/tables.list + ) +' + test_expect_success 'pack-refs: compacts tables' ' test_when_finished "rm -rf repo" && git init repo &&
Whenever we commit a new table to the reftable stack we will end up invoking auto-compaction of the stack to keep the total number of tables at bay. This auto-compaction may fail though in case at least one of the tables which we are about to compact is locked. This is indicated by the compaction function returning a positive value. We do not handle this case though, and thus bubble that return value up the calling chain, which will ultimately cause a failure. Fix this bug by handling positive return values. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/stack.c | 13 ++++++++++++- t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)