From b6d583c26a5bf024b1da835a6aa7ace3acbca167 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 28 Aug 2019 14:56:19 +0200 Subject: [PATCH] db: Move tracking of pending statements into the `struct db` We now have a much stronger consistency check from the combination of transaction wrapping, tal memory leak detection. Tramsaction wrapping ensures that each statement is executed before the transaction is committed. The commit is also driven by the `io_loop`, which means that it is no longer possible for us to have statements outside of transactions and transactions are guaranteed to commit at the round's end. By adding the tal-awareness we can also get a much better indication as to whether we have un-freed statements flying around, which we can test at the end of the round as well. Signed-off-by: Christian Decker --- lightningd/lightningd.c | 6 +-- lightningd/test/run-find_my_abspath.c | 3 -- wallet/db.c | 73 +++++++-------------------- wallet/db.h | 3 -- wallet/db_common.h | 6 +++ 5 files changed, 25 insertions(+), 66 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 53ac077ab..9041b06ac 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -571,11 +571,7 @@ static void pidfile_create(const struct lightningd *ld) * extra sanity checks, and it's also a good point to free the tmpctx. */ static int io_poll_lightningd(struct pollfd *fds, nfds_t nfds, int timeout) { - /*~ In particular, we should *not* have left a database transaction - * open! */ - db_assert_no_outstanding_statements(); - - /* The other checks and freeing tmpctx are common to all daemons. */ + /* These checks and freeing tmpctx are common to all daemons. */ return daemon_poll(fds, nfds, timeout); } diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c index 99a4b22bf..4d645e333 100644 --- a/lightningd/test/run-find_my_abspath.c +++ b/lightningd/test/run-find_my_abspath.c @@ -45,9 +45,6 @@ void daemon_setup(const char *argv0 UNNEEDED, /* Generated stub for daemon_shutdown */ void daemon_shutdown(void) { fprintf(stderr, "daemon_shutdown called!\n"); abort(); } -/* Generated stub for db_assert_no_outstanding_statements */ -void db_assert_no_outstanding_statements(void) -{ fprintf(stderr, "db_assert_no_outstanding_statements called!\n"); abort(); } /* Generated stub for db_begin_transaction_ */ void db_begin_transaction_(struct db *db UNNEEDED, const char *location UNNEEDED) { fprintf(stderr, "db_begin_transaction_ called!\n"); abort(); } diff --git a/wallet/db.c b/wallet/db.c index 5e62f918c..07a19547f 100644 --- a/wallet/db.c +++ b/wallet/db.c @@ -456,59 +456,16 @@ static struct migration dbmigrations[] = { /* Leak tracking. */ #if DEVELOPER -/* We need a global here, since caller has no context. Yuck! */ -static struct list_head db_statements = LIST_HEAD_INIT(db_statements); - -struct db_statement { - struct list_node list; - sqlite3_stmt *stmt; - const char *origin; -}; - -static struct db_statement *find_statement(sqlite3_stmt *stmt) +static void db_assert_no_outstanding_statements(struct db *db) { - struct db_statement *i; + struct db_stmt *stmt; - list_for_each(&db_statements, i, list) { - if (i->stmt == stmt) - return i; - } - return NULL; -} - -void db_assert_no_outstanding_statements(void) -{ - struct db_statement *dbstat; - - dbstat = list_top(&db_statements, struct db_statement, list); - if (dbstat) - db_fatal("Unfinalized statement %s", dbstat->origin); -} - -static void dev_statement_start(sqlite3_stmt *stmt, const char *origin) -{ - struct db_statement *dbstat = tal(NULL, struct db_statement); - dbstat->stmt = stmt; - dbstat->origin = origin; - list_add(&db_statements, &dbstat->list); -} - -static void dev_statement_end(sqlite3_stmt *stmt) -{ - struct db_statement *dbstat = find_statement(stmt); - list_del_from(&db_statements, &dbstat->list); - tal_free(dbstat); + stmt = list_top(&db->pending_statements, struct db_stmt, list); + if (stmt) + db_fatal("Unfinalized statement %s", stmt->location); } #else -static void dev_statement_start(sqlite3_stmt *stmt, const char *origin) -{ -} - -static void dev_statement_end(sqlite3_stmt *stmt) -{ -} - -void db_assert_no_outstanding_statements(void) +static void db_assert_no_outstanding_statements(struct db *db) { } #endif @@ -531,7 +488,6 @@ static void trace_sqlite3(void *dbv, const char *stmt) void db_stmt_done(sqlite3_stmt *stmt) { - dev_statement_end(stmt); sqlite3_finalize(stmt); } @@ -550,7 +506,6 @@ sqlite3_stmt *db_select_prepare_(const char *location, struct db *db, const char if (err != SQLITE_OK) db_fatal("%s: %s: %s", location, query, sqlite3_errmsg(db->sql)); - dev_statement_start(stmt, location); return stmt; } @@ -573,6 +528,10 @@ struct db_stmt *db_prepare_v2_(const char *location, struct db *db, if (strncmp(query_id, "./", 2) == 0) query_id += 2; + if (!db->in_transaction) + db_fatal("Attempting to prepare a db_stmt outside of a " + "transaction: %s", location); + /* Look up the query by its ID */ for (size_t i = 0; i < db->config->num_queries; i++) { if (streq(query_id, db->config->queries[i].query)) { @@ -598,6 +557,8 @@ struct db_stmt *db_prepare_v2_(const char *location, struct db *db, tal_add_destructor(stmt, db_stmt_free); + list_add(&db->pending_statements, &stmt->list); + return stmt; } @@ -680,7 +641,6 @@ sqlite3_stmt *db_prepare_(const char *location, struct db *db, const char *query if (err != SQLITE_OK) db_fatal("%s: %s: %s", location, query, sqlite3_errmsg(db->sql)); - dev_statement_start(stmt, location); return stmt; } @@ -713,7 +673,7 @@ sqlite3_stmt *db_select_(const char *location, struct db *db, const char *query) static void destroy_db(struct db *db) { - db_assert_no_outstanding_statements(); + db_assert_no_outstanding_statements(db); sqlite3_close(db->sql); } @@ -768,7 +728,7 @@ void db_commit_transaction(struct db *db) { bool ok; assert(db->in_transaction); - db_assert_no_outstanding_statements(); + db_assert_no_outstanding_statements(db); ok = db->config->commit_tx_fn(db); if (!ok) @@ -822,7 +782,7 @@ static struct db *db_open(const tal_t *ctx, char *filename) db = tal(ctx, struct db); db->filename = tal_strdup(db, filename); db->sql = sql; - + list_head_init(&db->pending_statements); for (size_t i=0; iname)) { db->config = configs[i]; @@ -1662,6 +1622,8 @@ bool db_exec_prepared_v2(struct db_stmt *stmt TAKES) const char *expanded_sql; bool ret = stmt->db->config->exec_fn(stmt); stmt->executed = true; + list_del_from(&stmt->db->pending_statements, &stmt->list); + if (stmt->db->config->expand_fn != NULL && ret && !stmt->query->readonly) { expanded_sql = stmt->db->config->expand_fn(stmt); @@ -1691,5 +1653,6 @@ bool db_query_prepared(struct db_stmt *stmt) assert(stmt->query->readonly); ret = stmt->db->config->query_fn(stmt); stmt->executed = true; + list_del_from(&stmt->db->pending_statements, &stmt->list); return ret; } diff --git a/wallet/db.h b/wallet/db.h index 6e2f113dd..8ef764a5c 100644 --- a/wallet/db.h +++ b/wallet/db.h @@ -173,9 +173,6 @@ void db_exec_prepared_(const char *caller, struct db *db, sqlite3_stmt *stmt); /* Wrapper around sqlite3_finalize(), for tracking statements. */ void db_stmt_done(sqlite3_stmt *stmt); -/* Call when you know there should be no outstanding db statements. */ -void db_assert_no_outstanding_statements(void); - #define sqlite3_column_arr(ctx, stmt, col, type) \ ((type *)sqlite3_column_arr_((ctx), (stmt), (col), \ sizeof(type), TAL_LABEL(type, "[]"), \ diff --git a/wallet/db_common.h b/wallet/db_common.h index bfc89a5b6..5652bdd6c 100644 --- a/wallet/db_common.h +++ b/wallet/db_common.h @@ -2,6 +2,7 @@ #define LIGHTNING_WALLET_DB_COMMON_H #include "config.h" #include +#include #include #include #include @@ -25,6 +26,8 @@ struct db { const char **changes; + /* List of statements that have been created but not executed yet. */ + struct list_head pending_statements; char *error; }; @@ -62,6 +65,9 @@ struct db_binding { }; struct db_stmt { + /* Our entry in the list of pending statements. */ + struct list_node list; + /* Database we are querying */ struct db *db;