Should not the "graph_db::commit_transaction()" be thread safe? ( at least, while writing to Pmem version). Without it, uncommitted versions are read.
1) Issue Description: graph_db::commit_transaction() is not thread safe and probably must not throw(). Please refer to the attached pdf for a visual description.
2) Scenario : During the commit process:
step 1.The properties are copied from the dirty node to Pmem version.
step 2. The time stamps on the Pmem version is updated by the committing transaction.
Between this step:1 and step:2, an older "read transaction" can read the uncommitted version.
3) How to reproduce?
Consider 3 transactions :
Txn-1 : add a node.
Txn-2 : Read the node.
Txn-3 : Updates the node ( Txn-2 and Txn-3 are concurrent transactions)
Insert the below test case in "transaction_test.cpp" and comment out all remaining test cases.
4) Test case and debug code:
` TEST_CASE("Test concurrency: update during read" "[transaction]") {
#ifdef USE_PMDK
auto pop = prepare_pool();
auto gdb = create_graph_db(pop);
#else
auto gdb = create_graph_db();
#endif
node::id_t nid = 0;
barrier b1{}, b2{};
// Just add a node
auto tx = gdb->begin_transaction(); // Txn-1
nid = gdb->add_node("Actor",
{{"name", boost::any(std::string("Mark Wahlberg"))},
{"age", boost::any(48)}});
gdb->commit_transaction();// Thread 1:
auto t1 = std::thread( [&] () { // Txn-2: read the node
// read the node
auto tx = gdb->begin_transaction();
auto &n = gdb->node_by_id(nid);
b1.notify(); // so that txn-3 can start.
b2.wait(); // wiat till txn-3 does a update but not yet committed
auto nd = gdb->get_node_description(n);
REQUIRE(nd.label == "Actor"); // here it fails because this txn-2 sees "updated Actor"
REQUIRE(get_property<int>(nd.properties, "age") == 48); //here too.. it sees 52 instead of 48
gdb->commit_transaction();});
//Thread 2:
auto t2 = std::thread( [&] () { // Txn-3 : update the same node
// update the node
b1.wait(); // ensure that update starts after read Txn
auto tx = gdb->begin_transaction();
auto &n = gdb->node_by_id(nid);
gdb->update_node(n, //update
{
{ "age", boost::any(52)},
},
"Updated Actor");
b2.notify(); // notify Txn-2 that update is over but not yet committed
gdb->commit_transaction();});
t1.join(); t2.join();
#ifdef USE_PMDK
drop_graph_db(pop, gdb);
#endif
} `
Since there are only two concurrent transactions in the above scenario, the issue cannot be reproduced consistently. So, introduce a small delay-code after line https://dbgit.prakinf.tu-ilmenau.de/code/poseidon_core/-/blob/null_values/src/storage/graph_db.cpp#L150
The delay-code to be inserted into void graph_db::commit_dirty_node(transaction_ptr tx, node::id_t node_id) {} after the above line is shown below:
std::this_thread::sleep_for(std::chrono::milliseconds(100));
5)Note : The delay-code may not be needed when there are n-concurrent transactions.
** 6)The output :**
transaction_test.cpp:131: FAILED:
REQUIRE( nd.label == "Actor" )
with expansion:
"Updated Actor" == "Actor" //=====> Txn-2 has read the uncommitted version instead of reading valid version.
terminate called after throwing an instance of 'Catch::TestFailureException'
Similar logic applies to relationships.
Scenario-2
Let Txn-1 : add node Txn-2 : read node (where Txn-1 and Txn-2 are concurrent transactions)
The same above concurrency issue may occur.