fix: masetr-slave maybe inconsistent due to bgsave may provide a wrong binlog offset

[cheniujh]

该PR修复了 Issue #2805

问题重述：
Pika做bgsave时会给DB::dbs_rw_这把大锁上写锁，而所有命令执行时都会在链路上给这把dbs_rw_上读锁。所以bgsave上了写锁以后，相当于暂时阻写来打快照，在bgsave对dbs_rw_的写锁范围内，做了两件事：1 打快照 2 获取此时的最新binlog offset(从节点全量传输完快照之后才知道该从哪里开始续传binlog)
问题在于：在命令执行链路上的读锁，其范围只覆盖了DoCommand()，也就是写DB，Dobinlog()是在该读锁放开以后采取执行的。这就意味着：当bgsave处成功独占了dbs_rw_之后，写链路上可能还有未执行完的DoBinlog（也就是还有Binlog正在写入），那么bgsave此时获取的binlog offset实际上是一个偏前的位置，按照正确预期bgsave应该等到这些Binlog完全落盘了才能去获取Binlog offset，因为pika的binlog目前不幂等，得到一个偏前的位置，会在后续造成Slave重复消费某部分Binlog，进而导致主从不一致。

解决方案：
本PR将命令执行链路上针对dbs_rw_的读锁修改了覆盖范围，让他将DoCommand，Dobinlog都覆盖进去。换而言之，一条普通命令在执行前会先获取dbs_rw_的读锁，之后在完成了WriteDB, WriteBInlog之后才会放开读锁。这就能确保当bgsave能给dbs_rw_加上独占的写锁时，之前的所有请求/命令对应的Binlog也已经落盘，这样获取到的Binlog Offset就是准确的。

另外由于调整了锁范围，所以进行了性能测试对比（本PR vs 原代码），大致结论为：

• 1 每次负载500W请求，共测试4次，平均值如下：
  原代码：QPS为153401， P99为3.279ms
  本PR：QPS为152590， P99为3.359ms
  结论：本PR在500万请求量下，QPS降幅在0.08%，P99的增幅在2%左右，未见明显性能下降。

• 2 每次负载1亿请求，共测试1次，具体如下：
  原代码：QPS为144849， P99为3.615ms
  本PR：QPS为145694， P99为3.335ms
  结论：本PR在1亿请求量下未引起性能下降。

PS:

1. 单条负载大小512KB，具体测试命令如下：
   ./redis-benchmark -h * -p 9221 -t set -n 100000000/5000000 -r 10000000000000 -d 512 -c 300 --threads 20
2. 详细测试数据请见下方附1

This PR fixes Issue #2805.

Problem Restatement:
When Pika performs bgsave, it acquires a write lock on the DB::dbs_rw_ mutex, while all command executions acquire a read lock on the same dbs_rw_ mutex. Consequently, when bgsave acquires the write lock, it temporarily blocks writes to take a snapshot. Within the scope of bgsave's write lock on dbs_rw_, two tasks are performed: 1) taking a snapshot and 2) obtaining the latest binlog offset (to determine where to resume binlog transmission after the slave completes the snapshot).

The issue lies in the fact that the read lock on the command execution path only covers DoCommand(), which involves writing to the DB. DoBinlog() is executed after the read lock is released. This means that when bgsave successfully acquires the exclusive write lock on dbs_rw_, there may still be pending DoBinlog operations (i.e., binlogs still being written). Thus, the binlog offset obtained by bgsave is prematurely captured. Ideally, bgsave should wait for all pending binlogs to be fully written before capturing the binlog offset. Since Pika's binlog is currently non-idempotent, obtaining a premature binlog offset can lead to the slave repeatedly consuming certain portions of the binlog, resulting in master-slave inconsistencies.

Solution:
This PR modifies the read lock coverage on the command execution path for dbs_rw_. It extends the read lock to encompass both DoCommand and DoBinlog. In other words, a normal command will first acquire a read lock on dbs_rw_, and it will only release this read lock after completing both WriteDB and WriteBinlog. This ensures that when bgsave acquires the exclusive write lock on dbs_rw_, all previous requests/commands and their corresponding binlogs have been fully written. As a result, the binlog offset obtained will be accurate.

[coderabbitai]

Walkthrough

The recent changes enhance the control flow of database locking in the pika_command module. By conditioning the acquisition and release of database locks on the command's suspension state, the updates improve performance by avoiding unnecessary locking. The removal of the DEFER block for unlocking promotes clearer management of lock lifecycles, ensuring better efficiency and clarity in command processing.

Changes

File	Change Summary
src/pika_command.cc	Modified InternalProcessCommand to conditionally acquire/release database locks based on command suspension. Removed DEFER block from DoCommand for explicit lock management.

Poem

🐇 In the code, we've made a leap,
Locks are lighter, no need to creep.
When commands are paused, we take a break,
Efficiency blooms, for performance's sake!
Hoppin' through logic, so clear and bright,
Celebrate changes, oh what a delight! 🎉

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6c0e0d4 and ada3f48.

Files selected for processing (1)

• src/pika_command.cc (1 hunks)

Additional comments not posted (3)

src/pika_command.cc (3)

871-874: Ensure the correctness of conditional locking logic.

The conditional acquisition of the shared lock based on IsSuspend() improves performance by avoiding unnecessary locking. However, verify that the IsSuspend() method accurately reflects the command's suspension state to prevent potential data races or deadlocks.

---

882-884: Ensure the correctness of conditional unlocking logic.

The conditional release of the shared lock based on IsSuspend() ensures that the lock is only released if it was acquired. Verify that the IsSuspend() method accurately reflects the command's suspension state to prevent potential data races or deadlocks.

---

875-875: Verify the removal of the DEFER block for unlocking.

The removal of the DEFER block for unlocking the database lock indicates a shift towards more explicit control of the lock's lifecycle. Ensure that this change does not introduce any potential issues with lock management and that the explicit control is correctly implemented.

[cheniujh]

附1：
参数: 500W负载，benchmark 50线程数：
./redis-benchmark -h * -p 9221 -t set -n 5000000 -r 10000000000000 -d 512 -c 300 --threads 50
本PR:
平均QPS: 152590
平均p99: 3.359
第一次：
Summary:
throughput summary: 152257.98 requests per second
latency summary (msec):
avg min p50 p95 p99 max
1.955 0.832 1.871 2.415 3.735 20.815
第二次：
Summary:
throughput summary: 152239.45 requests per second
latency summary (msec):
avg min p50 p95 p99 max
1.949 1.040 1.863 2.407 3.391 27.967
第三次：
Summary:
throughput summary: 152457.61 requests per second
latency summary (msec):
avg min p50 p95 p99 max
1.947 0.576 1.855 2.407 3.423 124.287
第四次：
Summary:
throughput summary: 153407.19 requests per second
latency summary (msec):
avg min p50 p95 p99 max
1.940 0.704 1.855 2.407 3.295 119.295

2 原版
平均QPS: 153401
平均p99: 3.279
第一次：
Summary:
throughput summary: 154573.84 requests per second
latency summary (msec):
avg min p50 p95 p99 max
1.923 1.320 1.847 2.391 2.927 23.999

第二次：
Summary:
throughput summary: 152452.97 requests per second
latency summary (msec):
avg min p50 p95 p99 max
1.946 1.064 1.871 2.399 3.023 19.279

第三次：
Summary:
throughput summary: 153407.19 requests per second
latency summary (msec):
avg min p50 p95 p99 max
1.935 0.560 1.855 2.415 3.279 84.991
第四次：
Summary:
throughput summary: 152225.53 requests per second
latency summary (msec):
avg min p50 p95 p99 max
1.951 0.560 1.871 2.407 3.535 187.391

参数: 1亿负载，benchmark 20线程数：
./redis-benchmark-cjh -h * -p 9221 -t set -n 100000000 -r 10000000000000 -d 512 -c 300 --threads 20

1 本PR:
Summary:
throughput summary: 145694.02 requests per second
latency summary (msec):
avg min p50 p95 p99 max
2.051 0.296 1.887 2.455 3.335 2142.207

2 原版代码：
Summary:
throughput summary: 144849.66 requests per second
latency summary (msec):
avg min p50 p95 p99 max
2.063 1.064 1.895 2.455 3.615 2351.103