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

More fts table fixes #13810

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

bepaald
Copy link

@bepaald bepaald commented Nov 21, 2024

First time contributor checklist

Contributor checklist

  • I am following the Code Style Guidelines
  • I have tested my contribution on these devices:
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

This is strongly related to #13809. The exact same FTS-table-drop-and-recreate pattern is used in these two migrations (239, and 242). These will certainly again cause database corruptions. For more information, please see #13809.

I am not currently aware of open issues caused by this problem (I only found them now just by grepping for "message_fts" through the migration-helpers), and I have not tested these two 1-liners, but both the problem and solution are identical to #13809 (which was thoroughly investigated and tested).

If i'm understanding everything right, some people may already have inconsistent FTS tables due to these migrations, so I strongly suggest a new database version and a migration like this:

object V2XX_EnsureSearchTableConsistency : SignalDatabaseMigration {

  private const val FTS_TABLE_NAME = "message_fts"
  db.execSQL("INSERT INTO $FTS_TABLE_NAME ($FTS_TABLE_NAME) VALUES('rebuild')")

}

I'd be willing to write a patch like this if requested. Then after that, it should just be a matter of keeping the table consistent.

Thanks!

@greyson-signal
Copy link
Contributor

I think these changes are reasonable, thank you! They'll be in 7.27.

I wanna hold off on a new migration that just does a rebuild, though. On large installs this can take 7-10 seconds and can be very disruptive, so I want to take more of a "wait and see" approach.

@bepaald
Copy link
Author

bepaald commented Nov 22, 2024

Thanks!

In regards to a new migration: it is possible to do the actual rebuild conditionally on whether or not the fts table is good. For this use INSERT INTO message_fts(message_fts, rank) VALUES('integrity-check', 1); and check the return code. (reference: https://www.sqlite.org/fts5.html#the_integrity_check_command). This way there is no penalty for users if the fts table is good (most users I would think). For others, indeed it could take some time, but it could be argued that it needs to be done in their case regardless.

Note also that if it's not done, similar problems like #13034 and #13506 could easily happen again if any future migration touches the message table again. (at least assuming the separate job that exists for rebuilding does not run during the migrations.)

But I'll leave that up to you (though I'm still willing to create a pr if desired). Thank you for all your hard work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants