Fix secondary index behavior in Consensus Commit

[brfrn169]

Description

Currently, when users specify an index key for Get or Scan operations with the Consensus Commit transaction manager, we do not add any additional conditions for the before columns. However, as described in the following Javadoc, we need to add the same conditions for the before columns to ensure correct results:

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

Lines 407 to 472 in cc720db

	/**
	* Converts the given conjunctions to include conditions on before images.
	*
	* <p>This is necessary because we might miss prepared records whose before images match the
	* original conditions when reading from storage. For example, suppose we have the following
	* records in storage:
	*
	* <pre>
	* | partition_key | clustering_key | column | status | before_column | before_status |
	* |---------------|----------------|--------|-----------|---------------|----------------|
	* | 0 | 0 | 1000 | COMMITTED | | |
	* | 0 | 1 | 200 | PREPARED | 1000 | COMMITTED |
	* </pre>
	*
	* If we scan records with the condition "column = 1000" without converting the condition
	* (conjunction), we only get the first record, not the second one, because the condition does not
	* match. However, the second record has not been committed yet, so we should still retrieve it,
	* considering the possibility that the record will be rolled back.
	*
	* <p>To handle such cases, we convert the conjunctions to include conditions on the before image.
	* For example, if the original condition is:
	*
	* <pre>
	* column = 1000
	* </pre>
	*
	* We convert it to:
	*
	* <pre>
	* column = 1000 OR before_column = 1000
	* </pre>
	*
	* <p>Here are more examples:
	*
	* <p>Example 1:
	*
	* <pre>
	* {@code column >= 500 AND column < 1000}
	* </pre>
	*
	* becomes:
	*
	* <pre>
	* {@code (column >= 500 AND column < 1000) OR (before_column >= 500 AND before_column < 1000)}
	* </pre>
	*
	* <p>Example 2:
	*
	* <pre>
	* {@code column1 = 500 OR column2 != 1000}
	* </pre>
	*
	* becomes:
	*
	* <pre>
	* {@code column1 = 500 OR column2 != 1000 OR before_column1 = 500 OR before_column2 != 1000}
	* </pre>
	*
	* This way, we can ensure that prepared records whose before images satisfy the original scan
	* conditions are not missed during the scan.
	*
	* @param conjunctions the conjunctions to convert
	* @param metadata the table metadata of the target table
	* @param convertIndexedColumns whether to convert conditions on indexed columns
	* @return the converted conjunctions
	*/

However, if we simply add the same conditions to the before columns, the secondary index will no longer be used, which can significantly degrade performance.

Therefore, after further discussion, we decided to redefine Get/Scan operations using secondary indexes as eventually consistent operations. This means they:

• Do NOT include conditions on before columns
• Can efficiently use secondary indexes
• May return slightly stale data

Since SERIALIZABLE isolation level requires the strongest consistency guarantees and cannot tolerate eventually consistent reads, we now prohibit secondary index operations when using SERIALIZABLE isolation.

ADDED:
I’ve summarized the behavior here:
#3133 (comment)

Related issues and/or PRs

N/A

Changes made

1. New Operation Checker (ConsensusCommitOperationChecker)

• Renamed and expanded ConsensusCommitMutationOperationChecker to check all operations (Get, Scan, and Mutations)
• Added validation for Get operations:
  • SERIALIZABLE check: Rejects Get operations using secondary indexes (unless the index column is part of the primary key)
• Added validation for Scan operations:
  • SERIALIZABLE check: Rejects Scan operations using secondary indexes (unless the index column is part of the primary key)
  • SERIALIZABLE check: For cross-partition scans (ScanAll), rejects conditions on any indexed columns

2. Updated Conjunction Conversion Logic

• Modified ConsensusCommitUtils.convertConjunctions():
  • Added convertIndexedColumns parameter to control whether conditions on indexed columns should be converted to include before-column conditions
  • For cross-partition scans (ScanAll), indexed column conditions are NOT converted (passed as-is)
  • For partition scans and Gets, all conditions including indexed columns ARE converted

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

Prohibited Get and Scan operations using secondary indexes when the isolation level is SERIALIZABLE in the Consensus Commit transaction manager, as these operations are now defined as eventually consistent and cannot guarantee the strict consistency required by SERIALIZABLE isolation.

[gemini-code-assist]

Summary of Changes

Hello @brfrn169, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors and strengthens the validation mechanisms within the Consensus Commit transaction protocol. By introducing a unified operation checker, it enforces more rigorous constraints on how Get, Scan, and Mutation operations interact with transaction metadata and secondary indexes. These changes are particularly impactful for transactions operating under the SERIALIZABLE isolation level, where certain index-based operations are now explicitly disallowed to maintain transactional integrity and correctness. The overall goal is to enhance the robustness and predictability of transaction behavior.

Highlights

• Unified Operation Checker: A new ConsensusCommitOperationChecker class has been introduced to centralize and consolidate validation logic for Get, Scan, and Mutation operations within Consensus Commit transactions. This replaces the previous ConsensusCommitMutationOperationChecker.
• Enhanced Validation for Read Operations: The new checker extends validation to Get and Scan operations, preventing the use of transaction metadata columns in projections, conditions, or orderings, unless explicitly enabled via configuration (isIncludeMetadataEnabled).
• Restrictions for SERIALIZABLE Isolation Level: Stricter rules are now enforced for the SERIALIZABLE isolation level: Get and Scan operations using secondary indexes are disallowed, and cross-partition Scan operations (ScanAll) with conditions on indexed columns are also prohibited.
• Core Component Updates: The ConsensusCommit and ConsensusCommitManager classes have been refactored to integrate and utilize the new ConsensusCommitOperationChecker, ensuring all relevant operations pass through the updated validation pipeline.
• Conditional Index Conversion in Utils: The ConsensusCommitUtils class has been updated to conditionally convert conditions on indexed columns for Scan operations, specifically preventing such conversions for ScanAll operations to align with the new SERIALIZABLE isolation restrictions.
• New Error Codes: Several new CoreError codes have been added to provide specific error messages for the newly introduced validation failures, improving clarity for users encountering these restrictions.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces important fixes and improvements related to secondary index behavior in Consensus Commit, especially for the SERIALIZABLE isolation level.

The main changes include:

• A new ConsensusCommitOperationChecker class is introduced, which consolidates and extends the validation logic for Get, Scan, and Mutation operations. This is a good refactoring that improves code organization.
• New restrictions are added for the SERIALIZABLE isolation level:
  • Get and Scan operations using a secondary index are now disallowed.
  • Conditions on indexed columns in cross-partition scans (ScanAll) are also disallowed.
• The logic for preparing scan operations in ConsensusCommitUtils has been updated to handle conditions on indexed columns in ScanAll operations differently, which seems to be a performance optimization.
• MutationConditionsValidator has been refactored to reduce its dependency on TransactionContext, which is a nice improvement for decoupling.

The changes are well-implemented, and the test suite has been updated comprehensively to cover the new behavior, including new integration tests that validate the new restrictions.

Overall, this is a high-quality pull request that improves the correctness and robustness of the transaction implementation. I have no further comments.

[Copilot]

Pull Request Overview

This PR enhances operation checking in consensus commit transactions by introducing restrictions for SERIALIZABLE isolation level and extending the operation checker to validate read operations. The changes prevent index-based operations and conditions on indexed columns in specific scenarios when using SERIALIZABLE isolation, as these operations cannot guarantee serializability.

Key changes:

• Renames and expands ConsensusCommitMutationOperationChecker to ConsensusCommitOperationChecker with support for Get/Scan validation
• Adds SERIALIZABLE isolation checks to reject index-based Get/Scan operations and conditions on indexed columns in cross-partition scans
• Refactors condition conversion logic to skip indexed column conditions for ScanAll operations

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
ConsensusCommitOperationChecker.java	New class replacing ConsensusCommitMutationOperationChecker with added Get/Scan validation and SERIALIZABLE isolation checks
ConsensusCommitMutationOperationChecker.java	Removed - replaced by ConsensusCommitOperationChecker
ConsensusCommit.java, TwoPhaseConsensusCommit.java	Updated to call operation checker for Get/Scan operations and use renamed checker
ConsensusCommitManager.java, TwoPhaseConsensusCommitManager.java	Updated to instantiate ConsensusCommitOperationChecker with include_metadata flag
ConsensusCommitUtils.java	Modified conjunction conversion to conditionally skip indexed columns based on scan type
MutationConditionsValidator.java	Simplified to accept transactionId string instead of full TransactionContext
CrudHandler.java	Updated to pass transactionId to MutationConditionsValidator
CoreError.java	Added error codes for projection/ordering metadata restrictions and SERIALIZABLE index operation restrictions
ConsensusCommitSpecificIntegrationTestBase.java	Updated tests to reflect SERIALIZABLE restrictions and removed obsolete index operation tests
Various test files	Updated to use renamed ConsensusCommitOperationChecker and adjust mock behavior

Comments suppressed due to low confidence (1)

core/src/main/java/com/scalar/db/common/CoreError.java:439

• The error message format for CONSENSUS_COMMIT_CONDITION_NOT_ALLOWED_TO_TARGET_TRANSACTION_METADATA_COLUMNS was changed from accepting one parameter (Column: %s) to accepting two parameters (Table: %s; Column: %s) in the new usage, but the error enum definition was not updated. This will cause a format string mismatch. The error definition should be updated to match the new usage pattern seen in line 72 and 232 of ConsensusCommitOperationChecker.java where buildMessage(get.forFullTableName().get(), column) and buildMessage(mutation.forFullTableName().get(), column) are called.

  CONSENSUS_COMMIT_CONDITION_NOT_ALLOWED_TO_TARGET_TRANSACTION_METADATA_COLUMNS(
      Category.USER_ERROR,
      "0100",
      "The condition is not allowed to target transaction metadata columns. Column: %s",
      "",
      ""),

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[josh-wong]

I've left a couple of minor suggestions. Other than that, LGTM. Thank you!🙇🏻‍♂️

[komamitsu]

However, if we simply add the same conditions to the before columns, the secondary index will no longer be used, which can significantly degrade performance.

Just out of curiosity, but using indexes for a normal column and the before column separately and merging them as follows don't work?

  result1 = Scan.withIndexOn(col = 100)
  result2 = Scan.withIndexOn(before_col = 100)
  result = mergeAndDeduplicateResults(result1, result2)

[brfrn169]

However, if we simply add the same conditions to the before columns, the secondary index will no longer be used, which can significantly degrade performance.

Just out of curiosity, but using indexes for a normal column and the before column separately and merging them as follows don't work?

  result1 = Scan.withIndexOn(col = 100)
  result2 = Scan.withIndexOn(before_col = 100)
  result = mergeAndDeduplicateResults(result1, result2)

@komamitsu Yes, that manual merge index approach should work.

[feeblefakie]

Overall, looking good!
Left some questions for clarification. PTAL!

[jnmt]

Thank you for the careful thought on this. I left a clarification question. PTAL!

[jnmt]

@brfrn169 @feeblefakie  This is off topic, but could we handle scan using index columns well even under serializable isolation if we had "after" image as transaction metadata instead of before image? Rollforward and rollback mechanisms must be completely changed, though.

[feeblefakie]

LGTM! Thank you!

[jnmt]

LGTM, thank you!

[Torch3333]

LGTM, thank you!

[komamitsu]

LGTM, thank you!