feat(mssql): support native upsert via MERGE WITH (HOLDLOCK)#5794
feat(mssql): support native upsert via MERGE WITH (HOLDLOCK)#5794MarkusLund wants to merge 2 commits intoprisma:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds MSSQL native upsert support: enables NativeUpsert capability, extends the Merge AST with WHEN MATCHED and builders to construct MERGE from INSERT ... ON CONFLICT UPDATE, updates the Visitor API and MSSQL visitor to emit Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
|
|
MarkusLund
force-pushed
the
fix-mssql-native-upsert-holdlock
branch
from
c09666f to
4c0306b
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds native upsert support for MSSQL by translating OnConflict::Update into MERGE ... WITH (HOLDLOCK) statements, providing atomic upsert semantics equivalent to PostgreSQL's and SQLite's ON CONFLICT DO UPDATE. This eliminates race conditions that previously existed with the transactional SELECT-then-INSERT/UPDATE fallback approach under concurrent load.
Changes:
- Enables
NativeUpsertcapability for the MSSQL connector and addsMerge::from_insert_with_update()to convertINSERT ... ON CONFLICT UPDATEintoMERGE ... WHEN MATCHED THEN UPDATE - Emits
WITH (HOLDLOCK)on all MSSQLMERGEstatements (bothDoNothingandUpdatepaths) and extracts a sharedbuild_using_query()helper to eliminate duplication - Extends test assertion helpers and adds comprehensive unit tests for the new MERGE generation (single unique, compound, schema-qualified, multi-row, conditions, returning, error cases)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
psl/psl-core/src/builtin_connectors/mssql_datamodel_connector.rs |
Adds NativeUpsert capability flag for MSSQL |
quaint/src/ast/merge.rs |
Adds from_insert_with_update(), build_on_conditions_from_constraints(), and shared build_using_query() helper; adds when_matched field to Merge struct |
quaint/src/ast/insert.rs |
Updates doc comment for OnConflict to reflect MSSQL support |
quaint/src/visitor/mssql.rs |
Handles OnConflict::Update in compatibility_modifications, emits WITH (HOLDLOCK) and WHEN MATCHED THEN UPDATE SET in MERGE visitor; updates all existing test expectations |
quaint/src/tests/upsert.rs |
Adds mssql tag to integration upsert tests |
query-engine/connector-test-kit-rs/query-engine-tests/tests/new/native_upsert.rs |
Extends assertion helpers to detect MERGE INTO alongside ON CONFLICT |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c155a584-97dd-45cb-b89a-fda174b6e368
📒 Files selected for processing (6)
psl/psl-core/src/builtin_connectors/mssql_datamodel_connector.rsquaint/src/ast/insert.rsquaint/src/ast/merge.rsquaint/src/tests/upsert.rsquaint/src/visitor/mssql.rsquery-engine/connector-test-kit-rs/query-engine-tests/tests/new/native_upsert.rs
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fda4f8bd-2030-4a62-ad60-8847a1479dc5
📒 Files selected for processing (3)
quaint/src/ast/merge.rsquaint/src/visitor.rsquaint/src/visitor/mssql.rs
Implement native upsert for MSSQL by translating INSERT ... ON CONFLICT into MERGE ... WITH (HOLDLOCK) statements, providing atomic upsert semantics and eliminating race conditions from the transactional SELECT-then-INSERT/UPDATE fallback. - Enable NativeUpsert capability for the MSSQL connector - Add Merge::from_insert_with_update() for ON CONFLICT UPDATE → MERGE - Emit WITH (HOLDLOCK) on all MSSQL MERGE statements - Propagate errors via Result instead of panicking in MERGE conversion - Change compatibility_modifications trait method to return Result - Preserve source row order in multi-row MERGE USING clauses
MarkusLund
force-pushed
the
fix-mssql-native-upsert-holdlock
branch
from
aa89183 to
4a76627
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
T-SQL syntax requires table hints between the table name and alias: MERGE INTO [table] WITH (HOLDLOCK) AS [alias] not: MERGE INTO [table] AS [alias] WITH (HOLDLOCK)
| // T-SQL requires: <target_table> WITH (<hint>) [AS <alias>] | ||
| self.visit_table(merge.table.clone(), false)?; | ||
| self.write(" WITH (HOLDLOCK)")?; | ||
| if let Some(ref alias) = merge.table.alias { | ||
| self.write(" AS ")?; | ||
| self.delimited_identifiers(&[&*alias])?; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the specific code section mentioned
cat -n quaint/src/visitor/mssql.rs | sed -n '655,680p'Repository: prisma/prisma-engines
Length of output: 1108
🏁 Script executed:
# Now let's find and examine the Merge::from_insert_with_update method
rg "from_insert_with_update" --type rust -A 20Repository: prisma/prisma-engines
Length of output: 3038
🏁 Script executed:
# Let's look at the Merge struct definition and understand its structure
rg "impl Merge" --type rust -A 5Repository: prisma/prisma-engines
Length of output: 1496
🏁 Script executed:
# Search for how the ON clause is generated in MERGE statements
rg "visit_merge" --type rust -B 2 -A 30Repository: prisma/prisma-engines
Length of output: 4768
🏁 Script executed:
# Let's check for any tests related to MERGE with aliases
fd "\.rs$" | xargs grep -l "MERGE.*AS\|merge.*alias" 2>/dev/null | head -10Repository: prisma/prisma-engines
Length of output: 183
🏁 Script executed:
# Get the complete from_insert_with_update method
rg "from_insert_with_update" --type rust -A 50 quaint/src/ast/merge.rsRepository: prisma/prisma-engines
Length of output: 2276
🏁 Script executed:
# Find where on_conditions are built in Merge construction
rg "on_conditions" --type rust quaint/src/ast/merge.rs | head -20Repository: prisma/prisma-engines
Length of output: 508
🏁 Script executed:
# Let's look at the Merge struct definition to understand its fields
rg "pub struct Merge" --type rust -A 10Repository: prisma/prisma-engines
Length of output: 1374
🏁 Script executed:
# Get the full implementation of build_on_conditions_from_constraints
rg "fn build_on_conditions_from_constraints" --type rust -A 30 quaint/src/ast/merge.rsRepository: prisma/prisma-engines
Length of output: 1260
🏁 Script executed:
# Check if there are any tests for MERGE statements with aliases
fd "\.rs$" quaint/src/tests -o quaint/tests 2>/dev/null | xargs grep -l "merge\|MERGE" 2>/dev/null | head -5Repository: prisma/prisma-engines
Length of output: 47
🏁 Script executed:
# Search for test files related to mssql or merge
fd "test" --type f quaint/ | grep -E "mssql|merge"Repository: prisma/prisma-engines
Length of output: 47
Fix MERGE alias handling to ensure consistent table references in ON clause.
The ON clause generation in from_insert_with_update() uses a table_ref with alias: None (quaint/src/ast/merge.rs), while the rendered merge target can include an alias. This produces invalid T-SQL: MERGE INTO table AS t ... ON table.id = dual.id (should reference t.id, not table.id). Either normalize aliases away for MERGE targets or thread the alias through ON-clause generation. Add a regression test for MERGE with aliased targets.
Summary
Adds native upsert support for SQL Server by translating
OnConflict::UpdateintoMERGE ... WITH (HOLDLOCK), giving MSSQL the same atomic upsert semantics that PostgreSQL and SQLite already have viaON CONFLICT DO UPDATE.Motivation
Currently, MSSQL upserts fall back to a transactional query graph: a
SELECTto check existence, then either anINSERTorUPDATEdepending on the result. While wrapped in a transaction, this multi-step approach can still surface unique constraint violations under concurrent load because the gap between the read and the write is not fully serializable.MERGE WITH (HOLDLOCK)replaces this with a single atomic statement that holds a range lock during the match phase, eliminating the race condition entirely.WITH (HOLDLOCK)is applied at the MSSQLMERGEvisitor level, so it affects both the newOnConflict::Updatepath and the existingOnConflict::DoNothingpath. This is intentional: both MERGE variants benefit from serializable-level locking on SQL Server.Changes
NativeUpsertcapability for the MSSQL connectorMerge::from_insert_with_update()to convertINSERT ... ON CONFLICT UPDATEintoMERGE ... WHEN MATCHED THEN UPDATEMERGE ONpredicates from explicit conflict columns (compound keys supported)[dbo].[table]) in generatedONclausesWITH (HOLDLOCK)on all MSSQLMERGEstatementsbuild_using_query()helper to avoid duplication betweenDoNothingandUpdatepathsnative_upsert.rsassertion helpers to detectMERGE INTOin addition toON CONFLICTTest plan
cargo test -p quaint test_native_upsert --lib— unit tests for rendered SQL (single unique, compound, schema-qualified, multi-row, conditions, returning, error cases)cargo test -p quaint upsert -- --nocapture— SQLite integration passing locallyquery-engine-testsnative upsert suite)Summary by CodeRabbit
New Features
Documentation
Tests
Refactor