chore: simplify transaction code #4602

romange · 2025-02-12T15:32:18Z

No description provided.

romange · 2025-02-13T14:35:48Z

src/server/blocking_controller.cc

-      if (head->NotifySuspended(owner_->committed_txid(), sid, key)) {
+      DVLOG(2) << "WQ-Pop " << head->DebugId() << " from key " << key << " committed txid "
+               << owner_->committed_txid();
+      if (head->NotifySuspended(sid, key)) {


removed owner_->committed_txid() argument that has not been used there

romange · 2025-02-13T14:36:33Z

src/server/engine_shard.cc

@@ -584,10 +584,10 @@ void EngineShard::PollExecution(const char* context, Transaction* trans) {
  // If any of the following flags are present, we are guaranteed to run in this function:
  // 1. AWAKED_Q -> Blocking transactions are executed immediately after waking up, they don't
  // occupy a place in txq and have highest priority
-  // 2. SUSPENDED_Q -> Suspended shards are run to clean up and finalize blocking keys
+  // 2. WAS_SUSPENDED -> Suspended transactions are run to clean up and finalize blocking keys


renamed to WAS_SUSPENDED to denote the suspension event happened some time in the past

romange · 2025-02-13T14:38:25Z

src/server/transaction.cc

@@ -577,7 +577,7 @@ void Transaction::PrepareMultiForScheduleSingleHop(Namespace* ns, ShardId sid, D
 }

 // Runs in the dbslice thread. Returns true if the transaction concluded.
-bool Transaction::RunInShard(EngineShard* shard, bool txq_ooo) {
+bool Transaction::RunInShard(EngineShard* shard) {


removing txq_ooo and use sd.local_mask for this (historically OOO was determined by the coordinator but now we do it per shard)

romange · 2025-02-13T14:38:52Z

src/server/transaction.cc

  FinishHop();
+
+  // And then poll execution to continue processing the queued transactions.
+  shard->PollExecution("unwatchcb", nullptr);


swapping the order

romange · 2025-02-13T14:39:08Z

src/server/transaction.cc

-    bc->NotifyPending();
-
-  shard->PollExecution("unlockmulti", nullptr);
+  shard->FinalizeMulti(this);


the code moved as it into FinalizeMulti

romange · 2025-02-13T14:39:48Z

src/server/transaction.cc

@@ -1432,7 +1435,6 @@ bool Transaction::NotifySuspended(TxId committed_txid, ShardId sid, string_view
  CHECK(it != args.cend());

  // Change state to awaked and store index of awakened key
-  sd.local_mask &= ~SUSPENDED_Q;


now we keep WAS_SUSPENDED forever.

adiholden · 2025-02-13T15:24:04Z

src/server/transaction.cc

-  bool was_suspended = sd.local_mask & SUSPENDED_Q;
+  // was_suspended is true meaning that this transaction was suspended and then
+  // it was woken up by another transaction in either this thread or a key in another thread.
+  // if awaked_prerun is true - it means it was woken up by a transaction in this thread,


can you give example for a transaction that is woken up by transactions that runs on different thread?
From what I know the suspended transactions are waiting on a key to be added and once this key is added they run, so it meant that the waking transaction added this key i.e run on the same thread

blpop that waits on multiple keys can be waken up on one shard and still be "suspended" on another shard

it's not really suspended, since it will advance, but it's not gonna be awaken which is reserved only for the shard that really notified that tx.

adiholden · 2025-02-13T15:46:13Z

src/server/transaction.cc

@@ -609,7 +614,7 @@ bool Transaction::RunInShard(EngineShard* shard, bool txq_ooo) {
  // If we're the head of tx queue (txq_ooo is false), we remove ourselves upon first invocation
  // and successive hops are run by continuation_trans_ in engine shard.
  // Otherwise we can remove ourselves only when we're concluding (so no more hops will follow).
-  if ((is_concluding || !txq_ooo) && sd.pq_pos != TxQueue::kEnd) {
+  if ((is_concluding || !is_ooo) && sd.pq_pos != TxQueue::kEnd) {


I think that you changed behaviour here.
Now is_ooo is based on OUT_OF_ORDER
but what if this transaction is the head of tx queue? line 670 in engine_shard.cc
here you will not remove it from head , line 671 set continuation_trans_ to head.

maybe we can add a check that if we set continuation_trans_ than sd.pq_pos is TxQueue::kEnd

good catch, I thought about this but decided it's ok, now I need to see why the dcheck fails.

ok, reverted the logic, changed the names of the variables, hopefully it's more clear now

adiholden · 2025-02-13T16:23:05Z

src/server/transaction.cc

@@ -618,7 +623,7 @@ bool Transaction::RunInShard(EngineShard* shard, bool txq_ooo) {
  // For multi we unlock transaction (i.e. its keys) in UnlockMulti() call.
  // If it's a final hop we should release the locks.
  if (is_concluding) {
-    bool became_suspended = sd.local_mask & SUSPENDED_Q;
+    bool became_suspended = sd.local_mask & WAS_SUSPENDED;


I think that it will be a little bit more clear to write
became_suspended = !was_suspended && sd.local_mask & WAS_SUSPENDED
and the if statement in line 640 will be if (!became_suspended)

Signed-off-by: Roman Gershman <roman@dragonflydb.io>

adiholden · 2025-02-16T08:18:58Z

src/server/transaction.cc

-  // Otherwise we can remove ourselves only when we're concluding (so no more hops will follow).
-  if ((is_concluding || !txq_ooo) && sd.pq_pos != TxQueue::kEnd) {
+  // Otherwise we can remove ourselves only when we're concluding (so no more hops follow).
+  if (sd.pq_pos != TxQueue::kEnd && (is_concluding || allow_q_removal)) {


The logic looks right.
Minor comment is that I understand that we allow queue removal only if we are concluding or we are the tx is head , therefore I think it will be more clear to pass is_head instead of allow_q_removal, as this param has no meaning when you call run transaction which is not in txq.

it's actually not correct. I tried first sd.pq_pos == txq()->Head() without passing anything. but for multi-hop transactions that are OOO we do not set continous_transaction_, therefore we may break consistency guarantees if we remove them even if they are at head.

I dont understand than.
If there is a multi hop transaction that is OOO and it was at head position we will call the RunInShard with allow_q_removal = true because it will run from the flow of engine_shard.cc:669 rigtht?

no, we do not enter there if there is a continuous transaction defined.
but we can also call from if (trans && disarmed) { check (the last invocation)

BorysTheDev previously approved these changes Feb 13, 2025

View reviewed changes

romange dismissed BorysTheDev’s stale review via d8f97cc February 13, 2025 13:03

romange force-pushed the Pr5 branch from 4ed047b to d8f97cc Compare February 13, 2025 13:03

romange requested a review from adiholden February 13, 2025 13:29

romange commented Feb 13, 2025

View reviewed changes

adiholden reviewed Feb 13, 2025

View reviewed changes

romange added 2 commits February 13, 2025 23:05

chore: simplify transaction code

89dbc44

Signed-off-by: Roman Gershman <roman@dragonflydb.io>

chore: address comments

Loading
Loading status checks…

e6ffc02

romange force-pushed the Pr5 branch from 9e2dba6 to e6ffc02 Compare February 13, 2025 21:05

romange requested a review from adiholden February 16, 2025 07:38

adiholden reviewed Feb 16, 2025

View reviewed changes

adiholden approved these changes Feb 16, 2025

View reviewed changes

romange merged commit 2436094 into main Feb 16, 2025
10 checks passed

romange deleted the Pr5 branch February 16, 2025 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: simplify transaction code #4602

chore: simplify transaction code #4602

romange commented Feb 12, 2025

romange Feb 13, 2025

romange Feb 13, 2025

romange Feb 13, 2025

romange Feb 13, 2025

romange Feb 13, 2025

romange Feb 13, 2025

adiholden Feb 13, 2025

romange Feb 13, 2025 •

edited

Loading

romange Feb 13, 2025

adiholden Feb 13, 2025

adiholden Feb 13, 2025

romange Feb 13, 2025

romange Feb 13, 2025

adiholden Feb 13, 2025

adiholden Feb 16, 2025

romange Feb 16, 2025

adiholden Feb 16, 2025

romange Feb 16, 2025

chore: simplify transaction code #4602

chore: simplify transaction code #4602

Conversation

romange commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange Feb 13, 2025 •

edited

Loading