Skip to content

Core: Prevent stale CachingCatalog entries after commit - #17388

Draft
ysung wants to merge 1 commit into
apache:mainfrom
ysung:fix/caching-catalog-race-17338
Draft

Core: Prevent stale CachingCatalog entries after commit#17388
ysung wants to merge 1 commit into
apache:mainfrom
ysung:fix/caching-catalog-race-17338

Conversation

@ysung

@ysung ysung commented Jul 27, 2026

Copy link
Copy Markdown

This fixes #17338, a race in CachingCatalog. If the cache expires while a write is in progress, another thread can reload and cache the old table state just before the write commits. The commit succeeds, but later readers may keep receiving that stale table until the cache expires again.

After a successful commit, CachingCatalog now atomically checks the cached entry for that table. It keeps the exact table that performed the commit—which preserves the stable object identity Spark relies on—but removes a different entry that may have been loaded during the write. Cached metadata tables are kept when they share the committing table's operations and removed when their origin changed; an additional guard prevents an invalidated metadata-table load from publishing stale state afterward. This PR covers BaseTable; specialized implementations such as server-planning RESTTable are handled in follow-up #17401.

Part of #17338


AI Disclosure

  • Model: GPT-5
  • Platform/Tool: Codex
  • Human Oversight: partially reviewed
  • Prompt Summary: Reconcile exact BaseTable cache entries after commit without recursive cache mutation and add deterministic concurrent regression coverage.

@github-actions github-actions Bot added the core label Jul 27, 2026
@ysung
ysung force-pushed the fix/caching-catalog-race-17338 branch 5 times, most recently from 280d84a to 270fd09 Compare July 28, 2026 16:20
@ysung
ysung marked this pull request as ready for review July 28, 2026 16:34
@ysung
ysung force-pushed the fix/caching-catalog-race-17338 branch 2 times, most recently from d5e60b9 to 70d751b Compare July 28, 2026 18:00
@ysung
ysung marked this pull request as draft July 28, 2026 18:55
@ysung
ysung force-pushed the fix/caching-catalog-race-17338 branch from 70d751b to 101c58a Compare July 28, 2026 19:03
@github-actions github-actions Bot added the spark label Jul 28, 2026
@ysung
ysung force-pushed the fix/caching-catalog-race-17338 branch 3 times, most recently from bcebfcd to 4900433 Compare July 28, 2026 22:32
Generated-by: Codex

Co-authored-by: Humzah Kiani <humzah.kiani@affirm.com>

Co-authored-by: Codex (GPT-5) <noreply@openai.com>
@ysung
ysung force-pushed the fix/caching-catalog-race-17338 branch from 4900433 to 06f9a4a Compare July 29, 2026 01:19
@RussellSpitzer

Copy link
Copy Markdown
Member

This PR Description is not human friendly, I would strongly recommend summarizing it down to a paragraph or two to explain the problem and what you are doing to solve it.

@RussellSpitzer

Copy link
Copy Markdown
Member

So the underlying issue here is that the CachingCatalog isn't actually racing, we are intentionally serving bounded-stale results in order to avoid a serializability problem.

The basic problem is this:

  • I have a query that refers to table "X" several times.
  • Spark can load "X" separately for different references to it (self-joins, CTEs reused across executions, a lazily-resolved plan re-touched at execution time).
  • If "X" is at a different state when a subsequent load comes in, you essentially get two different "X" tables in the same query plan.
  • This leads to a broken plan that produces very wrong results.

So the CachingCatalog exists (partly) to avoid this scenario. By guaranteeing that for a certain amount of time (the cache TTL) we ignore changes made by other threads, we avoid ending up with the same table read at multiple points in time within a single query. The guarantee here is bounded staleness, not freshness, and that's a deliberate trade, not a bug. This is a structural problem in Spark that folks are still trying to fix even now.

Because of this, I don't think we can "fix" this the way the PR proposes. Evicting or reconciling the entry on commit reintroduces exactly the two-snapshots-in-one-plan failure the cache exists to suppress; it isn't a narrower or safer variant of invalidate-on-commit. (Retaining the committing instance doesn't help here either, since callers re-read currentSnapshot() on every load.)

For users who understand the risk and want always-fresh reads, they can disable the cache (cache-enabled=false) and have the catalog reload on every access. Notably, the Iceberg source itself and the incremental-query tests already set cache-enabled=false for this reason.

@xndai

xndai commented Jul 29, 2026

Copy link
Copy Markdown
Contributor
If "X" is at a different state when a subsequent load comes in, you essentially get two different "X" tables in the same query plan. This leads to a broken plan that produces very wrong results. 

So the CachingCatalog exists (partly) to avoid this scenario.

I think this is totally a client's responsibility and client's choice. When to refresh "X" completely depends on the transaction semantics provided by the client. Say if client does "Read Committed", then they can refresh any time they want as long as they read the committed snapshots. And if the semantics is "Snapshot Read", then the snapshot "X" needs to remain unchanged for the duration of the transaction. To force a refresh on server side using a TTL seems wrong to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race Condition in CachingCatalog

3 participants