git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 8/8] reftable/stack: handle locked tables during auto-compaction

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 2, 2024, 23:24 UTC
Message-ID
<xmqqo76a8q4l.fsf@gitster.g>
In-Reply-To
<dc2217830700acaac50d96361352ff433aa57a4d.1722435214.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 29 quoted lines
> -	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;

OK, so this case no longer jumps to "done" label. Starting from the high end of the st->readers[] array down to what we manged to take locks on will be processed with the existing logic that compacted first..last, which makes sense.

Show 18 quoted lines
> +			} else if (errno == EEXIST) {
>  				err = REFTABLE_LOCK_ERROR;
> -			else
> +				goto done;
> +			} else {
>  				err = REFTABLE_IO_ERROR;
> -			goto done;
> +				goto done;
> +			}
>  		}
>  
>  		/*
> @@ -1270,7 +1308,7 @@ static int stack_compact_range(struct reftable_stack *st,
>  	 * delete the files after we closed them on Windows, so this needs to
>  	 * happen first.
>  	 */
> -	err = reftable_stack_reload_maybe_reuse(st, first < last);
> +	err = reftable_stack_reload_maybe_reuse(st, first_to_replace < last_to_replace);
What is this change about?

No code changed that computes first_to_replace and last_to_replace? Perhaps before this step, using first/last and using first_to_replace/last_to_replace did not make any difference, because we never dealt with a case where we failed to lock any of the tables?

I am wondering if this would be helped by a no-op clarification before the actual behaviour change, similar to how step 4/8 added the nlocks variable.

Thanks.
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 20 of 50 in “reftable: improvements and fixes for compaction”
  1. 0/8 reftable: improvements and fixes for compactionPatrick Steinhardt, Jul 31, 2024
  2. 1/8 reftable/stack: refactor function to gather table sizesPatrick Steinhardt, Jul 31, 2024
  3. Junio C HamanoAug 2, 2024
  4. 2/8 reftable/stack: test compaction with already-locked tablesPatrick Steinhardt, Jul 31, 2024
  5. Junio C HamanoAug 2, 2024
  6. Patrick SteinhardtAug 5, 2024
  7. 3/8 reftable/stack: update stats on failed full compactionPatrick Steinhardt, Jul 31, 2024
  8. 4/8 reftable/stack: simplify tracking of table locksPatrick Steinhardt, Jul 31, 2024
  9. Justin ToblerJul 31, 2024
  10. Junio C HamanoAug 2, 2024
  11. Patrick SteinhardtAug 5, 2024
  12. 5/8 reftable/stack: do not die when fsyncing lock file filesPatrick Steinhardt, Jul 31, 2024
  13. 6/8 reftable/stack: use lock_file when adding table to "tables.list"Patrick Steinhardt, Jul 31, 2024
  14. Justin ToblerJul 31, 2024
  15. Patrick SteinhardtAug 1, 2024
  16. 7/8 reftable/stack: fix corruption on concurrent compactionPatrick Steinhardt, Jul 31, 2024
  17. Justin ToblerAug 1, 2024
  18. Patrick SteinhardtAug 1, 2024
  19. 8/8 reftable/stack: handle locked tables during auto-compactionPatrick Steinhardt, Jul 31, 2024
  20. Junio C HamanoAug 2, 2024
  21. Patrick SteinhardtAug 5, 2024
  22. 0/9 reftable: improvements and fixes for compactionPatrick Steinhardt, Aug 5, 2024
  23. 1/9 reftable/stack: refactor function to gather table sizesPatrick Steinhardt, Aug 5, 2024
  24. 2/9 reftable/stack: extract function to setup stack with N tablesPatrick Steinhardt, Aug 5, 2024
  25. 3/9 reftable/stack: test compaction with already-locked tablesPatrick Steinhardt, Aug 5, 2024
  26. Karthik NayakAug 8, 2024
  27. 4/9 reftable/stack: update stats on failed full compactionPatrick Steinhardt, Aug 5, 2024
  28. 5/9 reftable/stack: simplify tracking of table locksPatrick Steinhardt, Aug 5, 2024
  29. 6/9 reftable/stack: do not die when fsyncing lock file filesPatrick Steinhardt, Aug 5, 2024
  30. 7/9 reftable/stack: use lock_file when adding table to "tables.list"Patrick Steinhardt, Aug 5, 2024
  31. 8/9 reftable/stack: fix corruption on concurrent compactionPatrick Steinhardt, Aug 5, 2024
  32. Karthik NayakAug 8, 2024
  33. Patrick SteinhardtAug 8, 2024
  34. 9/9 reftable/stack: handle locked tables during auto-compactionPatrick Steinhardt, Aug 5, 2024
  35. Justin ToblerAug 6, 2024
  36. Patrick SteinhardtAug 7, 2024
  37. Justin ToblerAug 7, 2024
  38. Karthik NayakAug 8, 2024
  39. Karthik NayakAug 8, 2024
  40. 0/9 reftable: improvements and fixes for compactionPatrick Steinhardt, Aug 8, 2024
  41. 1/9 reftable/stack: refactor function to gather table sizesPatrick Steinhardt, Aug 8, 2024
  42. 2/9 reftable/stack: extract function to setup stack with N tablesPatrick Steinhardt, Aug 8, 2024
  43. 3/9 reftable/stack: test compaction with already-locked tablesPatrick Steinhardt, Aug 8, 2024
  44. 4/9 reftable/stack: update stats on failed full compactionPatrick Steinhardt, Aug 8, 2024
  45. 5/9 reftable/stack: simplify tracking of table locksPatrick Steinhardt, Aug 8, 2024
  46. 6/9 reftable/stack: do not die when fsyncing lock file filesPatrick Steinhardt, Aug 8, 2024
  47. 7/9 reftable/stack: use lock_file when adding table to "tables.list"Patrick Steinhardt, Aug 8, 2024
  48. 8/9 reftable/stack: fix corruption on concurrent compactionPatrick Steinhardt, Aug 8, 2024
  49. 9/9 reftable/stack: handle locked tables during auto-compactionPatrick Steinhardt, Aug 8, 2024
  50. Karthik NayakAug 8, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.