Skip commit-state for read-only transactions in Consensus Commit #2765

brfrn169 · 2025-06-13T07:10:56Z

Description

This PR updates the code to omit writing the commit state for read-only transactions in Consensus Commit to improve performance. This behavior is enabled by default but can be disabled by setting the property scalar.db.consensus_commit.coordinator.write_omission_on_read_only.enabled to false.

Related issues and/or PRs

N/A

Changes made

Add inline comments. Please take a look for the details.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

Changed to omit commit-state for read-only transactions in Consensus Commit to improve performance. This behavior is enabled by default but can be disabled by setting the property scalar.db.consensus_commit.coordinator.write_omission_on_read_only.enabled to false.

Copilot

Pull Request Overview

This PR updates the Consensus Commit implementation to skip the commit‐state step for read-only transactions. The key changes include:

Adding a boolean parameter to commit methods to distinguish read-only transactions.
Updating tests to verify the new behavior for read-only and non-read-only modes.
Introducing helper methods in Snapshot and CrudHandler to support read-only checks and related conditional logic.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
core/src/test/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitTest.java	Updated test cases to pass the new `readOnly` flag in commit and rollback methods.
core/src/test/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManagerTest.java	Adjusted tests to verify proper group commit behavior based on transaction mode.
core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java	Added tests for commit behavior with group commit functionality in various modes.
core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerTest.java	Modified extensive test scenarios to use the new commit API with a `readOnly` parameter.
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java	Introduced helper methods to check for absence of writes/deletes and reads.
core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java	Added an accessor method for the read-only flag.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java	Updated logic to skip reserving group commits for read-only transactions.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit.java	Adjusted commit and rollback logic to conditionally handle read-only transactions.
core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommit.java	Overrode commit to cancel group commit when appropriate based on transaction mode.
core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java	Modified the commit method to skip preparation, commit state, and record commits for read-only transactions.

Comments suppressed due to low confidence (4)

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java:224

Consider adding an inline comment explaining why group commit reservation is skipped when the transaction is read-only.

if (!readOnly && isGroupCommitEnabled()) {

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java:109

Add Javadoc or inline comments describing the behavior of the new 'readOnly' flag and how it affects the commit process.

public void commit(Snapshot snapshot, boolean readOnly) throws CommitException, UnknownTransactionStatusException {

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommit.java:44

Provide an inline comment to clarify why group commit cancellation is only invoked for non-read-only transactions with no writes and deletes.

if (!readOnly && snapshot.hasNoWritesAndDeletes()) {

core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java:220

Consider adding Javadoc comments to the new helper methods (hasNoWritesAndDeletes and hasNoReads) to clarify their intended use and behavior.

public boolean hasNoWritesAndDeletes() {

brfrn169 · 2025-06-13T07:15:57Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java

+    if (!hasNoWritesAndDeletesInSnapshot) {
+      try {
+        prepare(snapshot);
+      } catch (PreparationException e) {
+        safelyCallOnFailureBeforeCommit(snapshot);
+        abortState(snapshot.getId());
+        rollbackRecords(snapshot);
+        if (e instanceof PreparationConflictException) {
+          throw new CommitConflictException(e.getMessage(), e, e.getTransactionId().orElse(null));
+        }
+        throw new CommitException(e.getMessage(), e, e.getTransactionId().orElse(null));
+      } catch (Exception e) {
+        safelyCallOnFailureBeforeCommit(snapshot);
+        throw e;
      }
-      throw new CommitException(e.getMessage(), e, e.getTransactionId().orElse(null));
-    } catch (Exception e) {
-      safelyCallOnFailureBeforeCommit(snapshot);
-      throw e;
    }


If no writes and deletes, skip prepare-records.

brfrn169 · 2025-06-13T07:16:28Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java

+    if (!snapshot.hasNoReads()) {
+      try {
+        validate(snapshot);
+      } catch (ValidationException e) {
+        safelyCallOnFailureBeforeCommit(snapshot);
+        abortState(snapshot.getId());
+        rollbackRecords(snapshot);
+        if (e instanceof ValidationConflictException) {
+          throw new CommitConflictException(e.getMessage(), e, e.getTransactionId().orElse(null));
+        }
+        throw new CommitException(e.getMessage(), e, e.getTransactionId().orElse(null));
+      } catch (Exception e) {
+        safelyCallOnFailureBeforeCommit(snapshot);
+        throw e;
      }
-      throw new CommitException(e.getMessage(), e, e.getTransactionId().orElse(null));
-    } catch (Exception e) {
-      safelyCallOnFailureBeforeCommit(snapshot);
-      throw e;
    }


If no reads, skip validate-record.

brfrn169 · 2025-06-13T07:16:50Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java

+    if (!hasNoWritesAndDeletesInSnapshot) {
+      commitState(snapshot);
+      commitRecords(snapshot);
+    }


If no writes and deletes, skip commit-state and commit-records.

brfrn169 · 2025-06-13T07:17:51Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java

+    if (!readOnly && isGroupCommitEnabled()) {
      assert groupCommitter != null;
      txId = groupCommitter.reserve(txId);
    }


In read-only mode, skip reserving a slot in the group commit.

jnmt

Thanks for the heads up.

I checked the Auditor's recovery behavior. It first tries to abort a lock holder transaction regardless of read/write types. If a read-only transaction does not put the commit state by this PR, this abort process will always result in creating aborted status record and make the Auditor release locks.

Releasing itself is OK for read locks, but I think it's pretty confusing since the created status record is not just garbage but shows the wrong status (aborted) even if the read-only transaction is actually committed.

So, from the ScalarDL perspective, it would be better to configure disabling this optimization. The default enabled would be acceptable since we can disable it when creating the transaction manager on the ScalarDL side.

@feeblefakie Please correct me if I miss something and let us know your opinion.

jnmt · 2025-06-14T00:46:51Z

@brfrn169 I missed one point.

this abort process will always result in creating aborted status record

This is caused by DistributedTransactionManager.abort() of the read-only transaction. Should we also skip putting the state here if the transaction was read-only? If skipped, my concern will be resolved, and the configuration option is not necessary.

scalardb/core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java

Line 486 in 19aac3d

return commit.abortState(txId);

brfrn169 · 2025-06-14T01:19:05Z

@jnmt Thank you for reviewing this PR and checking the behavior of the Auditor!

This is caused by DistributedTransactionManager.abort() of the read-only transaction. Should we also skip putting the state here if the transaction was read-only? If skipped, my concern will be resolved, and the configuration option is not necessary.

Actually, we cannot determine whether the transaction associated with the specified transaction ID is read-only or not. So I don’t think we can handle that on the ScalarDB side.
How about on the Auditor side? If the Auditor can detect that the transaction is read-only, could it skip calling DistributedTransactionManager.abort()?

jnmt · 2025-06-14T02:14:55Z

How about on the Auditor side? If the Auditor can detect that the transaction is read-only, could it skip calling DistributedTransactionManager.abort()?

Unfortunately, I don’t think we can easily determine if a transaction is read-only in the recovery context. It might be possible by re-executing the transaction based on the request log, but it’s too much and not feasible for this purpose, I think.

komamitsu

LGTM! 👍

Torch3333

LGTM, thank you!

feeblefakie

Overall, looking good.
Left some suggestions. PTAL!

feeblefakie · 2025-06-16T04:31:41Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java

-      if (e instanceof PreparationConflictException) {
-        throw new CommitConflictException(e.getMessage(), e, e.getTransactionId().orElse(null));
+
+    if (!hasNoWritesAndDeletesInSnapshot) {


I think double negative is a bit hard to read.
Since all the usage of hasNoWritesAndDeletesInSnapshot is with negation (!), how about making the method like hasSomeWritesAndDeletesInSnapshot so that we don't have to negate all the if clauses?

Fixed in 6c6299d. Thanks!

feeblefakie · 2025-06-16T04:31:50Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java

-      rollbackRecords(snapshot);
-      if (e instanceof ValidationConflictException) {
-        throw new CommitConflictException(e.getMessage(), e, e.getTransactionId().orElse(null));
+    if (!snapshot.hasNoReads()) {


brfrn169 · 2025-06-16T06:00:57Z

@feeblefakie What do you think about @jnmt's comment?
#2765 (review)

Should we go with making the skipping commit-state behavior configurable?

feeblefakie · 2025-06-16T09:49:39Z

@brfrn169 As @jnmt mentioned, I think we need a coordinator write even for read-only operations.
So, can you make it configurable?

@jnmt
If we use its own transaction state management mechanism (enabled by tx_state_management), we don't need to rely on the coordinator behavior changes.
If the underlying database provides atomicity for the entire storage (STORAGE), we don't have to double the writes for the transaction coordination.

…_on_read_only.enabled`

brfrn169 · 2025-06-17T06:14:23Z

@feeblefakie @jnmt Added the scalar.db.consensus_commit.coordinator.write_omission_on_read_only.enabled property to control the omission behavior. Please take a look when you have time!

jnmt

Thank you for adding the config option! LGTM, although I left a minor comment that doesn't block merging this.

jnmt · 2025-06-17T06:51:40Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java

-  public void commit(Snapshot snapshot) throws CommitException, UnknownTransactionStatusException {
+  public void commit(Snapshot snapshot, boolean readOnly)
+      throws CommitException, UnknownTransactionStatusException {
+    boolean hasSomeWritesOrDeletesInSnapshot = !readOnly && snapshot.hasSomeWritesOrDeletes();


Super minor. I feel "Some" is a bit redundant if you don't have any special intention.

Suggested change

boolean hasSomeWritesOrDeletesInSnapshot = !readOnly && snapshot.hasSomeWritesOrDeletes();

boolean hasWritesOrDeletesInSnapshot = !readOnly && snapshot.hasWritesOrDeletes();

Fixed in e783b13. Thanks!

feeblefakie

LGTM! Thank you!
Just one suggestion for the naming.

feeblefakie · 2025-06-18T02:37:04Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java

-  public void commit(Snapshot snapshot) throws CommitException, UnknownTransactionStatusException {
+  public void commit(Snapshot snapshot, boolean readOnly)
+      throws CommitException, UnknownTransactionStatusException {
+    boolean hasSomeWritesOrDeletesInSnapshot = !readOnly && snapshot.hasWritesOrDeletes();


Suggested change

boolean hasSomeWritesOrDeletesInSnapshot = !readOnly && snapshot.hasWritesOrDeletes();

boolean hasWritesOrDeletesInSnapshot = !readOnly && snapshot.hasWritesOrDeletes();

If Some is removed, should it be consistent?

Good catch! Fixed in 2c13b1c. Thanks!

Skip commit-state for read-only transactions

19aac3d

brfrn169 requested a review from Copilot June 13, 2025 07:10

brfrn169 self-assigned this Jun 13, 2025

brfrn169 added the improvement label Jun 13, 2025

Copilot AI reviewed Jun 13, 2025

View reviewed changes

brfrn169 commented Jun 13, 2025

View reviewed changes

brfrn169 requested review from Torch3333, feeblefakie, jnmt and komamitsu June 13, 2025 07:24

jnmt reviewed Jun 13, 2025

View reviewed changes

Fix

e7cefba

komamitsu approved these changes Jun 16, 2025

View reviewed changes

Torch3333 approved these changes Jun 16, 2025

View reviewed changes

feeblefakie reviewed Jun 16, 2025

View reviewed changes

Fix based on feedback

6c6299d

brfrn169 requested a review from feeblefakie June 16, 2025 06:01

Add a property `scalar.db.consensus_commit.coordinator.write_omission…

a40322c

…_on_read_only.enabled`

brfrn169 requested a review from jnmt June 17, 2025 06:13

jnmt approved these changes Jun 17, 2025

View reviewed changes

Fix based on feedback

e783b13

feeblefakie approved these changes Jun 18, 2025

View reviewed changes

brfrn169 added 2 commits June 18, 2025 11:40

[skip ci] Fix based on feedback

2c13b1c

Merge branch 'master' into skip-commit-state-for-read-only-transactions

a303699

brfrn169 merged commit 960a825 into master Jun 18, 2025
55 checks passed

brfrn169 deleted the skip-commit-state-for-read-only-transactions branch June 18, 2025 04:35

feeblefakie pushed a commit that referenced this pull request Jun 18, 2025

Skip commit-state for read-only transactions in Consensus Commit (#2765)

fa22503

feeblefakie mentioned this pull request Jun 18, 2025

Backport to branch(3) : Skip commit-state for read-only transactions in Consensus Commit #2784

Merged

jnmt mentioned this pull request Jul 24, 2025

Disable the coordinator write omission in ScalarDB scalar-labs/scalardl#203

Merged

7 tasks

	boolean hasSomeWritesOrDeletesInSnapshot = !readOnly && snapshot.hasSomeWritesOrDeletes();
	boolean hasWritesOrDeletesInSnapshot = !readOnly && snapshot.hasWritesOrDeletes();

Skip commit-state for read-only transactions in Consensus Commit #2765

Skip commit-state for read-only transactions in Consensus Commit #2765

Uh oh!

Conversation

brfrn169 commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnmt left a comment

Choose a reason for hiding this comment

Uh oh!

jnmt commented Jun 14, 2025

Uh oh!

brfrn169 commented Jun 14, 2025

Uh oh!

jnmt commented Jun 14, 2025

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

Torch3333 left a comment

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brfrn169 commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feeblefakie commented Jun 16, 2025

Uh oh!

brfrn169 commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

brfrn169 commented Jun 13, 2025 •

edited

Loading

brfrn169 commented Jun 16, 2025 •

edited

Loading

brfrn169 commented Jun 17, 2025 •

edited

Loading