Skip to content

Conversation

@mamcx
Copy link
Contributor

@mamcx mamcx commented Aug 14, 2023

Description of Changes

On [test] was not possible to re-open the db because the data was incorrectly replayed after drop.

For example when tried to run:

    #[test]
    fn test_auto_inc_reload() -> ResultTest<()> {
        let (stdb, tmp_dir) = make_test_db()?;

        let mut tx = stdb.begin_tx();
        let schema = TableDef {
            table_name: "MyTable".to_string(),
            columns: vec![ColumnDef {
                col_name: "my_col".to_string(),
                col_type: AlgebraicType::I64,
                is_autoinc: true,
            }],
            indexes: vec![],
            table_type: StTableType::User,
            table_access: StAccess::Public,
        };
        let table_id = stdb.create_table(&mut tx, schema)?;

        let sequence = stdb.sequence_id_from_name(&tx, "MyTable_my_col_seq")?;
        assert!(sequence.is_some(), "Sequence not created");

        stdb.insert(&mut tx, table_id, product![AlgebraicValue::I64(0)])?;

        let mut rows = stdb
            .iter_by_col_range(&tx, table_id, 0, AlgebraicValue::I64(0)..)?
            .map(|r| *r.view().elements[0].as_i64().unwrap())
            .collect::<Vec<i64>>();
        rows.sort();

        assert_eq!(rows, vec![1]);

        stdb.commit_tx(tx)?;
        drop(stdb);

        dbg!("reopen...");
//HERE IT FAILS TO RECOVER THE DATA(sometimes!)
        let stdb = open_db(&tmp_dir, false)?;

        let mut tx = stdb.begin_tx();

        stdb.insert(&mut tx, table_id, product![AlgebraicValue::I64(0)])?;

        let mut rows = stdb
            .iter_by_col_range(&tx, table_id, 0, AlgebraicValue::I64(0)..)?
            .map(|r| *r.view().elements[0].as_i64().unwrap())
            .collect::<Vec<i64>>();
        rows.sort();

        assert_eq!(rows, vec![1, 2]);

        Ok(())
    }

Each run of the test will lead to this content on disk randomly:

Ok:

8e00 0000 0000 0000 0000 0000 0000 0000
0000 0000 0004 0000 0080 0100 0000 1904
0000 0000 0000 0002 0000 0002 0706 0000
006d 795f 636f 6c01 8000 0000 0080 f234
dca8 b06e 3521 1c5b c83b da92 6809 6e8f
0dce a265 1190 206b 228f 6997 3d12 8004
0000 0008 0100 0000 0000 0000 8002 0000
0080 645d 56fb 0fe5 d532 ccc9 51c3 f0c6
302b 29c9 061e 4d50 59f0 c3a8 1d46 bccc
3bbf

Failed:

Zero tables reloaded:

8e00 0000 0000 0000 0000 0000 0000 0000
0000 0000 0004 0000 0080 0400 0000 0801
0000 0000 0000 0080 0000 0000 80f2 34dc
a8b0 6e35 211c 5bc8 3bda 9268 096e 8f0d
cea2 6511 9020 6b22 8f69 973d 1280 0100
0000 1904 0000 0000 0000 0002 0000 0002
0706 0000 006d 795f 636f 6c01 8002 0000
0080 645d 56fb 0fe5 d532 ccc9 51c3 f0c6
302b 29c9 061e 4d50 59f0 c3a8 1d46 bccc
3bbf 

Not reload the created table:

8e00 0000 0000 0000 0000 0000 0000 0000
0000 0000 0004 0000 0080 0100 0000 1904
0000 0000 0000 0002 0000 0002 0706 0000
006d 795f 636f 6c01 8004 0000 0008 0100
0000 0000 0000 8000 0000 0080 f234 dca8
b06e 3521 1c5b c83b da92 6809 6e8f 0dce
a265 1190 206b 228f 6997 3d12 8002 0000
0080 645d 56fb 0fe5 d532 ccc9 51c3 f0c6
302b 29c9 061e 4d50 59f0 c3a8 1d46 bccc
3bbf 

With this PR, the TxState use BtreeMap to keep a stable order and now the files on disk keep the same results.

API

  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

let mut mlog = mlog.lock().unwrap();
mlog.append(&bytes)?;
mlog.sync_all()?;
let mut odb = self.odb.lock().unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not place were we sync the odb, is correct to put this here?

@mamcx mamcx requested a review from cloutiertyler August 14, 2023 16:49
@mamcx mamcx marked this pull request as ready for review August 15, 2023 18:26
@mamcx mamcx force-pushed the mamcx/db_reopen_binary branch from 63bb070 to ff2e74b Compare August 30, 2023 18:16
@mamcx mamcx requested review from kim and kulakowski August 30, 2023 18:17
Copy link
Contributor

@kim kim left a comment

Choose a reason for hiding this comment

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

I do not fully understand why it makes the test fail, but a deterministic order seems like a good idea.

if self.fsync {
mlog.sync_all()?;
let mut odb = self.odb.lock().unwrap();
odb.sync_all()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems important 😅

@mamcx mamcx merged commit bb322b5 into master Aug 31, 2023
bfops added a commit that referenced this pull request Jul 16, 2025
* Version Packages

* revert major version bump

* changelog

* fix changelog

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Zeke Foppa <[email protected]>
bfops pushed a commit that referenced this pull request Jul 17, 2025
* Add docs for standalone config.toml

* Update docs/cli-reference/standalone-config.md

Co-authored-by: Phoebe Goldman <[email protected]>

* pre formatting

---------

Co-authored-by: Phoebe Goldman <[email protected]>
bfops added a commit that referenced this pull request Aug 7, 2025
* Version Packages

* revert major version bump

* changelog

* fix changelog

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Zeke Foppa <[email protected]>
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.

3 participants