Add commit-loop instrumentation and ducklake_commit_stats() - #35
Conversation
* Bump DuckDB submodule to v1.5.5 Moves the pin from v1.5.3 (14eca11b) to v1.5.5 (d8cdaa33). No DuckLake source changes were needed - the extension builds clean against v1.5.5 and the full suite passes: 16083 assertions in 451 test cases. Note this does NOT bring in VARIANT extract pushdown. That lives only on duckdb main; the v1.5-variegata branch is 20 commits past v1.5.5 and still ships the pre-rewrite reader (variant_shredded_conversion.cpp, no IsPushdownExtract), so upstream is not backporting it to the 1.5 line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR * Build against v1.5.5 in CI, not just locally The submodule bump alone was not enough. MainDistributionPipeline's get-duckdb-version job reads .github/duckdb-version and passes it to _extension_distribution.yml, which checks out that DuckDB version itself rather than using the submodule. So CI - including the tagged release build that produces ducklake-linux-{amd64,arm64}.duckdb_extension - would still have compiled against v1.5.3 while the submodule said v1.5.5. Local `make release` uses the submodule and was genuinely built and tested against v1.5.5; only the CI path was wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR * Port upstream DuckDB-test skips for v1.5.5 Bumping to v1.5.5 pulls in DuckDB's newer test suite, and the DBMS Catalog Tests job runs that suite against a DuckLake backend. 11 of its 4087 cases failed - 10 of them tests that do not exist at v1.5.3 - so these are newly-added upstream conformance tests exposing long-standing DuckLake gaps, not regressions from the bump. Upstream ducklake already made exactly these calls when it moved to duckdb main (see "Fix and skip duckdb tests in ducklake", which added 136 lines to this file). Every entry here is ported verbatim from upstream main's attach_ducklake.json, reasons included, so we are not inventing policy: - native compression/storage-file internals (patas, rle, dict_fsst) - native db-file invalidation, checkpoint and WAL-revert semantics - DuckLake gives table entries a new object id on ALTER, so the oid stability assertion cannot hold - parquet writer does not support non-root VARIANT columns - TIMETZ second-precision offsets are quantized by parquet storage - two with upstream's own FIXME/"different error message" notes test_add_col_if_not_exists_default is covered by upstream's skip_error_messages entry rather than a path skip, so that string is added too. Note the diff is larger than the change: json round-trip normalised pre-existing inconsistent indentation. Semantically this is exactly +10 skip paths and +1 skip_error_message, nothing removed or altered - verified by comparing the parsed JSON before and after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Consolidates the two release lines onto posthog/v1.5.5 as the single branch for the DuckDB v1.5.5 era. Both branches already pinned the same DuckDB submodule commit (d8cdaa33); this merge brings over the .github/duckdb-version bump and attach_ducklake.json config updates that landed via #33 on the v1.5.3 branch, alongside the v1.5.5 CI and test-skip commits unique to this branch. After this merges, PRs should target posthog/v1.5.5 and the next release tag (v1.0-posthog.7) cuts from here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fork/posthog/v1.5.3: Bump DuckDB submodule to v1.5.5 (#33)
fuziontech
left a comment
There was a problem hiding this comment.
Adversarial self-review (requested by James). Verified against the full diff and a local build of the branch.
Verified correct:
RetryOnErrorsemantics are provably unchanged:ClassifyCommitErroruses the identical lowercased substring checks in the identical order (primary key→unique→conflict→concurrent), andIsRetryableCauseaccepts exactly the same set. The split of the original combinedprimary key || uniquecheck into two ordered buckets cannot change the boolean outcome.- Counter placement:
attemptsper iteration;backoff_msincremented immediately before the actualsleep_forinside the same#ifndef DUCKDB_NO_THREADSguard (no phantom backoff when threads are disabled);total_commit_msvia RAII covers every exit path including throws;RecordConflict(NONE)is a no-op so non-retryable errors don't pollute the conflict buckets. - Table function paginates via
offset+STANDARD_VECTOR_SIZE, so >2048 rows (200+ catalogs) works; returns zero rows with nothing attached. - Registry:
unique_ptrmap values keep handed-out references stable across rehashes; atomics make the hot path lock-free (lock only on first-touch of a catalog and inSnapshot()).
Notes for reviewers (known, accepted):
- Uncounted abort bucket: an error that is retryable-by-cause but aborts because
can_retry == false(conflict-check-phase failure) before max retries incrementsattempts+ a conflict bucket but neitherretries_exhaustednornonretryable_errors. Consequence: failed commits ≠ exhausted + nonretryable. If reconciliation againstxact_rollbackneeds exactness, a fifth stat (aborted_conflict_check) can be added later without breaking the contract — the duckgres consumer (PostHog/duckgres#1012) deliberately passes unknownconflicts.*causes through and drops unknown non-conflict stats, so it's forward-compatible. Snapshot()"consistent" is per-counter, not cross-counter: it holds the registry lock but concurrent commits can advance counters between individualload()s, so a snapshot can momentarily show e.g.attemptsincremented without itssuccesses. Harmless for monotonic counters + rate() math; just don't build invariant checks on exact cross-stat equality at a single instant (the sqllogictest correctly uses>=).- Label collision: two DuckDB instances in one process attaching different lakes under the same alias merge into one label (the registry is process-global by design). Not a concern for duckgres/millpond topologies (one lake per process today).
CI note: the heavy suites were also run locally pre-push (basic 39, functions 96, concurrent 277, transaction 260) — all pass.
Adds a process-global, mutex-guarded registry of per-catalog commit-loop
counters (all cumulative and monotonic per process):
- attempts: one per commit-loop iteration
- successes: commit loop finished successfully
- retries_exhausted: gave up after max retries on a retryable error
- nonretryable_errors: errors where RetryOnError returned false
- backoff_ms: total ms slept in retry backoff
- total_commit_ms: total wall-clock ms inside the commit retry loop
- conflicts.{primary_key,unique,conflict,concurrent}: conflicts by cause,
classified with the same substring buckets RetryOnError uses
The classification lives in a single helper
(DuckLakeCommitStatsRegistry::ClassifyCommitError) that now also backs
DuckLakeTransaction::RetryOnError - retry semantics are unchanged.
Counters are labeled by the DuckLake catalog's attached name for
client-side commits and by the metadata schema name for server-side
(ducklake_commit) commits, falling back to "default" when unset.
New table function ducklake_commit_stats() exposes the registry as
(catalog VARCHAR, stat VARCHAR, value BIGINT) rows - one row per
(catalog, stat) - and is callable without an attached DuckLake catalog
(returns zero rows before any commit). This schema is a contract shared
with a duckgres-side consumer.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b0acf34 to
87f29f2
Compare
|
CI caught a real config-coverage gap in the test (not in the instrumentation): the "Postgres/SQLite" lane also runs the quack RPC server config, where data-only commits execute their commit loop in the server process — so the client-side registry legitimately doesn't count the INSERT, and Fixed in 87f29f2 by making the test config-agnostic: thresholds anchor on DDL commits (always client-side, even on quack) with a NOTE comment explaining why, and the monotonicity probe now uses a second |
Summary
Metrics-only instrumentation of the commit retry loop — the first step of the collision-observability plan. Production context: megaduck's metadata store shows a 12.4% transaction rollback rate while every existing metric reads ~0, because the extension's internal retry loop absorbs collisions invisibly (duckgres only sees conflicts that escape the loop, and millpond exports nothing). This PR makes the loop observable from inside.
No behavior changes:
RetryOnErrornow delegates to the newClassifyCommitErrorhelper using the exact same substring checks in the same order (verified by the passing concurrent/transaction suites — 537 assertions); all counters are additive.What's added
DuckLakeCommitStatsRegistry— process-global, per-catalog counters (std::atomic, mutex-guarded map): attempts, successes, retries_exhausted, nonretryable_errors, backoff_ms, total_commit_ms (RAII timer, all exit paths), and per-cause conflicts.ducklake_commit_stats()table function →(catalog VARCHAR, stat VARCHAR, value BIGINT), one row per (catalog, stat). Stat names:attempts,successes,retries_exhausted,nonretryable_errors,backoff_ms,total_commit_ms,conflicts.{primary_key,unique,conflict,concurrent}. This schema is a contract consumed by feat(duckdbservice): export ducklake commit-loop stats from ducklake_commit_stats() duckgres#1012 (worker-side Prometheus re-export, already merged-ready and dormant until an extension binary ships this function).ducklake_commit()uses the metadata schema name; fallbackdefault.Known edge case
An error that is retryable-by-cause but aborts because the conflict-check phase failed (
can_retry == falsebefore max retries) counts toward attempts and its conflict bucket but neitherretries_exhaustednornonretryable_errors— it fits neither definition. Documented rather than invented a fifth counter; can revisit if it shows up in practice.Test plan
GEN=ninja make releasecleantest/sql/functions/ducklake_commit_stats.test(12 assertions): counters after attach/create/insert, zero rows with no ducklake attachedducklake_basic(39),functions/*(96),concurrent/*(277),transaction/*(260, 1 skip needing httpfs) — all passStacked on #34 (branch includes the consolidation merge; diff vs
posthog/v1.5.5shows only this PR's changes). Follow-ups: millpond poller (separate repo — the writer where megaduck's numbers will actually appear), then tagv1.0-posthog.7+ duckgresDUCKLAKE_EXTENSION_TAGbump to ship it.🤖 Generated with Claude Code
https://claude.ai/code/session_01LUnQHp46rmLKdupmDUr7kG