You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "srielau (via GitHub)" <gi...@apache.org> on 2026/05/19 07:49:20 UTC
[PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
srielau opened a new pull request, #55977:
URL: https://github.com/apache/spark/pull/55977
### What changes were proposed in this pull request?
Break the `SessionCatalog`/`CatalogManager` lock-order inversion that can deadlock concurrent `USE SCHEMA` / `USE CATALOG` and unqualified function resolution on the same `SparkSession`.
- `CatalogManager.setCurrentNamespace` / `setCurrentCatalog`: snapshot the dispatch decision under the manager's intrinsic lock, run the `v1SessionCatalog` callbacks **outside** the lock, then publish the new state under the lock again. This stops the "manager lock then catalog lock" arm of the cycle.
- Add `CatalogManager.sessionFunctionKindsForUnqualifiedResolution()` that snapshots `(currentCatalog, currentNamespace, sessionPath)` in a single critical section. The `v1SessionCatalog.getCurrentDatabase` read needed for the default-namespace fallback is taken **before** the manager lock, so the helper never re-introduces the deadlock cycle while still avoiding torn-state observations under racing path updates.
- Route `SessionCatalog.sessionFunctionKindsInResolutionOrder` and `FunctionResolution.isSessionBeforeBuiltinInPath` through that single helper, so the lookup loop and the `session-before-builtin` predicate share one consistent snapshot.
- Tighten the doc comments on the affected methods to document the locking contract and prevent future regressions.
### Why are the changes needed?
After SPARK-56750 wired `CatalogManager` into `SessionCatalog` as the live source for path-driven session function kinds, two paths form a lock-order inversion:
- **Arm 1** (`SessionCatalog.synchronized` -> `CatalogManager.synchronized`): unqualified function resolution evaluating the live PATH reaches into `CatalogManager.sqlResolutionPathEntries` (which synchronizes on the manager) while holding the catalog's intrinsic lock at peer call sites.
- **Arm 2** (`CatalogManager.synchronized` -> `SessionCatalog.synchronized`): `setCurrentNamespace` / `setCurrentCatalog` hold the manager's lock and then call back into `v1SessionCatalog.setCurrentDatabase*`, which synchronizes on `SessionCatalog`.
Two threads sharing a `SparkSession` -- one running any SQL with an unqualified function reference, the other running `USE SCHEMA` / `USE CATALOG` -- can wedge on each other's intrinsic locks. The hazard is independent of `spark.sql.functionResolution.sessionOrder`: Arm 1 still acquires the manager lock just to read what the order is, and Arm 2 has nothing to do with order at all. See [SPARK-56939](https://issues.apache.org/jira/browse/SPARK-56939) for a standalone repro.
### Does this PR introduce _any_ user-facing change?
No. This is a concurrency fix; serial behavior is unchanged. The only observable difference under contention is that the session no longer deadlocks.
### How was this patch tested?
- New regression test in `SetPathSuite`, `SPARK-56939: concurrent USE SCHEMA and unqualified function lookups do not deadlock`, that follows the JIRA's exact repro: one thread alternates `USE SCHEMA s1` / `USE SCHEMA s2`, another runs unqualified `count(*)` queries. Without the fix the threads hang on each other's intrinsic locks; with the fix the test completes within the 30s budget.
- ``build/sbt 'sql/testOnly *SetPathSuite'`` -- 60/60 pass.
- ``build/sbt 'catalyst/testOnly *SessionCatalogSuite *CatalogManagerSuite *FunctionResolution*'`` -- 119/119 pass.
- ``build/sbt 'sql/testOnly *SQLFunctionSuite *SQLViewSuite'`` -- 70/70 pass.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor / Claude Opus 4.7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #55977:
URL: https://github.com/apache/spark/pull/55977#discussion_r3272828330
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -132,15 +132,33 @@ class CatalogManager(
}
}
- def setCurrentNamespace(namespace: Array[String]): Unit = synchronized {
- if (isSessionCatalog(currentCatalog) && namespace.length == 1) {
+ def setCurrentNamespace(namespace: Array[String]): Unit = {
+ // SPARK-56939: do NOT hold [[CatalogManager]]'s intrinsic lock across the callbacks below.
+ // [[v1SessionCatalog.setCurrentDatabaseWithNameCheck]] briefly synchronizes on
+ // [[SessionCatalog]], and concurrent unqualified function resolution acquires the
+ // [[SessionCatalog]] lock and then reaches into [[CatalogManager]] via
+ // [[sqlResolutionPathEntries]]; nesting the manager lock outside the catalog lock here
+ // would invert that order and deadlock. Snapshot the dispatch decision under the lock,
+ // run callbacks outside it, then publish the new namespace under the lock again.
+ //
+ // Concurrency trade-off versus the pre-SPARK-56939 atomic version: the `isSession`
+ // snapshot can drift if a concurrent [[setCurrentCatalog]] switches to a v2 catalog
+ // between this read and the v1 callback below -- the callback would still touch
+ // `v1.currentDb` even though the active catalog is no longer the session catalog. A
+ // later switch back to the session catalog resets `v1.currentDb` to `default` (see
+ // [[setCurrentCatalog]]), so long-term state remains consistent; only the intermediate
+ // observation is novel. Acceptable trade-off against the deadlock alternative.
Review Comment:
Good catch -- the prior comment scoped the trade-off to v1-side staleness only. Restructured the block in commit f504667f into two labeled cases: (a) the v1-side `isSession` drift it already documented, and (b) the sticky CM-side publish-overwrite race you describe (concurrent `setCurrentCatalog` completing between the v1 callback returning and the publish, leaving `_currentNamespace = Some(namespace)` published under a different `_currentCatalogName` than the snapshotted one, with no auto-recovery). Framed (b) explicitly as last-writer-wins for racing `USE` commands -- the conventional expectation -- so it reads as an accepted trade-off rather than a hidden anomaly.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -221,6 +239,15 @@ class CatalogManager(
* When PATH is enabled and a session path is in effect (stored or via
* [[SQLConf#DEFAULT_PATH]]), formats the resolved entries. Otherwise falls back to the legacy
* resolutionSearchPath.
+ *
+ * SPARK-56939 note: this is currently the only intentional `CatalogManager.synchronized ->
+ * SessionCatalog.synchronized` nest left in this class. The transitive call into
+ * [[v1SessionCatalog.getCurrentDatabase]] happens via [[currentNamespace]], which fetches
+ * the v1 current database under the CM lock. It is safe today because no code path holds
+ * [[SessionCatalog]]'s intrinsic lock while waiting on [[CatalogManager]]'s -- the
+ * SPARK-56939 fix removed every such SC->CM ordering. Any future change that introduces a
+ * new SC->CM ordering must avoid resurrecting the deadlock by also taking
+ * `currentPathString` (or any other CM->SC nest) into account.
Review Comment:
Applied your suggested rewording (f504667f) -- inverted the clauses so the modal verb attaches unambiguously.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -265,6 +292,48 @@ class CatalogManager(
currentCatalog, currentNamespace,
currentCatalog, currentNamespace)
+ /**
+ * Snapshot the live PATH-derived [[SessionCatalog.SessionFunctionKind]] order used by
+ * unqualified function/table-function resolution.
+ *
+ * The `(currentCatalog, _currentNamespace, sessionPath)` triple is read together inside a
+ * single CM critical section so a concurrent `USE` / `SET PATH` cannot return a torn
+ * snapshot for those three fields (e.g. catalog from one observation, explicit namespace
+ * from another).
+ *
+ * The `v1SessionCatalog.getCurrentDatabase` read needed for the default-namespace fallback
+ * is taken OUTSIDE the CM lock and is therefore intentionally racy w.r.t. a concurrent
+ * `USE SCHEMA`. That staleness is harmless for this helper's output: this method consumes
+ * `effectiveNs` only to expand `CURRENT_SCHEMA` markers in the SQL path, and
+ * [[CatalogManager.systemFunctionKindsFromPath]] only retains literal `system.builtin` /
+ * `system.session` entries from the resolved path -- it never inspects any
+ * `(catalog, namespace)` derived from `v1`. So if `v1CurrentDb` lags by one `USE SCHEMA`,
+ * a `CURRENT_SCHEMA` entry might briefly resolve to the previous database, but the kinds
+ * list (the only thing returned here) is unaffected. Lifting the read inside the CM lock
+ * would re-introduce the SPARK-56939 lock-order inversion this helper exists to avoid.
Review Comment:
Applied (f504667f). "Lifting" -> "Moving".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #55977: [SPARK-56939][SQL] Resolve deadlock between USE and function lookup
URL: https://github.com/apache/spark/pull/55977
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #55977:
URL: https://github.com/apache/spark/pull/55977#discussion_r3272766367
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -221,6 +239,15 @@ class CatalogManager(
* When PATH is enabled and a session path is in effect (stored or via
* [[SQLConf#DEFAULT_PATH]]), formats the resolved entries. Otherwise falls back to the legacy
* resolutionSearchPath.
+ *
+ * SPARK-56939 note: this is currently the only intentional `CatalogManager.synchronized ->
+ * SessionCatalog.synchronized` nest left in this class. The transitive call into
+ * [[v1SessionCatalog.getCurrentDatabase]] happens via [[currentNamespace]], which fetches
+ * the v1 current database under the CM lock. It is safe today because no code path holds
+ * [[SessionCatalog]]'s intrinsic lock while waiting on [[CatalogManager]]'s -- the
+ * SPARK-56939 fix removed every such SC->CM ordering. Any future change that introduces a
+ * new SC->CM ordering must avoid resurrecting the deadlock by also taking
+ * `currentPathString` (or any other CM->SC nest) into account.
Review Comment:
Minor wording: "must avoid resurrecting the deadlock by also taking ... into account" attaches ambiguously — it can be parsed as "the resurrection is done by taking it into account." Inverting the clauses removes the ambiguity.
```suggestion
* SPARK-56939 fix removed every such SC->CM ordering. Any future change that introduces a
* new SC->CM ordering must take `currentPathString` (or any other CM->SC nest) into
* account to avoid resurrecting the deadlock.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -132,15 +132,33 @@ class CatalogManager(
}
}
- def setCurrentNamespace(namespace: Array[String]): Unit = synchronized {
- if (isSessionCatalog(currentCatalog) && namespace.length == 1) {
+ def setCurrentNamespace(namespace: Array[String]): Unit = {
+ // SPARK-56939: do NOT hold [[CatalogManager]]'s intrinsic lock across the callbacks below.
+ // [[v1SessionCatalog.setCurrentDatabaseWithNameCheck]] briefly synchronizes on
+ // [[SessionCatalog]], and concurrent unqualified function resolution acquires the
+ // [[SessionCatalog]] lock and then reaches into [[CatalogManager]] via
+ // [[sqlResolutionPathEntries]]; nesting the manager lock outside the catalog lock here
+ // would invert that order and deadlock. Snapshot the dispatch decision under the lock,
+ // run callbacks outside it, then publish the new namespace under the lock again.
+ //
+ // Concurrency trade-off versus the pre-SPARK-56939 atomic version: the `isSession`
+ // snapshot can drift if a concurrent [[setCurrentCatalog]] switches to a v2 catalog
+ // between this read and the v1 callback below -- the callback would still touch
+ // `v1.currentDb` even though the active catalog is no longer the session catalog. A
+ // later switch back to the session catalog resets `v1.currentDb` to `default` (see
+ // [[setCurrentCatalog]]), so long-term state remains consistent; only the intermediate
+ // observation is novel. Acceptable trade-off against the deadlock alternative.
Review Comment:
Nit (non-blocking): the expanded block correctly documents the `isSession`-snapshot drift wrt `v1.currentDb`, but there's a second drift mode in the same window that the comment doesn't mention. Between the v1 callback returning (L155) and the publish at L159-161, a concurrent `setCurrentCatalog` can complete fully — switching `_currentCatalogName` to a v2 catalog and clearing `_currentNamespace = None` — before this method's L159 publish overwrites that with `Some(namespace)`. End state: `_currentNamespace = Some([userSchema])` is published under a *different* `_currentCatalogName` than the one active when L151 made the dispatch decision, and unlike the `v1.currentDb` drift this one is sticky — there's no analogous "switch back resets it" recovery. It's still last-writer-wins for two racing USE commands (acceptable), but the trade-off picture in the current comment is scoped to v1-side staleness only. Worth one more sentence either acknowledging the `_currentNamespace` vs `
_currentCatalogName` mismatch window, or stating explicitly that the documented trade-off covers v1 state only.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -265,6 +292,48 @@ class CatalogManager(
currentCatalog, currentNamespace,
currentCatalog, currentNamespace)
+ /**
+ * Snapshot the live PATH-derived [[SessionCatalog.SessionFunctionKind]] order used by
+ * unqualified function/table-function resolution.
+ *
+ * The `(currentCatalog, _currentNamespace, sessionPath)` triple is read together inside a
+ * single CM critical section so a concurrent `USE` / `SET PATH` cannot return a torn
+ * snapshot for those three fields (e.g. catalog from one observation, explicit namespace
+ * from another).
+ *
+ * The `v1SessionCatalog.getCurrentDatabase` read needed for the default-namespace fallback
+ * is taken OUTSIDE the CM lock and is therefore intentionally racy w.r.t. a concurrent
+ * `USE SCHEMA`. That staleness is harmless for this helper's output: this method consumes
+ * `effectiveNs` only to expand `CURRENT_SCHEMA` markers in the SQL path, and
+ * [[CatalogManager.systemFunctionKindsFromPath]] only retains literal `system.builtin` /
+ * `system.session` entries from the resolved path -- it never inspects any
+ * `(catalog, namespace)` derived from `v1`. So if `v1CurrentDb` lags by one `USE SCHEMA`,
+ * a `CURRENT_SCHEMA` entry might briefly resolve to the previous database, but the kinds
+ * list (the only thing returned here) is unaffected. Lifting the read inside the CM lock
+ * would re-introduce the SPARK-56939 lock-order inversion this helper exists to avoid.
Review Comment:
Minor wording: "Lifting the read inside the CM lock" is contradictory — "lifting" connotes moving up/out, while the intent here is moving the read *into* the locked region.
```suggestion
* list (the only thing returned here) is unaffected. Moving the read inside the CM lock
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #55977:
URL: https://github.com/apache/spark/pull/55977#discussion_r3266732184
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -265,6 +275,42 @@ class CatalogManager(
currentCatalog, currentNamespace,
currentCatalog, currentNamespace)
+ /**
+ * Snapshot the live PATH-derived [[SessionCatalog.SessionFunctionKind]] order used by
+ * unqualified function/table-function resolution.
+ *
+ * The kinds list is computed by reading the current catalog, current namespace, and the
Review Comment:
Minor: this docstring states the (catalog, namespace, path) triple is observed atomically, but `v1CurrentDb` (read at L299) is intentionally outside the lock. The kinds extraction only consumes `system.builtin` / `system.session` entries, so a torn v1 read can't affect the result — but readers tracing this code may not see why the staleness is harmless. One extra clause acknowledging that v1's contribution to `effectiveNs` is intentionally racy (and explaining why it doesn't matter for the kinds list) would head off confusion.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -330,15 +376,32 @@ class CatalogManager(
catalog(_currentCatalogName.getOrElse(conf.getConf(SQLConf.DEFAULT_CATALOG)))
}
- def setCurrentCatalog(catalogName: String): Unit = synchronized {
- // `setCurrentCatalog` is noop if it doesn't switch to a different catalog.
- if (currentCatalog.name() != catalogName) {
- catalog(catalogName)
- _currentCatalogName = Some(catalogName)
- _currentNamespace = None
+ def setCurrentCatalog(catalogName: String): Unit = {
+ // SPARK-56939: see [[setCurrentNamespace]]. Avoid nesting [[CatalogManager]]'s lock
+ // across [[v1SessionCatalog.setCurrentDatabase]] (which synchronizes on
+ // [[SessionCatalog]]) to prevent a lock-order inversion with concurrent unqualified
+ // function resolution.
+ val needsSwitch = synchronized {
+ // `setCurrentCatalog` is noop if it doesn't switch to a different catalog.
+ if (currentCatalog.name() != catalogName) {
+ // Force-load the named catalog while holding the manager lock to keep the
+ // not-found error semantics; if loading fails, throw before mutating state.
+ catalog(catalogName)
+ true
+ } else {
+ false
+ }
+ }
+ if (needsSwitch) {
// Reset the current database of v1 `SessionCatalog` when switching current catalog, so that
// when we switch back to session catalog, the current namespace definitely is ["default"].
+ // Run this BEFORE publishing the new catalog name so that if a reader observes the new
Review Comment:
This justification only tells half the story. "If a reader observes the new catalog, the v1 state is already consistent with it" is true, but the symmetric case is now allowed: while the new name is *not yet* published, a concurrent reader sees `(oldCatalog, v1.currentDb=default)` — a torn state the pre-PR atomic version forbade. If the old catalog was the session catalog (the common case), the user's previous namespace is briefly invisible to that reader. Worth mentioning the trade-off so a future maintainer doesn't read this comment and conclude the new ordering is strictly better.
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -977,6 +977,83 @@ class SetPathSuite extends SharedSparkSession {
}
}
+ test("SPARK-56939: concurrent USE SCHEMA and unqualified function lookups do not deadlock") {
+ // Regression for SPARK-56939. Prior to the fix, [[CatalogManager.setCurrentNamespace]]
+ // held the manager's intrinsic lock while calling into
+ // [[SessionCatalog.setCurrentDatabaseWithNameCheck]] (which takes the catalog's
+ // intrinsic lock), while concurrent unqualified function resolution acquired the
+ // catalog's intrinsic lock and then reached back into the manager via
+ // [[CatalogManager.sqlResolutionPathEntries]]. That lock-order inversion deadlocked the
+ // session whenever a `USE SCHEMA` raced with any unqualified function reference.
+ //
+ // The hazard is independent of [[SQLConf.PATH_ENABLED]] and the
+ // resolution-order setting, so this test exercises the default configuration.
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s1")
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s2")
+ try {
+ val budget = 200
+ val iterations = new java.util.concurrent.atomic.AtomicInteger(0)
+ val barrier = new java.util.concurrent.CyclicBarrier(2)
+ val errors = new java.util.concurrent.ConcurrentLinkedQueue[Throwable]()
+
+ val useThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ sql(if ((i % 2) == 0) "USE SCHEMA spark_56939_s1" else "USE SCHEMA spark_56939_s2")
Review Comment:
Both `setCurrentNamespace` and `setCurrentCatalog` were rewritten with the same split-lock pattern, but only `USE SCHEMA` is exercised here. A symmetric thread that toggles `USE CATALOG` between `spark_catalog` and a registered v2 catalog would close coverage of the `setCurrentCatalog` arm and guard against asymmetric regressions in one path or the other.
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -977,6 +977,83 @@ class SetPathSuite extends SharedSparkSession {
}
}
+ test("SPARK-56939: concurrent USE SCHEMA and unqualified function lookups do not deadlock") {
+ // Regression for SPARK-56939. Prior to the fix, [[CatalogManager.setCurrentNamespace]]
+ // held the manager's intrinsic lock while calling into
+ // [[SessionCatalog.setCurrentDatabaseWithNameCheck]] (which takes the catalog's
+ // intrinsic lock), while concurrent unqualified function resolution acquired the
+ // catalog's intrinsic lock and then reached back into the manager via
+ // [[CatalogManager.sqlResolutionPathEntries]]. That lock-order inversion deadlocked the
+ // session whenever a `USE SCHEMA` raced with any unqualified function reference.
+ //
+ // The hazard is independent of [[SQLConf.PATH_ENABLED]] and the
+ // resolution-order setting, so this test exercises the default configuration.
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s1")
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s2")
+ try {
+ val budget = 200
+ val iterations = new java.util.concurrent.atomic.AtomicInteger(0)
+ val barrier = new java.util.concurrent.CyclicBarrier(2)
+ val errors = new java.util.concurrent.ConcurrentLinkedQueue[Throwable]()
+
+ val useThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ sql(if ((i % 2) == 0) "USE SCHEMA spark_56939_s1" else "USE SCHEMA spark_56939_s2")
+ i += 1
+ }
+ } catch {
+ case t: Throwable => errors.add(t)
+ }
+ }, "SPARK-56939-use-schema")
+
+ val lookupThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ // Unqualified `count(*)` exercises the kinds-order provider that resolves
+ // against the live PATH via [[CatalogManager]] -- the side of the cycle
+ // that previously acquired the catalog lock first and then the manager lock.
+ val n = sql("SELECT count(*) FROM VALUES (1), (2), (3) AS t(a)")
+ .head().getLong(0)
+ assert(n == 3L, s"unexpected count: $n at iteration $i")
+ iterations.incrementAndGet()
+ i += 1
+ }
+ } catch {
+ case t: Throwable => errors.add(t)
+ }
+ }, "SPARK-56939-lookup")
+
+ useThread.start()
+ lookupThread.start()
+
+ // Generous join: 30s is plenty for 200 cheap queries on either side and gives a
+ // clear failure signal if the implementation regresses into a deadlock.
+ val joinMillis = 30000L
+ useThread.join(joinMillis)
+ lookupThread.join(joinMillis)
+
+ assert(!useThread.isAlive,
+ "USE SCHEMA thread did not finish; lock-order inversion between SessionCatalog and " +
+ "CatalogManager likely returned (SPARK-56939).")
Review Comment:
Minor wording: "likely returned" reads ambiguously (it can be parsed transitively). Suggest "likely regressed" — and the same change at L1045.
```suggestion
"CatalogManager likely regressed (SPARK-56939).")
```
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -977,6 +977,83 @@ class SetPathSuite extends SharedSparkSession {
}
}
+ test("SPARK-56939: concurrent USE SCHEMA and unqualified function lookups do not deadlock") {
+ // Regression for SPARK-56939. Prior to the fix, [[CatalogManager.setCurrentNamespace]]
+ // held the manager's intrinsic lock while calling into
+ // [[SessionCatalog.setCurrentDatabaseWithNameCheck]] (which takes the catalog's
+ // intrinsic lock), while concurrent unqualified function resolution acquired the
+ // catalog's intrinsic lock and then reached back into the manager via
+ // [[CatalogManager.sqlResolutionPathEntries]]. That lock-order inversion deadlocked the
+ // session whenever a `USE SCHEMA` raced with any unqualified function reference.
+ //
+ // The hazard is independent of [[SQLConf.PATH_ENABLED]] and the
+ // resolution-order setting, so this test exercises the default configuration.
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s1")
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s2")
+ try {
+ val budget = 200
+ val iterations = new java.util.concurrent.atomic.AtomicInteger(0)
+ val barrier = new java.util.concurrent.CyclicBarrier(2)
+ val errors = new java.util.concurrent.ConcurrentLinkedQueue[Throwable]()
+
+ val useThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ sql(if ((i % 2) == 0) "USE SCHEMA spark_56939_s1" else "USE SCHEMA spark_56939_s2")
+ i += 1
+ }
+ } catch {
+ case t: Throwable => errors.add(t)
+ }
+ }, "SPARK-56939-use-schema")
+
+ val lookupThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ // Unqualified `count(*)` exercises the kinds-order provider that resolves
+ // against the live PATH via [[CatalogManager]] -- the side of the cycle
+ // that previously acquired the catalog lock first and then the manager lock.
+ val n = sql("SELECT count(*) FROM VALUES (1), (2), (3) AS t(a)")
+ .head().getLong(0)
+ assert(n == 3L, s"unexpected count: $n at iteration $i")
+ iterations.incrementAndGet()
+ i += 1
+ }
+ } catch {
+ case t: Throwable => errors.add(t)
+ }
+ }, "SPARK-56939-lookup")
+
+ useThread.start()
+ lookupThread.start()
+
+ // Generous join: 30s is plenty for 200 cheap queries on either side and gives a
+ // clear failure signal if the implementation regresses into a deadlock.
+ val joinMillis = 30000L
+ useThread.join(joinMillis)
+ lookupThread.join(joinMillis)
+
+ assert(!useThread.isAlive,
+ "USE SCHEMA thread did not finish; lock-order inversion between SessionCatalog and " +
+ "CatalogManager likely returned (SPARK-56939).")
+ assert(!lookupThread.isAlive,
+ "Lookup thread did not finish; lock-order inversion between SessionCatalog and " +
+ "CatalogManager likely returned (SPARK-56939).")
Review Comment:
Same as above:
```suggestion
"CatalogManager likely regressed (SPARK-56939).")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on PR #55977:
URL: https://github.com/apache/spark/pull/55977#issuecomment-4489711627
Thanks for the thorough review @cloud-fan! Pushed a followup commit (ecf913ab) that addresses every item, including the three general observations from the review summary that weren't anchored to specific lines:
- **"Serial behavior is unchanged" oversimplifies.** Expanded the `setCurrentCatalog` and `setCurrentNamespace` block comments to spell out the two new concurrent torn-state windows the split-lock pattern admits (briefly invisible old namespace; drifted `isSession` snapshot) and framed them as deliberate trade-offs against the deadlock alternative.
- **`CatalogManager.reset()` still nested CM->SC.** Applied the same split-lock pattern there too so the locking contract is uniform across every CM mutator that calls back into `v1SessionCatalog`. `reset` is `private[sql]` and test-only today, but the asymmetry was a footgun for future test helpers; symmetric pattern + a doc comment explains why.
- **`currentPathString` is the only remaining intentional CM->SC nest.** Added a doc note on `currentPathString` explicitly labeling it as such and pointing out that any future change introducing a new SC->CM ordering must account for it so the SPARK-56939 deadlock cannot resurface via `CURRENT_PATH()` readers.
- **Stale references in `SessionCatalog`.** Refreshed the comments on `lookupBuiltinOrTempTableFunction` and `lookupFunctionInfo` so they describe the post-SPARK-56939 invariant (catalog lock must never be held while reaching into `CatalogManager` from a function-resolution path) rather than citing the now-removed `setCurrentNamespace` counter-party.
The regression test was extended with a third `USE`-catalog thread (see inline reply) so both arms of the split-lock pattern are exercised symmetrically.
All suites green locally: `SetPathSuite` 60/60, catalyst catalog suites 119/119, `SQLFunctionSuite` + `SQLViewSuite` 70/70.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #55977:
URL: https://github.com/apache/spark/pull/55977#discussion_r3267793373
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -265,6 +275,42 @@ class CatalogManager(
currentCatalog, currentNamespace,
currentCatalog, currentNamespace)
+ /**
+ * Snapshot the live PATH-derived [[SessionCatalog.SessionFunctionKind]] order used by
+ * unqualified function/table-function resolution.
+ *
+ * The kinds list is computed by reading the current catalog, current namespace, and the
Review Comment:
Good catch. Reworked the docstring (commit ecf913ab) to spell out the contract: the kinds extraction in `systemFunctionKindsFromPath` only retains literal `system.builtin` / `system.session` entries, so even if `v1CurrentDb` lags by one `USE SCHEMA`, a stale `CURRENT_SCHEMA` expansion can change the path **content** but never the **kinds list** that this helper returns. Added that justification + a forward pointer to the deadlock cycle so future readers don't lift the v1 read inside the lock.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala:
##########
@@ -330,15 +376,32 @@ class CatalogManager(
catalog(_currentCatalogName.getOrElse(conf.getConf(SQLConf.DEFAULT_CATALOG)))
}
- def setCurrentCatalog(catalogName: String): Unit = synchronized {
- // `setCurrentCatalog` is noop if it doesn't switch to a different catalog.
- if (currentCatalog.name() != catalogName) {
- catalog(catalogName)
- _currentCatalogName = Some(catalogName)
- _currentNamespace = None
+ def setCurrentCatalog(catalogName: String): Unit = {
+ // SPARK-56939: see [[setCurrentNamespace]]. Avoid nesting [[CatalogManager]]'s lock
+ // across [[v1SessionCatalog.setCurrentDatabase]] (which synchronizes on
+ // [[SessionCatalog]]) to prevent a lock-order inversion with concurrent unqualified
+ // function resolution.
+ val needsSwitch = synchronized {
+ // `setCurrentCatalog` is noop if it doesn't switch to a different catalog.
+ if (currentCatalog.name() != catalogName) {
+ // Force-load the named catalog while holding the manager lock to keep the
+ // not-found error semantics; if loading fails, throw before mutating state.
+ catalog(catalogName)
+ true
+ } else {
+ false
+ }
+ }
+ if (needsSwitch) {
// Reset the current database of v1 `SessionCatalog` when switching current catalog, so that
// when we switch back to session catalog, the current namespace definitely is ["default"].
+ // Run this BEFORE publishing the new catalog name so that if a reader observes the new
Review Comment:
Fair point -- the prior comment only told the half-story that the new ordering forbids. Expanded it (ecf913ab) to also acknowledge the symmetric window: between `v1.setCurrentDatabase(default)` and the publish, a concurrent reader of `currentNamespace` sees `(oldCatalog, v1.currentDb = default)`, which when the old catalog was the session catalog briefly hides the user's previous namespace. Framed both halves as a concurrency trade-off against the deadlock alternative so a future maintainer can't read the comment as "strictly better." Did the same for `setCurrentNamespace`'s drifted `isSession` snapshot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #55977:
URL: https://github.com/apache/spark/pull/55977#discussion_r3267796553
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -977,6 +977,83 @@ class SetPathSuite extends SharedSparkSession {
}
}
+ test("SPARK-56939: concurrent USE SCHEMA and unqualified function lookups do not deadlock") {
+ // Regression for SPARK-56939. Prior to the fix, [[CatalogManager.setCurrentNamespace]]
+ // held the manager's intrinsic lock while calling into
+ // [[SessionCatalog.setCurrentDatabaseWithNameCheck]] (which takes the catalog's
+ // intrinsic lock), while concurrent unqualified function resolution acquired the
+ // catalog's intrinsic lock and then reached back into the manager via
+ // [[CatalogManager.sqlResolutionPathEntries]]. That lock-order inversion deadlocked the
+ // session whenever a `USE SCHEMA` raced with any unqualified function reference.
+ //
+ // The hazard is independent of [[SQLConf.PATH_ENABLED]] and the
+ // resolution-order setting, so this test exercises the default configuration.
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s1")
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s2")
+ try {
+ val budget = 200
+ val iterations = new java.util.concurrent.atomic.AtomicInteger(0)
+ val barrier = new java.util.concurrent.CyclicBarrier(2)
+ val errors = new java.util.concurrent.ConcurrentLinkedQueue[Throwable]()
+
+ val useThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ sql(if ((i % 2) == 0) "USE SCHEMA spark_56939_s1" else "USE SCHEMA spark_56939_s2")
+ i += 1
+ }
+ } catch {
+ case t: Throwable => errors.add(t)
+ }
+ }, "SPARK-56939-use-schema")
+
+ val lookupThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ // Unqualified `count(*)` exercises the kinds-order provider that resolves
+ // against the live PATH via [[CatalogManager]] -- the side of the cycle
+ // that previously acquired the catalog lock first and then the manager lock.
+ val n = sql("SELECT count(*) FROM VALUES (1), (2), (3) AS t(a)")
+ .head().getLong(0)
+ assert(n == 3L, s"unexpected count: $n at iteration $i")
+ iterations.incrementAndGet()
+ i += 1
+ }
+ } catch {
+ case t: Throwable => errors.add(t)
+ }
+ }, "SPARK-56939-lookup")
+
+ useThread.start()
+ lookupThread.start()
+
+ // Generous join: 30s is plenty for 200 cheap queries on either side and gives a
+ // clear failure signal if the implementation regresses into a deadlock.
+ val joinMillis = 30000L
+ useThread.join(joinMillis)
+ lookupThread.join(joinMillis)
+
+ assert(!useThread.isAlive,
+ "USE SCHEMA thread did not finish; lock-order inversion between SessionCatalog and " +
+ "CatalogManager likely returned (SPARK-56939).")
+ assert(!lookupThread.isAlive,
+ "Lookup thread did not finish; lock-order inversion between SessionCatalog and " +
+ "CatalogManager likely returned (SPARK-56939).")
Review Comment:
Applied (ecf913ab).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #55977:
URL: https://github.com/apache/spark/pull/55977#discussion_r3267795313
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -977,6 +977,83 @@ class SetPathSuite extends SharedSparkSession {
}
}
+ test("SPARK-56939: concurrent USE SCHEMA and unqualified function lookups do not deadlock") {
+ // Regression for SPARK-56939. Prior to the fix, [[CatalogManager.setCurrentNamespace]]
+ // held the manager's intrinsic lock while calling into
+ // [[SessionCatalog.setCurrentDatabaseWithNameCheck]] (which takes the catalog's
+ // intrinsic lock), while concurrent unqualified function resolution acquired the
+ // catalog's intrinsic lock and then reached back into the manager via
+ // [[CatalogManager.sqlResolutionPathEntries]]. That lock-order inversion deadlocked the
+ // session whenever a `USE SCHEMA` raced with any unqualified function reference.
+ //
+ // The hazard is independent of [[SQLConf.PATH_ENABLED]] and the
+ // resolution-order setting, so this test exercises the default configuration.
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s1")
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s2")
+ try {
+ val budget = 200
+ val iterations = new java.util.concurrent.atomic.AtomicInteger(0)
+ val barrier = new java.util.concurrent.CyclicBarrier(2)
+ val errors = new java.util.concurrent.ConcurrentLinkedQueue[Throwable]()
+
+ val useThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ sql(if ((i % 2) == 0) "USE SCHEMA spark_56939_s1" else "USE SCHEMA spark_56939_s2")
Review Comment:
Added a third thread (ecf913ab) that toggles `USE spark_56939_testcat` <-> `USE spark_catalog`, exercising the `setCurrentCatalog` arm symmetrically with the existing `USE SCHEMA` thread. Registered an `InMemoryCatalog` under `spark_56939_testcat` in the test setup. The schema-thread now tolerates `NoSuchNamespaceException` since the catalog-thread can switch to the v2 testcat (which doesn't have those schemas) -- that's an expected interleaving outcome, not a deadlock symptom. Test completes in ~5s on the default config.
_Aside on the syntax: `USE CATALOG <name>` isn't a real Spark SQL form; the grammar has just `USE identifierReference` and resolves a single identifier to a catalog if one is registered. So the test uses `USE $v2Catalog` / `USE spark_catalog` (parser-accepted) while still describing the operation as "USE CATALOG" in the test name/comments since that is the conceptual operation._
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -977,6 +977,83 @@ class SetPathSuite extends SharedSparkSession {
}
}
+ test("SPARK-56939: concurrent USE SCHEMA and unqualified function lookups do not deadlock") {
+ // Regression for SPARK-56939. Prior to the fix, [[CatalogManager.setCurrentNamespace]]
+ // held the manager's intrinsic lock while calling into
+ // [[SessionCatalog.setCurrentDatabaseWithNameCheck]] (which takes the catalog's
+ // intrinsic lock), while concurrent unqualified function resolution acquired the
+ // catalog's intrinsic lock and then reached back into the manager via
+ // [[CatalogManager.sqlResolutionPathEntries]]. That lock-order inversion deadlocked the
+ // session whenever a `USE SCHEMA` raced with any unqualified function reference.
+ //
+ // The hazard is independent of [[SQLConf.PATH_ENABLED]] and the
+ // resolution-order setting, so this test exercises the default configuration.
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s1")
+ sql("CREATE SCHEMA IF NOT EXISTS spark_56939_s2")
+ try {
+ val budget = 200
+ val iterations = new java.util.concurrent.atomic.AtomicInteger(0)
+ val barrier = new java.util.concurrent.CyclicBarrier(2)
+ val errors = new java.util.concurrent.ConcurrentLinkedQueue[Throwable]()
+
+ val useThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ sql(if ((i % 2) == 0) "USE SCHEMA spark_56939_s1" else "USE SCHEMA spark_56939_s2")
+ i += 1
+ }
+ } catch {
+ case t: Throwable => errors.add(t)
+ }
+ }, "SPARK-56939-use-schema")
+
+ val lookupThread = new Thread(() => {
+ try {
+ barrier.await()
+ var i = 0
+ while (i < budget && errors.isEmpty) {
+ // Unqualified `count(*)` exercises the kinds-order provider that resolves
+ // against the live PATH via [[CatalogManager]] -- the side of the cycle
+ // that previously acquired the catalog lock first and then the manager lock.
+ val n = sql("SELECT count(*) FROM VALUES (1), (2), (3) AS t(a)")
+ .head().getLong(0)
+ assert(n == 3L, s"unexpected count: $n at iteration $i")
+ iterations.incrementAndGet()
+ i += 1
+ }
+ } catch {
+ case t: Throwable => errors.add(t)
+ }
+ }, "SPARK-56939-lookup")
+
+ useThread.start()
+ lookupThread.start()
+
+ // Generous join: 30s is plenty for 200 cheap queries on either side and gives a
+ // clear failure signal if the implementation regresses into a deadlock.
+ val joinMillis = 30000L
+ useThread.join(joinMillis)
+ lookupThread.join(joinMillis)
+
+ assert(!useThread.isAlive,
+ "USE SCHEMA thread did not finish; lock-order inversion between SessionCatalog and " +
+ "CatalogManager likely returned (SPARK-56939).")
Review Comment:
Applied (ecf913ab) at both call sites.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56939][SQL] Resolve deadlock between USE and function lookup [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #55977:
URL: https://github.com/apache/spark/pull/55977#issuecomment-4498540135
thanks, merging to master/4.x/4.2!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org