PBS-42 feature: Implement basic support for COM_BINLOG_DUMP packet handling (part 1) by percona-ysorokin · Pull Request #191 · Percona-Lab/percona-binlog-server

percona-ysorokin · 2026-09-23T11:39:34Z

https://perconadev.atlassian.net/browse/PBS-42

Introduced new 'util::byte_range' class for representing byte ranges
in files. It supports empty ranges, full ranges, open-ended ranges,
and specific byte ranges.
'binsrv::basic_storage_backend' interface extended to support ranged
reads (via optional parameter of type 'util::byte_range').
Increased the size of max in-memory object supported by the
'get_object()' operation in both 's3' and 'filesystem' implementations
of the 'binsrv::basic_storage_backend'.
Reworked 'util::read_file_content()' utility function to support
ranged filesystem reads.
Fixed some typos in code comments.

kamil-holubicki · 2026-09-23T12:09:59Z

+  if (offset != 0 || length.has_value()) {
+    std::string range{"bytes=" + std::to_string(offset) + "-"};
+    if (length.has_value()) {
+      range += std::to_string(offset + length.value() - 1);


What if offset == 0 and length == 0?

Good point. Yes, we'd better make an early return in this case.

kamil-holubicki · 2026-09-23T12:15:38Z

+            "received memory content");
+      }
+    } else {
+      if (content_length > max_memory_object_size) {


isn't stream_handler called after receiving the object from s3? So it is already in the memory.

Yes, this is a compromise we had to make here. Unfortunately, there is no way in S3 API to "read no more than N bytes from an S3 object" in a single request. So, we either need to send another request (HeadObject) before this call, or simply rely on the fact that we won't be abusing this function.
See the TODO item above.
I don't like this either to be honest.

kamil-holubicki · 2026-09-23T12:17:21Z

+  std::uint64_t read_length{};
+  if (length.has_value()) {
+    read_length = length.value();
+    if (offset + read_length > file_size) {


overflow possible. use read_length > file_size - offset

Yes, good point.

kamil-holubicki · 2026-09-23T12:18:43Z

 public:
-  static constexpr std::size_t max_memory_object_size{1048576U};
+  // 256 MB
+  static constexpr std::size_t max_memory_object_size{256UZ << 20UZ};


we have the same in s3_storage_backend

Will move this to basic_storage_backend.

Copilot

Copilot review overview

🟡 Changes recommended

The S3 ranged-read path can materialize oversized responses before enforcing the memory limit.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Enforce suffix size limit before downloading object · New

What changed in this PR

Adds ranged object/file reads via a new util::byte_range abstraction to support upcoming binlog streaming functionality.

Changes:

Adds byte-range validation and formatting.
Extends filesystem and S3 storage APIs with ranged reads.
Updates memory limits, callers, and related comments.

File	Summary
`src/util/file_operations_helpers.hpp`	Declares ranged file reads.
`src/util/file_operations_helpers.cpp`	Implements ranged filesystem reads.
`src/util/byte_range.hpp`	Defines the byte-range API.
`src/util/byte_range.cpp`	Implements validation and formatting.
`src/util/byte_range_fwd.hpp`	Adds the forward declaration.
`src/binsrv/storage_core.hpp`	Corrects comments.
`src/binsrv/storage_core.cpp`	Corrects comments.
`src/binsrv/s3_storage_backend.hpp`	Updates the S3 backend interface.
`src/binsrv/s3_storage_backend.cpp`	Implements S3 ranged requests.
`src/binsrv/main_config.cpp`	Updates file-read usage.
`src/binsrv/keyring_record_collection.cpp`	Updates file-read usage.
`src/binsrv/filesystem_storage_backend.hpp`	Updates the filesystem backend interface.
`src/binsrv/filesystem_storage_backend.cpp`	Implements ranged filesystem access.
`src/binsrv/basic_storage_backend.hpp`	Extends the backend API and memory limit.
`src/binsrv/basic_storage_backend.cpp`	Forwards ranges to implementations.
`mtr/binlog_streaming/t/checkpointing.test`	Corrects a comment.
`extra/mysql_protocol/mysql/harness/stdx/ranges.h`	Corrects a comment.
`CMakeLists.txt`	Registers byte-range sources.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    if (range.has_length()) {
+      if (content_length != range.get_length()) {
+        util::exception_location().raise<std::out_of_range>(
+            "The requested S3 object range does not match the length of the "
+            "received memory content");
+      }
+    } else {
+      if (content_length > max_memory_object_size) {
+        util::exception_location().raise<std::out_of_range>(
+            "S3 object is too large to be loaded in memory");


kamil-holubicki · 2026-09-24T12:00:46Z

+
+  [[nodiscard]] bool has_length() const noexcept { return length_.has_value(); }
+
+  [[nodiscard]] std::uint64_t get_length() const noexcept {


It will return 0 in both cases: if has length and it is 0 or it doesn't have length. That forces the caller to always check has_length() anyways. Wouldn't it be better to return optional?

Yes, I just wanted it to be a simple accessor (with noexcept).
And instead of

byte_range br{...}; if (bt.get_length_opt().vas_value()) { std::cout << bt.get_length_opt().value(); }

write something like

byte_range br{...}; if (bt.has_length()) { std::cout << bt.get_length(); }

Although I understand that value_or() makes another unnecessary nullness check in my implementation.

kamil-holubicki · 2026-09-24T12:02:23Z

+  if (length_.has_value()) {
+    result += std::to_string(offset_ + *length_ - 1ULL);
+  }
+  return result;


It can be strange to get empty string, when is_full and on the other hand have an exception when is_empty
OK, I understand that it is used to form HTTP request, but in such a case it is rather an s3 class thing, not byte_range class responsibility. In other words, when I call to_string() I expect it to return meaningful string for logs for example, not a specific behavior that only s3 backend knows why

Agree. If we extracted this byte_range into util as a generic reusable component, let's not bind us to S3 format.

…ndling (part 1) https://perconadev.atlassian.net/browse/PBS-42 * Introduced new 'util::byte_range' class for representing byte ranges in files. It supports empty ranges, full ranges, open-ended ranges, and specific byte ranges. * 'binsrv::basic_storage_backend' interface extended to support ranged reads (via optional parameter of type 'util::byte_range'). * Increased the size of max in-memory object supported by the 'get_object()' operation in both 's3' and 'filesystem' implementations of the 'binsrv::basic_storage_backend'. * Reworked 'util::read_file_content()' utility function to support ranged filesystem reads. * Fixed some typos in code comments.

kamil-holubicki reviewed Sep 23, 2026

View reviewed changes

percona-ysorokin force-pushed the com_binlog_dump_range_reads branch from 832ca27 to 883fb21 Compare September 23, 2026 23:26

percona-ysorokin requested a lite review from Copilot September 24, 2026 10:03

Copilot started reviewing on behalf of percona-ysorokin September 24, 2026 10:03 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

kamil-holubicki approved these changes Sep 24, 2026

View reviewed changes

percona-ysorokin force-pushed the com_binlog_dump_range_reads branch from 883fb21 to e06bdf2 Compare September 24, 2026 12:57

percona-ysorokin merged commit 716d245 into Percona-Lab:main Sep 28, 2026
8 checks passed

Navigation Menu

PBS-42 feature: Implement basic support for COM_BINLOG_DUMP packet handling (part 1) - #191

PBS-42 feature: Implement basic support for COM_BINLOG_DUMP packet handling (part 1)#191
percona-ysorokin merged 1 commit into
Percona-Lab:mainfrom
percona-ysorokin:com_binlog_dump_range_reads

percona-ysorokin commented Sep 23, 2026 •

edited

Loading

Uh oh!

kamil-holubicki Sep 23, 2026

Uh oh!

percona-ysorokin Sep 23, 2026

Uh oh!

kamil-holubicki Sep 23, 2026

Uh oh!

percona-ysorokin Sep 23, 2026 •

edited

Loading

Uh oh!

kamil-holubicki Sep 23, 2026

Uh oh!

percona-ysorokin Sep 23, 2026

Uh oh!

kamil-holubicki Sep 23, 2026

Uh oh!

percona-ysorokin Sep 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

kamil-holubicki Sep 24, 2026

Uh oh!

percona-ysorokin Sep 24, 2026

Uh oh!

kamil-holubicki Sep 24, 2026 •

edited

Loading

Uh oh!

percona-ysorokin Sep 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		[[nodiscard]] bool has_length() const noexcept { return length_.has_value(); }

		[[nodiscard]] std::uint64_t get_length() const noexcept {

Conversation

percona-ysorokin commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

percona-ysorokin Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamil-holubicki Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

percona-ysorokin commented Sep 23, 2026 •

edited

Loading

percona-ysorokin Sep 23, 2026 •

edited

Loading

kamil-holubicki Sep 24, 2026 •

edited

Loading