From 7b74aead59d1167c72a1f135cd53a0adb624aeb8 Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 27 Mar 2019 23:11:17 -0400 Subject: [PATCH 1/6] support backing up to an instance of sqlite3.Database The destination database must be locked, as per https://www.sqlite.org/c3ref/backup_finish.html ("Concurrent usage of database handles"). --- src/backup.cc | 71 +++++++++++++++++++++++++++++++++++---------- src/backup.h | 13 +++++---- test/backup.test.js | 51 ++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 21 deletions(-) diff --git a/src/backup.cc b/src/backup.cc index 9f3893b96..fec415085 100644 --- a/src/backup.cc +++ b/src/backup.cc @@ -133,9 +133,8 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) Napi::TypeError::New(env, "Database object expected").ThrowAsJavaScriptException(); return; } - else if (length <= 1 || !info[1].IsString()) { - Napi::TypeError::New(env, "Filename expected").ThrowAsJavaScriptException(); - return; + else if (length <= 1 || !(info[1]->IsString() || info[1]->IsObject())) { + return Nan::ThrowTypeError("Filename or database object expected"); } else if (length <= 2 || !info[2].IsString()) { Napi::TypeError::New(env, "Source database name expected").ThrowAsJavaScriptException(); @@ -155,6 +154,12 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) } Database* db = Napi::ObjectWrap::Unwrap(info[0].As()); + Database* otherDb = NULL; + if (info[1]->IsObject()) { + // A database instance was passed instead of a filename + otherDb = Napi::ObjectWrap::Unwrap(info[1].As()); + otherDb->Ref(); + } Napi::String filename = info[1].As(); Napi::String sourceName = info[2].As(); Napi::String destName = info[3].As(); @@ -168,11 +173,26 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) init(db); InitializeBaton* baton = new InitializeBaton(db, info[5].As(), this); + baton->otherDb = otherDb; baton->filename = filename.Utf8Value(); baton->sourceName = sourceName.Utf8Value(); baton->destName = destName.Utf8Value(); baton->filenameIsDest = filenameIsDest.Value(); - db->Schedule(Work_BeginInitialize, baton); + + if (otherDb) { + otherDb->Schedule(Work_BeforeInitialize, baton, true); + } else { + db->Schedule(Work_BeginInitialize, baton); + } + info.GetReturnValue().Set(info.This()); +} + +void Backup::Work_BeforeInitialize(Database::Baton* baton) { + InitializeBaton *initBaton = static_cast(baton); + // at this point, the target database object is locked (it is + // important that its database connection remains unused). + initBaton->otherDb->pending++; + baton->db->Schedule(Work_BeginInitialize, baton); } void Backup::Work_BeginInitialize(Database::Baton* baton) { @@ -195,22 +215,32 @@ void Backup::Work_Initialize(napi_env e, void* data) { sqlite3_mutex* mtx = sqlite3_db_mutex(baton->db->_handle); sqlite3_mutex_enter(mtx); - backup->status = sqlite3_open(baton->filename.c_str(), &backup->_otherDb); + backup->message = ""; + if (baton->otherDb) { + backup->otherDb = baton->otherDb; + backup->_otherDbHandle = baton->otherDb->_handle; + backup->status = SQLITE_OK; + if (!baton->filenameIsDest) { + backup->status = SQLITE_MISUSE; + backup->message = "do not toggle filenameIsDest when backing up between sqlite3.Database instances"; + } + } else { + backup->otherDb = NULL; + backup->status = sqlite3_open(baton->filename.c_str(), &backup->_otherDbHandle); + } if (backup->status == SQLITE_OK) { backup->_handle = sqlite3_backup_init( - baton->filenameIsDest ? backup->_otherDb : backup->db->_handle, + baton->filenameIsDest ? backup->_otherDbHandle : backup->db->_handle, baton->destName.c_str(), - baton->filenameIsDest ? backup->db->_handle : backup->_otherDb, + baton->filenameIsDest ? backup->db->_handle : backup->_otherDbHandle, baton->sourceName.c_str()); } - backup->_destDb = baton->filenameIsDest ? backup->_otherDb : backup->db->_handle; + backup->_destDbHandle = baton->filenameIsDest ? backup->_otherDbHandle : backup->db->_handle; if (backup->status != SQLITE_OK) { - backup->message = std::string(sqlite3_errmsg(backup->_destDb)); - sqlite3_close(backup->_otherDb); - backup->_otherDb = NULL; - backup->_destDb = NULL; + if (backup->message == "") backup->message = std::string(sqlite3_errmsg(backup->_destDbHandle)); + backup->FinishSqlite(); } sqlite3_mutex_leave(mtx); @@ -354,6 +384,13 @@ void Backup::FinishAll() { CleanQueue(); FinishSqlite(); db->Unref(); + if (otherDb) { + assert(otherDb->locked); + otherDb->pending--; + otherDb->Process(); + otherDb->Unref(); + otherDb = NULL; + } } void Backup::FinishSqlite() { @@ -361,11 +398,13 @@ void Backup::FinishSqlite() { sqlite3_backup_finish(_handle); _handle = NULL; } - if (_otherDb) { - sqlite3_close(_otherDb); - _otherDb = NULL; + if (_otherDbHandle) { + if (!otherDb) { + sqlite3_close(_otherDbHandle); + } + _otherDbHandle = NULL; } - _destDb = NULL; + _destDbHandle = NULL; } Napi::Value Backup::IdleGetter(const Napi::CallbackInfo& info) { diff --git a/src/backup.h b/src/backup.h index c15b77bfe..27741bb36 100644 --- a/src/backup.h +++ b/src/backup.h @@ -113,6 +113,7 @@ class Backup : public Napi::ObjectWrap { struct InitializeBaton : Database::Baton { Backup* backup; + Database* otherDb; std::string filename; std::string sourceName; std::string destName; @@ -147,9 +148,10 @@ class Backup : public Napi::ObjectWrap { void init(Database* db_) { db = db_; + otherDb = NULL; _handle = NULL; - _otherDb = NULL; - _destDb = NULL; + _otherDbHandle = NULL; + _destDbHandle = NULL; inited = false; locked = true; completed = false; @@ -157,7 +159,6 @@ class Backup : public Napi::ObjectWrap { remaining = -1; pageCount = -1; finished = false; - db->Ref(); } Backup(const Napi::CallbackInfo& info); @@ -183,6 +184,7 @@ class Backup : public Napi::ObjectWrap { void RetryErrorSetter(const Napi::CallbackInfo& info, const Napi::Value& value); protected: + static void Work_BeforeInitialize(Database::Baton* baton); static void Work_BeginInitialize(Database::Baton* baton); static void Work_Initialize(napi_env env, void* data); static void Work_AfterInitialize(napi_env env, napi_status status, void* data); @@ -197,10 +199,11 @@ class Backup : public Napi::ObjectWrap { void GetRetryErrors(std::set& retryErrorsSet); Database* db; + Database* otherDb; sqlite3_backup* _handle; - sqlite3* _otherDb; - sqlite3* _destDb; + sqlite3* _otherDbHandle; + sqlite3* _destDbHandle; int status; std::string message; diff --git a/test/backup.test.js b/test/backup.test.js index c05c298f8..e9d99eb30 100644 --- a/test/backup.test.js +++ b/test/backup.test.js @@ -276,4 +276,55 @@ describe('backup', function() { }); }); }); + + it ('can backup between two sqlite3.Database instances', function(done) { + var src = new sqlite3.Database(':memory:', function(err) { + if (err) throw err; + src.exec("CREATE TABLE space (txt TEXT)", function(err) { + if (err) throw err; + src.exec("INSERT INTO space(txt) VALUES('monkey')", function(err) { + if (err) throw err; + var dest = new sqlite3.Database(':memory:', function(err) { + if (err) throw err; + var backup = src.backup(dest); + backup.step(-1); + backup.finish(function(err) { + if (err) throw err; + assertRowsMatchDb(src, 'space', dest, 'space', done); + }); + }); + }); + }); + }); + }); + + it ('locks destination when backing up between two sqlite3.Database instances', function(done) { + var src = new sqlite3.Database(':memory:', function(err) { + if (err) throw err; + src.exec("CREATE TABLE space (txt TEXT)", function(err) { + if (err) throw err; + src.exec("INSERT INTO space(txt) VALUES('monkey')", function(err) { + if (err) throw err; + var dest = new sqlite3.Database(':memory:', function(err) { + if (err) throw err; + var backup = src.backup(dest, function(err) { + if (err) throw err; + var finished = false; + // This action on dest db should be held until backup finishes. + dest.exec("CREATE TABLE space2 (txt TEXT)", function(err) { + if (err) throw err; + assert(finished); + done(); + }); + backup.step(-1); + backup.finish(function(err) { + if (err) throw err; + finished = true; + }); + }); + }); + }); + }); + }); + }); }); From 401937943af31cd6e7844deb124e315df4c58400 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoshi=20J=C3=A4ger?= Date: Tue, 25 Oct 2022 06:41:31 +0200 Subject: [PATCH 2/6] Resolved several issues and added comments. --- src/backup.cc | 21 ++++++++++++++------- src/backup.h | 5 +++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/backup.cc b/src/backup.cc index fec415085..a6a541c62 100644 --- a/src/backup.cc +++ b/src/backup.cc @@ -133,8 +133,9 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) Napi::TypeError::New(env, "Database object expected").ThrowAsJavaScriptException(); return; } - else if (length <= 1 || !(info[1]->IsString() || info[1]->IsObject())) { - return Nan::ThrowTypeError("Filename or database object expected"); + else if (length <= 1 || !(info[1].IsString() || info[1].IsObject())) { + Napi::TypeError::New(env, "Filename or database object expected").ThrowAsJavaScriptException(); + return; } else if (length <= 2 || !info[2].IsString()) { Napi::TypeError::New(env, "Source database name expected").ThrowAsJavaScriptException(); @@ -155,10 +156,9 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) Database* db = Napi::ObjectWrap::Unwrap(info[0].As()); Database* otherDb = NULL; - if (info[1]->IsObject()) { + if (info[1].IsObject()) { // A database instance was passed instead of a filename otherDb = Napi::ObjectWrap::Unwrap(info[1].As()); - otherDb->Ref(); } Napi::String filename = info[1].As(); Napi::String sourceName = info[2].As(); @@ -170,7 +170,7 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) info.This().As().DefineProperty(Napi::PropertyDescriptor::Value("destName", destName)); info.This().As().DefineProperty(Napi::PropertyDescriptor::Value("filenameIsDest", filenameIsDest)); - init(db); + init(db, otherDb); InitializeBaton* baton = new InitializeBaton(db, info[5].As(), this); baton->otherDb = otherDb; @@ -184,7 +184,6 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) } else { db->Schedule(Work_BeginInitialize, baton); } - info.GetReturnValue().Set(info.This()); } void Backup::Work_BeforeInitialize(Database::Baton* baton) { @@ -217,14 +216,19 @@ void Backup::Work_Initialize(napi_env e, void* data) { backup->message = ""; if (baton->otherDb) { + // If another database instance was passed, + // link other (locked) db to the backup state backup->otherDb = baton->otherDb; backup->_otherDbHandle = baton->otherDb->_handle; + backup->status = SQLITE_OK; if (!baton->filenameIsDest) { backup->status = SQLITE_MISUSE; backup->message = "do not toggle filenameIsDest when backing up between sqlite3.Database instances"; } } else { + // Do not initialize otherDb and continue with normal + // initialization by using the filename that was provided backup->otherDb = NULL; backup->status = sqlite3_open(baton->filename.c_str(), &backup->_otherDbHandle); } @@ -384,13 +388,14 @@ void Backup::FinishAll() { CleanQueue(); FinishSqlite(); db->Unref(); + if (otherDb) { assert(otherDb->locked); otherDb->pending--; otherDb->Process(); otherDb->Unref(); otherDb = NULL; - } + } } void Backup::FinishSqlite() { @@ -400,6 +405,8 @@ void Backup::FinishSqlite() { } if (_otherDbHandle) { if (!otherDb) { + // Only close the database if it was + // not passed as a descriptor already. sqlite3_close(_otherDbHandle); } _otherDbHandle = NULL; diff --git a/src/backup.h b/src/backup.h index 27741bb36..e3387678a 100644 --- a/src/backup.h +++ b/src/backup.h @@ -146,9 +146,9 @@ class Backup : public Napi::ObjectWrap { Baton* baton; }; - void init(Database* db_) { + void init(Database* db_, Database* otherDb_) { db = db_; - otherDb = NULL; + otherDb = otherDb_; _handle = NULL; _otherDbHandle = NULL; _destDbHandle = NULL; @@ -159,6 +159,7 @@ class Backup : public Napi::ObjectWrap { remaining = -1; pageCount = -1; finished = false; + db->Ref(); } Backup(const Napi::CallbackInfo& info); From ed7d224280d25d68df808bf4d3cdff47422cd412 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoshi=20J=C3=A4ger?= Date: Tue, 25 Oct 2022 07:45:09 +0200 Subject: [PATCH 3/6] fix: backup-online: database instance was casted to string mistakenly. --- src/backup.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/backup.cc b/src/backup.cc index a6a541c62..35e6666b6 100644 --- a/src/backup.cc +++ b/src/backup.cc @@ -156,11 +156,17 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) Database* db = Napi::ObjectWrap::Unwrap(info[0].As()); Database* otherDb = NULL; + + Napi::String filename; + if (info[1].IsObject()) { // A database instance was passed instead of a filename otherDb = Napi::ObjectWrap::Unwrap(info[1].As()); - } - Napi::String filename = info[1].As(); + filename = Napi::String::New(env, ""); + } else { + filename = info[1].As(); + } + Napi::String sourceName = info[2].As(); Napi::String destName = info[3].As(); Napi::Boolean filenameIsDest = info[4].As(); @@ -393,7 +399,6 @@ void Backup::FinishAll() { assert(otherDb->locked); otherDb->pending--; otherDb->Process(); - otherDb->Unref(); otherDb = NULL; } } From 63a16955176edc4a3bb0cd52f18c817923f375db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoshi=20J=C3=A4ger?= Date: Tue, 25 Oct 2022 07:47:29 +0200 Subject: [PATCH 4/6] fix: added Ref() and Unref() calls to the database instance that was passed --- src/backup.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/backup.cc b/src/backup.cc index 35e6666b6..1835efd97 100644 --- a/src/backup.cc +++ b/src/backup.cc @@ -162,6 +162,8 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) if (info[1].IsObject()) { // A database instance was passed instead of a filename otherDb = Napi::ObjectWrap::Unwrap(info[1].As()); + otherDb->Ref(); + filename = Napi::String::New(env, ""); } else { filename = info[1].As(); @@ -399,6 +401,7 @@ void Backup::FinishAll() { assert(otherDb->locked); otherDb->pending--; otherDb->Process(); + otherDb->Unref(); otherDb = NULL; } } From b8c98e3a8012c5623114fcea0b7a85b7387c90e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoshi=20J=C3=A4ger?= Date: Tue, 25 Oct 2022 11:08:28 +0200 Subject: [PATCH 5/6] fix: pass Backup instance to second callback argument --- src/backup.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backup.cc b/src/backup.cc index 1835efd97..cb1a85eb5 100644 --- a/src/backup.cc +++ b/src/backup.cc @@ -273,8 +273,8 @@ void Backup::Work_AfterInitialize(napi_env e, napi_status status, void* data) { backup->inited = true; Napi::Function cb = baton->callback.Value(); if (!cb.IsEmpty() && cb.IsFunction()) { - Napi::Value argv[] = { env.Null() }; - TRY_CATCH_CALL(backup->Value(), cb, 1, argv); + Napi::Value argv[] = { env.Null(), backup->Value() }; + TRY_CATCH_CALL(backup->Value(), cb, 2, argv); } } BACKUP_END(); From 8d87dcc90454928a6919132f57ccdf57a888d3b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoshi=20J=C3=A4ger?= Date: Tue, 25 Oct 2022 11:08:54 +0200 Subject: [PATCH 6/6] ts: added typescript declarations for Backup --- lib/sqlite3.d.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/sqlite3.d.ts b/lib/sqlite3.d.ts index b27b0cf51..9f57a673d 100644 --- a/lib/sqlite3.d.ts +++ b/lib/sqlite3.d.ts @@ -93,12 +93,26 @@ export class Statement extends events.EventEmitter { each(...params: any[]): this; } +export class Backup extends events.EventEmitter { + step(pages: number, callback?: (error: Error, backup: Backup) => void): Backup; + finish(callback?: (error: Error, backup: Backup) => void): Backup; + + get idle(): boolean; + get completed(): boolean; + get failed(): boolean; + get remaining(): number; + get pageCount(): number; +} + export class Database extends events.EventEmitter { constructor(filename: string, callback?: (err: Error | null) => void); constructor(filename: string, mode?: number, callback?: (err: Error | null) => void); close(callback?: (err: Error | null) => void): void; + backup(destination: string | Database, destName: string, sourceName: string, filenameIsDest = true, callback?: (this: Backup, err: Error | null, backup: Backup) => void): this; + backup(destination: string | Database, callback?: (this: Backup, err: Error | null, backup: Backup) => void): this; + run(sql: string, callback?: (this: RunResult, err: Error | null) => void): this; run(sql: string, params: any, callback?: (this: RunResult, err: Error | null) => void): this; run(sql: string, ...params: any[]): this; @@ -201,5 +215,6 @@ export interface sqlite3 { RunResult: RunResult; Statement: typeof Statement; Database: typeof Database; + Backup: typeof Backup; verbose(): this; } \ No newline at end of file