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

No need to handle freelist as a specical case when freeing a page #788

Merged
merged 1 commit into from
Jul 22, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions internal/freelist/shared.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,6 @@ func (t *shared) Free(txid common.Txid, p *common.Page) {
allocTxid, ok := t.allocs[p.Id()]
if ok {
delete(t.allocs, p.Id())
} else if p.IsFreelistPage() {
// Freelist is always allocated by prior tx.
allocTxid = txid - 1
Copy link
Member

@fuweid fuweid Jul 18, 2024

Choose a reason for hiding this comment

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

The Line 79 txp.alloctx = append(txp.alloctx, allocTxid) could append zero into alloctx.

There are two cases we should consider:

Rollback

The zero value will be covered by the rollback. So it looks fine.

tx := txp.alloctx[i]
if tx == 0 {
continue
}

Release by opening new writable TX

This change looks safe to me because the page will be free after all the read-only TX are closed.
I think the original Freelist is always allocated by prior tx. might free the page which can be read by opening read-only TX.

for i := 0; i < len(txp.ids); i++ {
if atx := txp.alloctx[i]; atx < begin || atx > end {
continue
}
m = append(m, txp.ids[i])

}

for id := p.Id(); id <= p.Id()+common.Pgid(p.Overflow()); id++ {
Expand Down