-
-
Notifications
You must be signed in to change notification settings - Fork 262
refactor(pag): Use fallocate
as the primary method for extending a file
#8758
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
refactor(pag): Use fallocate
as the primary method for extending a file
#8758
Conversation
…file - This should eliminate double writes of pages when file is extended. - If fallocate is not supported by filesystem fallback to the old method of writing zeroes to the file.
return std::nullopt; | ||
|
||
// File was extended, reset cached value | ||
maxPageNumber = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was the original code, but still wondering -- why we cannot do maxPageNumber += reqPages
, given we know the file was extended successfully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason I found is that if our page size is less than OS page size, we may get different value from PIO_get_number_of_pages()
because it uses fstat()
, but in all other cases we should get the same value. And as I can see in current code we cannot call PageSpace::extend()
from multiple threads due to fetching PIP with write lock before calling PageSpace::extend()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't it be called from BackupManager::endBackup()
and ensureDiskSpace()
simultaneously? If not, please add a comment near the increment explaining why it's safe. If there are any doubts, better leave it being reset to zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, calls to extendDatabase()
inside endBackup()
happens when backup_state
is hdr_nbak_stalled
, so calls to ensureDiskSpace()
will not result in the file being extended. Also there is BackupManager::StateReadGuard stateGuard(tdbb);
in ensureDiskSpace()
which provides even more protection against races from nbakup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it is very bug-prone in future, if this logic will be changed, so I believe it is better to reset the cached value to zero (and we shouldn't see a significant performance improvement, the additional call to fstat()
to obtain the file size should be fast anyway, as it only uses information from inode (I hope it's fast)).
Restore gets the biggest performance boost from this patch. I tested restore on a database with one table
CREATE TABLE TAB1(V1 VARCHAR(25), V2 VARCHAR(25), V3 VARCHAR(25), V4 VARCHAR(25))
, with a file size of about 16 GB. The testing was done on two different PCs with the same OS (Ubuntu 24.04):Intel Core i5-10400 + WD Red SN700 1000 GB (111150WD)
: +10% speedup with-par 8
;Ryzen 7 7700 + KINGSTON SKC3000S1024G (EIFK31.6)
: +18% speedup with-par 8
;On Windows, the performance improvement is small (about 3-5%), so the real target of this patch is Linux.