Merge pull request #100 from pi-hole/fix/DB_GC

Refactor database thread

FTL.h

@@ -223,4 +223,4 @@ bool daemonmode;
 bool database;
 long int lastdbindex;
 bool travis;
-bool needDBGC;
+bool DBdeleteoldqueries;

Makefile

@@ -30,11 +30,10 @@ CC=gcc
 HARDENING_FLAGS=-fstack-protector -D_FORTIFY_SOURCE=2 -O3 -Wl,-z,relro,-z,now -pie -fPIE
 DEBUG_FLAGS=-rdynamic -fno-omit-frame-pointer #-fsanitize=address
 # -DSQLITE_OMIT_LOAD_EXTENSION: This option omits the entire extension loading mechanism from SQLite, including sqlite3_enable_load_extension() and sqlite3_load_extension() interfaces. (needs -ldl linking option, otherwise)
-# -DSQLITE_THREADSAFE=0: causes all of the mutex and thread-safety logic in SQLite to be omitted. This is the single compile-time option that makes the most difference in optimizing the performance of SQLite.
 # -DSQLITE_DEFAULT_MEMSTATUS=0: This setting causes the sqlite3_status() interfaces that track memory usage to be disabled. This helps the sqlite3_malloc() routines run much faster, and since SQLite uses sqlite3_malloc() internally, this helps to make the entire library faster.
 # -DSQLITE_OMIT_DEPRECATED: Omitting deprecated interfaces and features will not help SQLite to run any faster. It will reduce the library footprint, however. And it is the right thing to do.
 # -DSQLITE_OMIT_PROGRESS_CALLBACK: The progress handler callback counter must be checked in the inner loop of the bytecode engine. By omitting this interface, a single conditional is removed from the inner loop of the bytecode engine, helping SQL statements to run slightly faster.
-SQLITEFLAGS=-DSQLITE_OMIT_LOAD_EXTENSION -DSQLITE_THREADSAFE=0 -DSQLITE_DEFAULT_MEMSTATUS=0 -DSQLITE_OMIT_DEPRECATED -DSQLITE_OMIT_PROGRESS_CALLBACK -DSQLITE_OMIT_MEMORYDB
+SQLITEFLAGS=-DSQLITE_OMIT_LOAD_EXTENSION -DSQLITE_DEFAULT_MEMSTATUS=0 -DSQLITE_OMIT_DEPRECATED -DSQLITE_OMIT_PROGRESS_CALLBACK -DSQLITE_OMIT_MEMORYDB
 CCFLAGS=-I$(IDIR) -Wall -Wextra -Wno-unused-parameter -D_FILE_OFFSET_BITS=64 $(HARDENING_FLAGS) $(DEBUG_FLAGS) $(CFLAGS) $(SQLITEFLAGS)
 LIBS=-pthread
 

database.c

@@ -12,10 +12,19 @@
 
 sqlite3 *db;
 bool database = false;
+bool DBdeleteoldqueries = false;
 long int lastdbindex = 0;
 
+pthread_mutex_t dblock;
+
 enum { DB_VERSION, DB_LASTTIMESTAMP };
 
+void dbclose(void)
+{
+ sqlite3_close(db);
+ pthread_mutex_unlock(&dblock);
+}
+
 float get_db_filesize(void)
 {
  struct stat st;
@@ -37,10 +46,11 @@ static int callback(void *NotUsed, int argc, char **argv, char **azColName){
 
 bool dbopen(void)
 {
+ pthread_mutex_lock(&dblock);
  int rc = sqlite3_open_v2(FTLfiles.db, &db, SQLITE_OPEN_READWRITE, NULL);
  if( rc ){
  logg("Cannot open database: %s", sqlite3_errmsg(db));
- sqlite3_close(db);
+ dbclose();
  return false;
  }
 
@@ -82,18 +92,13 @@ bool dbquery(const char *format, ...)
 
 }
 
-void dbclose(void)
-{
- sqlite3_close(db);
-}
-
 bool db_create(void)
 {
  bool ret;
  int rc = sqlite3_open_v2(FTLfiles.db, &db, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL);
  if( rc ){
  logg("Can't create database: %s", sqlite3_errmsg(db));
- sqlite3_close(db);
+ dbclose();
  return false;
  }
  // Create Queries table in the database
@@ -128,17 +133,24 @@ void db_init(void)
  int rc = sqlite3_open_v2(FTLfiles.db, &db, SQLITE_OPEN_READWRITE, NULL);
  if( rc ){
  logg("Cannot open database: %s", sqlite3_errmsg(db));
- sqlite3_close(db);
+ dbclose();
 
  logg("Creating new (empty) database");
  if (!db_create())
  {
- logg("Database not available ");
+ logg("Database not available");
  database = false;
  return;
  }
  }
 
+ if (pthread_mutex_init(&dblock, NULL) != 0)
+ {
+ printf("FATAL: DB mutex init failed\n");
+ // Return failure
+ exit(EXIT_FAILURE);
+ }
+
  logg("Database initialized");
  database = true;
 }
@@ -161,7 +173,7 @@ int db_get_FTL_property(unsigned int ID)
  rc = sqlite3_prepare(db, querystring, -1, &dbstmt, NULL);
  if( rc ){
  printf("Cannot read from database: %s", sqlite3_errmsg(db));
- sqlite3_close(db);
+ dbclose();
  return -1;
  }
  free(querystring);
@@ -170,7 +182,7 @@ int db_get_FTL_property(unsigned int ID)
  sqlite3_step(dbstmt);
  if( rc ){
  printf("Cannot evaluate in database: %s", sqlite3_errmsg(db));
- sqlite3_close(db);
+ dbclose();
  return -1;
  }
 
@@ -222,18 +234,13 @@ int get_number_of_queries_in_DB(void)
  return result;
 }
 
-void *DB_thread(void *val)
+void save_to_DB(void)
 {
- // Set thread name
- prctl(PR_SET_NAME,"DB",0,0,0);
-
- // Lock FTL's data structure, since it is likely that it will be changed here
- enable_thread_lock("DB_thread");
-
+ // Open database
  if(!dbopen())
  {
- logg("Failed to open DB in thread");
- return NULL;
+ logg("Failed to open DB in save_to_DB()");
+ return;
  }
 
  int lasttimestamp = db_get_FTL_property(DB_LASTTIMESTAMP);
@@ -296,7 +303,7 @@ void *DB_thread(void *val)
  sqlite3_reset(stmt);
 
  if( rc != SQLITE_DONE ){
- logg("DB thread - SQL error: %s", sqlite3_errmsg(db));
+ logg("save_to_DB() - SQL error: %s", sqlite3_errmsg(db));
  saved_error++;
  continue;
  }
@@ -332,42 +339,56 @@ void *DB_thread(void *val)
  if(saved_error > 0)
  logg(" Queries NOT stored in DB: %u (due to an error)", saved_error);
  }
-
- // Release thread lock
- disable_thread_lock("DB_thread");
-
- return NULL;
 }
 
-void *DB_GC_thread(void *val)
+void delete_old_queries_in_DB(void)
 {
- // Set thread name
- prctl(PR_SET_NAME,"DB-GC",0,0,0);
-
- // Need no lock on FTL's data structure, so this can work
- // in parallel w/o affecting FTL's core responsibilities
-
+ // Open database
  if(!dbopen())
  {
- logg("Failed to open DB in GC thread");
- database = true;
- return NULL;
+ logg("Failed to open DB in delete_old_queries_in_DB()");
+ return;
  }
 
  int timestamp = time(NULL) - config.maxDBdays * 86400;
 
  if(!dbquery("DELETE FROM queries WHERE timestamp <= %i", timestamp))
  {
  dbclose();
- logg("DB-GC error: Deleting queries due to age of entries failed!");
+ logg("delete_old_queries_in_DB(): Deleting queries due to age of entries failed!");
  database = true;
- return NULL;
+ return;
  }
 
  // Close database
  dbclose();
 
  // Re-enable database actions
  database = true;
+}
+
+void *DB_thread(void *val)
+{
+ // Set thread name
+ prctl(PR_SET_NAME,"DB",0,0,0);
+
+ if(!DBdeleteoldqueries)
+ {
+ // Lock FTL's data structure, since it is likely that it will be changed here
+ enable_thread_lock("DB_thread");
+
+ // Save data to database
+ save_to_DB();
+
+ // Release thread lock
+ disable_thread_lock("DB_thread");
+ }
+ else
+ {
+ // No thread locks needed
+ delete_old_queries_in_DB();
+ DBdeleteoldqueries = false;
+ }
+
  return NULL;
 }

main.c

@@ -13,6 +13,7 @@
 
 char * username;
 bool needGC = false;
+bool needDBGC = false;
 
 int main (int argc, char* argv[]) {
  username = getUserName();
@@ -112,20 +113,7 @@ int main (int argc, char* argv[]) {
  }
 
  if(database)
- {
- // Disable any other DB accesses while doing this and wait one second
- // so that currently running transactions can still finish
- database = false;
- sleepms(1000);
-
- // Launch DB GC thread
- pthread_t DBGCthread;
- if(pthread_create( &DBGCthread, &attr, DB_GC_thread, NULL ) != 0)
- {
- logg("Unable to open DB GC thread. Exiting...");
- killed = 1;
- }
- }
+ DBdeleteoldqueries = true;
 
  // Avoid immediate re-run of GC thread
  while(((time(NULL) - GCdelay)%GCinterval) == 0)

request.c

@@ -114,11 +114,6 @@ void process_request(char *client_message, int *sock)
  processed = true;
  getDBstats(sock);
  }
- else if(command(client_message, ">dbclean"))
- {
- processed = true;
- needDBGC = true;
- }
 
  // End of queryable commands
  if(processed)

routines.h

@@ -70,4 +70,3 @@ void *GC_thread(void *val);
 void db_init(void);
 void *DB_thread(void *val);
 int get_number_of_queries_in_DB(void);
-void *DB_GC_thread(void *val);