feat: sqlite vacuum and optional migrations #633
Conversation
Jenkins BuildsClick to see older builds (17)
|
cmd/waku/flags.go
Outdated
| @@ -298,6 +298,19 @@ var ( | |||
| Destination: &options.Store.DatabaseURL, | |||
| EnvVars: []string{"WAKUNODE2_STORE_MESSAGE_DB_URL"}, | |||
| }) | |||
| StoreMessageDBVacuum = altsrc.NewBoolFlag(&cli.BoolFlag{ | |||
| Name: "store-message-db-vacuum", | |||
| Usage: "Enable database vacuuming at start. Only supported by SQLite database engine.", | |||
There was a problem hiding this comment.
Looks like postgres also supports vacuum https://www.postgresql.org/docs/current/sql-vacuum.html.
Maybe we should allow this for postgres as well.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes it could be good to have it also on Postgres.
| db, err := sql.Open("pgx", dburl) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| if shouldVacuum { | ||
| err := executeVacuum(db, logger) |
There was a problem hiding this comment.
Just wondering, since vacuuming of db can take time upto minutes.
Should this be a bocking call or should we start this process in a go-routine and report the result back in async way?
Do we have any test indicating how much time a vacuum takes for our store db?
There was a problem hiding this comment.
Reading how SQLite VACUUM documentation, it will prevent writes to be executed, so it should be a blocking call.
No tests, but this command works by creating a temporary copy of the db, so the time it will take depends on the amount of data stored in the DB.
For postgresql, I modified the PR to use VACUUM FULL, just so the behavior of the command is the same as SQLite, but I think we shouldnt expect people to use vacuum with PostgreSQL since it is possible to setup routine vacuuming in it https://www.postgresql.org/docs/current/routine-vacuuming.html#AUTOVACUUM
There was a problem hiding this comment.
Ok, maybe the caller can do an async status to the user then. Rather than us doing it.
I am just concerned since this is an API call by the user, blocking it for soo long may not be a good idea.
But its ok for now, we can change it if we get user feedback.
Description
This adds new flags already available in nwaku to allow users to execute the vacuum procedure on sqlite databases and choose whether they want to execute migrations or not
cc: @apentori
Changes
--store-message-db-vacuumand--store-message-db-migrateDBSettingsthat can be used to pass settings required for different RDBMS (might be refactored away in the future)