B-tree leaf page merge (GSoC 2026, Salma El-Sayed) — review summary
Review of the B-tree index-bloat-reduction / leaf-page-merge ("MERGE_AWAY") GSoC 2026 work, done during the 2026-07-29 Hacking Postgres session.
Where the code is — the branch usually cited, salmaaliia/postgres@tooling, is only the April proposal tooling (pageinspect candidate-finder, no core changes). The real work lives on:
salma/merge — WIP, full history (~25 commits)salma/merge-patch-v1.2 — a cleaned 3-patch series (created 2026-07-29, rebased onto upstream): nbtree core (+1376), pageinspect (+857), amcheck (+155). This is what the file:line refs below are on.
What's actually implemented (single-backend happy path — coherent, better than typical GSoC midpoint)
- Merge operation in new
nbtmerge.c: candidate scan → re-verify under exclusive locks on L/R/parent → downlink redirect + delete → flag set in a critical section.
- Page states
BTP_MERGED (R) / BTP_MERGED_AWAY (L tombstone); L truncated + BTMergedAwayPageData { safemergexid }, mirroring BTDeletedPageData. Full state machine merged-away → half-dead → deleted → unlinked driven by VACUUM.
- Scan recovery: forward and backward dedup via a saved sorted TID array +
bsearch subtraction.
- amcheck + pageinspect understand the merged-away state. Partial-VACUUM-cleanup resume exists.
Top 3 issues to raise at -hackers
- No WAL, at all. Two explicit
TODO: WAL admissions inside critical sections that dirty buffers with no XLogInsert. Not crash-safe, no standby story — and worse, the final stage reuses _bt_pagedel, which is WAL-logged, so a standby would replay an unlink of a merge it never saw. Replication is broken by construction today. Until xl_btree_merge (+ redo + standby-conflict handling) exists, the rest can't be meaningfully judged.
- Merge-group link stored in
pd_prune_xid, not special-space spare bytes (a drift from the design pitch). Already caused one silent-TID-loss bug through _bt_split (fixed 07-29); needs manual propagation in every page-rewrite path (split, dedup, …) and hijacks a field other code assumes is zero on index pages. Needs a real on-disk-format / BTREE_VERSION / pg_upgrade argument.
- Zero tests + parallel scans unhandled + build red. Injection points are wired in but nothing consumes them — no isolation spec, TAP, or regress case in any commit. Parallel scans will hang (missing
_bt_parallel_release in the tombstone path) or return duplicates (per-worker filter). nbtmerge.c is absent from src/backend/access/nbtree/meson.build, so CI is red on arrival.
Andrey's ~8KB question (answered)
ItemPointerData savedMergeTids[MaxTIDsPerBTreePage] (~8.1 KB) is embedded in BTScanOpaqueData and palloc'd in btbeginscan. So it is once-per-scan, not per-recursive-call — but it is not lazy: every scan on every index pays it even if no merge is ever encountered. Trivial fix (pointer, palloc on first recovery). It's also placed after the /* keep these last for efficiency */ comment, violating that layout rule.
Other committability notes
- Hardcoded
strcmp(RelationGetRelationName(rel), "merge_test_idx") == 0 && blkno == 4 gate on the injection points, executed on every page step of every B-tree scan in the system — and it's exactly the "expensive predicate before the cheap one" pattern (the strcmp runs before the direction/blkno checks). Must go.
- Missing PGDG copyright header in
nbtmerge.c; duplicate #include; debug elog(LOG, "BTREE_MERGE_TRACE…") left on the merge branch; the three new pageinspect functions are declared PARALLEL SAFE though bt_merge() writes pages.
- The merge trigger currently lives in pageinspect (an inspection extension doing writes) — belongs in VACUUM or a dedicated maintenance function.
bt_merge() runs under only AccessShareLock yet the VACUUM cleanup assumes a single structural modifier → should be ShareUpdateExclusive at minimum.
Bottom line: genuine, thought-through single-backend prototype (partial-cleanup resume, group validation, amcheck invariants) — but a prototype: no WAL, no tests, no parallel-scan support, test hooks in the hot path. GSoC "success" is reasonably: get it to a committable-shaped conversation on invariants + design, not necessarily committed by November.
Generated during the live session via a Fable review agent over the actual branches.
B-tree leaf page merge (GSoC 2026, Salma El-Sayed) — review summary
Review of the B-tree index-bloat-reduction / leaf-page-merge ("MERGE_AWAY") GSoC 2026 work, done during the 2026-07-29 Hacking Postgres session.
Where the code is — the branch usually cited,
salmaaliia/postgres@tooling, is only the April proposal tooling (pageinspect candidate-finder, no core changes). The real work lives on:salma/merge— WIP, full history (~25 commits)salma/merge-patch-v1.2— a cleaned 3-patch series (created 2026-07-29, rebased onto upstream): nbtree core (+1376), pageinspect (+857), amcheck (+155). This is what the file:line refs below are on.What's actually implemented (single-backend happy path — coherent, better than typical GSoC midpoint)
nbtmerge.c: candidate scan → re-verify under exclusive locks on L/R/parent → downlink redirect + delete → flag set in a critical section.BTP_MERGED(R) /BTP_MERGED_AWAY(L tombstone); L truncated +BTMergedAwayPageData { safemergexid }, mirroringBTDeletedPageData. Full state machine merged-away → half-dead → deleted → unlinked driven by VACUUM.bsearchsubtraction.Top 3 issues to raise at -hackers
TODO: WALadmissions inside critical sections that dirty buffers with noXLogInsert. Not crash-safe, no standby story — and worse, the final stage reuses_bt_pagedel, which is WAL-logged, so a standby would replay an unlink of a merge it never saw. Replication is broken by construction today. Untilxl_btree_merge(+ redo + standby-conflict handling) exists, the rest can't be meaningfully judged.pd_prune_xid, not special-space spare bytes (a drift from the design pitch). Already caused one silent-TID-loss bug through_bt_split(fixed 07-29); needs manual propagation in every page-rewrite path (split, dedup, …) and hijacks a field other code assumes is zero on index pages. Needs a real on-disk-format /BTREE_VERSION/ pg_upgrade argument._bt_parallel_releasein the tombstone path) or return duplicates (per-worker filter).nbtmerge.cis absent fromsrc/backend/access/nbtree/meson.build, so CI is red on arrival.Andrey's ~8KB question (answered)
ItemPointerData savedMergeTids[MaxTIDsPerBTreePage](~8.1 KB) is embedded inBTScanOpaqueDataand palloc'd inbtbeginscan. So it is once-per-scan, not per-recursive-call — but it is not lazy: every scan on every index pays it even if no merge is ever encountered. Trivial fix (pointer, palloc on first recovery). It's also placed after the/* keep these last for efficiency */comment, violating that layout rule.Other committability notes
strcmp(RelationGetRelationName(rel), "merge_test_idx") == 0 && blkno == 4gate on the injection points, executed on every page step of every B-tree scan in the system — and it's exactly the "expensive predicate before the cheap one" pattern (the strcmp runs before the direction/blkno checks). Must go.nbtmerge.c; duplicate#include; debugelog(LOG, "BTREE_MERGE_TRACE…")left on themergebranch; the three new pageinspect functions are declaredPARALLEL SAFEthoughbt_merge()writes pages.bt_merge()runs under onlyAccessShareLockyet the VACUUM cleanup assumes a single structural modifier → should be ShareUpdateExclusive at minimum.Bottom line: genuine, thought-through single-backend prototype (partial-cleanup resume, group validation, amcheck invariants) — but a prototype: no WAL, no tests, no parallel-scan support, test hooks in the hot path. GSoC "success" is reasonably: get it to a committable-shaped conversation on invariants + design, not necessarily committed by November.
Generated during the live session via a Fable review agent over the actual branches.