Skip to content

Fix page_count pragma #2099

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

Merged
merged 5 commits into from
Jul 24, 2025
Merged

Fix page_count pragma #2099

merged 5 commits into from
Jul 24, 2025

Conversation

meteorgan
Copy link
Contributor

@meteorgan meteorgan commented Jul 15, 2025

Closes: #1415

What this PR does

  1. Removes database initialization from the read_tx function.
  2. Adds checks for database initialization when executing .schema, .indexes, .tables and .import commands, as they rely on sqlite_schema table.

About the second issue

I think we have another solution for the second issue: create the sqlite_schema table in Schema only during page1 initialization, rather than during Schema initialization.

Pros

This approach has the advantage of unifying the logic for the sqlite_schema table with other user tables when running select statements

Cons

  • we still need to check error codes for commands like .schema.
  • this approach may increase the complexity of the pager implementation.

I'd like to hear your thoughts and feedback.

@meteorgan meteorgan force-pushed the fix-page-count branch 2 times, most recently from 0d6d908 to 948a5d9 Compare July 23, 2025 09:58
@github-actions github-actions bot added the cli label Jul 23, 2025
@meteorgan meteorgan force-pushed the fix-page-count branch 2 times, most recently from 0528a2a to cf4bb87 Compare July 23, 2025 12:29
@meteorgan meteorgan marked this pull request as ready for review July 23, 2025 13:10
@jussisaurio
Copy link
Collaborator

I think sqlite_schema initialization during page1 initialization makes intuitive sense. Do you want to do it here or leave it as a follow-up issue to be implemented by whoever?

@meteorgan
Copy link
Contributor Author

I think sqlite_schema initialization during page1 initialization makes intuitive sense. Do you want to do it here or leave it as a follow-up issue to be implemented by whoever?

Let me take care of it here.

@meteorgan
Copy link
Contributor Author

meteorgan commented Jul 24, 2025

I think sqlite_schema initialization during page1 initialization makes intuitive sense. Do you want to do it here or leave it as a follow-up issue to be implemented by whoever?

I should note that most work of the sqlite_shema table initialization has been completed during page1 initialization.

turso/core/storage/pager.rs

Lines 1428 to 1439 in 46f5609

// Create the sqlite_schema table, for this we just need to create the btree page
// for the first page of the database which is basically like any other btree page
// but with a 100 byte offset, so we just init the page so that sqlite understands
// this is a correct page.
btree_init_page(
&page1,
PageType::TableLeaf,
DATABASE_HEADER_SIZE,
(default_header.get_page_size() - default_header.reserved_space as u32) as u16,
);
let write_counter = Rc::new(RefCell::new(0));
begin_write_btree_page(self, &page1.get(), write_counter.clone())?;

the remaining task is adding sqlite_schema table to Schema which occurs during Schema creation now.

@meteorgan
Copy link
Contributor Author

meteorgan commented Jul 24, 2025

the remaining task is adding sqlite_schema table to Schema which occurs during Schema creation.

Moving this logic to page1 initialization presents some difficulties:

  1. several code paths rely on sqlite_schema information before the database is fully initialized. including:
    let table = schema.get_btree_table(SQLITE_TABLEID).unwrap();
    let sqlite_schema_cursor_id = program.alloc_cursor_id(CursorType::BTreeTable(table.clone()));
    program.emit_insn(Insn::OpenWrite {
    cursor_id: sqlite_schema_cursor_id,
    root_page: 1usize.into(),
    db: 0,
    });
  2. the Mutex<Arc<Schema>> in database prevents modifications in the pager

@penberg penberg merged commit 2141293 into tursodatabase:main Jul 24, 2025
59 checks passed
@meteorgan meteorgan deleted the fix-page-count branch July 24, 2025 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty database page count is not consistent with SQLite
3 participants