From 37ff827e5989d5b344a380f602fa319634d29b05 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 14 Feb 2022 14:53:25 +0800 Subject: [PATCH 1/4] Use CRC64 as checksum for LogFile. Refine some testing codes Signed-off-by: JaySon-Huang --- dbms/src/Storages/Page/V3/LogFile/LogFormat.h | 7 ++--- .../Storages/Page/V3/LogFile/LogReader.cpp | 2 +- dbms/src/Storages/Page/V3/LogFile/LogWriter.h | 12 ++++----- .../Storages/Page/V3/tests/gtest_wal_log.cpp | 26 +++++++++---------- 4 files changed, 23 insertions(+), 24 deletions(-) diff --git a/dbms/src/Storages/Page/V3/LogFile/LogFormat.h b/dbms/src/Storages/Page/V3/LogFile/LogFormat.h index 62785d5b850..10eb1c4800f 100644 --- a/dbms/src/Storages/Page/V3/LogFile/LogFormat.h +++ b/dbms/src/Storages/Page/V3/LogFile/LogFormat.h @@ -29,13 +29,14 @@ static constexpr UInt8 MaxRecordType = RecyclableLastType; static constexpr UInt32 BLOCK_SIZE = 32 * 1024; static_assert(BLOCK_SIZE < std::numeric_limits::max()); -using ChecksumClass = Digest::CRC32; // TODO: CRC64 +// CRC32 or CRC64; City128, XXH3 is not well tested for LogFile +using ChecksumClass = Digest::CRC64; using ChecksumType = ChecksumClass::HashType; -static constexpr UInt32 CHECKSUM_FIELD_SIZE = sizeof(ChecksumType); +static constexpr UInt32 CHECKSUM_FIELD_SIZE = ChecksumClass::hash_size; -// If the size of payload is larger than `BLOCK_SIZE`, it will be splited into +// If the size of payload is larger than `BLOCK_SIZE`, it will be splitted into // fragments. So `PAYLOAD_FIELD_SIZE` must be fit in UInt16. static constexpr UInt32 PAYLOAD_FIELD_SIZE = sizeof(UInt16); diff --git a/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp b/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp index 6e9915b44be..199de44df26 100644 --- a/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp +++ b/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp @@ -383,7 +383,7 @@ UInt8 LogReader::readPhysicalRecord(std::string_view * result, size_t * drop_siz } else if (err != 0) return err; - // else parse header successe. + // else parse header success. if (verify_checksum) { diff --git a/dbms/src/Storages/Page/V3/LogFile/LogWriter.h b/dbms/src/Storages/Page/V3/LogFile/LogWriter.h index 9ab1813ba46..ab7332b8ede 100644 --- a/dbms/src/Storages/Page/V3/LogFile/LogWriter.h +++ b/dbms/src/Storages/Page/V3/LogFile/LogWriter.h @@ -25,23 +25,23 @@ namespace PS::V3 * +-----+-------------+--+----+----------+------+-- ... ----+ * File | r0 | r1 |P | r2 | r3 | r4 | | * +-----+-------------+--+----+----------+------+-- ... ----+ - * <--- kBlockSize ------>|<-- kBlockSize ------>| + * <---- BlockSize ------>|<--- BlockSize ------>| * rn = variable size records * P = Padding * - * Data is written out in kBlockSize chunks. If next record does not fit + * Data is written out in BlockSize chunks. If next record does not fit * into the space left, the leftover space will be padded with \0. * * Legacy record format: * * +--------------+-----------+-----------+--- ... ---+ - * |CheckSum (4B) | Size (2B) | Type (1B) | Payload | + * |CheckSum (8B) | Size (2B) | Type (1B) | Payload | * +--------------+-----------+-----------+--- ... ---+ * - * CheckSum = 32bit hash computed over the record type and payload using checksum algo + * CheckSum = 64bit hash computed over the record type and payload using checksum algo (CRC64) * Size = Length of the payload data * Type = Type of record - * (kZeroType, kFullType, kFirstType, kLastType, kMiddleType ) + * (ZeroType, FullType, FirstType, LastType, MiddleType) * The type is used to group a bunch of records together to represent * blocks that are larger than kBlockSize * Payload = Byte stream as long as specified by the payload size @@ -49,7 +49,7 @@ namespace PS::V3 * Recyclable record format: * * +--------------+-----------+-----------+----------------+--- ... ---+ - * |CheckSum (4B) | Size (2B) | Type (1B) | Log number (4B)| Payload | + * |CheckSum (8B) | Size (2B) | Type (1B) | Log number (4B)| Payload | * +--------------+-----------+-----------+----------------+--- ... ---+ * * Same as above, with the addition of diff --git a/dbms/src/Storages/Page/V3/tests/gtest_wal_log.cpp b/dbms/src/Storages/Page/V3/tests/gtest_wal_log.cpp index 696cac0195c..b0a416a9991 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_wal_log.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_wal_log.cpp @@ -202,19 +202,17 @@ class LogFileRWTest : public ::testing::TestWithParam> PageUtil::ftruncateFile(wr_file, writtenBytes() - bytes); } - void fixChecksum(int header_offset, int len, bool recyclable) + void fixChecksum(int header_offset, int payload_len, bool recyclable) { // Compute crc of type/len/data int header_size = recyclable ? Format::RECYCLABLE_HEADER_SIZE : Format::HEADER_SIZE; - Digest::CRC32 digest; + Format::ChecksumClass digest; - size_t crc_buff_size = header_size - Format::CHECKSUM_START_OFFSET + len; + size_t crc_buff_size = header_size - Format::CHECKSUM_START_OFFSET + payload_len; char crc_buff[crc_buff_size]; PageUtil::readFile(wr_file, header_offset + Format::CHECKSUM_START_OFFSET, crc_buff, crc_buff_size, nullptr); - digest.update( - crc_buff, - crc_buff_size); + digest.update(crc_buff, crc_buff_size); auto checksum = digest.checksum(); PageUtil::writeFile(wr_file, header_offset, reinterpret_cast(&checksum), sizeof(checksum), nullptr); @@ -407,7 +405,7 @@ TEST_P(LogFileRWTest, BadRecordType) TEST_P(LogFileRWTest, TruncatedTrailingRecordIsIgnored) { write("foo"); - shrinkSize(4); // Drop all payload as well as a header byte + shrinkSize(3 + sizeof(Format::MaxRecordType)); // Drop all payload as well as a header byte resetReader(); ASSERT_EQ("EOF", read()); // Truncated last record is ignored, not treated as an error @@ -425,7 +423,7 @@ TEST_P(LogFileRWTest, TruncatedTrailingRecordIsNotIgnored) } write("foo"); - shrinkSize(4); // Drop all payload as well as a header byte + shrinkSize(3 + sizeof(Format::MaxRecordType)); // Drop all payload as well as a header byte resetReader(WALRecoveryMode::AbsoluteConsistency); ASSERT_EQ("EOF", read()); // Truncated last record is ignored, not treated as an error @@ -447,8 +445,8 @@ TEST_P(LogFileRWTest, BadLength) write(repeatedString("bar", payload_size)); write("foo"); resetReader(); - // Least significant size byte is stored in header[4]. - incrementByte(4, 1); + // Least significant size byte is stored in header[SizePos]. + incrementByte(Format::CHECKSUM_FIELD_SIZE, 1); if (!recyclable_log) { ASSERT_EQ("foo", read()); @@ -498,11 +496,11 @@ TEST_P(LogFileRWTest, BadLengthAtEndIsNotIgnored) TEST_P(LogFileRWTest, ChecksumMismatch) { write("foooooo"); - incrementByte(0, 14); + incrementByte(0, Format::HEADER_SIZE + 7); ASSERT_EQ("EOF", read()); if (!recyclable_log) { - ASSERT_EQ(14, droppedBytes()); + ASSERT_EQ(Format::HEADER_SIZE + 7, droppedBytes()); ASSERT_EQ("OK", matchError("checksum mismatch")); } else @@ -560,7 +558,7 @@ TEST_P(LogFileRWTest, MissingLastIsIgnored) { write(repeatedString("bar", PS::V3::Format::BLOCK_SIZE)); // Remove the LAST block, including header. - shrinkSize(14); + shrinkSize(2 * (recyclable_log ? Format::RECYCLABLE_HEADER_SIZE : Format::HEADER_SIZE)); ASSERT_EQ("EOF", read()); ASSERT_EQ("", reportMessage()); ASSERT_EQ(0, droppedBytes()); @@ -577,7 +575,7 @@ TEST_P(LogFileRWTest, MissingLastIsNotIgnored) resetReader(WALRecoveryMode::AbsoluteConsistency); write(repeatedString("bar", PS::V3::Format::BLOCK_SIZE)); // Remove the LAST block, including header. - shrinkSize(14); + shrinkSize(2 * (recyclable_log ? Format::RECYCLABLE_HEADER_SIZE : Format::HEADER_SIZE)); ASSERT_EQ("EOF", read()); ASSERT_GT(droppedBytes(), 0); ASSERT_EQ("OK", matchError("Corruption: error reading trailing data")); From 60820e5efd1d3050966471c102eceda1cb9e2906 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 14 Mar 2022 21:29:13 +0800 Subject: [PATCH 2/4] Debug log --- dbms/src/Storages/Page/V3/LogFile/LogReader.cpp | 10 ++++++++++ dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp b/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp index 199de44df26..cee855caa1e 100644 --- a/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp +++ b/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -306,6 +307,7 @@ LogReader::deserializeHeader(LogReader::RecyclableHeader * hdr, size_t * drop_si readIntBinary(hdr->checksum, *file); readIntBinary(hdr->length, *file); readChar(hdr->type, *file); + LOG_FMT_DEBUG(&Poco::Logger::get("fff"), "Read crc: {:0X} length: {} type: {}", hdr->checksum, hdr->length, static_cast(hdr->type)); size_t header_size = Format::HEADER_SIZE; if (hdr->type >= Format::RecyclableFullType && hdr->type <= Format::RecyclableLastType) { @@ -393,6 +395,14 @@ UInt8 LogReader::readPhysicalRecord(std::string_view * result, size_t * drop_siz hdr.length + header_size - Format::CHECKSUM_START_OFFSET); if (Format::ChecksumType actual_checksum = digest.checksum(); actual_checksum != hdr.checksum) { + LOG_FMT_DEBUG( + &Poco::Logger::get("fff"), + "checksum for pos:{}, size:{}, cks:{:0X}, exp_cks:{:0X}, c:{}", + buffer.data() - header_pos, + hdr.length + header_size - Format::CHECKSUM_START_OFFSET, + actual_checksum, + hdr.checksum, + Redact::keyToHexString(header_pos + Format::CHECKSUM_START_OFFSET, hdr.length + header_size - Format::CHECKSUM_START_OFFSET)); // Drop the rest of the buffer since "length" itself may have // been corrupted and if we trust it, we could find some // fragment of a real log record that just happens to look diff --git a/dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp b/dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp index 5b5773785d9..0c14340a725 100644 --- a/dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp +++ b/dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -181,6 +182,13 @@ void LogWriter::emitPhysicalRecord(Format::RecordType type, ReadBuffer & payload writeString(header_buff.buffer().begin(), header_buff.count(), write_buffer); writeString(payload.position(), length, write_buffer); + LOG_FMT_DEBUG( + &Poco::Logger::get("fff"), + "CRC: {:08X} header: {} payload: {}", + checksum, + Redact::keyToHexString(header_buff.buffer().begin(), header_buff.count()), + Redact::keyToHexString(payload.position(), length)); + block_offset += header_size + length; } } // namespace DB::PS::V3 From df626ec0362b4564f01a819e206af4745b99f34d Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 14 Mar 2022 21:29:32 +0800 Subject: [PATCH 3/4] Revert "Debug log" This reverts commit 60820e5efd1d3050966471c102eceda1cb9e2906. --- dbms/src/Storages/Page/V3/LogFile/LogReader.cpp | 10 ---------- dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp | 8 -------- 2 files changed, 18 deletions(-) diff --git a/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp b/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp index cee855caa1e..199de44df26 100644 --- a/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp +++ b/dbms/src/Storages/Page/V3/LogFile/LogReader.cpp @@ -1,7 +1,6 @@ #include #include #include -#include #include #include #include @@ -307,7 +306,6 @@ LogReader::deserializeHeader(LogReader::RecyclableHeader * hdr, size_t * drop_si readIntBinary(hdr->checksum, *file); readIntBinary(hdr->length, *file); readChar(hdr->type, *file); - LOG_FMT_DEBUG(&Poco::Logger::get("fff"), "Read crc: {:0X} length: {} type: {}", hdr->checksum, hdr->length, static_cast(hdr->type)); size_t header_size = Format::HEADER_SIZE; if (hdr->type >= Format::RecyclableFullType && hdr->type <= Format::RecyclableLastType) { @@ -395,14 +393,6 @@ UInt8 LogReader::readPhysicalRecord(std::string_view * result, size_t * drop_siz hdr.length + header_size - Format::CHECKSUM_START_OFFSET); if (Format::ChecksumType actual_checksum = digest.checksum(); actual_checksum != hdr.checksum) { - LOG_FMT_DEBUG( - &Poco::Logger::get("fff"), - "checksum for pos:{}, size:{}, cks:{:0X}, exp_cks:{:0X}, c:{}", - buffer.data() - header_pos, - hdr.length + header_size - Format::CHECKSUM_START_OFFSET, - actual_checksum, - hdr.checksum, - Redact::keyToHexString(header_pos + Format::CHECKSUM_START_OFFSET, hdr.length + header_size - Format::CHECKSUM_START_OFFSET)); // Drop the rest of the buffer since "length" itself may have // been corrupted and if we trust it, we could find some // fragment of a real log record that just happens to look diff --git a/dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp b/dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp index 0c14340a725..5b5773785d9 100644 --- a/dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp +++ b/dbms/src/Storages/Page/V3/LogFile/LogWriter.cpp @@ -1,6 +1,5 @@ #include #include -#include #include #include #include @@ -182,13 +181,6 @@ void LogWriter::emitPhysicalRecord(Format::RecordType type, ReadBuffer & payload writeString(header_buff.buffer().begin(), header_buff.count(), write_buffer); writeString(payload.position(), length, write_buffer); - LOG_FMT_DEBUG( - &Poco::Logger::get("fff"), - "CRC: {:08X} header: {} payload: {}", - checksum, - Redact::keyToHexString(header_buff.buffer().begin(), header_buff.count()), - Redact::keyToHexString(payload.position(), length)); - block_offset += header_size + length; } } // namespace DB::PS::V3 From fb3af3135d0b00937a457619c637734b3fbd74b2 Mon Sep 17 00:00:00 2001 From: JaySon Date: Tue, 15 Mar 2022 11:16:04 +0800 Subject: [PATCH 4/4] Apply suggestions from code review --- dbms/src/Storages/Page/V3/LogFile/LogFormat.h | 1 - 1 file changed, 1 deletion(-) diff --git a/dbms/src/Storages/Page/V3/LogFile/LogFormat.h b/dbms/src/Storages/Page/V3/LogFile/LogFormat.h index 10eb1c4800f..3a815a2d30d 100644 --- a/dbms/src/Storages/Page/V3/LogFile/LogFormat.h +++ b/dbms/src/Storages/Page/V3/LogFile/LogFormat.h @@ -29,7 +29,6 @@ static constexpr UInt8 MaxRecordType = RecyclableLastType; static constexpr UInt32 BLOCK_SIZE = 32 * 1024; static_assert(BLOCK_SIZE < std::numeric_limits::max()); -// CRC32 or CRC64; City128, XXH3 is not well tested for LogFile using ChecksumClass = Digest::CRC64; using ChecksumType = ChecksumClass::HashType;