Skip to content
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

fix: cache layer crash and data confusion #2217

Merged
merged 4 commits into from
Dec 29, 2023

Conversation

lqxhub
Copy link
Collaborator

@lqxhub lqxhub commented Dec 17, 2023

fix:#2216 修复因为flag计算错误,导致的部分命令缓存没有生效问题

修复 zset类型缓存读取错误和空指针崩溃问题

@github-actions github-actions bot added the ☢️ Bug Something isn't working label Dec 17, 2023
@lqxhub
Copy link
Collaborator Author

lqxhub commented Dec 17, 2023

出现了一个问题, 在我本地用 debug 模式编译的二进制可以通过测试,

使用release模式编译后, zset 类型的测试出错

image
使用 redis 命令 复现

出现的问题是 从缓存读取的数据 全是0

image

在这里加log 打印出来的数据是正常的, 再往里会调用到RcZAddrediscache 库里的, 断点没断到

然后所有 zset 类型的读取缓存, 读到的 scoremember 都是 0, 通过log 已经可以确定是 从缓存中读到的就是0, 不是 pika层转换出错了

怀疑的点 使用release 编译时, 由于 gcc的优化, 导致了写入出错了, 但是 在调用 RcZAdd 写入时,返回的 是正确的值

@lqxhub
Copy link
Collaborator Author

lqxhub commented Dec 17, 2023

image
通过打 log 看, 在这里写入的值是正确的,

image

通过log 看 这里读到的值就是 0 0 了

@lqxhub lqxhub linked an issue Dec 18, 2023 that may be closed by this pull request
cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameTtl, std::move(ttlptr)));
////PttlCmd
std::unique_ptr<Cmd> pttlptr =
std::make_unique<PttlCmd>(kCmdNamePttl, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv | kCmdFlagsDoThroughDB | kCmdFlagsReadCache);
std::make_unique<PttlCmd>(kCmdNamePttl, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKeySpace|kCmdFlagsFast | kCmdFlagsDoThroughDB | kCmdFlagsReadCache);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里为啥要写 kCmdFlagsKeySpace,执行 KeySpace这个命令是非常耗时的,kCmdFlagsFast和slow这俩应该是快慢分离用的吧,当前的代码应该没有用 到

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是参考 redis的分类做的, 所有和操作key相关的命令都是keyspace的 比如 del, ttl 等等

kCmdFlagsFast 这个是用在 ACL里做 快慢 分类的, 因为我不想再重新弄一次 解冲突, 就把 ACL里相关的带到这里了

这段代码是从ACL分支里复制出来的, 然后做了一点修改

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是参考 redis的分类做的, 所有和操作key相关的命令都是keyspace的 比如 del, ttl 等等

kCmdFlagsFast 这个是用在 ACL里做 快慢 分类的, 因为我不想再重新弄一次 解冲突, 就把 ACL里相关的带到这里了

这段代码是从ACL分支里复制出来的, 然后做了一点修改

redis是内存的,查内存没关系,但是pika是磁盘的 ,keyspace意味着查磁盘,这里不能带这个

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是参考 redis的分类做的, 所有和操作key相关的命令都是keyspace的 比如 del, ttl 等等
kCmdFlagsFast 这个是用在 ACL里做 快慢 分类的, 因为我不想再重新弄一次 解冲突, 就把 ACL里相关的带到这里了
这段代码是从ACL分支里复制出来的, 然后做了一点修改

redis是内存的,查内存没关系,但是pika是磁盘的 ,keyspace意味着查磁盘,这里不能带这个

那我们就要为 这个操作 key 的命令单独加一个 分类了, 但是我建议还是 把 操作key的分为 keyspace 这样 ACL里也能和redis保持同步, 因为ACL里有 根据 命令分类划分权限的功能. 以前在 pika里 操作 key的命令 被划分到 kCmdFlagsKv 里, 这样就带来一个问题, 操作key的命令 和 操作 string类型的命令 混在一起了, 没法做权限区分, 在缓存这里也有问题,
如果我再配置里 没有开 string 类型的缓存, 那么 del 命令就不能操作 缓存了, 这样就会带了 缓存 数据 的错误, 我之前测试的时候, 就是因为 string 类型的 缓存没开, 导致 在 使用 del 命令的时候, 没有删除缓存的数据, 造成的buff测试不通过

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是参考 redis的分类做的, 所有和操作key相关的命令都是keyspace的 比如 del, ttl 等等
kCmdFlagsFast 这个是用在 ACL里做 快慢 分类的, 因为我不想再重新弄一次 解冲突, 就把 ACL里相关的带到这里了
这段代码是从ACL分支里复制出来的, 然后做了一点修改

redis是内存的,查内存没关系,但是pika是磁盘的 ,keyspace意味着查磁盘,这里不能带这个

那我们就要为 这个操作 key 的命令单独加一个 分类了, 但是我建议还是 把 操作key的分为 keyspace 这样 ACL里也能和redis保持同步, 因为ACL里有 根据 命令分类划分权限的功能. 以前在 pika里 操作 key的命令 被划分到 kCmdFlagsKv 里, 这样就带来一个问题, 操作key的命令 和 操作 string类型的命令 混在一起了, 没法做权限区分, 在缓存这里也有问题, 如果我再配置里 没有开 string 类型的缓存, 那么 del 命令就不能操作 缓存了, 这样就会带了 缓存 数据 的错误, 我之前测试的时候, 就是因为 string 类型的 缓存没开, 导致 在 使用 del 命令的时候, 没有删除缓存的数据, 造成的buff测试不通过

这里我感觉宁可不做这个缓存 或者这个ACL也不能说给pika的性能 带来负担,redis的方案不适合pika的 话我感觉就需要做一下权衡,一味带进去可能keyspace一次 pika遭不住

这个是参考 redis的分类做的, 所有和操作key相关的命令都是keyspace的 比如 del, ttl 等等
kCmdFlagsFast 这个是用在 ACL里做 快慢 分类的, 因为我不想再重新弄一次 解冲突, 就把 ACL里相关的带到这里了
这段代码是从ACL分支里复制出来的, 然后做了一点修改

redis是内存的,查内存没关系,但是pika是磁盘的 ,keyspace意味着查磁盘,这里不能带这个

那我们就要为 这个操作 key 的命令单独加一个 分类了, 但是我建议还是 把 操作key的分为 keyspace 这样 ACL里也能和redis保持同步, 因为ACL里有 根据 命令分类划分权限的功能. 以前在 pika里 操作 key的命令 被划分到 kCmdFlagsKv 里, 这样就带来一个问题, 操作key的命令 和 操作 string类型的命令 混在一起了, 没法做权限区分, 在缓存这里也有问题, 如果我再配置里 没有开 string 类型的缓存, 那么 del 命令就不能操作 缓存了, 这样就会带了 缓存 数据 的错误, 我之前测试的时候, 就是因为 string 类型的 缓存没开, 导致 在 使用 del 命令的时候, 没有删除缓存的数据, 造成的buff测试不通过

那我理解 哪怕不做ACL或者不做这个命令的cache,也不能搞进去把集群性能影响了,适合redis的不一定 pika

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是参考 redis的分类做的, 所有和操作key相关的命令都是keyspace的 比如 del, ttl 等等
kCmdFlagsFast 这个是用在 ACL里做 快慢 分类的, 因为我不想再重新弄一次 解冲突, 就把 ACL里相关的带到这里了
这段代码是从ACL分支里复制出来的, 然后做了一点修改

redis是内存的,查内存没关系,但是pika是磁盘的 ,keyspace意味着查磁盘,这里不能带这个

那我们就要为 这个操作 key 的命令单独加一个 分类了, 但是我建议还是 把 操作key的分为 keyspace 这样 ACL里也能和redis保持同步, 因为ACL里有 根据 命令分类划分权限的功能. 以前在 pika里 操作 key的命令 被划分到 kCmdFlagsKv 里, 这样就带来一个问题, 操作key的命令 和 操作 string类型的命令 混在一起了, 没法做权限区分, 在缓存这里也有问题, 如果我再配置里 没有开 string 类型的缓存, 那么 del 命令就不能操作 缓存了, 这样就会带了 缓存 数据 的错误, 我之前测试的时候, 就是因为 string 类型的 缓存没开, 导致 在 使用 del 命令的时候, 没有删除缓存的数据, 造成的buff测试不通过

这里我感觉宁可不做这个缓存 或者这个ACL也不能说给pika的性能 带来负担,redis的方案不适合pika的 话我感觉就需要做一下权衡,一味带进去可能keyspace一次 pika遭不住

这个是参考 redis的分类做的, 所有和操作key相关的命令都是keyspace的 比如 del, ttl 等等
kCmdFlagsFast 这个是用在 ACL里做 快慢 分类的, 因为我不想再重新弄一次 解冲突, 就把 ACL里相关的带到这里了
这段代码是从ACL分支里复制出来的, 然后做了一点修改

redis是内存的,查内存没关系,但是pika是磁盘的 ,keyspace意味着查磁盘,这里不能带这个

那我们就要为 这个操作 key 的命令单独加一个 分类了, 但是我建议还是 把 操作key的分为 keyspace 这样 ACL里也能和redis保持同步, 因为ACL里有 根据 命令分类划分权限的功能. 以前在 pika里 操作 key的命令 被划分到 kCmdFlagsKv 里, 这样就带来一个问题, 操作key的命令 和 操作 string类型的命令 混在一起了, 没法做权限区分, 在缓存这里也有问题, 如果我再配置里 没有开 string 类型的缓存, 那么 del 命令就不能操作 缓存了, 这样就会带了 缓存 数据 的错误, 我之前测试的时候, 就是因为 string 类型的 缓存没开, 导致 在 使用 del 命令的时候, 没有删除缓存的数据, 造成的buff测试不通过

那我理解 哪怕不做ACL或者不做这个命令的cache,也不能搞进去把集群性能影响了,适合redis的不一定 pika

我刚看了一下 以前没有 keyspace 这个flag和这个分类 只是在 命令里加一个 flag 应该不会影响到 集群吧

@@ -450,9 +453,9 @@ class CmdRes {
struct UnblockTaskArgs {
std::string key;
std::shared_ptr<Slot> slot;
net::DispatchThread* dispatchThread{ nullptr };
net::DispatchThread* dispatchThread{nullptr};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个按照原格式比较好吧

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个文件我怕后面冲突太多, 没敢format, 我想的是 这个等后面 这几个大的pr合并了, 统一做一次formart吧, 现formart 冲突带代码太多了

@@ -495,6 +498,8 @@ class Cmd : public std::enable_shared_from_this<Cmd> {

void Initial(const PikaCmdArgsType& argv, const std::string& db_name);

uint32_t flag() const;
bool hasFlag(uint32_t flag) const;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个函数好像没有人用到

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是我加的, 但是现在的写法里都没用, 后面我看一下 是改一下现在的代码 还是 把这两个函数删了

@AlexStocks AlexStocks merged commit 5a11578 into OpenAtomFoundation:unstable Dec 29, 2023
12 of 13 checks passed
KKorpse pushed a commit to KKorpse/pika that referenced this pull request Jan 11, 2024
* fix cache layer crash and data confusion

* Delete the ACL flag and unused flags

* Modify the rediscache cmake parameter
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
* fix cache layer crash and data confusion

* Delete the ACL flag and unused flags

* Modify the rediscache cmake parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.5.3 bug ☢️ Bug Something isn't working core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: pika cache layer crash
3 participants