Introduce RESET request #435

cole-miller · 2022-11-18T20:39:30Z

This PR implements a new RESET request that allows a dqlite client to restore the database it has opened to a pristine state. Instead of deleting the in-memory database "file", we use the SQLITE_DBCONFIG_RESET_DATABASE method described here, which goes through the normal VFS channels. This still requires a separate request because of the out-of-band sqlite3_db_config calls. The implementation of the new request mimics what would happen if we did EXEC_SQL("VACUUM"), with some simplifications since we don't need to run a barrier or bind parameters.

Closes #422

Signed-off-by: Cole Miller cole.miller@canonical.com

cole-miller · 2022-11-18T20:42:12Z

I am unsure about this remark in the SQLite docs:

If the database connection is newly opened, make sure it has read the database schema by preparing then discarding some query against the database, or calling sqlite3_table_column_metadata(), ignoring any errors. This step is only necessary if the application desires to keep the database in WAL mode after the reset if it was in WAL mode before the reset.

I think we do want to keep the database in WAL mode after the reset, but it's unclear to me how the server can implement "touching" the schema in this way without knowing what tables (if any) are present in the DB.

codecov · 2022-11-18T20:44:12Z

Codecov Report

Merging #435 (8dc07ed) into master (0c38457) will decrease coverage by 13.15%.
The diff coverage is 44.08%.

❗ Current head 8dc07ed differs from pull request most recent head 8550472. Consider uploading reports for the commit 8550472 to get more accurate results

@@             Coverage Diff             @@
##           master     #435       +/-   ##
===========================================
- Coverage   73.47%   60.32%   -13.16%     
===========================================
  Files          31       31               
  Lines        4679     4771       +92     
  Branches     1462     1413       -49     
===========================================
- Hits         3438     2878      -560     
- Misses        745     1022      +277     
- Partials      496      871      +375

Impacted Files	Coverage Δ
src/leader.c	`49.52% <35.08%> (-20.59%)`	⬇️
src/gateway.c	`43.80% <42.30%> (-14.93%)`	⬇️
src/vfs.c	`79.04% <100.00%> (-7.10%)`	⬇️
src/message.c	`0.00% <0.00%> (-100.00%)`	⬇️
src/request.c	`0.00% <0.00%> (-100.00%)`	⬇️
src/response.c	`0.00% <0.00%> (-100.00%)`	⬇️
src/transport.c	`45.75% <0.00%> (-24.19%)`	⬇️
src/conn.c	`49.24% <0.00%> (-23.62%)`	⬇️
src/client.c	`52.84% <0.00%> (-20.21%)`	⬇️
src/db.c	`42.37% <0.00%> (-20.13%)`	⬇️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cole-miller · 2022-11-18T20:45:25Z

Ah, it looks like SQLITE_DBCONFIG_RESET_DATABASE was added in SQLite v3.24.0, which is not available on Bionic. We could do some #ifdef stuff to detect an old SQLite and just fail RESET requests in this case?

freeekanayaka · 2022-11-19T09:01:28Z

Ah, it looks like SQLITE_DBCONFIG_RESET_DATABASE was added in SQLite v3.24.0, which is not available on Bionic. We could do some #ifdef stuff to detect an old SQLite and just fail RESET requests in this case?

+1

freeekanayaka · 2022-11-19T09:06:37Z

sqlite3_table_column_metadata

I'm not entirely sure why this requirement is there, but maybe you can use sqlite3_table_column_metadata against one of the virtual tables/columns that are guaranteed to be there, e.g. sqlite3_table_column_metadata(db, NULL, "sqlite_master", "name", ...).

MathieuBordere · 2022-11-21T07:09:38Z

Ah, it looks like SQLITE_DBCONFIG_RESET_DATABASE was added in SQLite v3.24.0, which is not available on Bionic. We could do some #ifdef stuff to detect an old SQLite and just fail RESET requests in this case?

I would maybe check it at runtime too, with sqlite3_version or sqlite3_libversion.

src/leader.c

MathieuBordere · 2022-11-21T07:23:10Z

src/leader.c

+	rv = sqlite3_db_config(l->conn, SQLITE_DBCONFIG_RESET_DATABASE, 1, 0);
+	assert(rv == 0);
+	exec->status = sqlite3_exec(l->conn, "VACUUM", NULL, NULL, NULL);
+	rv = sqlite3_db_config(l->conn, SQLITE_DBCONFIG_RESET_DATABASE, 0, 0);


Out of curiosity, does dqlite's memory footprint shrink after resetting a DB? If it's not shrinking, we should look into it.

Yes, that would be a good thing to measure. Let me see if I can add some basic tracing of memory consumption before and after the VACUUM.

Hmm, I don't see any change in number of DB pages, number of WAL frames, or number of shm regions after leaderApplyFrames returns vs. before VACUUM. @freeekanayaka, can you think of anything that would be preventing our VFS from dropping allocated memory when SQLite is done using it? (I could also just be looking in the wrong places -- I don't fully understand the VFS subsystem yet.)

Does the VACUUM actually succeed?

Yes, it does -- I just tried adding an extra tracef to confirm.

Hmm, I don't see any change in number of DB pages, number of WAL frames, or number of shm regions after leaderApplyFrames returns vs. before VACUUM. @freeekanayaka, can you think of anything that would be preventing our VFS from dropping allocated memory when SQLite is done using it? (I could also just be looking in the wrong places -- I don't fully understand the VFS subsystem yet.)

Not on the top of my head, but I also don't have a clear picture of the changes that are being made. The SQLite memory allocator has some helper functions to track the amount of allocated memory, perhaps that could be useful to check.

cole-miller · 2022-11-21T18:24:55Z

Latest push:

gates RESET request on SQLite version check (compile-time and run-time)
runs a barrier before VACUUM
traces the state of memory allocations before and after a DB reset -- I'm not necessarily proposing the way I set this up as a permanent addition, just enabling others to reproduce what I saw (allocations not being dropped after VACUUM)

Signed-off-by: Cole Miller <cole.miller@canonical.com>

MathieuBordere · 2022-11-22T07:22:02Z

Okay, we should first figure out why the database is not shrinking before merging this. Maybe it waits until a checkpoint command, not sure.

cole-miller · 2022-11-29T20:28:24Z

This implementation of RESET doesn't work when I rebase on top of #440. That's expected, since one of the first things done in the implementation of VACUUM is to ATTACH an additional temporary database to the given connection. I don't think we should merge this unless and until we can support ATTACH DATABASE properly.

cole-miller · 2023-04-12T04:12:10Z

Closing in favor of an issue

cole-miller mentioned this pull request Nov 18, 2022

Implement database drop operation #431

Closed

MathieuBordere reviewed Nov 21, 2022

View reviewed changes

cole-miller force-pushed the reset branch from 14bdd3e to 02abbc5 Compare November 21, 2022 18:21

cole-miller force-pushed the reset branch from 02abbc5 to 44a0310 Compare November 21, 2022 18:34

Introduce RESET request

4cac151

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller force-pushed the reset branch 3 times, most recently from 8dc07ed to 6946015 Compare November 21, 2022 21:19

test

8550472

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller force-pushed the reset branch from 6946015 to 8550472 Compare November 21, 2022 21:21

This was referenced Nov 29, 2022

A better error message attempting to use ATTACH DATABASE #419

Closed

row size limit? getting "Error: disk I/O error" from dqlite-demo #432

Closed

cole-miller marked this pull request as draft November 29, 2022 20:28

cole-miller mentioned this pull request Nov 30, 2022

Support attaching additional databases to a dqlite connection #441

Open

MathieuBordere mentioned this pull request Mar 31, 2023

Shell: Support for WITH clauses (or more complex queries) canonical/go-dqlite#239

Closed

cole-miller closed this Apr 12, 2023

cole-miller mentioned this pull request Aug 18, 2023

Consider periodically using VACUUM to reduce memory footprint #530

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce RESET request #435

Introduce RESET request #435

cole-miller commented Nov 18, 2022

cole-miller commented Nov 18, 2022

codecov bot commented Nov 18, 2022 •

edited

cole-miller commented Nov 18, 2022 •

edited

freeekanayaka commented Nov 19, 2022

freeekanayaka commented Nov 19, 2022

MathieuBordere commented Nov 21, 2022 •

edited

MathieuBordere Nov 21, 2022

cole-miller Nov 21, 2022

cole-miller Nov 21, 2022 •

edited

MathieuBordere Nov 21, 2022

cole-miller Nov 21, 2022

freeekanayaka Nov 22, 2022

cole-miller commented Nov 21, 2022

MathieuBordere commented Nov 22, 2022 •

edited

cole-miller commented Nov 29, 2022

cole-miller commented Apr 12, 2023

Introduce RESET request #435

Introduce RESET request #435

Conversation

cole-miller commented Nov 18, 2022

cole-miller commented Nov 18, 2022

codecov bot commented Nov 18, 2022 • edited

Codecov Report

cole-miller commented Nov 18, 2022 • edited

freeekanayaka commented Nov 19, 2022

freeekanayaka commented Nov 19, 2022

MathieuBordere commented Nov 21, 2022 • edited

MathieuBordere Nov 21, 2022

Choose a reason for hiding this comment

cole-miller Nov 21, 2022

Choose a reason for hiding this comment

cole-miller Nov 21, 2022 • edited

Choose a reason for hiding this comment

MathieuBordere Nov 21, 2022

Choose a reason for hiding this comment

cole-miller Nov 21, 2022

Choose a reason for hiding this comment

freeekanayaka Nov 22, 2022

Choose a reason for hiding this comment

cole-miller commented Nov 21, 2022

MathieuBordere commented Nov 22, 2022 • edited

cole-miller commented Nov 29, 2022

cole-miller commented Apr 12, 2023

codecov bot commented Nov 18, 2022 •

edited

cole-miller commented Nov 18, 2022 •

edited

MathieuBordere commented Nov 21, 2022 •

edited

cole-miller Nov 21, 2022 •

edited

MathieuBordere commented Nov 22, 2022 •

edited