wallet/db.c: Speed up deletion of single peers.

[ZmnSCPxj]

Fixes #4325

I petition adding this to the 0.9.3 release candidate, as, it gives a massive speedup on peer deletion, does not change semantics of the database, just pure speedup, and the change is very small.

[ZmnSCPxj]

Aww crike. Looks like the database system of C-Lightning has gotten a whole lot more complicated since I last poked at it. Sigh.

[cdecker]

What issues are you fighting with? Maybe I can be of assistance?

[cdecker]

Let me quote your excellent research leading to this PR here:

Here is the query plan that SQLITE3 creates for a DELETE FROM peers WHERE id=?:

QUERY PLAN
|--SEARCH TABLE peers USING INTEGER PRIMARY KEY (rowid=?)
|--SCAN TABLE channels
|--SCAN TABLE channels
|--SCAN TABLE channel_state_changes
|--SEARCH TABLE penalty_bases USING COVERING INDEX sqlite_autoindex_penalty_bases_1 (channel_id=?)
|--SEARCH TABLE channel_feerates USING COVERING INDEX sqlite_autoindex_channel_feerates_1 (channel_id=?)
|--SCAN TABLE transaction_annotations
|--SCAN TABLE channeltxs
|--SEARCH TABLE htlc_sigs USING COVERING INDEX channel_idx (channelid=?)
|--SEARCH TABLE channel_htlcs USING COVERING INDEX sqlite_autoindex_channel_htlcs_1 (channel_id=?)
|--SCAN TABLE channel_state_changes
|--SEARCH TABLE penalty_bases USING COVERING INDEX sqlite_autoindex_penalty_bases_1 (channel_id=?)
|--SEARCH TABLE channel_feerates USING COVERING INDEX sqlite_autoindex_channel_feerates_1 (channel_id=?)
|--SCAN TABLE channeltxs
|--SEARCH TABLE htlc_sigs USING COVERING INDEX channel_idx (channelid=?)
|--SEARCH TABLE channel_htlcs USING COVERING INDEX sqlite_autoindex_channel_htlcs_1 (channel_id=?)
|--SCAN TABLE forwarded_payments USING COVERING INDEX sqlite_autoindex_forwarded_payments_1
|--SEARCH TABLE forwarded_payments USING COVERING INDEX sqlite_autoindex_forwarded_payments_1 (in_htlc_id=?)
|--SCAN TABLE forwarded_payments
`--SEARCH TABLE forwarded_payments USING COVERING INDEX sqlite_autoindex_forwarded_payments_1 (in_htlc_id=?)

I believe every SCAN TABLE is a lost opportunity to speed up the deletion; our goal would be to look at each line above that has SCAN TABLE and figure out what index is missing.

For example, forwarded_payments is a fair ~6000 row table on my system (which has a mere ~150,000-row channel_htlcs). According to SQLITE the .schema forwarded_payments is:

CREATE TABLE forwarded_payments
  (  in_htlc_id INTEGER REFERENCES channel_htlcs(id) ON DELETE SET NULL,
     out_htlc_id INTEGER REFERENCES channel_htlcs(id) ON DELETE SET NULL,
     in_channel_scid INTEGER,
     out_channel_scid INTEGER,
     in_msatoshi INTEGER,
     out_msatoshi INTEGER,
     state INTEGER,
     received_time INTEGER,
     resolved_time INTEGER,
     failcode INTEGER,
     UNIQUE(in_htlc_id, out_htlc_id)
  );

So there are two foreign constraints, one on in_htlc_id the other on out_htlc_id. Let us look at the result from determining the query plan related to forwarded_payments:

|--SCAN TABLE forwarded_payments USING COVERING INDEX sqlite_autoindex_forwarded_payments_1
|--SEARCH TABLE forwarded_payments USING COVERING INDEX sqlite_autoindex_forwarded_payments_1 (in_htlc_id=?)
|--SCAN TABLE forwarded_payments
`--SEARCH TABLE forwarded_payments USING COVERING INDEX sqlite_autoindex_forwarded_payments_1 (in_htlc_id=?)

It looks like SQLITE3 is able to scan for in_htlc_id using the autoindex, which presumably was magically created by SQLITE3 from the UNIQUE specification. This suggests to me that SQLITE3 is able to index via a single column from a multi-column index if the that column is the leftmost in the multi-column index --- presumably the UNIQUE(in_htlc_id, out_htlc_id) specification creates a multi-column index and SQLITE3 is only able to use it for single columns for the leftmost in_htlc_id in the index, and not out_htlc_id.

So on my experimental copy of my database I added:

CREATE INDEX forwarded_payments_out ON forwarded_payments(out_htlc_id);

And I got these numbers:

sqlite> delete from peers where id=7;                                
Run Time: real 0.197 user 0.059066 sys 0.042177

Without the index:

sqlite> delete from peers where id=7;
Run Time: real 1.022 user 0.864742 sys 0.060958

That is a massive 80% reduction in time for removing one peer.

[ZmnSCPxj]

It seems I need to somehow generate a bunch of files, most of which are generated by some kind of python script. Unfortunately I had to reinstall OS last December and now the Python on my system is complaining about stuff I don't understand because Python, and I cannot remember how I got it working the last time, have to go dig through it again first.

The core diff is this:

diff --git a/wallet/db.c b/wallet/db.c
index d1316ede4..efc9bc7b2 100644
--- a/wallet/db.c
+++ b/wallet/db.c
@@ -669,6 +669,11 @@ static struct migration dbmigrations[] = {
     /* A reference into our own offers table, if it was made from one */
     {SQL("ALTER TABLE payments ADD COLUMN local_offer_id BLOB DEFAULT NULL REFERENCES offers(offer_id);"), NULL},
     {SQL("ALTER TABLE channels ADD funding_tx_remote_sigs_received INTEGER DEFAULT 0;"), NULL},
+
+    /* Speeds up deletion of one peer from the database, measurements suggest
+     * it cuts down the time by 80%.  */
+    {SQL("CREATE INDEX forwarded_payments_out_htlc_id"
+        " ON forwarded_payments (out_htlc_id);"), NULL},
 };
 
 /* Leak tracking. */

This triggers some code that generates the other files but the NO_PYTHON build fails because apparently the other files are generated by some python script.

If you can generate the other files on my behalf that would be great....

[cdecker]

Yep, we started adding these generated files to cut down on dependencies a while ago, which now causes conflicts and errors if the files aren't being regenerated. I can fixup the PR with a newly generated _gen.sql file and test it if you'd like :-)

[ZmnSCPxj]

Yes please, thanks.

[cdecker]

Turns out it was just annoyed at the SHA256SUM in the footer not matching the generated po-file.

[cdecker]

Seems like you're not permitting maintainers to push onto your fork, so here's the diff:

diff --git a/wallet/db_postgres_sqlgen.c b/wallet/db_postgres_sqlgen.c
index c2df8238d..72ce72a2a 100644
--- a/wallet/db_postgres_sqlgen.c
+++ b/wallet/db_postgres_sqlgen.c
@@ -1780,10 +1780,10 @@ struct db_query db_postgres_queries[] = {
     },
 };
 
-#define DB_POSTGRES_QUERY_COUNT 294
+#define DB_POSTGRES_QUERY_COUNT 295
 
 #endif /* HAVE_POSTGRES */
 
 #endif /* LIGHTNINGD_WALLET_GEN_DB_POSTGRES */
 
-// SHA256STAMP:ca47a99b5c64139f4556f3bf77a6d984cb9ab6b38bcd2851bc551c75c5cc240a
+// SHA256STAMP:910328b3c942486b746f7f5d2a91fad65ae9eb54032a5828705132bfa1b86eaf
diff --git a/wallet/db_sqlite3_sqlgen.c b/wallet/db_sqlite3_sqlgen.c
index 58be3c146..47d563a40 100644
--- a/wallet/db_sqlite3_sqlgen.c
+++ b/wallet/db_sqlite3_sqlgen.c
@@ -1780,10 +1780,10 @@ struct db_query db_sqlite3_queries[] = {
     },
 };
 
-#define DB_SQLITE3_QUERY_COUNT 294
+#define DB_SQLITE3_QUERY_COUNT 295
 
 #endif /* HAVE_SQLITE3 */
 
 #endif /* LIGHTNINGD_WALLET_GEN_DB_SQLITE3 */
 
-// SHA256STAMP:ca47a99b5c64139f4556f3bf77a6d984cb9ab6b38bcd2851bc551c75c5cc240a
+// SHA256STAMP:910328b3c942486b746f7f5d2a91fad65ae9eb54032a5828705132bfa1b86eaf

Applying this should make the non-pythonic build happy as well :-)

[ZmnSCPxj]

Thanks!

[cdecker]

Typo.

Suggested change

	#define DB_SQLITE4_QUERY_COUNT 294
	#define DB_SQLITE3_QUERY_COUNT 294

[cdecker]

By the way, if you allow maintainers to push I can fix these tiny things up myself, rather than having to take a roundtrip to you 😉

[ZmnSCPxj]

It is set, do not know why it is not working:

[cdecker]

My bad, must've failed on my side. Sorry for the lecture :-)

[cdecker]

ACK 3ecca0c