search

canonical / dqlite

Embeddable, replicated and fault-tolerant SQL engine.
https://dqlite.io
Other
3.88k stars 216 forks source link

Introduce RESET request #435

Closed cole-miller closed 1 year ago

cole-miller commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

Codecov Report

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

:exclamation: 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%) :arrow_down:
src/gateway.c 43.80% <42.30%> (-14.93%) :arrow_down:
src/vfs.c 79.04% <100.00%> (-7.10%) :arrow_down:
src/message.c 0.00% <0.00%> (-100.00%) :arrow_down:
src/request.c 0.00% <0.00%> (-100.00%) :arrow_down:
src/response.c 0.00% <0.00%> (-100.00%) :arrow_down:
src/transport.c 45.75% <0.00%> (-24.19%) :arrow_down:
src/conn.c 49.24% <0.00%> (-23.62%) :arrow_down:
src/client.c 52.84% <0.00%> (-20.21%) :arrow_down:
src/db.c 42.37% <0.00%> (-20.13%) :arrow_down:
... 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

cole-miller commented 1 year ago

Latest push:

MathieuBordere commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Closing in favor of an issue