Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force LSN order with wrapper structure #7071

Merged
merged 15 commits into from
Mar 21, 2024

Conversation

jbajic
Copy link
Contributor

@jbajic jbajic commented Mar 8, 2024

Summary of changes

Add a wrapper structure 'OrderedBatchUpdates' to enforce an order of updates by LSN.

Closes #6707

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@jbajic jbajic requested a review from a team as a code owner March 8, 2024 17:53
@jbajic jbajic requested a review from jcsp March 8, 2024 17:53
@jcsp jcsp requested a review from VladLazar March 8, 2024 19:11
pageserver/src/pgdatadir_mapping.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/timeline.rs Outdated Show resolved Hide resolved
@VladLazar VladLazar added the approved-for-ci-run Changes are safe to trigger CI for the PR label Mar 15, 2024
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Mar 15, 2024
@vipvap vipvap mentioned this pull request Mar 15, 2024
Copy link

github-actions bot commented Mar 15, 2024

2706 tests run: 2576 passed, 0 failed, 130 skipped (full report)


Flaky tests (1)

Postgres 15

  • test_compute_pageserver_connection_stress: debug

Code coverage* (full report)

  • functions: 28.4% (7155 of 25228 functions)
  • lines: 46.9% (43808 of 93380 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
33cb904 at 2024-03-20T11:32:01.749Z :recycle:

@VladLazar
Copy link
Contributor

For some reason I thought that VecMap needs the next key to be greater or equal than the latest one. Seems that's not the case (needs to be strictly greater) which is why the tests fail. In this specific case, we can have multiple values at the same Lsn.

We can create an enum which specifies the desired ordering constraints and either:

  1. make it a generic param for VecMap (something like this)
  2. Make VecMap::default use the current ordering and add a VecMap::new which takes custom orderings.

I think (1) is kind of ugly since rust generics are not as powerful as C++ templates.

cc: @jbajic

@jbajic
Copy link
Contributor Author

jbajic commented Mar 15, 2024

For some reason I thought that VecMap needs the next key to be greater or equal than the latest one. Seems that's not the case (needs to be strictly greater) which is why the tests fail. In this specific case, we can have multiple values at the same Lsn.

We can create an enum which specifies the desired ordering constraints and either:

  1. make it a generic param for VecMap (something like this)
  2. Make VecMap::default use the current ordering and add a VecMap::new which takes custom orderings.

I think (1) is kind of ugly since rust generics are not as powerful as C++ templates.

cc: @jbajic

I don't see a problem with also making a key of VecMap be a tuple of (Lsn, Key), since from my understanding such pair should be unique, since it does not make sense that within the same LSN, a key is being changed twice. Am I right in assuming that?

@VladLazar
Copy link
Contributor

For some reason I thought that VecMap needs the next key to be greater or equal than the latest one. Seems that's not the case (needs to be strictly greater) which is why the tests fail. In this specific case, we can have multiple values at the same Lsn.
We can create an enum which specifies the desired ordering constraints and either:

  1. make it a generic param for VecMap (something like this)
  2. Make VecMap::default use the current ordering and add a VecMap::new which takes custom orderings.

I think (1) is kind of ugly since rust generics are not as powerful as C++ templates.
cc: @jbajic

I don't see a problem with also making a key of VecMap be a tuple of (Lsn, Key), since from my understanding such pair should be unique, since it does not make sense that within the same LSN, a key is being changed twice. Am I right in assuming that?

Hmm. You'd have to kmerge by (Lsn, Key) tuples for that to work. The downside here is that comparisons become slightly more expensive. I think I prefer tweaking the VecMap (but only marginally).

@VladLazar VladLazar added the approved-for-ci-run Changes are safe to trigger CI for the PR label Mar 19, 2024
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Mar 19, 2024
@VladLazar VladLazar added the approved-for-ci-run Changes are safe to trigger CI for the PR label Mar 20, 2024
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Mar 20, 2024
libs/utils/src/vec_map.rs Show resolved Hide resolved
libs/utils/src/vec_map.rs Outdated Show resolved Hide resolved
libs/utils/src/vec_map.rs Outdated Show resolved Hide resolved
libs/utils/src/vec_map.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@VladLazar VladLazar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

libs/utils/src/vec_map.rs Outdated Show resolved Hide resolved
libs/utils/src/vec_map.rs Outdated Show resolved Hide resolved
@VladLazar VladLazar added the approved-for-ci-run Changes are safe to trigger CI for the PR label Mar 20, 2024
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Mar 20, 2024
Copy link
Contributor

@VladLazar VladLazar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks again, @jbajic!

@VladLazar VladLazar merged commit 94138c1 into neondatabase:main Mar 21, 2024
88 of 90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add wrapper type to enforce Lsn order for TimelineWriter::put_batch
3 participants