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

freelist hashmap/array inconsistency on page reload #791

Closed
tjungblu opened this issue Jul 9, 2024 · 1 comment · Fixed by #794
Closed

freelist hashmap/array inconsistency on page reload #791

tjungblu opened this issue Jul 9, 2024 · 1 comment · Fixed by #794

Comments

@tjungblu
Copy link
Contributor

tjungblu commented Jul 9, 2024

While testing the interface in #786, I've noticed a slight divergence in reload behavior between the array and freelist implementation.

Below testcase showcases the difference very well. TL;DR: we create a freelist with free page ids 5,6,8 and then we create a new one which frees the 5 through 9 in a transaction, marking the whole span as pending.

Now reloading the freelist again from the page that contains 5,6,8 will show different results:

func TestFreeList_reload_page_dedupe(t *testing.T) {
	var buf [4096]byte
	f := newTestFreelist()
	f.readIDs([]common.Pgid{5, 6, 8})

	p := (*common.Page)(unsafe.Pointer(&buf[0]))
	if err := f.write(p); err != nil {
		t.Fatal(err)
	}

	f2 := newTestFreelist()
	f2.free(common.Txid(5), common.NewPage(5, common.LeafPageFlag, 0, 4))
	// reload should deduplicate as a pending page when reading from p's freelist
	f2.reload(p)

	if len(f2.getFreePageIDs()) != 0 {
		t.Fatalf("expected empty; got=%v", f2.getFreePageIDs())
	}
	if exp := []common.Pgid{5, 6, 7, 8, 9}; !reflect.DeepEqual(exp, f2.pending[5].ids) {
		t.Fatalf("exp=%v; got=%v", exp, f2.pending[5].ids)
	}
}

The array freelist will pass and detect the span as pending, the hashmap will have the overlapping pages pending and free.

It stems from the fact that on init, the hashmap will not honor an empty list as an initial state (as in, it will not attempt to initialize anything):
https://github.com/etcd-io/bbolt/blob/main/freelist_hmap.go#L197-L200

Whereas the array will happily take what it's passed and reindex accordingly:
https://github.com/etcd-io/bbolt/blob/main/freelist_array.go#L60-L63

@ahrtr
Copy link
Member

ahrtr commented Jul 9, 2024

Thanks @tjungblu for the finding. Indeed the arrary and hashmap freelist have different behavior on readIDs/init. The behavior of array freelist should be correct.

f2.free(common.Txid(5), common.NewPage(5, common.LeafPageFlag, 0, 4))

freelist will panic when it tries to free an already freed page.

tjungblu added a commit to tjungblu/bbolt that referenced this issue Jul 9, 2024
This reorders some statements in the hashmap initialization to ensure we
always start fresh, even when no pageids were passed to it.

fixes etcd-io#791

Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu added a commit to tjungblu/bbolt that referenced this issue Jul 9, 2024
This reorders some statements in the hashmap initialization to ensure we
always start fresh, even when no pageids were passed to it.

fixes etcd-io#791

Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu added a commit to tjungblu/bbolt that referenced this issue Jul 11, 2024
This reorders some statements in the hashmap initialization to ensure we
always start fresh, even when no pageids were passed to it.

fixes etcd-io#791

Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu added a commit to tjungblu/bbolt that referenced this issue Jul 22, 2024
This reorders some statements in the hashmap initialization to ensure we
always start fresh, even when no pageids were passed to it.

fixes etcd-io#791

Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu added a commit to tjungblu/bbolt that referenced this issue Jul 22, 2024
This reorders some statements in the hashmap initialization to ensure we
always start fresh, even when no pageids were passed to it.

fixes etcd-io#791

Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu added a commit to tjungblu/bbolt that referenced this issue Jul 22, 2024
This reorders some statements in the hashmap initialization to ensure we
always start fresh, even when no pageids were passed to it.

fixes etcd-io#791

Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu added a commit to tjungblu/bbolt that referenced this issue Jul 22, 2024
This reorders some statements in the hashmap initialization to ensure we
always start fresh, even when no pageids were passed to it.

fixes etcd-io#791

Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
@ahrtr ahrtr closed this as completed in ce50f55 Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants