Skip to content

prolly: plug remaining TableEntry zName leaks across call sites#464

Merged
timsehn merged 1 commit intomasterfrom
fix/catalog-zname-leak-remaining
Apr 15, 2026
Merged

prolly: plug remaining TableEntry zName leaks across call sites#464
timsehn merged 1 commit intomasterfrom
fix/catalog-zname-leak-remaining

Conversation

@timsehn
Copy link
Copy Markdown
Collaborator

@timsehn timsehn commented Apr 15, 2026

Follow-up to #461. Applies doltliteFreeCatalog to the remaining call sites of doltliteLoadCatalog that had the same "free the array backbone but not the per-entry zName" leak.

Two shapes of caller

(A) Read-only consumers — just walk the returned array and free at the end. Mechanical replacement of sqlite3_free(aX) with doltliteFreeCatalog(aX, nX):

  • doltlite_at.c, doltlite_history.c, doltlite_ref.c
  • doltlite_status.c (statusFilter)
  • doltlite_diff.c (collectSummaryForWorking + collectSummaryForCommit)
  • doltlite_diff_table.c (loadTblRootAtCommit)
  • doltlite_diff_stat.c (dsComputeTableStats, dsCollectTableNames, dssFilter)
  • doltlite_schema_diff.c, doltlite_schema_merge.c
  • doltlite_blame.c, doltlite_dbpage.c

(B) Shallow-copy consumers — move TableEntry values between arrays (the source transfers zName ownership non-explicitly). Rewritten as dup-on-copy so each array has independent ownership and can be freed with doltliteFreeCatalog:

  • doltlite_branch.c checkout-paths branch (aWorkingaSource append / overwrite / memmove-delete)
  • doltlite.c doltliteAddFunc explicit-name path (aStagedaWorking)
  • doltlite.c doltliteCommitFunc addModifiedOnly path (aStagedaWorking)
  • doltlite.c doltliteResetFunc path-branch (aStagedaHead)
  • doltlite.c doltliteResetFunc --hard untracked-preserve (aWorkingaTarget in-place overwrite)

Deliberately left alone

doltlite_merge.c's mergeCatalogPass1 / Pass2 build aMerged with a mix of shallow-copies from aOurs / aTheirs / aAnc (borrowing zName) and fresh sqlite3_mprintf allocations at line 1259 in the i<nTheirs loop. Mixed ownership makes any naive free strategy wrong:

  • doltliteFreeCatalog on the sources double-frees the aliased entries
  • plain sqlite3_free on aMerged leaks the fresh ones

Proper fix requires restructuring pass1/pass2 to always dup into aMerged — that's a bigger surgery and is left for a follow-up. First attempt at treating merge.c the same way as the others produced SIGABRT under doltlite_reset.sh's merge-conflict scenario, confirming the hazard. Reverted the merge change for this PR.

Test plan

  • make doltlite — clean
  • bash test/run_doltlite_tests.sh — 39/39 suites
  • bash test/vc_oracle_add_test.sh — 16/16
  • bash test/vc_oracle_status_test.sh — 11/11
  • bash test/vc_oracle_branch_test.sh — 16/16
  • bash test/vc_oracle_reset_test.sh — 15/15

🤖 Generated with Claude Code

@timsehn timsehn force-pushed the fix/catalog-zname-leak-remaining branch from 5b7a250 to 7f64c1a Compare April 15, 2026 01:46
Follow-up to #461, which introduced doltliteFreeCatalog and fixed
the dolt_add -A path. Applying the helper across the rest of the
doltliteLoadCatalog call sites that had the same class of leak:
free the array backbone but not the per-entry zName strings.

Two shapes of caller:

1) Read-only consumers that just walk the returned array and free
   it at the end. Mechanical replacement of sqlite3_free(aX) with
   doltliteFreeCatalog(aX, nX):
     - doltlite_at.c
     - doltlite_history.c
     - doltlite_ref.c (all three frees)
     - doltlite_status.c (statusFilter: aHead/aStaged/aWorking)
     - doltlite_diff.c (collectSummaryForWorking + collectSummaryForCommit)
     - doltlite_diff_table.c (loadTblRootAtCommit)
     - doltlite_diff_stat.c (dsComputeTableStats, dsCollectTableNames, dssFilter)
     - doltlite_schema_diff.c (loadSchemaFromCatalog + sdFilter)
     - doltlite_schema_merge.c (applySchemaMergeActions)
     - doltlite_blame.c (loadTableRoot)
     - doltlite_dbpage.c (dbpage page-1 header synth)

2) Shallow-copy consumers that move TableEntry values between
   arrays (the source transfers zName ownership non-explicitly).
   Rewritten as dup-on-copy so each array has independent
   ownership and can be freed with doltliteFreeCatalog:
     - doltlite_branch.c (checkout-paths branch: aWorking <- aSource
       append, in-place overwrite, memmove-delete)
     - doltlite.c doltliteAddFunc explicit-name path: aStaged <-
       aWorking with append, overwrite, and memmove-delete
     - doltlite.c doltliteCommitFunc addModifiedOnly: aStaged <-
       aWorking with append, overwrite, and memmove-delete
     - doltlite.c doltliteResetFunc path branch: aStaged <- aHead
       with append, overwrite, and memmove-delete
     - doltlite.c doltliteResetFunc --hard untracked-preserve:
       aWorking <- aTarget in-place overwrite

Deliberately LEFT ALONE: doltlite_merge.c's mergeCatalogPass1 /
Pass2 build aMerged with a mix of shallow-copies from aOurs /
aTheirs / aAnc (borrowing zName) and fresh sqlite3_mprintf
allocations (line 1259 in the i<nTheirs loop). aMerged's mixed
ownership makes any free strategy wrong — doltliteFreeCatalog on
the sources double-frees the aliased entries, plain free leaks
the fresh ones. Proper fix requires restructuring pass1/pass2 to
always dup into aMerged; that's a bigger surgery and is left for
a follow-up. First attempt at treating merge.c the same way as
the others produced SIGABRT under doltlite_reset.sh's merge
conflict scenario, confirming the hazard.

Full regression green: 39/39 doltlite suites, 16/16 add oracle,
11/11 status oracle, 16/16 branch oracle, 15/15 reset oracle.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@timsehn timsehn force-pushed the fix/catalog-zname-leak-remaining branch from 7f64c1a to 52dfe5b Compare April 15, 2026 02:01
@github-actions
Copy link
Copy Markdown

Sysbench-Style Benchmark: Doltlite vs SQLite

In-Memory

Reads

Test SQLite (us) Doltlite (us) Multiplier
oltp_point_select 69,024 62,016 0.90
oltp_range_select 44,992 41,984 0.93
oltp_sum_range 13,024 17,024 1.31
oltp_order_range 4,992 6,048 1.21
oltp_distinct_range 6,016 8,000 1.33
oltp_index_scan 7,008 10,016 1.43
select_random_points 24,992 29,024 1.16
select_random_ranges 8,960 20,000 2.23
covering_index_scan 12,000 23,008 1.92
groupby_scan 56,000 63,968 1.14
index_join 6,016 8,992 1.49
index_join_scan 3,008 6,016 2.00
types_table_scan 10,976 12,992 1.18
table_scan 1,984 1,952 0.98
oltp_read_only 220,000 248,000 1.13

Writes

Test SQLite (us) Doltlite (us) Multiplier
oltp_bulk_insert 32,000 40,992 1.28
oltp_insert 19,968 31,968 1.60
oltp_update_index 48,064 84,992 1.77
oltp_update_non_index 39,008 56,000 1.44
oltp_delete_insert 44,992 64,992 1.44
oltp_write_only 22,016 30,976 1.41
types_delete_insert 29,024 32,000 1.10
oltp_read_write 132,000 219,040 1.66

File-Backed

Reads

Test SQLite (us) Doltlite (us) Multiplier
oltp_point_select 185,984 92,000 0.49
oltp_range_select 65,984 79,008 1.20
oltp_sum_range 22,016 21,984 1.00
oltp_order_range 6,976 6,016 0.86
oltp_distinct_range 7,008 12,000 1.71
oltp_index_scan 15,008 12,992 0.87
select_random_points 33,984 33,984 1.00
select_random_ranges 17,984 12,992 0.72
covering_index_scan 21,984 28,000 1.27
groupby_scan 54,976 64,000 1.16
index_join 10,976 10,976 1.00
index_join_scan 5,984 5,024 0.84
types_table_scan 13,984 14,016 1.00
table_scan 992 2,016 2.03
oltp_read_only 530,016 320,960 0.61

Writes

Test SQLite (us) Doltlite (us) Multiplier
oltp_bulk_insert 30,976 40,992 1.32
oltp_insert 22,016 40,032 1.82
oltp_update_index 48,992 90,016 1.84
oltp_update_non_index 36,000 58,976 1.64
oltp_delete_insert 45,984 73,024 1.59
oltp_write_only 25,024 35,968 1.44
types_delete_insert 27,008 32,992 1.22
oltp_read_write 127,008 266,976 2.10

10000 rows, single CLI invocation per test, workload-only timing via SQL timestamps.

Performance Ceiling Check (3x)

All tests within 3x ceiling.

@timsehn timsehn merged commit b6881e5 into master Apr 15, 2026
4 checks passed
timsehn added a commit that referenced this pull request Apr 15, 2026
detect_leaks=0 was set because the stock sqlite3 shell trips the
short-lived-leak detector on process exit — not a doltlite bug,
but loud enough to hide real leaks in our CI signal. Now that
#461 and #464 plugged the TableEntry zName leaks across every
catalog caller, we want that signal back so the next leak gets
caught at PR time.

Enable detect_leaks=1 and wire a narrow LSAN_OPTIONS suppressions
file at .github/lsan.supp that filters only the specific stock
sqlite allocations that leak by design (shell input buffer,
builtin test extensions). Every other allocation gets full
coverage — if CI flags a leak, it's either in doltlite or in a
path we own.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@timsehn timsehn mentioned this pull request Apr 15, 2026
1 task
timsehn added a commit that referenced this pull request Apr 15, 2026
detect_leaks=0 was set because the stock sqlite3 shell trips the
short-lived-leak detector on process exit — not a doltlite bug,
but loud enough to hide real leaks in our CI signal. Now that
#461 and #464 plugged the TableEntry zName leaks across every
catalog caller, we want that signal back so the next leak gets
caught at PR time.

Enable detect_leaks=1 and wire a narrow LSAN_OPTIONS suppressions
file at .github/lsan.supp that filters only the specific stock
sqlite allocations that leak by design (shell input buffer,
builtin test extensions). Every other allocation gets full
coverage — if CI flags a leak, it's either in doltlite or in a
path we own.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@timsehn timsehn deleted the fix/catalog-zname-leak-remaining branch April 17, 2026 18:39
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