[SPARK-56939][SQL] Resolve deadlock between USE and function lookup

[srielau]

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 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

[cloud-fan]

Summary

Prior state and problem. Two paths between SessionCatalog and CatalogManager form a lock-order inversion on a shared SparkSession:

• Arm 2: CatalogManager.setCurrentNamespace / setCurrentCatalog held the manager's intrinsic lock and then called into v1SessionCatalog.setCurrentDatabase*, which takes SessionCatalog.synchronized.
• Arm 1: peer call sites that hold SessionCatalog.synchronized reach CatalogManager.sqlResolutionPathEntries, which is synchronized on the manager.

lookupBuiltinOrTempFunction was already kept un-synchronized (with an explicit comment at SessionCatalog.scala:2574) precisely because Arm 1 was anticipated — but Arm 2 was not. Two threads sharing a session — one running USE SCHEMA, the other any unqualified function reference — could wedge on each other's intrinsic locks, as reproduced in SPARK-56939.

Design approach. Snapshot-then-act split-lock pattern:

• For setCurrentNamespace / setCurrentCatalog: snapshot the dispatch decision under the CM lock, run the v1 callback outside the lock, then re-acquire the lock to publish.
• A new helper sessionFunctionKindsForUnqualifiedResolution consolidates the lookup-side snapshot into a single critical section so the path-driven kinds order (a) doesn't re-enter SessionCatalog under the manager lock and (b) doesn't observe a torn (catalog, namespace) tuple. It deliberately reads v1.getCurrentDatabase before the CM lock so the helper itself doesn't deadlock.
• The two callers (SessionCatalog.sessionFunctionKindsInResolutionOrder and FunctionResolution.isSessionBeforeBuiltinInPath) are routed through the helper so the predicate and the lookup loop share one snapshot.

Key design decisions made by this PR.

• setCurrentCatalog now performs v1.setCurrentDatabase(default) before publishing _currentCatalogName (previously after). This trades pre-PR atomicity for slightly more robust error semantics (a requireDbExists failure no longer leaves CM with a half-published new catalog name).
• The helper always reads v1.getCurrentDatabase even when the value will be unused (when an explicit _currentNamespace is present). Lifting the read inside the lock would re-introduce the deadlock cycle, as the comment notes.

Implementation sketch. Touches three files: CatalogManager (new helper + split locks on setCurrent*), SessionCatalog (route through the helper, with locking-contract doc), FunctionResolution (route through the helper). One regression test in SetPathSuite.

This is a clean, well-scoped concurrency fix and the new regression test is faithful to the JIRA repro. None of the items below are blockers; the inline comments are tightening, and the general observations are mostly forward-looking guards.

General observations (not anchored to changed lines)

• Concurrent USE CATALOG + USE SCHEMA semantics. The PR description's "serial behavior is unchanged" is accurate, but the split-lock pattern admits new concurrent interleavings that the pre-PR atomic version did not. Two examples worth thinking through:

  • During setCurrentCatalog, between v1.setCurrentDatabase(default) (L400) and the publish (L401–404), a reader of currentNamespace observes (oldCatalog, v1.currentDb=default). If the old catalog is the session catalog, the user's previous namespace is briefly invisible — concurrent SQL analysis could miss relations in the prior namespace. The comment at L398–399 frames the new ordering as strictly better; in fairness it should acknowledge this symmetric torn-state window.
  • In setCurrentNamespace, the isSession snapshot at L143 can drift if setCurrentCatalog switches to a v2 catalog before the v1 callback runs — v1.setCurrentDatabaseWithNameCheck would then still stomp v1.currentDb even though the active catalog is no longer session. Long-term state stays consistent (a later switch back to the session catalog resets v1.currentDb to default), but the intermediate observation is novel.

  These are arguably acceptable concurrency-mode trade-offs given the deadlock alternative, but the PR description currently addresses only the serial case.

• CatalogManager.reset() (L414) still holds CM lock across v1SessionCatalog.setCurrentDatabase — the same lock-order inversion the PR removes from setCurrent*. Tests-only and private[sql], so practically harmless, but the asymmetry leaves the contract uneven. Either apply the same split-lock pattern or document why reset is exempt.

• currentPathString (L235) still nests CM → SC. It's synchronized on CM and transitively calls v1SessionCatalog.getCurrentDatabase via currentNamespace. Safe today only because no current code path holds SC and then waits for CM. Worth a one-line doc note that this is the only remaining intentional CM→SC nest, so future changes that introduce SC→CM ordering elsewhere don't unintentionally resurrect the deadlock involving CURRENT_PATH() readers.

• Stale references in SessionCatalog. The comments at SessionCatalog.scala:2574–2578 and :2731–2734 cite setCurrentNamespace as the deadlock counter-party. Post-PR that exact scenario no longer exists. The non-synchronization is still defensible as a forward guard; one option is to refresh the comments to reflect the new invariant (e.g., "kept non-synchronized so the SPARK-56939 CM↔SC ordering invariant survives") rather than describing the now-removed deadlock.

[cloud-fan]

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.

[cloud-fan]

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.

[cloud-fan]

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.

[cloud-fan]

Minor wording: "likely returned" reads ambiguously (it can be parsed transitively). Suggest "likely regressed" — and the same change at L1045.

Suggested change

	"CatalogManager likely returned (SPARK-56939).")
	"CatalogManager likely regressed (SPARK-56939).")

[cloud-fan]

Same as above:

Suggested change

	"CatalogManager likely returned (SPARK-56939).")
	"CatalogManager likely regressed (SPARK-56939).")