3
\$\begingroup\$

I need to seed a database with user data.

When trying to write records to a file using sqlite3 with node, I tried writing the logic in a naive manner, without any error checks - but it was failing with numerous SQLITE_BUSY database locked errors.

Then I added code to retry in case of an error, but was insufficient - ended up with corrupt tables, and so I added transactions as suggested on freenode.

Now, it works. However the problem is that it is pathetically slow - takes up to 5 minutes for around 50 records. Also, I need to pass in a retries parameter of roughly number_of_users * 2 in order for the process to complete successfully or else it bails out. Something is not correct, but I cannot pin it down.

What is wrong?

//in the constructor ...
function TestController (api_url_root,number_of_users,number_of_devices) {
...
    this.user_write_q = async.queue(function (task,callback) {
        that.registerUserWithAPI(task.username, task.email, task.password, task.runAfterAllUsersAdded, callback);
    }, 1);

    this.user_write_q.drain = function() {
        console.log("Finished adding all users - queue drained");
    }
...
}


TestController.prototype.addUsers = function (callback_after_all_users_have_been_added) {

    this.pending_db_writes_for_user = this.number_of_users;
    fs.appendFileSync(this.user_list_file_path,"user email, password, api_key, api_secret\n");

    for(var i=0;i < this.number_of_users;i++) {
        var username    = Faker.Name.findName();
        var useremail   = Faker.Internet.email();
        var password    = Faker.Lorem.words(1)[0];
        this.user_write_q.push({username: username, email: useremail, password: password, runAfterAllUsersAdded: callback_after_all_users_have_been_added});
        console.log("Remaining requests: " + that.user_write_q.length());
    }
};


TestController.prototype.registerUserWithAPI = function (username, useremail, password, callback_after_all_users_have_been_added, queueCallback) {

    var controller = this;

    var post_options = {
        host: this.api_url_root,
        path: '/users/'+ useremail,
        method: 'POST',
        headers: {
            'Content-type': 'application/json'
        }
    }

    var post_req = http.request(post_options, function (res) {
            var data = "";
            res.setEncoding('utf8');
            res.on('data', function(chunk) {
                data += chunk;
            });

            res.on('end', function() {
                var dat = JSON.parse(data);
               controller.writeUserToLocalDatabase(controller.number_of_users*2,username,useremail,password,dat.api_key,dat.api_secret, queueCallback, callback_after_all_users_have_been_added);
               });
        });

    var post_data = '{"name":"'+username+'","password":"'+ password + '"}';
    post_req.write(post_data);
    post_req.end();
}


TestController.prototype.writeUserToLocalDatabase = function (retries,username,useremail,password,api_key,api_secret, queueCallback, callback_after_all_users_have_been_added) {
    var dbase = this.connectToDb();
    var controller = this;

    if (retries < 0) {
        console.log("Bailing out of writeUserToLocalDatabase after max retry attempts");
        return;
    }

    dbase.run("BEGIN TRANSACTION");

    var stmt = dbase.prepare("insert into user_table values(?,?,?,?,?)", function(err) {
          if(err !== null) {
            console.log("Error when trying to write user to database in prepare! "+err);
            dbase.run("ROLLBACK");
            dbase.close(function(err){if(err !== null) {console.log("Close failed in in prepare "+err);}});
            controller.writeUserToLocalDatabase(--retries,username,useremail,password,api_key,api_secret, queueCallback, callback_after_all_users_have_been_added);
         }
    });

    stmt.run(username,useremail,password,api_key,api_secret, function(err) {
        if(err !== null) {
            //console.log("Error when trying to write user to database in run! "+err);
            stmt.finalize();
            dbase.run("ROLLBACK");
            dbase.close(function(err){if(err !== null) {console.log("Close failed in in run "+err);}});
            controller.writeUserToLocalDatabase(--retries,username,useremail,password,api_key,api_secret,queueCallback,  callback_after_all_users_have_been_added);
        }
        else {
            stmt.finalize(function(err) {
                if(err !== null && err !== undefined) {
                    console.log("Error when trying to write user to database in finalize ! "+err);
                    dbase.run("ROLLBACK");
                    dbase.close(function(err){if(err !== null) {console.log("Close failed in in finalize "+err);}});
                    controller.writeUserToLocalDatabase(--retries,username,useremail,password,api_key,api_secret, queueCallback, callback_after_all_users_have_been_added);
                }
                else {
                    dbase.run("COMMIT");
                    controller.appendToUserFile(useremail,password,api_key,api_secret);
                    controller.pending_db_writes_for_user--;
                    if (controller.pending_db_writes_for_user <= 0) {
                        console.log("Finished writing all users, now invoking db.close with callback");
                        queueCallback();
                        dbase.close(callback_after_all_users_have_been_added);
                    }
                    else {
                        console.log("Remaining users to add: "+controller.pending_db_writes_for_user);
                        dbase.close(function(err){if(err !== null) {console.log("Close failed in after adding user "+err);}});
                        queueCallback();
                    }
                }
            });
        }
    });
}

EDIT --- The code works acceptably. I managed to improve the performance by orders of magnitude by using async.queue with a queue length of 1 - now about 5 records per second as compared to 20 seconds per record. However this remains an open question about the best way to do concurrent writes to an sqlite3 database from node.

\$\endgroup\$
  • 2
    \$\begingroup\$ I would simply generate a file with INSERT statements in Node. Then upload that file straight into your SQLITE database. I do not believe that your approach is meant for ETL. \$\endgroup\$ – konijn Apr 8 '14 at 12:36
  • 1
    \$\begingroup\$ What @konijn said. 5 minutes for 50 records sounds utterly insane. In this case you're (presumably) inserting valid, trusted data, so you don't need to muck around with prepared statements, transactions and all the other things you usually have to deal with. You can just truncate (or drop) the table and load all the rows wholesale from a file. Or just point this.dbpath to a pre-seeded .sqlite file for that matter \$\endgroup\$ – Flambino Apr 8 '14 at 15:04
  • \$\begingroup\$ Okay ... but a part of this fake data comes from a web api server, this function gets called after the api key and secret is received from the server. The simple way out would be to just append to a simple text file, however, using the sqlite3 module is a good opportunity to try working with the async nature of node, and say 50 writes would not seem to be a far fetched scenario even for a non ETL scenario \$\endgroup\$ – saraf Apr 9 '14 at 0:09
  • \$\begingroup\$ @BenVlodgi yes, thanks, I looked at the help center section. As I mentioned in the edit, the code now does what I want. The question is about performance - and looking at the help center, it lists performance in the very top of codereview.stackexchange.com/help/on-topic \$\endgroup\$ – saraf Apr 10 '14 at 23:31
1
\$\begingroup\$

From a once over:

  • The constructor could use some rework:

    • You are not using any of the parameters, you might as well not declare them
    • It would be cleaner to just send task to registerUserWithAPI since it has almost all the info
    • lowerCamelCase is the standard
    • The magical constant 1 should be documented

    so you could have something like this instead:

    function TestController () {
    
        var concurrency = 1; //Only allow one task at a time
    
        this.userCreationQueue = async.queue(function (task,callback) {
            that.registerUserWithAPI(task, callback);
        }, concurrency );
    
        this.userCreationQueue.drain = function() {
          console.log("Finished adding all users - queue drained");
        }
    }
    

    A minor side note, perhaps your test is super slow because of that concurrency setting ?

  • You are writing a lot of code, with really length names to a) determine when the last record is written b) what to do in that case. In my mind, that is what drain was designed for. The database connection should have been managed in function TestController and should have been closed in drain. This would make your code leaner..

  • Also, please fix indentation, var post_req is a bit of a mess

In the end, it seems like you have been troubleshooting this code a lot, and you have left a number of things in there because you were debugging with console.log statements. You should fix that. Also, you should try to loosen up the concurrency value; 1 at a time, is not a good test.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks, setting concurrency to anything more than 1 slows up the operation, causing multiple errors during the writes, almost as if the sqlite3 library was not designed for concurrency. I had to use the async module just to throttle the writes. The correct way of using sqlite3 is a part of the question. \$\endgroup\$ – saraf Apr 12 '14 at 2:46
  • \$\begingroup\$ Also, this is just for setting up the test instance data - registering users and devices with a cloud api and recording the seed data to a local database. A Test Runner will then use this seed data to do the actual testing. This class should be named TestConfigurator or something similar. \$\endgroup\$ – saraf Apr 12 '14 at 2:51
  • \$\begingroup\$ The large volume of code is because when doing concurrent writes, SQLITE_BUSY database locked errors would occur at random points and hence wrapping the operation in a transaction and rolling back any time the error occurs was necessary. \$\endgroup\$ – saraf Apr 12 '14 at 2:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.