Skip to content

fix(sql): support shared locking reads - #26598

Merged
XuPeng-SH merged 3 commits into
matrixorigin:mainfrom
iamlinjunhong:m-24733
Aug 3, 2026
Merged

fix(sql): support shared locking reads#26598
XuPeng-SH merged 3 commits into
matrixorigin:mainfrom
iamlinjunhong:m-24733

Conversation

@iamlinjunhong

Copy link
Copy Markdown
Contributor

Support SELECT ... FOR SHARE and LOCK IN SHARE MODE with shared lock targets.

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #24733

What this PR does / why we need it:

fix(sql): support shared locking reads

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep-reviewed at 0845cc4.

[P1] Preserve the shared lock mode across SELECT unwrapping and rewrites

selectLockMode is derived only from the current stmt before the ParenSelect stripping loop and is then passed as a local argument. Accepted query shapes that replace/rebind the SELECT therefore lose the requested mode:

  1. ((select n_nationkey from nation for share)) builds successfully but produces no LOCK_OP at all.
  2. select n_regionkey, row_number() over (order by n_regionkey) from nation group by n_regionkey with rollup for share goes through rewriteRollupWindowSelect; the generated SELECT has no lock info, so rebinding defaults to Exclusive and produces two Exclusive lock targets.

The first query silently provides no locking protection; the second unnecessarily conflicts with other shared readers. I reproduced both by inspecting every LOCK_OP.LockTargets[].Mode after runOneStmt: expected Shared, observed nil and [Exclusive, Exclusive], respectively. Please carry the query-block lock type through unwrapping/generated SELECTs (or make it explicit scoped builder state) and add regressions for these paths.

The added focused parser/planner tests pass 5x under race, full parser and plan package tests pass, vet passes, and CI is green; those tests currently cover only the direct binding path.

Support SELECT ... FOR SHARE and LOCK IN SHARE MODE with shared lock targets.
@iamlinjunhong

Copy link
Copy Markdown
Contributor Author

Deep-reviewed at 0845cc4.

[P1] Preserve the shared lock mode across SELECT unwrapping and rewrites

selectLockMode is derived only from the current stmt before the ParenSelect stripping loop and is then passed as a local argument. Accepted query shapes that replace/rebind the SELECT therefore lose the requested mode:

  1. ((select n_nationkey from nation for share)) builds successfully but produces no LOCK_OP at all.
  2. select n_regionkey, row_number() over (order by n_regionkey) from nation group by n_regionkey with rollup for share goes through rewriteRollupWindowSelect; the generated SELECT has no lock info, so rebinding defaults to Exclusive and produces two Exclusive lock targets.

The first query silently provides no locking protection; the second unnecessarily conflicts with other shared readers. I reproduced both by inspecting every LOCK_OP.LockTargets[].Mode after runOneStmt: expected Shared, observed nil and [Exclusive, Exclusive], respectively. Please carry the query-block lock type through unwrapping/generated SELECTs (or make it explicit scoped builder state) and add regressions for these paths.

The added focused parser/planner tests pass 5x under race, full parser and plan package tests pass, vet passes, and CI is green; those tests currently cover only the direct binding path.

fixed

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep-reviewed at d560c75. No blocking findings.

The previous P1 is closed: the binder now derives the effective query-block lock info after parenthesis unwrapping, carries it into the synthesized ROLLUP/window SELECT, and applies the selected mode to every collected target. The two original counterexamples now produce Shared targets only.

I also checked the same state-propagation shape across DISTINCT, ORDER/LIMIT, joins, derived tables, scalar/EXISTS subqueries, UNION branches, and mixed Shared/Exclusive UNION branches. Modes remain scoped to the owning query block. A real embedded-cluster test confirmed that FOR SHARE and LOCK IN SHARE MODE readers are mutually compatible, while an UPDATE on the same row times out until both readers commit.

Validation on the exact head:

  • forced mysql_sql.go regeneration is clean;
  • focused parser test: count=20;
  • focused planner test: count=20;
  • focused planner race test: count=5;
  • full MySQL parser and plan packages pass;
  • go vet passes for both owning packages;
  • CI is green.

@mergify mergify Bot added the queued label Aug 3, 2026
@mergify

mergify Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-08-03 08:04 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • 🟠 Checks running · in-place
  • 🚫 Left the queue2026-08-03 09:56 UTC · at 77f21a7d1199bb7d7ee2e63e7628ec5d38a5e159

This pull request spent 1 hour 52 minutes 29 seconds in the queue, with no time running CI.

Reason

The pull request can't be updated

This pull request seems to come from a fork, and Mergify needs the author's permission to update its branch.
The author needs to enable "Allow edits from maintainers" on this pull request, or update the branch manually.

Hint

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

  • Requeue this pull request

@XuPeng-SH
XuPeng-SH merged commit 6411de3 into matrixorigin:main Aug 3, 2026
24 of 26 checks passed
@mergify mergify Bot added dequeued and removed queued labels Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dequeued size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants