Exponentially backoff when out of background workers

[konskov]

The scheduler detects the following three types of job failures:

1.Jobs that fail to launch (due to shortage of background workers)
2.Jobs that throw a runtime error
3.Jobs that crash due to a process crashing

In cases 2 and 3, additive backoff is applied in calculating the next
start time of a failed job.
In case 1 we previously retried to launch all jobs that failed to launch
simultaneously.

This commit introduces exponential backoff in case 1,
randomly selecting a wait time in [2, 2 + 2^f] seconds at microsecond granularity.
The aim is to reduce the collision probability for jobs that compete
for a background worker. The maximum backoff value is 1 minute.
It does not change the behavior for cases 2 and 3.

Fixes #4562

[codecov]

Codecov Report

Merging #4567 (56928c5) into main (1fa8373) will decrease coverage by 0.00%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##             main    #4567      +/-   ##
==========================================
- Coverage   90.81%   90.81%   -0.01%     
==========================================
  Files         224      224              
  Lines       42272    42298      +26     
==========================================
+ Hits        38391    38412      +21     
- Misses       3881     3886       +5     

Impacted Files	Coverage Δ
tsl/src/nodes/decompress_chunk/decompress_chunk.c	95.20% <80.00%> (-0.14%)	⬇️
src/bgw/job.c	94.58% <100.00%> (ø)
src/bgw/job_stat.c	90.18% <100.00%> (+0.84%)	⬆️
src/bgw/scheduler.c	83.62% <100.00%> (-0.16%)	⬇️
src/nodes/chunk_append/exec.c	94.50% <100.00%> (ø)
src/nodes/chunk_insert_state.c	97.61% <100.00%> (+<0.01%)	⬆️
src/loader/bgw_message_queue.c	85.52% <0.00%> (-2.64%)	⬇️
tsl/src/nodes/data_node_dispatch.c	96.72% <0.00%> (+0.23%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed212b4...56928c5. Read the comment docs.

[mkindahl]

In general, this looks good, but it would be good if you could add an outline in the commit message where you describe how:

• Jobs that fail while executing are re-tried
• Jobs that fail to start the background worker are re-tried (IIRC, this is different from the one above)
• Jobs that succeed are re-tried

[gayyappan]

IIRC we had exponential backoff initially and then switched to additive backoff. The problem is with jobs that have fairly longer schedules. Say the job is scheduled to run once a day. With exponential backoff, you will quickly schedule a failed job too far into the future. A better way might be to add a hard upper bound check for the next start time after a failure.
i.e. if job failed:

      new_start_time = last_failure_time + schedule interval (with some back off)
      ub_start_time = last_failue_time + '1 h'
      if  ub_start_time < new_start_time
          new_start_time = worst_case.

Also make sure to test and catch potential overflow with exponential backoffs.

[konskov]

Thank you so much for reviewing! Currently there is an upper bound but it is probably too high (10 times the schedule interval, and for jobs that fail to launch due to background worker shortage, the upper bound is 1 minute). I will change it to something fixed and smaller, '1h' as you suggest.

[mfundul]

Suggested change

	else
	{
	// retry every 2 seconds
	ival = IntervalPGetDatum(&retry_ival);
	}
	else
	// retry every 2 seconds
	ival = IntervalPGetDatum(&retry_ival);

nit

[mfundul]

Suggested change

	else
	{
	ival_max = IntervalPGetDatum(&interval_max);
	}
	else
	ival_max = IntervalPGetDatum(&interval_max);

nit

[mfundul]

Exponential backoff works when the scheduled time is picked randomly between now() and now() + ival (regardless of jitter). If we always pick now() + ival then we risk livelocking the conflicting jobs ad infinitum.

This is orthogonal to jitter, which helps with scheduling at non-integer multiples of time units, so as to not generate high load spikes in the system (e.g. due to everything happening at integer seconds instead of spreading them out).

[konskov]

You are right, we are risking that. So for first case (out of background workers) I will change the backoff calculation to randomly pick a backoff value between [0, 2^f - 1] where f is the number of consecutive failed launches of the job.

[mfundul]

Suggested change

	int64 rand_backoff = random() % (max_slots * micros_per_minute);
	int64 rand_backoff = random() % (max_slots * USECS_PER_SEC);

I don't see anything to do with minutes here.

[konskov]

Indeed

[mfundul]

Suggested change

	if (!launch_failure)
	if (launch_failure)

I think?

[konskov]

No, that should be the logic there. Launch failure is when the job couldn't be started because of lack of background workers, and in that case, we have the random backoff in seconds calculated above.
In other cases (job threw runtime error) we have a retry period of 5 minutes, and so the backoff there grows like 5, 10, 20 minutes until it reaches the schedule interval to take the retry period into account. Unless you meant to put the launch_failure case first which would be more readable so I'll do that

[mfundul]

This comment is inside in ts_bgw_job_stat_next_start(), maybe remove?