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

recover() not called If the first log entry is rolled back #14

Closed
losfair opened this issue Nov 26, 2023 · 2 comments
Closed

recover() not called If the first log entry is rolled back #14

losfair opened this issue Nov 26, 2023 · 2 comments

Comments

@losfair
Copy link

losfair commented Nov 26, 2023

If the first entry is rolled back, LogManager::recover() is never called for the entire WAL.

To reproduce:

use okaywal::{LogManager, WriteAheadLog};

#[derive(Debug)]
struct TestLogManager;

impl LogManager for TestLogManager {
    fn recover(&mut self, entry: &mut okaywal::Entry<'_>) -> std::io::Result<()> {
        entry.read_all_chunks().unwrap();
        println!("recover: {:?}", entry);
        Ok(())
    }

    fn checkpoint_to(
        &mut self,
        _last_checkpointed_id: okaywal::EntryId,
        _checkpointed_entries: &mut okaywal::SegmentReader,
        _wal: &okaywal::WriteAheadLog,
    ) -> std::io::Result<()> {
        Ok(())
    }
}

fn main() {
    let _ = std::fs::remove_dir_all("./testdata");
    let log = WriteAheadLog::recover("./testdata", TestLogManager).unwrap();

    // this entry is rolled back
    log.begin_entry().unwrap();

    let mut entry = log.begin_entry().unwrap();
    entry.write_chunk(b"test").unwrap();
    entry.commit().unwrap();

    log.shutdown().unwrap();

    WriteAheadLog::recover("./testdata", TestLogManager).unwrap();
}

This prints nothing. If the first log.begin_entry() is commented out, recover() is called correctly.

@ecton ecton closed this as completed in 8fd1661 Nov 26, 2023
@ecton
Copy link
Member

ecton commented Nov 26, 2023

Thank you for reporting this! I will be releasing v0.3.1 with a fix for this shortly.

ecton added a commit that referenced this issue Nov 26, 2023
Closes #14

This was an unfortunate oversight. This edge case only occurs when the
first entry of a new log file is rolled back.

The code that was designed to re-write the file header when reverting to
length 0 was slightly wrong. The file header was being written at the
location of the rolled back entry due to checking if the fsynced length
was 0 rather than if the newly set length was 0. Since the file never
had a commit() success, synchronized_length was 0 but length was 5.

The fix is to check if length is 0 instead before rewriting the header.

The test case from #14 has been added as a new unit test.
@ecton
Copy link
Member

ecton commented Nov 26, 2023

The release is out, and I've yanked all previous versions on crates.io due to the risk of data loss. Thank you again for reporting this and providing such a simple test case.

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

No branches or pull requests

2 participants