Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: sqlite may dead lock in ci #5738

Merged
merged 6 commits into from
Mar 11, 2025
Merged

Conversation

yihong0618
Copy link
Contributor

Which issue does this PR close?

Closes #5737

This patch fix 5737 by using max_connections=1

root:
for PR #3233 bring sqlite
and metion that:

Hi, @Xuanwo The test_delete_stream test also causes a database is locked error on concurrent deletions in the local file database. Do you have any suggestions?

I tested local seems work fine, and waiting for CI

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@Xuanwo
Copy link
Member

Xuanwo commented Mar 11, 2025

I don't like this idea because it turns SqlitePool into a simple Mutex, preventing our users from reading concurrently. Maybe we could allow users to retry the request when they encounter a locked error? We can implement this by Error::set_temporary.

@yihong0618
Copy link
Contributor Author

I don't like this idea because it turns SqlitePool into a simple Mutex, preventing our users from reading concurrently. Maybe we could allow users to retry the request when they encounter a locked error? We can implement this by Error::set_temporary.

preventing our users from reading concurrently.

for the one connection it will not, and its sqlite most of the case is one user

@Xuanwo
Copy link
Member

Xuanwo commented Mar 11, 2025

for the one connection it will not, and its sqlite most of the case is one user

Hi, opendal has native support for op.read_with(path).concurrent(8).

@yihong0618
Copy link
Contributor Author

I don't like this idea because it turns SqlitePool into a simple Mutex, preventing our users from reading concurrently. Maybe we could allow users to retry the request when they encounter a locked error? We can implement this by Error::set_temporary.

preventing our users from reading concurrently.

for the one connection it will not, and its sqlite most of the case is one user

seems you are right for opendal ci tests case, will try to a better way
P.S. I do not think error handle is better

@yihong0618
Copy link
Contributor Author

I don't like this idea because it turns SqlitePool into a simple Mutex, preventing our users from reading concurrently. Maybe we could allow users to retry the request when they encounter a locked error? We can implement this by Error::set_temporary.

preventing our users from reading concurrently.

for the one connection it will not, and its sqlite most of the case is one user

seems you are right for opendal ci tests case, will try to a better way P.S. I do not think error handle is better

AND FYI: for most case, like lldap also force the connection to 1, I will try other way

lldap/lldap@884031d

@Xuanwo
Copy link
Member

Xuanwo commented Mar 11, 2025

It makes sense to me to add a new configuration option called max_connections, allowing users to set this value, similar to how lldap does. Hardcoding this value into max_connections=1 is not acceptable to me.

Even after adding max_connections, we still need to ensure that OpenDAL functions properly when max_connections is greater than 1. In OpenDAL's context, every operation is safe to retry since we don't have large transactions here. Additionally, db locked is indeed a retryable error in opendal context.

@yihong0618
Copy link
Contributor Author

yihong0618 commented Mar 11, 2025

It makes sense to me to add a new configuration option called max_connections, allowing users to set this value, similar to how lldap does. Hardcoding this value into max_connections=1 is not acceptable to me.

Even after adding max_connections, we still need to ensure that OpenDAL functions properly when max_connections is greater than 1. In OpenDAL's context, every operation is safe to retry since we don't have large transactions here. Additionally, db locked is indeed a retryable error in opendal context.

yes agree with you, will learn to find some better solution and force to 1 is a bad solution for this after check
the failed tests, if not a better way I'd like to use error and retry way, thanks

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Co-authored-by: Xuanwo <github@xuanwo.io>
@yihong0618 yihong0618 force-pushed the hy/fix_sqlite_dead_lock branch from 4134bdf to b1d7010 Compare March 11, 2025 07:26
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @yihong0618 for working on this.

@Xuanwo Xuanwo merged commit ec0dcd2 into apache:main Mar 11, 2025
94 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: failed for sqlite services database is locked
2 participants