db: don't start transactions unless we really need to.

We always start a transaction before processing, but there are cases where
we don't need to.  Switch to doing it on-demand.

This doesn't make a big difference for sqlite3, but it can for Postgres because
of the latency: 12% or so.  Every bit helps!

30 runs, min-max(mean+/-stddev):

	Postgres before:  8.842773-9.769030(9.19531+/-0.21)
	Postgres after: 8.007967-8.321856(8.14172+/-0.066)

	sqlite3 before: 7.486042-8.371831(8.15544+/-0.19)
	sqlite3 after: 7.973411-8.576135(8.3025+/-0.12)

Signed-off-by: Rusty Russell <[email protected]>

Author: rustyrussell

db/common.h

@@ -29,6 +29,8 @@
 struct db {
 	char *filename;
 	const char *in_transaction;
+	/* For lazy transaction activation */
+	bool transaction_started;
 
 	/* DB-specific context */
 	void *conn;

db/db_sqlite3.c

@@ -474,12 +474,14 @@ static char **prepare_table_manip(const tal_t *ctx,
 
 	/* But core insists we're "in a transaction" for all ops, so fake it */
 	db->in_transaction = "Not really";
+	db->transaction_started = true;
 	/* Turn off foreign keys first. */
 	db_prepare_for_changes(db);
 	db_exec_prepared_v2(take(db_prepare_untranslated(db,
 							 "PRAGMA foreign_keys = OFF;")));
 	db_report_changes(db, NULL, 0);
 	db->in_transaction = NULL;
+	db->transaction_started = false;
 
 	db_begin_transaction(db);
 	cmd = tal_fmt(tmpctx, "ALTER TABLE %s RENAME TO temp_%s;",
@@ -525,11 +527,13 @@ static bool complete_table_manip(struct db *db,
 
 	/* Allow links between them (esp. cascade deletes!) */
 	db->in_transaction = "Not really";
+	db->transaction_started = true;
 	db_prepare_for_changes(db);
 	db_exec_prepared_v2(take(db_prepare_untranslated(db,
 					       "PRAGMA foreign_keys = ON;")));
 	db_report_changes(db, NULL, 0);
 	db->in_transaction = NULL;
+	db->transaction_started = false;
 
 	/* migrations are performed inside transactions, so start one. */
 	db_begin_transaction(db);

db/exec.c

@@ -121,19 +121,29 @@ static void db_data_version_incr(struct db *db)
 
 void db_begin_transaction_(struct db *db, const char *location)
 {
-	bool ok;
 	if (db->in_transaction)
 		db_fatal(db, "Already in transaction from %s", db->in_transaction);
 
+	db->in_transaction = location;
 	/* No writes yet. */
 	db->dirty = false;
+}
+
+void db_need_transaction(struct db *db, const char *location)
+{
+	bool ok;
+
+	if (!db->in_transaction)
+		db_fatal(db, "Not in a transaction for %s", location);
+
+	if (db->transaction_started)
+		return;
 
 	db_prepare_for_changes(db);
 	ok = db->config->begin_tx_fn(db);
 	if (!ok)
 		db_fatal(db, "Failed to start DB transaction: %s", db->error);
-
-	db->in_transaction = location;
+	db->transaction_started = true;
 }
 
 bool db_in_transaction(struct db *db)
@@ -150,6 +160,13 @@ void db_commit_transaction(struct db *db)
 {
 	bool ok;
 	assert(db->in_transaction);
+
+	if (!db->transaction_started) {
+		db->in_transaction = NULL;
+		assert(!db->dirty);
+		return;
+	}
+
 	db_assert_no_outstanding_statements(db);
 
 	/* Increment before reporting changes to an eventual plugin. */
@@ -164,4 +181,5 @@ void db_commit_transaction(struct db *db)
 
 	db->in_transaction = NULL;
 	db->dirty = false;
+	db->transaction_started = false;
 }

db/exec.h

@@ -40,6 +40,11 @@ void db_begin_transaction_(struct db *db, const char *location);
 
 bool db_in_transaction(struct db *db);
 
+/**
+ * db_need_transaction: we now need to actually enable the transaction, if not
+ * already. */
+void db_need_transaction(struct db *db, const char *location);
+
 /**
  * db_commit_transaction - Commit a running transaction
  *

db/utils.c

@@ -3,6 +3,7 @@
 #include <ccan/tal/str/str.h>
 #include <common/trace.h>
 #include <db/common.h>
+#include <db/exec.h>
 #include <db/utils.h>
 
 /* Matches the hash function used in devtools/sql-rewrite.py */
@@ -143,6 +144,7 @@ bool db_query_prepared_canfail(struct db_stmt *stmt)
 	assert(stmt->query->readonly);
 	trace_span_start("db_query_prepared", stmt);
 	trace_span_tag(stmt, "query", stmt->query->query);
+	db_need_transaction(stmt->db, stmt->query->query);
 	ret = stmt->db->config->query_fn(stmt);
 	stmt->executed = true;
 	list_del_from(&stmt->db->pending_statements, &stmt->list);
@@ -174,9 +176,12 @@ bool db_step(struct db_stmt *stmt)
 
 void db_exec_prepared_v2(struct db_stmt *stmt TAKES)
 {
+	bool ret;
+
+	db_need_transaction(stmt->db, stmt->query->query);
 	trace_span_start("db_exec_prepared", stmt);
 	trace_span_tag(stmt, "query", stmt->query->query);
-	bool ret = stmt->db->config->exec_fn(stmt);
+	ret = stmt->db->config->exec_fn(stmt);
 	trace_span_end(stmt);
 
 	if (stmt->db->readonly)
@@ -358,6 +363,7 @@ struct db *db_open_(const tal_t *ctx, const char *filename,
 		db_fatal(db, "Unable to find DB queries for %s", db->config->name);
 
 	db->in_transaction = NULL;
+	db->transaction_started = false;
 	db->changes = NULL;
 
 	/* This must be outside a transaction, so catch it */