There should be no way of creating branch without specifying LSN, otherwise there is a race

[yeputons]

Follow-up of the last failures in #1667 (comment), see also #2062.

Looking at our neon_local timeline branch command, it works entirely with pageserver:

neon/neon_local/src/main.rs

Lines 690 to 710 in 39d86ed

	let start_lsn = branch_match
	.value_of("ancestor-start-lsn")
	.map(Lsn::from_str)
	.transpose()
	.context("Failed to parse ancestor start Lsn from the request")?;
	let timeline = pageserver
	.timeline_create(tenant_id, None, start_lsn, Some(ancestor_timeline_id))?
	.ok_or_else(|| anyhow!("Failed to create new timeline for tenant {}", tenant_id))?;
	let new_timeline_id = timeline.timeline_id;
	let last_record_lsn = timeline
	.local
	.expect("no local timeline info")
	.last_record_lsn;
	env.register_branch_mapping(new_branch_name.to_string(), tenant_id, new_timeline_id)?;
	println!(
	"Created timeline '{}' at Lsn {} for tenant: {}. Ancestor timeline: '{}'",
	timeline.timeline_id, last_record_lsn, tenant_id, ancestor_branch_name,
	);

Moreover, the test_tenant_relocation test runs it with --ancestor-branch-name parameter only, no --ancestor-start-lsn. So the pageserver decides what LSN to create the branch at. Hence, if the pageserver is not fully caught up with safekeepers, the branch starts a little earlier than it should, truncating parts of the last transaction (effectively erasing it from the second timeline, I guess).

For example, it may result in test_tenant_relocation failure even before there are relocations: it creates two branches, and the second branch is sometimes created slightly before the first is fully on the pageserver, resulting in divergence: https://app.circleci.com/pipelines/github/neondatabase/neon/7828/workflows/bce7b9dc-e390-4f42-8beb-1a2e81bf2b9d/jobs/80367

assert (500500,) == (1001000,)
  At index 0 diff: 500500 != 1001000
  Full diff:
  - (1001000,)
  + (500500,)
test_runner/batch_others/test_tenant_relocation.py:259: in test_tenant_relocation
    timeline_id_second, current_lsn_second = populate_branch(pg_second, create_table=False, expected_sum=1001000)
test_runner/batch_others/test_tenant_relocation.py:133: in populate_branch
    assert cur.fetchone() == (expected_sum, )
E   assert (500500,) == (1001000,)
E     At index 0 diff: 500500 != 1001000
E     Full diff:
E     - (1001000,)
E     + (500500,)

If so, I argue it's not a flaky test; it's a design bug. We should have precise semantics of what "branching off a named branch" is (probably in terms of commit_lsn/flush_lsn/whatever on Safekeepers), and there should be no way to violate it by misusing the API. For example, there should be no way to call the pageserver branch creation API without specifying the exact parent timeline and the exact LSN in it.

I suspect there may be other similar bugs lurking around; the pageserver is probably the source of truth in multiple places (see, e.g., #809). I suspect it would be impossible for our end users to debug unless we somehow expose both safekeepers/pageserver's LSNs, and it would be very hard if we do.

[LizardWizzard]

This problem was mentioned before, see #1371 proper solution requires rather big tweaks in console. E g when compute for the branch is running and you want to create a new branch you need to go to the compute, pick the current lsn here, and pass it to pageserver. I think this needs to be part of a console branching support epic

cc @ololobus

[ololobus]

when compute for the branch is running and you want to create a new branch you need to go to the compute, pick the current lsn here

That is racy anyway if compute is under load. Not sure how the API and UI should look like to the end user. Maybe we should always ask for the timestamp to branch? After that we will re-map it to LSN using the pageserver API and finally create a branch

Thanks for the reminder

[hlinnaka]

For example, it may result in test_tenant_relocation failure even before there are relocations: it creates two branches, and the second branch is sometimes created slightly before the first is fully on the pageserver, resulting in divergence: https://app.circleci.com/pipelines/github/neondatabase/neon/7828/workflows/bce7b9dc-e390-4f42-8beb-1a2e81bf2b9d/jobs/80367

This just happened again here: https://github.com/neondatabase/neon/runs/7413000700

[yeputons]

Current status:

• test_tenant_relocations was fixed in Fix test to be more robust with slow pageserver. #2131
• We allow creating a "Head" branch in our web interface. It calls the createProjectBranch endpoint that uses Safekeeper's commit_lsn to figure out the LSN for branch creation: https://github.com/neondatabase/cloud/blob/e9936c1c62c41b1416f9f4a8f134a712b7d8a38b/goapp/internal/controllers/publicapiv2/branch_create.go#L483-L492 . No race condition here since https://github.com/neondatabase/cloud/pull/3003/files, which is good.
• neon_local still goes directly to pageserver, race condition is possible: a user can do some SQL queries, then neon_local branch, and then see old data in the branch.

I think only these things need addressing:

• Our internal APIs can be better in not allowing branching without a specific LSN: Require LSN on branch creation internally (partial fix #2063) #3140
• neon_local branch should go to safekeepers for the branching LSN, just like the cloud: neon_cli timeline branch: retrive the branching LSN from Safekeepers #3222
• Maybe some kind of communication to the users about race conditions. They should not encounter them unless they branch a database which has some active writes. It's an expected race condition, but it may be nice to mention in endpoint documents anyways: Let users know about possible race conditions with branch creations #3141

[yeputons]

All PRs from the checklist above are created. As this is more of a tech debt issue, closing it now, it requires no further tracking.