Separate shadow pages from wal records and rework wal to use serializer #3204
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3204 +/- ##
==========================================
+ Coverage 92.28% 92.34% +0.05%
==========================================
Files 1161 1162 +1
Lines 44150 44192 +42
==========================================
+ Hits 40744 40808 +64
+ Misses 3406 3384 -22 ☔ View full report in Codecov by Sentry. |
ray6080
force-pushed
the
shadow-wal-separation
branch
from
f9e839a
to
1f4a573
Compare
ray6080
force-pushed
the
shadow-wal-separation
branch
from
3ca5ce7
to
17af40b
Compare
| } | ||
| return newVal; | ||
| } catch (std::bad_cast& e) { | ||
| KU_ASSERT(false); |
There was a problem hiding this comment.
What's the benefit of the assert here? It doesn't seem like it would be any clearer of an error than the exception.
Is is just so that it gets caught if we catch kuzu::common::Exception?
There was a problem hiding this comment.
It's just that it's easier to debug with ku_assert than catching the std::bad_cast exception manually.
| @@ -58,9 +63,17 @@ void BufferedFileReader::read(uint8_t* data, uint64_t size) { | |||
| } | |||
| } | |||
|
|
|||
| bool BufferedFileReader::finished() { | |||
| return bufferOffset >= bufferSize && fileSize <= fileOffset; | |||
There was a problem hiding this comment.
Is this always going to be correct? What if the last page in the file is only partially filled?
From where it's being used before it seems to me that it will continue reading the empty data at the end of the page, which will mess up the check for whether or not the last record is a WAL.
I think there should be some way of tracking how many records are in the WAL, or alternatively have a record type of 0 be used to mark invalid records, and then as long as the buffer gets reset to zero (which the bufferedfilewriter is doing) then it can break after it reaches a record with a type of 0.
There was a problem hiding this comment.
bufferSize = std::min(fileSize - fileOffset, BUFFER_SIZE); bufferSize is bounded by the actual fileSize. so it won't read empty data at the end of the last page if it's partially filled.
There was a problem hiding this comment.
Ensuring wal is not corrupted is another thing we need to handle. I feel we might need to do checksum for records.
| KU_UNREACHABLE; | ||
| } | ||
| } | ||
| walRecord->type = type; |
There was a problem hiding this comment.
Isn't this redundant since it's also being set in the constructors?
There was a problem hiding this comment.
Not really. deserializer uses default constructor which doesn't take in type. though a better way is to remove default constructor. I will do that in another pr later.
src/storage/wal_replayer.cpp
Outdated
| "Unrecognized WAL record type inside WALReplayer::replay. recordType: " + | ||
| walRecordTypeToString(walRecord.recordType)); | ||
| throw RuntimeException("Unrecognized WAL record type inside WALReplayer::replay. type: " + | ||
| walRecordTypeToString(walRecord.type)); |
There was a problem hiding this comment.
Won't walRecordTypeToString always produce KU_UNREACHABLE here? I think it would only be used if one of the known types is missing here (for that matter, what's CREATE_REL_TABLE_GROUP_RECORD for? It doesn't seem to be used).
There was a problem hiding this comment.
Yeah. nice catch here. I cleaned up these toString functions. should just put KU_UNREACHABLE here.
Removed CREATE_REL_TABLE_GROUP_RECORD. not used for now.
| @@ -135,5 +135,5 @@ TEST_F(NodeInsertionDeletionTests, TruncatedWalTest) { | |||
| walFileInfo->truncate(BufferPoolConstants::PAGE_4KB_SIZE); | |||
| } | |||
| // Re-open database | |||
| database = std::make_unique<Database>(databasePath); | |||
| EXPECT_THROW(database = std::make_unique<Database>(databasePath), RuntimeException); | |||
There was a problem hiding this comment.
I'm not sure it's helpful to include this test if we expect it to fail now.
There was a problem hiding this comment.
I will keep it for now as a test on malformed wal file. we should add more tests on malformed wal, and rework this later.
No description provided.