Fix truncateLsn update #101
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In case of recovery, minimal safekeeper's flushLsn can be greater than truncateLsn, and streaming can skip some messages, resulting in assertion error.
arssher
requested changes
Nov 9, 2021
| XLogRecPtr lsn = UnknownXLogRecPtr; | ||
| for (int i = 0; i < n_walkeepers; i++) | ||
| { | ||
| if (walkeeper[i].feedback.flushLsn < lsn) |
There was a problem hiding this comment.
As long as term_history patch is not merged, we also must check here epoch like GetAcknowledgedByQuorumWALPosition does, otherwise flushLsn is essentially lie, it points to wrong history. And if you check the epoch, I believe acknowledgedLsn becomes unnecessary.
(but check msgQueueHead->ackMask == ((1 << n_walkeepers) - 1) in queue cleanup is still needed due to 0-sized messages, though it would be nice to simplify this as well)
| * ack only on record boundaries. | ||
| */ | ||
| minFlushLsn = CalculateMinFlushLsn(); | ||
| if (minFlushLsn > truncateLsn && minFlushLsn <= minQuorumLsn && minFlushLsn <= acknowledgedLsn) |
There was a problem hiding this comment.
How minFlushLsn <= minQuorumLsn can be false? I think this should be removed. I also considered CalculateMinFlushLsn returning UnknownXLogRecPtr, but obviously it can't do that.
arssher
approved these changes
Nov 9, 2021
| * ack only on record boundaries. | ||
| */ | ||
| minFlushLsn = CalculateMinFlushLsn(); | ||
| if (minFlushLsn > truncateLsn) |
There was a problem hiding this comment.
It shouldn't go backwards, so I think we can directly assign.
There was a problem hiding this comment.
Fails in test_restart_compute[True] for me, looks like minFlushLsn = 0/0 if safekeepers are empty.
ololobus
pushed a commit
that referenced
this pull request
Nov 10, 2021
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
lubennikovaav
pushed a commit
that referenced
this pull request
Feb 9, 2022
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
MMeent
pushed a commit
that referenced
this pull request
Jul 7, 2022
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
MMeent
pushed a commit
that referenced
this pull request
Aug 18, 2022
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
lubennikovaav
pushed a commit
that referenced
this pull request
Nov 21, 2022
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
MMeent
pushed a commit
that referenced
this pull request
Feb 10, 2023
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
MMeent
pushed a commit
that referenced
this pull request
Feb 10, 2023
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
MMeent
pushed a commit
that referenced
this pull request
May 11, 2023
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
tristan957
pushed a commit
that referenced
this pull request
Aug 10, 2023
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
tristan957
pushed a commit
that referenced
this pull request
Nov 8, 2023
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
tristan957
pushed a commit
that referenced
this pull request
Nov 8, 2023
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
tristan957
pushed a commit
that referenced
this pull request
Feb 5, 2024
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
tristan957
pushed a commit
that referenced
this pull request
Feb 5, 2024
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
tristan957
pushed a commit
that referenced
this pull request
Feb 6, 2024
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
tristan957
pushed a commit
that referenced
this pull request
May 10, 2024
truncateLsn is now advanced to `Min(walkeeper[i].feedback.flushLsn)` with taking epochs into account.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a bug when candidateTruncateLsn was distant from truncateLsn and a lot of CPU time was spent in HandleWalKeeperResponse trying to advance truncateLsn. This PR fixes this by changing logic to update truncateLsn, using
Min(walkeeper[i].feedback.flushLsn)as a candidate for new truncateLsn.