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

Cleanup prepared statement during node close #675

Merged
merged 2 commits into from
Jul 26, 2024

Conversation

marco6
Copy link
Contributor

@marco6 marco6 commented Jul 25, 2024

After preparing more queries on k8s-dqlite, I've been hitting quite frequently some segfaults. They all seemed to be happening when a query is running and the node is shutting down.

Apparently, they were. There was some missing cleanup logic around prepared statements in gateway__leader_close. This PR adds that bit.

I see that codecov report is showing a small regression in coverage. If someone from the dqlite team could guide me in testing this, I would be up for it.

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.87%. Comparing base (054b511) to head (e6ac700).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #675      +/-   ##
==========================================
+ Coverage   77.84%   77.87%   +0.02%     
==========================================
  Files         197      197              
  Lines       27761    27779      +18     
  Branches     5493     5538      +45     
==========================================
+ Hits        21611    21633      +22     
- Misses       4333     4420      +87     
+ Partials     1817     1726      -91     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cole-miller cole-miller self-requested a review July 25, 2024 15:37
@cole-miller
Copy link
Contributor

cole-miller commented Jul 25, 2024

I see that codecov report is showing a small regression in coverage. If someone from the dqlite team could guide me in testing this, I would be up for it.

I believe this is an artifact of codecov comparing the coverage numbers between your branch's HEAD and master's HEAD, instead of using the merge commit. If you look at the report, the patch lines that are listed as uncovered are all from the diff between merge_base(marco6/fix-prepared-cleanup, canonical/master), not from your commit. (I've noticed this happening before with other PRs.) So the coverage is not a blocker for merging.

@cole-miller
Copy link
Contributor

cole-miller commented Jul 25, 2024

For testing, I would try to add something in test/unit/test_gateway.c, which supports triggering gateway closure (grep for CLUSTER_DEPOSE). The tricky part will be getting that to happen while the gateway is in the middle of sending rows. Maybe this could be done relatively easily by just having a query that returns a ton of rows, even something like SELECT * FROM generate_series(1)? That might end up being slow or terribly CPU-intensive, though. If it doesn't work out, we could add some fault-injection that enables pausing the uv loop thread while it's in the middle of sending rows, so the window isn't so narrow. [Edit: ignore this, of course the gateway unit tests are single-threaded, so there is no "window" to hit.]

Copy link
Contributor

@cole-miller cole-miller left a comment

Choose a reason for hiding this comment

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

Test looks great, thank you!

@cole-miller cole-miller merged commit 39ffc97 into canonical:master Jul 26, 2024
18 checks passed
@marco6 marco6 deleted the fix-prepared-cleanup branch July 26, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants