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

[ptabor]

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]

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{

[ptabor]

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.

[ahrtr]

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]

#381

[ptabor]

Dropping on behalf for: #381