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

RO db should allow access to tx.Page(...) thus bbolt pages #371

Closed
wants to merge 1 commit into from

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Dec 30, 2022

I discoved that after change #365 the 'bbolt pages' command stopped working.

The problem is this check:

bbolt/tx.go

Line 663 in b654ce9

if tx.db.freelist.freed(pgid(id)) {
that assumes that freepages has been loaded.

We can either preload the freepages when opening a RO file:

bbolt/db.go

Lines 280 to 284 in b654ce9

if db.readOnly {
return db, nil
}
db.loadFreelist()

but this would slow-down opening RO files.

Thus for now I disable checking whether page is free...

Signed-off-by: Piotr Tabor ptab@google.com

@ahrtr
Copy link
Member

ahrtr commented Jan 3, 2023

Thanks @ptabor. I was planning to add test cases to cover all the bolt commands in main_test.go, but unfortunately I do not get time to do it so far. Otherwise, I should can discover this issue earlier.

Usually we don't need to load freelist at all for the readonly mode. But it's needed in some cases/commands (e.g, check and pages commands), we just need to load freelist on demand for such cases instead of for all readonly cases. FYI.

bbolt/tx.go

Lines 418 to 419 in b654ce9

// Force loading free list if opened in ReadOnly mode.
tx.db.loadFreelist()

So I'd suggest to make change something like below,

$ git diff
diff --git a/tx.go b/tx.go
index 269a18f..9d847eb 100644
--- a/tx.go
+++ b/tx.go
@@ -651,6 +651,9 @@ func (tx *Tx) Page(id int) (*PageInfo, error) {
                return nil, nil
        }
 
+       // Force loading free list if opened in ReadOnly mode.
+       tx.db.loadFreelist()
+
        // Build the page info.
        p := tx.db.page(pgid(id))
        info := &PageInfo{

cmd/bbolt/main_test.go Outdated Show resolved Hide resolved
@ptabor
Copy link
Contributor Author

ptabor commented Jan 3, 2023

Thanks @ptabor. I was planning to add test cases to cover all the bolt commands in main_test.go, but unfortunately I do not get time to do it so far. Otherwise, I should can discover this issue earlier.

Usually we don't need to load freelist at all for the readonly mode. But it's needed in some cases/commands (e.g, check and pages commands), we just need to load freelist on demand for such cases instead of for all readonly cases. FYI.

bbolt/tx.go

Lines 418 to 419 in b654ce9

// Force loading free list if opened in ReadOnly mode.
tx.db.loadFreelist()

So I'd suggest to make change something like below,

$ git diff
diff --git a/tx.go b/tx.go
index 269a18f..9d847eb 100644
--- a/tx.go
+++ b/tx.go
@@ -651,6 +651,9 @@ func (tx *Tx) Page(id int) (*PageInfo, error) {
                return nil, nil
        }
 
+       // Force loading free list if opened in ReadOnly mode.
+       tx.db.loadFreelist()
+
        // Build the page info.
        p := tx.db.page(pgid(id))
        info := &PageInfo{

@ahrtr I don't expect getters (and Page(id)) is a getter to perform mutating operation.
And this operation (loadFreelist) is especially difficult (long latency, high probability of detecting data corruption).

We might emit warning (although we don't have good logging practices in bbolt and just console.log would be very spammy, how to use this properly.... But either we need explicit RO_WITH_FREELIST mode of opening, or
exposed db.PrepareForPageInfo() call to explicitly load the free-pages.

I discoved that after change etcd-io#365 the
'bbolt pages' command stopped working.

The problem is this check:
https://github.com/etcd-io/bbolt/blob/b654ce922133ff49bb385297f30835ca357bac5f/tx.go#L663
that assumes that freepages has been loaded.

We can either preload the freepages when opening a RO file:
  https://github.com/etcd-io/bbolt/blob/b654ce922133ff49bb385297f30835ca357bac5f/db.go#L280-L284
but this would slow-down opening RO files.

Thus for now I disable checking whether page is free...

Signed-off-by: Piotr Tabor <ptab@google.com>
@ahrtr
Copy link
Member

ahrtr commented Jan 3, 2023

I don't expect getters (and Page(id)) is a getter to perform mutating operation.

loadFreelist doesn't change anything in the db file, but of course, it changes the freelist in memory.

And this operation (loadFreelist) is especially difficult (long latency, high probability of detecting data corruption).

This is a good point.

But either we need explicit RO_WITH_FREELIST mode of opening, or exposed db.PrepareForPageInfo() call to explicitly load the free-pages.

Can we add a field something like PreLoadFreelist in Options. It defaults to false, but If it's opened in write mode, it's always set as true. If readonly mode, then only load freelist when it's set as true by users. We also need to call loadFreelist in check and Page (See comment #371 (comment))

@ahrtr
Copy link
Member

ahrtr commented Jan 11, 2023

#381

@ptabor
Copy link
Contributor Author

ptabor commented Jan 11, 2023

Dropping on behalf for: #381

@ptabor ptabor closed this Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants