Skip to content

perf: cache Transaction.table_metadata between reads - #3784

Open
devseunggwan wants to merge 3 commits into
apache:mainfrom
devseunggwan:perf/cache-transaction-table-metadata
Open

perf: cache Transaction.table_metadata between reads#3784
devseunggwan wants to merge 3 commits into
apache:mainfrom
devseunggwan:perf/cache-transaction-table-metadata

Conversation

@devseunggwan

@devseunggwan devseunggwan commented Aug 13, 2026

Copy link
Copy Markdown

Rationale for this change

Transaction.table_metadata replays every staged update through update_table_metadata, which ends in model_copy(deep=True). So a single read deep-copies the whole metadata object, snapshot list included, and its cost tracks table history rather than the work being done. Callers read the property many times per operation.

#2674 and #3301 hoisted repeated reads out of loops in snapshot.py. This goes after the same cost at the source: repeated reads of an unchanged transaction state now recompute once.

This carries forward #3302 by @rynewang, which @Fokko approved back in April and which the stale bot then closed for inactivity. Its branch is on a fork with maintainerCanModify off so it can't be reopened from outside, hence a new PR; authorship is kept via Co-authored-by.

Numbers

We hit this in production. A writer that appends 1–6 rows at a time went from a mean of 2495 ms to 10302 ms when we upgraded 0.9.1 → 0.11.1 — same payloads, but the tables it writes to have long snapshot histories. With this cache it came back to 2856 ms. (10-minute bucket means over matched windows, n = 41–75 per bucket.)

Isolated against a local SqlCatalog, 20 timed appends per depth, 3 runs with the arms alternated, median:

snapshots without with
0 7.7 ms 4.3 ms 1.8×
100 42.4 ms 13.2 ms 3.2×
300 104.4 ms 24.3 ms 4.3×
500 173.4 ms 36.2 ms 4.8×

update_table_metadata calls per append drop from 22 to 2 at every depth, and the resulting table is identical (rows, snapshots, schema). The multiplier grows with depth because each remaining recompute still copies a longer snapshot list — not because the cache does more work on deep histories. For the same reason the curve doesn't flatten: 4.3 ms at depth 0 against 36.2 ms at depth 500.

On the simpler alternative from #3302

@geruh suggested if not self._updates: return self._table.metadata rather than a cache. That covers a bare append but misses the expensive case — CreateTableTransaction._initial_changes() seeds _updates with ~10 entries before any write, so it's never empty for the snapshot producer's lifetime. There's a test for that case now (test_transaction_table_metadata_cached_with_updates_already_staged) so it doesn't have to rest on an argument.

Concurrency

The property does get read from worker threads — _SnapshotProducer._manifests() submits _write_added_manifest and _write_delete_manifest to the shared executor, and both reach it. A race there is harmless: two threads can both miss and both compute, the loser's result is dropped, and the entry is an immutable tuple assigned in a single store, so there's no half-built state to observe. If anything this narrows an existing gap, since without the cache each thread re-stamps last_updated_ms on its own and concurrent readers can already see metadata that differs.

Are these changes tested?

Two new tests in tests/table/test_init.py, covering repeated reads and the already-staged case above. Both fail with the property reverted — I checked, rather than assuming they discriminate.

One existing test needed rework. test_snapshot_producer_bounded_metadata_access from #3301 asserts that _MergeAppendFiles.__init__ makes exactly one more update_table_metadata call than its superclass. That count is a proxy for how many times the property is read, and the cache breaks the proxy — repeated reads collapse into one recompute, so an un-hoisting would slip through unnoticed.

Rather than loosen the assertion I switched the oracle to count property reads directly, which is what hoisting actually removes and which the cache doesn't affect. The original == 1 stands. It passes with the cache and against main without it, and putting __init__ back to three separate reads fails it.

Locally: make test passes (3931 passed, 3 skipped), ruff check is clean, and mypy reports nothing on pyiceberg/table/__init__.py that it doesn't already report on main.

Are there any user-facing changes?

One: last_updated_ms on the returned metadata is now stable across repeated reads of the same logical state instead of being re-stamped with now() on every access. The timestamp written at commit time is unchanged. Nothing reads it expecting a fresh value per access — outside table/metadata.py and the commit path, the only consumer is cli/output.py, which prints it.

devseunggwan and others added 3 commits August 13, 2026 19:32
`Transaction.table_metadata` replays every staged update through
`update_table_metadata`, whose last step is `model_copy(deep=True)`. The cost of
a single read therefore scales with the size of the metadata -- the snapshot
list in particular -- and callers read the property many times per operation.

Cache the result keyed on the identity of its two inputs. `_updates` is a tuple,
so every `+=` rebinds it to a new object, and `Table.metadata` is replaced
wholesale on refresh and commit; identity equality on both is therefore
sufficient for invalidation without any explicit cache-clearing at mutation
sites.

Carries forward apache#3302 by Ruiyang Wang, which was approved and then closed by the
stale bot. That PR predates apache#3301, whose
`test_snapshot_producer_bounded_metadata_access` pins the hoisted access count
with an equality assertion; the cache absorbs that access too, so the assertion
is relaxed to an upper bound.

Co-authored-by: Ruiyang Wang <rynewang@users.noreply.github.com>
Six lines of rationale was the only multi-line comment in the file outside the
license header; the surrounding style is single-line. The trade-off it described
is in the PR description.
The guard asserted that _MergeAppendFiles.__init__ triggers exactly one more
update_table_metadata call than its superclass. Recompute count is a proxy for
read count that the cache breaks: repeated reads of an unchanged state collapse
to one recompute, so an un-hoisting became invisible and the assertion had to be
relaxed to an upper bound.

Counting property reads directly restores the original assertion and makes the
guard orthogonal to caching. Verified both ways: it passes with and without the
cache, and un-hoisting __init__ back to three separate reads fails it (8 - 5).

Confidence: high
Not-tested: only the _MergeAppendFiles path was mutation-probed; the _summary()
assertions were left as-is beyond the oracle swap.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant