Modlog: Support logging to a SQLite database

[AnnikaCodes]

This PR takes the next step in migrating modlogs to SQLite: starting dual logging to a SQLite database as well as text files.

I am currently implemented note ID search simply by streaming to regex in JavaScript; this seemed like the least bad option based on the discussions I've had.

[Zarel]

I am currently implemented note ID search simply by streaming to regex in JavaScript; this seemed like the least bad option based on the discussions I've had.

I would normally separately store a note ID, and then do a LIKE search in SQL. Was this considered, and if so, why was it rejected?

[AnnikaCodes]

I rejected the idea of a note_ids table because I am not aware of a satisfactory way to extract the IDs from old notes into a table, although we did discuss that as a possible solution for future modnotes.

Or did you mean having another column in the table for the toIDd note, and using a LIKE query on that? That's definitely feasible and would eliminate the need to define our own regex function in SQLite. I'd want to see what @monsanto thinks about that solution, though.

[Zarel]

Or did you mean having another column in the table for the toIDd note, and using a LIKE query on that? That's definitely feasible and would eliminate the need to define our own regex function in SQLite.

Yes, that's what I meant.

[monsanto]

This was suggested for killing the dependency on better-sqlite3. The reason not to do it would be the extra storage space required, but I don't think it really matters either way.

[monsanto]

I think I can do fast note ID search with a custom FTS5 tokenizer. This would require that I write a native sqlite extension in C or Rust. I would prefer doing the latter if there are appropriate bindings available. It is unclear how well this would work, but there is the potential that we will get the best of all possible approaches.

Since this would complicate install for users we could have a configuration option for enabling it, like we currently do for ripgrep. If its off we could just disable searching notes entirely, or fallback to slow LIKE %query%.

[monsanto]

Give this a shot. Make sure you have libsqlite3-dev installed or whatever the equivalent is on your platform. From /native run ./build.sh and sqlite3 ':memory:' < test.sql

If this works then

• Load fts_id_tokenizer.o
• Modify the schema to do CREATE VIRTUAL TABLE modlog_fts USING fts5(note, content=modlog, content_rowid=modlog_id, tokenize='id_tokenizer')
• Whenever you insert, also insert into modlog_fts
• Match on the note with ... FROM modlog_fts WHERE note MATCH '<query>'

See https://sqlite.org/fts5.html for more information

[AnnikaCodes]

annika@annika-mac ~/p/native (modlog-sql-new) [1]> /usr/local/opt/sqlite/bin/sqlite3 < test.sql 
** ids available: **
hello this is a test
with 1 numbers 2
Heizölrückstoßabdämpfung

** search for thisis: **
hello this is a test

** search for 1numbers2: **
with 1 numbers 2

** search for lrck: **
Heizölrückstoßabdämpfung
annika@annika-mac ~/p/native (modlog-sql-new)>

Looks good to me; I'll update the modlog logic to use it later today.

[monsanto]

I don't think SQLite is smart enough to do OR efficiently, the optimal query plan does parallel index scan + ordered merge (see the PM log where I manually applied this optimization). This is also the strategy you would use to search all fields at the same time, so maybe it is worth waiting on a refactor of this function

[AnnikaCodes]

How would you advise I refactor it? (All your other suggestions should be done.)

[monsanto]

Is this satisfactory? We have a lot of actions that are a combination of two words, like GTS GIVEAWAY. Would they expect a search on "giveaway" to return a GTS GIVEAWAY row?

[AnnikaCodes]

I'll post in WAIL.

[monsanto]

Unfortunately still doesn't work

monsanto@monsanto:~/downloads/pslogs/main$ ~/smogon/pokemon-showdown/tools/modlog/convert --db ./modlog.db --logdir ./modlog --from txt --to sqlite
Converting the text modlog files in ./modlog from txt to sqlite...
(node:9362) UnhandledPromiseRejectionWarning: RangeError: Too many parameter values were provided
    at ModlogConverterTxt.<anonymous> (/mnt/ssd1/smogon/pokemon-showdown/tools/modlog/converter.js:546:40)
    at ModlogConverterTxt.sqliteTransaction [as insertionTransaction] (/mnt/ssd1/smogon/pokemon-showdown/node_modules/better-sqlite3/lib/transaction.js:58:24)
    at insertEntries (/mnt/ssd1/smogon/pokemon-showdown/tools/modlog/converter.js:578:10)
    at ModlogConverterTxt.toSQLite (/mnt/ssd1/smogon/pokemon-showdown/tools/modlog/converter.js:597:47)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:9362) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:9362) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
monsanto@monsanto:~/downloads/pslogs/main$ ~/smogon/pokemon-showdown/tools/modlog/convert --db ./modlog.db --logdir ./modlog --from txt --to sqlite
Converting the text modlog files in ./modlog from txt to sqlite...
(node:9410) UnhandledPromiseRejectionWarning: SqliteError: table modlog_fts already exists
    at new ModlogConverterTxt (/mnt/ssd1/smogon/pokemon-showdown/tools/modlog/converter.js:533:18)
    at Function.convert (/mnt/ssd1/smogon/pokemon-showdown/tools/modlog/converter.js:684:22)
    at Object.<anonymous> (/mnt/ssd1/smogon/pokemon-showdown/tools/modlog/convert:45:17)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47
(Use `node --trace-warnings ...` to show where the warning was created)
(node:9410) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:9410) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

[AnnikaCodes]

Hm, I don't know if that is FTS related. Fixed it, though!

[monsanto]

Did you forget to push your fix?

[AnnikaCodes]

yeah lmao

[monsanto]

This creates N transactions instead of inserting N inside 1 transaction

[monsanto]

• I'm relatively confident in this plan of action and intend to merge this soon.

• Let's just remove the indexes for now, the base schema is sound, and since there won't be any querying for now we can just hold off. Having the dependency in will let me merge mia's work under an experimental flag.

• Like I mentioned on mia's PR, it isn't a good idea to have IF NOT EXISTS on the SQL. The initial schema code needs to run exactly once, and then after that migrations need to run exactly once.

• I would make the Config usesqlitemodlog instead of nosqlitemodlog and default to false. We don't want developers not looped into this feature & other servers to be aware of our work until it is ready for the prime time.

• The converter (and maybe other things at some point?) need to be able to import the Modlog class without it trying to open the modlog at databases/modlog.db.

If you handle the last two, I can handle the other two, and we can merge.

[AnnikaCodes]

I did the last two.

[monsanto]

Fine to merge, need to install extra dependencies, modify config, and hotpatch modlog

[AnnikaCodes]

One sec. I don't think hotpatching modlog directly will work now that we construct Modlog in rooms.ts.

[AnnikaCodes]

Okay whoever deploys needs to hotpatch chat first (to load the new hotpatching code) and then hotpatch modlog

[monsanto]

That is a nasty hack, it would be nice if a follow-up commit better isolated the modlog from /hotpatch

[AnnikaCodes]

Yes, once we restart and can depend on Rooms itll be simpler. I'd personally be inclined to put the MODLOG_PATH and stuff in Config instead but our precedent with these sorts of things is to use a const at the top of a file so I dont know if there's a reason we do that instead. Maybe to avoid cluttering config.js?

[monsanto]

I don't think you need to export these at all, they are implementation details that /hotpatch should not be aware of. The process manager lines above also fall under that umbrella. A better implementation strategy would be for modules that support hotpatch to come with methods to reload themselves.

[AnnikaCodes]

Sure, that sounds good. Id rather do that a separate commit though.