Misc cleanups (extracted from autoclean rework)

db/db_postgres.c

@@ -277,27 +277,18 @@ static bool db_postgres_rename_column(struct db *db,
 				      const char *tablename,
 				      const char *from, const char *to)
 {
-	PGresult *res;
 	char *cmd;
 
 	cmd = tal_fmt(db, "ALTER TABLE %s RENAME %s TO %s;",
 		      tablename, from, to);
-	res = PQexec(db->conn, cmd);
-	if (PQresultStatus(res) != PGRES_COMMAND_OK) {
-		db->error = tal_fmt(db, "Rename '%s' failed: %s",
-				    cmd, PQerrorMessage(db->conn));
-		PQclear(res);
-		return false;
-	}
-	PQclear(res);
+	db_exec_prepared_v2(take(db_prepare_untranslated(db, cmd)));
 	return true;
 }
 
 static bool db_postgres_delete_columns(struct db *db,
 				       const char *tablename,
 				       const char **colnames, size_t num_cols)
 {
-	PGresult *res;
 	char *cmd;
 
 	cmd = tal_fmt(db, "ALTER TABLE %s ", tablename);
@@ -307,14 +298,8 @@ static bool db_postgres_delete_columns(struct db *db,
 		tal_append_fmt(&cmd, "DROP %s", colnames[i]);
 	}
 	tal_append_fmt(&cmd, ";");
-	res = PQexec(db->conn, cmd);
-	if (PQresultStatus(res) != PGRES_COMMAND_OK) {
-		db->error = tal_fmt(db, "Delete '%s' failed: %s",
-				    cmd, PQerrorMessage(db->conn));
-		PQclear(res);
-		return false;
-	}
-	PQclear(res);
+
+	db_exec_prepared_v2(take(db_prepare_untranslated(db, cmd)));
 	return true;
 }
 

db/db_sqlite3.c

@@ -1,7 +1,9 @@
 #include "config.h"
 #include <ccan/ccan/tal/str/str.h>
 #include <common/utils.h>
+#include <db/bindings.h>
 #include <db/common.h>
+#include <db/exec.h>
 #include <db/utils.h>
 
 #if HAVE_SQLITE3
@@ -446,6 +448,7 @@ static bool colname_to_delete(const char **colnames,
 	return false;
 }
 
+/* Returns NULL if this doesn't look like a column definition */
 static const char *find_column_name(const tal_t *ctx,
 				    const char *sqlpart,
 				    size_t *after)
@@ -455,12 +458,13 @@ static const char *find_column_name(const tal_t *ctx,
 	while (isspace(sqlpart[start]))
 		start++;
 	*after = strspn(sqlpart + start, "abcdefghijklmnopqrstuvwxyz_0123456789") + start;
-	if (*after == start)
+	if (*after == start || !cisspace(sqlpart[*after]))
 		return NULL;
 	return tal_strndup(ctx, sqlpart + start, *after - start);
 }
 
-/* Move table out the way, return columns */
+/* Move table out the way, return columns.
+ * Note: with db_hook, frees tmpctx! */
 static char **prepare_table_manip(const tal_t *ctx,
 				  struct db *db, const char *tablename)
 {
@@ -487,49 +491,43 @@ static char **prepare_table_manip(const tal_t *ctx,
 	sql = tal_strdup(tmpctx, (const char *)sqlite3_column_text(stmt, 0));
 	sqlite3_finalize(stmt);
 
+	/* We MUST use generic routines to write to db, since they
+	 * mirror changes to the db hook! */
 	bracket = strchr(sql, '(');
-	if (!strstarts(sql, "CREATE TABLE") || !bracket) {
-		db->error = tal_fmt(db, "strange schema for %s: %s",
-				    tablename, sql);
-		return NULL;
-	}
+	if (!strstarts(sql, "CREATE TABLE") || !bracket)
+		db_fatal("Bad sql from prepare_table_manip %s: %s",
+			 tablename, sql);
 
 	/* Split after ( by commas: any lower case is assumed to be a field */
 	parts = tal_strsplit(ctx, bracket + 1, ",", STR_EMPTY_OK);
 
+	/* Now, we actually need to turn OFF transactions for a moment, as
+	 * this pragma only has an effect outside a tx! */
+	db_commit_transaction(db);
+
+	/* But core insists we're "in a transaction" for all ops, so fake it */
+	db->in_transaction = "Not really";
 	/* Turn off foreign keys first. */
-	sqlite3_prepare_v2(wrapper->conn, "PRAGMA foreign_keys = OFF;", -1, &stmt, NULL);
-	if (sqlite3_step(stmt) != SQLITE_DONE)
-		goto sqlite_stmt_err;
-	sqlite3_finalize(stmt);
+	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_begin_transaction(db);
 	cmd = tal_fmt(tmpctx, "ALTER TABLE %s RENAME TO temp_%s;",
 		      tablename, tablename);
-	sqlite3_prepare_v2(wrapper->conn, cmd, -1, &stmt, NULL);
-	if (sqlite3_step(stmt) != SQLITE_DONE)
-		goto sqlite_stmt_err;
-	sqlite3_finalize(stmt);
-
-	/* Make sure we do the same to backup! */
-	replicate_statement(wrapper, "PRAGMA foreign_keys = OFF;");
-	replicate_statement(wrapper, cmd);
+	db_exec_prepared_v2(take(db_prepare_untranslated(db, cmd)));
 
 	return parts;
-
-sqlite_stmt_err:
-	db->error = tal_fmt(db, "%s", sqlite3_errmsg(wrapper->conn));
-	sqlite3_finalize(stmt);
-	return tal_free(parts);
 }
 
 static bool complete_table_manip(struct db *db,
 				 const char *tablename,
 				 const char **coldefs,
 				 const char **oldcolnames)
 {
-	sqlite3_stmt *stmt;
 	char *create_cmd, *insert_cmd, *drop_cmd;
-	struct db_sqlite3 *wrapper = (struct db_sqlite3 *)db->conn;
 
 	/* Create table */
 	create_cmd = tal_fmt(tmpctx, "CREATE TABLE %s (", tablename);
@@ -540,13 +538,7 @@ static bool complete_table_manip(struct db *db,
 	}
 	tal_append_fmt(&create_cmd, ";");
 
-	sqlite3_prepare_v2(wrapper->conn, create_cmd, -1, &stmt, NULL);
-	if (sqlite3_step(stmt) != SQLITE_DONE)
-		goto sqlite_stmt_err;
-	sqlite3_finalize(stmt);
-
-	/* Make sure we do the same to backup! */
-	replicate_statement(wrapper, create_cmd);
+	db_exec_prepared_v2(take(db_prepare_untranslated(db, create_cmd)));
 
 	/* Populate table from old one */
 	insert_cmd = tal_fmt(tmpctx, "INSERT INTO %s SELECT ", tablename);
@@ -557,33 +549,24 @@ static bool complete_table_manip(struct db *db,
 	}
 	tal_append_fmt(&insert_cmd, " FROM temp_%s;", tablename);
 
-	sqlite3_prepare_v2(wrapper->conn, insert_cmd, -1, &stmt, NULL);
-	if (sqlite3_step(stmt) != SQLITE_DONE)
-		goto sqlite_stmt_err;
-	sqlite3_finalize(stmt);
-	replicate_statement(wrapper, insert_cmd);
+	db_exec_prepared_v2(take(db_prepare_untranslated(db, insert_cmd)));
 
 	/* Cleanup temp table */
 	drop_cmd = tal_fmt(tmpctx, "DROP TABLE temp_%s;", tablename);
-	sqlite3_prepare_v2(wrapper->conn, drop_cmd, -1, &stmt, NULL);
-	if (sqlite3_step(stmt) != SQLITE_DONE)
-		goto sqlite_stmt_err;
-	sqlite3_finalize(stmt);
-	replicate_statement(wrapper, drop_cmd);
+	db_exec_prepared_v2(take(db_prepare_untranslated(db, drop_cmd)));
+	db_commit_transaction(db);
 
 	/* Allow links between them (esp. cascade deletes!) */
-	sqlite3_prepare_v2(wrapper->conn, "PRAGMA foreign_keys = ON;", -1, &stmt, NULL);
-	if (sqlite3_step(stmt) != SQLITE_DONE)
-		goto sqlite_stmt_err;
-	sqlite3_finalize(stmt);
-	replicate_statement(wrapper, "PRAGMA foreign_keys = ON;");
-
+	db->in_transaction = "Not really";
+	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;
+
+	/* migrations are performed inside transactions, so start one. */
+	db_begin_transaction(db);
 	return true;
-
-sqlite_stmt_err:
-	db->error = tal_fmt(db, "%s", sqlite3_errmsg(wrapper->conn));
-	sqlite3_finalize(stmt);
-	return false;
 }
 
 static bool db_sqlite3_rename_column(struct db *db,
@@ -594,10 +577,11 @@ static bool db_sqlite3_rename_column(struct db *db,
 	const char **coldefs, **oldcolnames;
 	bool colname_found = false;
 
-	parts = prepare_table_manip(tmpctx, db, tablename);
+	parts = prepare_table_manip(NULL, db, tablename);
 	if (!parts)
 		return false;
 
+	tal_steal(tmpctx, parts);
 	coldefs = tal_arr(tmpctx, const char *, 0);
 	oldcolnames = tal_arr(tmpctx, const char *, 0);
 
@@ -642,10 +626,11 @@ static bool db_sqlite3_delete_columns(struct db *db,
 	const char **coldefs, **oldcolnames;
 	size_t colnames_found = 0;
 
-	parts = prepare_table_manip(tmpctx, db, tablename);
+	parts = prepare_table_manip(NULL, db, tablename);
 	if (!parts)
 		return false;
 
+	tal_steal(tmpctx, parts);
 	coldefs = tal_arr(tmpctx, const char *, 0);
 	oldcolnames = tal_arr(tmpctx, const char *, 0);
 

db/exec.c

@@ -45,8 +45,11 @@ u32 db_data_version_get(struct db *db)
 	u32 version;
 	stmt = db_prepare_v2(db, SQL("SELECT intval FROM vars WHERE name = 'data_version'"));
 	db_query_prepared(stmt);
-	db_step(stmt);
-	version = db_col_int(stmt, "intval");
+	/* This fails on uninitialized db, so "0" */
+	if (db_step(stmt))
+		version = db_col_int(stmt, "intval");
+	else
+		version = 0;
 	tal_free(stmt);
 	return version;
 }

db/utils.c

@@ -61,11 +61,38 @@ static void db_stmt_free(struct db_stmt *stmt)
 }
 
 
+static struct db_stmt *db_prepare_core(struct db *db,
+				       const char *location,
+				       const struct db_query *db_query)
+{
+	struct db_stmt *stmt = tal(db, struct db_stmt);
+	size_t num_slots = db_query->placeholders;
+
+	/* Allocate the slots for placeholders/bindings, zeroed next since
+	 * that sets the type to DB_BINDING_UNINITIALIZED for later checks. */
+	stmt->bindings = tal_arrz(stmt, struct db_binding, num_slots);
+	stmt->location = location;
+	stmt->error = NULL;
+	stmt->db = db;
+	stmt->query = db_query;
+	stmt->executed = false;
+	stmt->inner_stmt = NULL;
+
+	tal_add_destructor(stmt, db_stmt_free);
+
+	list_add(&db->pending_statements, &stmt->list);
+
+#if DEVELOPER
+	stmt->cols_used = NULL;
+#endif /* DEVELOPER */
+
+	return stmt;
+}
+
 struct db_stmt *db_prepare_v2_(const char *location, struct db *db,
 			       const char *query_id)
 {
-	struct db_stmt *stmt = tal(db, struct db_stmt);
-	size_t num_slots, pos;
+	size_t pos;
 
 	/* Normalize query_id paths, because unit tests are compiled with this
 	 * prefix. */
@@ -81,40 +108,33 @@ struct db_stmt *db_prepare_v2_(const char *location, struct db *db,
 	for (;;) {
 		if (!db->queries->query_table[pos].name)
 			db_fatal("Could not resolve query %s", query_id);
-		if (streq(query_id, db->queries->query_table[pos].name)) {
-			stmt->query = &db->queries->query_table[pos];
+		if (streq(query_id, db->queries->query_table[pos].name))
 			break;
-		}
 		pos = (pos + 1) % db->queries->query_table_size;
 	}
 
-	num_slots = stmt->query->placeholders;
-	/* Allocate the slots for placeholders/bindings, zeroed next since
-	 * that sets the type to DB_BINDING_UNINITIALIZED for later checks. */
-	stmt->bindings = tal_arr(stmt, struct db_binding, num_slots);
-	for (size_t i=0; i<num_slots; i++)
-		stmt->bindings[i].type = DB_BINDING_UNINITIALIZED;
-
-	stmt->location = location;
-	stmt->error = NULL;
-	stmt->db = db;
-	stmt->executed = false;
-	stmt->inner_stmt = NULL;
+	return db_prepare_core(db, location, &db->queries->query_table[pos]);
+}
 
-	tal_add_destructor(stmt, db_stmt_free);
+/* Provides replication and hook interface for raw SQL too */
+struct db_stmt *db_prepare_untranslated(struct db *db, const char *query)
+{
+	struct db_query *db_query = tal(NULL, struct db_query);
+	struct db_stmt *stmt;
 
-	list_add(&db->pending_statements, &stmt->list);
+	db_query->name = db_query->query = query;
+	db_query->placeholders = strcount(query, "?");
+	db_query->readonly = false;
 
-#if DEVELOPER
-	stmt->cols_used = NULL;
-#endif /* DEVELOPER */
+	/* Use raw accessors! */
+	db_query->colnames = NULL;
+	db_query->num_colnames = 0;
 
+	stmt = db_prepare_core(db, "db_prepare_untranslated", db_query);
+	tal_steal(stmt, db_query);
 	return stmt;
 }
 
-#define db_prepare_v2(db,query) \
-	db_prepare_v2_(__FILE__ ":" stringify(__LINE__), db, query)
-
 bool db_query_prepared(struct db_stmt *stmt)
 {
 	/* Make sure we don't accidentally execute a modifying query using a

db/utils.h

@@ -97,4 +97,12 @@ void db_assert_no_outstanding_statements(struct db *db);
  */
 const char **db_changes(struct db *db);
 
+/**
+ * Accessor for internal use.
+ *
+ * Like db_prepare_v2() but creates temporary noop translation, and
+ * assumes not a read-only op.  Use this inside db-specific backends
+ * to re-use the normal db hook and replication logic.
+ */
+struct db_stmt *db_prepare_untranslated(struct db *db, const char *query);
 #endif /* LIGHTNING_DB_UTILS_H */

doc/lightning-addgossip.7.md

@@ -42,4 +42,4 @@ RESOURCES
 
 Main web site: <https://github.com/ElementsProject/lightning>
 
-[comment]: # ( SHA256STAMP:4d9f888d10faca2bf94d1b52510cf21fbeebae4efda0946f03d04b0ef4bc88a2)
+[comment]: # ( SHA256STAMP:326e5801f65998e13e909d8b682e9fbc9824f3a43aa7da1d76b871882e52f293)

doc/lightning-autocleaninvoice.7.md

@@ -52,4 +52,4 @@ RESOURCES
 
 Main web site: <https://github.com/ElementsProject/lightning>
 
-[comment]: # ( SHA256STAMP:06bb1ef610c0f82d3d370f468575f2d6e837a11473acd1267baab829aa505052)
+[comment]: # ( SHA256STAMP:994e8f17bf35fc704f13206bf4c6909b525f9edcb1c8c4508345c720b007d34c)

doc/lightning-bkpr-channelsapy.7.md

@@ -65,4 +65,4 @@ RESOURCES
 
 Main web site: <https://github.com/ElementsProject/lightning>
 
-[comment]: # ( SHA256STAMP:296069892023c371e26305e8ab54d04a50aefeeff37a8a465d07893a8c39e61c)
+[comment]: # ( SHA256STAMP:8ec833f8261ab8b559f0d645d6da45322b388905413ef262d95f5039d533fdc8)

doc/lightning-bkpr-dumpincomecsv.7.md

@@ -57,4 +57,4 @@ RESOURCES
 
 Main web site: <https://github.com/ElementsProject/lightning>
 
-[comment]: # ( SHA256STAMP:4d9326acde55e8124ea61a7e2430f035d3a4957995a76ddef61ecb2a3debd042)
+[comment]: # ( SHA256STAMP:1375c000d025b6cb72daa3b2ea64ec3212ae1aa5552c0d87918fd869d2fc5a0b)

doc/lightning-bkpr-inspect.7.md

@@ -52,4 +52,4 @@ RESOURCES
 
 Main web site: <https://github.com/ElementsProject/lightning>
 
-[comment]: # ( SHA256STAMP:022a11cd4bdaec7b1b39e30faaf5489de1db08684a3952b3a3152343e91c8c4c)
+[comment]: # ( SHA256STAMP:ea50ea813e46669b522ebd466619ac6f7a4be5ae38b4f976a7db70a3c01b7fae)

doc/lightning-bkpr-listaccountevents.7.md

@@ -71,4 +71,4 @@ RESOURCES
 
 Main web site: <https://github.com/ElementsProject/lightning>
 
-[comment]: # ( SHA256STAMP:3b120f28969d50351823707c11f77a0264031b1ddec07794ac117b0e8e2d7a75)
+[comment]: # ( SHA256STAMP:1ac0919bf29ebc37a92283d15a9ffa06f0f46be5fb55920b335d0c43e02a6ee4)