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 <decker.christian@gmail.com>

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);
 }
 

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(); }

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)
-{
-	struct db_statement *i;
-
-	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)
+static void db_assert_no_outstanding_statements(struct db *db)
 {
-	struct db_statement *dbstat = tal(NULL, struct db_statement);
-	dbstat->stmt = stmt;
-	dbstat->origin = origin;
-	list_add(&db_statements, &dbstat->list);
-}
+	struct db_stmt *stmt;
 
-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; i<num_configs; i++)
 		if (streq(driver_name, configs[i]->name)) {
 			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;
 }

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, "[]"), \

wallet/db_common.h

@@ -2,6 +2,7 @@
 #define LIGHTNING_WALLET_DB_COMMON_H
 #include "config.h"
 #include <ccan/autodata/autodata.h>
+#include <ccan/list/list.h>
 #include <ccan/short_types/short_types.h>
 #include <sqlite3.h>
 #include <stdbool.h>
@@ -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;