Repository navigation
Conversation
📜 Change history & discussion (Agora / pg.ddx.io)I have enough. The deduplication feature this builds on is commit 0d861bb (Add deduplication to nbtree, PG13). Matthias van de Meent's dynamic prefix truncation is the active related effort but is a different mechanism (search-time skip, not on-disk compression). Suffix truncation (Peter Geoghegan, PG12) is also relevant. I have what I need. 🧵 Related discussion
🔗 Related commits / prior art
📋 Commitfest
🧭 Context for reviewers
Generated by pg-history via the Agora MCP server (pg.ddx.io). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * xl_btree_split flag bits. The left (original) page is restored from a | ||
| * full-page image when it is prefix-compressed, so it needs no flag; the | ||
| * right page is rebuilt from logged items, so redo must know to set | ||
| * BTP_PREFIX on it (its PREFIX item is included in the logged tuples). | ||
| */ |
There was a problem hiding this comment.
This newly-added comment is factually wrong and contradicts the actual redo code. The only reader of XLH_SPLIT_RPREFIX is btree_xlog_split() (nbtxlog.c:291-296), which restores the prefix-compressed right page from a full-page image (it requires BLK_RESTORED and elog(ERROR)s otherwise). It does NOT rebuild the page from logged items, and it never sets BTP_PREFIX explicitly. Correspondingly the primary registers block 1 with REGBUF_STANDARD | REGBUF_FORCE_IMAGE (no REGBUF_WILL_INIT, no logged tuple data) when P_HASPREFIX(ropaque) (nbtinsert.c:2157-2159). So the claims "the right page is rebuilt from logged items" and "its PREFIX item is included in the logged tuples" are both false. The flag's real purpose is to tell redo that block 1 must be restored from its forced FPI (and thus to skip the item-replay path). Please correct the comment to match the code. (high confidence)
| * xl_btree_split flag bits. The left (original) page is restored from a | |
| * full-page image when it is prefix-compressed, so it needs no flag; the | |
| * right page is rebuilt from logged items, so redo must know to set | |
| * BTP_PREFIX on it (its PREFIX item is included in the logged tuples). | |
| */ | |
| * xl_btree_split flag bits. Both halves of a prefix-compressed leaf split | |
| * are restored from full-page images forced on the primary (their coded | |
| * items are laid out in a physical order that _bt_restore_page would not | |
| * reproduce). The left (original) page is block 0, whose FPI redo already | |
| * detects via BLK_RESTORED, so it needs no flag. This flag tells redo that | |
| * block 1 (the right page) must likewise be restored from its forced FPI | |
| * rather than rebuilt from logged items. | |
| */ |
| SELECT | ||
| (SELECT md5(string_agg(u, '|' ORDER BY u)) FROM btree_prefix) = | ||
| (SELECT md5(string_agg(u, '|' ORDER BY u)) FROM btree_prefix WHERE u IS NOT NULL OR u IS NULL) | ||
| AS digest_consistent; |
There was a problem hiding this comment.
This "full differential digest" is tautological and cannot catch any regression. Both subqueries read the same table btree_prefix via the same access path, and the predicate u IS NOT NULL OR u IS NULL is always true, so the two digests are byte-for-byte identical by construction. digest_consistent returns t even if prefix compression is reverted or corrupts data. The comment promises comparing "the prefix index, the uncompressed index, and a sequential scan" but the SQL compares none of those. To actually distinguish the implementations, force each access path explicitly and compare: e.g. digest of a scan forced through btree_prefix_on (prefix) vs one forced through btree_prefix_off (uncompressed) vs a seqscan (enable_indexscan=off), and assert all three agree. As written this test is worthless.
| -- by keys that share a long prefix (URL-like), keys that share almost nothing | ||
| -- (md5), fixed-width zero-padded codes, and oversized (>126-byte) values. | ||
| -- | ||
| CREATE TABLE btree_prefix (u text, v text); |
There was a problem hiding this comment.
Locale/collation portability defect. The text column u is created with the database default collation, so =, BETWEEN, u > ..., ORDER BY u, and string_agg(u, '|' ORDER BY u) all depend on the server's collation. Under a non-C collation the recorded row order in expected/btree_index.out (e.g. the backward-scan output and every digest) will differ, breaking the buildfarm. This same test file already follows the C-collation convention (create temp table btree_bpchar (f1 text collate "C")). Declare the key column as u text COLLATE "C" (and the digest/ORDER BY accordingly) so contents and expected output are deterministic across platforms.
| SELECT 'https://example.com/other/' || lpad(g::text, 8, '0'), md5((-g)::text) | ||
| FROM generate_series(1, 10000) g; | ||
| -- Oversized first-column values (stored with shared = 0) and a NULL. | ||
| INSERT INTO btree_prefix VALUES (repeat('x', 300), 'big'), (NULL, 'nullkey'); |
There was a problem hiding this comment.
Boundary coverage is missing at the one place the encoding actually switches. BTREE_PREFIX_MAX is 126 and BTREE_PREFIX_RAW is 127 (see nbtree.h); the shared-count field is a single uint8 masked to 7 bits, so the coded/raw transition happens around 126/127 shared bytes, not at "300". repeat('x', 300) merely has zero shared prefix with the URL keys (shared = 0), which is a different path from the >126 case the comment claims to exercise. Add keys that share exactly 125/126/127 bytes with the page prefix to cover the boundary where the shared encoding changes behavior.
| ALTER INDEX btree_prefix_off SET (prefix_compression = on); | ||
| SELECT reloptions FROM pg_class WHERE relname = 'btree_prefix_off'; | ||
| -- A non-text (non-varlena) first column silently stores no prefix, but is legal. | ||
| CREATE INDEX btree_prefix_int ON btree_prefix ((length(u))) WITH (prefix_compression = on); |
There was a problem hiding this comment.
The non-varlena index is created immediately before DROP TABLE and never queried, so this only asserts that CREATE INDEX doesn't error. The comment claims the "silently stores no prefix, but is legal" path is covered, but the correctness of scans over that index (the actual behavior being tested) is never exercised. Add at least one query using this index (forced via enable_seqscan=off) and verify it returns the expected rows, or drop the index assertion if it adds no value.
| Prefix split halves, and a deduplication rewrite of a prefix page, are logged | ||
| by full-page image rather than by replaying individual tuples, because the |
There was a problem hiding this comment.
Minor documentation imprecision. The code (nbtinsert.c) forces the left page's full-page image based on whether the pre-split original page is BTP_PREFIX (P_HASPREFIX(origpage)), not based on whether the resulting left half ends up prefix-compressed -- a left half can end up plain yet still be FPI-logged because its source items are coded against the original prefix. The phrase "Prefix split halves ... are logged by full-page image" reads as if only the halves that remain prefix-compressed are FPI'd. Consider clarifying that the left half is FPI-logged whenever the original page was prefix-compressed. Low-confidence/low-severity nit; the overall mechanism is correct.
| SELECT md5(string_agg(u, '|' ORDER BY u)) = | ||
| (SELECT md5(string_agg(u, '|' ORDER BY u)) FROM btree_prefix) AS ios_ok | ||
| FROM (SELECT u FROM btree_prefix ORDER BY u) s; |
There was a problem hiding this comment.
This "index-only scan" check is tautological, same defect as the full differential digest below. Both operands derive from the identical source btree_prefix and are wrapped in string_agg(u, '|' ORDER BY u), which sorts its input, so the two md5 digests are byte-for-byte equal by construction regardless of which access path (prefix index, uncompressed index, or seqscan) each side uses. If key decoding were broken, both sides would return the same wrong digest and ios_ok would still be t. The test cannot catch a regression in the new prefix-decode path. To actually test decoding, compare the prefix index against an independent oracle, e.g. force the IOS over btree_prefix_on and compare its ordered output to the original literal values inserted (or to a seqscan result that provably did not use the index).
| -- by keys that share a long prefix (URL-like), keys that share almost nothing | ||
| -- (md5), fixed-width zero-padded codes, and oversized (>126-byte) values. | ||
| -- | ||
| CREATE TABLE btree_prefix (u text, v text); |
There was a problem hiding this comment.
The second column v is populated by every INSERT (md5 values) but never referenced by any query in this test block. It is dead test data that only adds noise and insert cost. Either drop the column (the index is on u only) or use it, e.g. to exercise a two-column prefix-compressed index.
| #define BTTupleStorageSize(relation) \ | ||
| (BTGetPrefixCompression(relation) ? \ | ||
| (BLCKSZ + (BLCKSZ / (int) MAXALIGN(sizeof(IndexTupleData) + 2)) * \ | ||
| (int) MAXALIGN(BTREE_PREFIX_MAX)) : \ | ||
| BLCKSZ) |
There was a problem hiding this comment.
Memory-safety bug (high confidence): BTTupleStorageSize falls back to plain BLCKSZ whenever BTGetPrefixCompression(relation) is false, but BTP_PREFIX pages are not tied to the current reloption value. Per the README and the code (flags are only cleared on split/empty/dedup-rewrite, never by an ALTER INDEX ... SET (prefix_compression = off)), BTP_PREFIX leaves persist in an index after the option is turned off. _bt_getitem() decodes based on the page flag P_HASPREFIX(opaque), not the reloption, so an index-only scan over a surviving BTP_PREFIX leaf will decode tuples whose combined size can exceed BLCKSZ and overflow so->currTuples/so->markTuples (allocated as half * 2 in btrescan). The sizing must not depend on the live option; size the buffer for the worst case whenever prefix compression is a btree index possibility (e.g. drop the BTGetPrefixCompression gate, or key it on whether the index could ever contain BTP_PREFIX pages).
| parameter on with ALTER INDEX does not rewrite anything; each leaf is converted | ||
| the next time it is written whole, and compressed and uncompressed leaves | ||
| coexist in the same index indefinitely. |
There was a problem hiding this comment.
Documentation gap (moderate confidence): this paragraph only describes turning the parameter on and asserts compressed and uncompressed leaves "coexist in the same index indefinitely", but it never states what happens when the parameter is turned off. The code keeps BTP_PREFIX on existing pages (flags are cleared only on split/empty/dedup-rewrite), so BTP_PREFIX leaves survive an ALTER INDEX ... SET (prefix_compression = off) and remain readable. This is not merely cosmetic: it is exactly the invariant the index-only-scan buffer sizing (BTTupleStorageSize in nbtree.h) must honor. Document that turning the option off also does not rewrite anything, so prefix pages can be read back with the option off.
|
Addressed the review in 6edd15c:
Re-ran on Fedora: regress + isolation + amcheck + pageinspect + pgstattuple all The four larger follow-ups (compose with deduplication, re-enable bottom-up |
There was a problem hiding this comment.
🔍 OCR found 19 issue(s).
- 17 inline, 2 in summary
📄 src/backend/access/nbtree/README
This paragraph describes the opposite of what the code does. A prefix split does NOT full-page-image its halves as the primary mechanism. nbtinsert.c registers block 0 with REGBUF_STANDARD and block 1 with REGBUF_WILL_INIT (not REGBUF_FORCE_IMAGE), and btree_xlog_split_prefix()/the prefix-dedup redo branch explicitly rebuild each coded half from the logged plain items of the pre-split page -- see the code comments "No full-page image is needed" (nbtxlog.c) and "avoiding a full-page image" (nbtxlog.h). A forced FPI is used only as an overflow fallback when the plain payload would exceed BLCKSZ (XLH_SPLIT_RFPI / XLH_DEDUP_RFPI). The README should state that redo reconstructs the coded layout from logged plain items (because _bt_restore_page's item-by-item replay cannot reproduce it), with FPI only as the overflow fallback -- not that FPI is the normal logging mechanism.
📄 src/backend/access/nbtree/README
This claim is inaccurate. A deduplication rewrite of a prefix page is NOT FPI-logged by default. In nbtdedup.c (_bt_dedup_pass), block 0 is registered with REGBUF_STANDARD and the chosen prefix plus the deduped plain items are logged via XLogRegisterBufData so redo re-lays-out the coded page. A forced full-page image (REGBUF_FORCE_IMAGE / XLH_DEDUP_RFPI) is used only as a fallback when the packed plain payload would exceed BLCKSZ (dedup_rfpi, set when off > BLCKSZ). The normal path logs individual (plain) tuples, the opposite of what this sentence states. (high confidence)
| * high key (nbtpage.c's _bt_mark_page_halfdead). Recorded in the vacuum | ||
| * record so redo does the same. |
There was a problem hiding this comment.
The comment is inaccurate: nothing about the PREFIX-item drop is recorded in the vacuum record. xl_btree_vacuum only carries ndeleted/nupdated and the offset/tuple payloads (unchanged by this patch), and btree_xlog_vacuum() re-derives the drop by independently re-evaluating P_HASPREFIX(opaque) && PageGetMaxOffsetNumber(page) == BTPageGetPrefixOff(opaque) after replaying the deletes. Redo does the same thing, but not because it was recorded. Reword to reflect that redo recomputes the same condition from the post-delete page state. (high confidence)
| * high key (nbtpage.c's _bt_mark_page_halfdead). Recorded in the vacuum | |
| * record so redo does the same. | |
| * high key (_bt_mark_page_halfdead). Redo (btree_xlog_vacuum) re-derives | |
| * this from the post-delete page state, so nothing extra is logged. |
| IndexTuple rightitems[MaxIndexTuplesPerPage]; | ||
| int nright = 0; |
There was a problem hiding this comment.
Stack buffer overflow risk during redo. rightitems[] and dummyright[] are fixed-size stack arrays sized to MaxIndexTuplesPerPage, but a prefix-compressed leaf can physically hold MORE line pointers than that limit: MaxIndexTuplesPerPage is derived assuming each tuple is at least MAXALIGN(sizeof(IndexTupleData) + 1) bytes, whereas a coded tuple can be smaller (1-byte varlena header + 1-byte shared code). The primary correctly sizes these arrays dynamically as palloc_array(IndexTuple, maxoff + 2) (see _bt_split in nbtinsert.c). The while (datalen > 0) loop increments nright with no bound check and trusts the WAL record's self-describing IndexTupleSize, so a legitimately overfull (or corrupt) record overruns the stack. Allocate these with palloc sized from the record, matching the primary.
| #define XLH_SPLIT_LPREFIX_ORIG 0x02 /* left half prefix is the orig page's */ | ||
| #define XLH_SPLIT_RPREFIX_ORIG 0x04 /* right half prefix is the orig page's */ |
There was a problem hiding this comment.
Dead scaffolding and misleading documentation. XLH_SPLIT_LPREFIX_ORIG and XLH_SPLIT_RPREFIX_ORIG are defined but never referenced anywhere (neither the primary in nbtinsert.c nor redo in nbtxlog.c reads them). The comment claims prefix bytes are "not logged ... when the *_PREFIX_ORIG flag is set", but the actual code always logs the prefix bytes (XLogRegisterBufData of leftprefix/rightprefix) and redo always reads them from the logged block data. Either the described optimization was never implemented (so remove the two flags and fix the comment), or the implementation is incomplete. As written, the comment describes behavior that does not exist.
| IndexTuple dummyright[MaxIndexTuplesPerPage]; | ||
| int dummynr; |
There was a problem hiding this comment.
Same unbounded fixed-size stack array as the right/left half of the split path: dummyright is MaxIndexTuplesPerPage, but _bt_split_leaf_collect writes *nright entries for a prefix page that can exceed MaxIndexTuplesPerPage. Even though this redo path only needs the left items, _bt_split_leaf_collect unconditionally fills the right array too, so dummyright can overflow. Size it to match the primary (maxoff + 2) via palloc.
| IndexTuple items[MaxIndexTuplesPerPage]; | ||
| int nitems = 0; |
There was a problem hiding this comment.
Same unbounded stack array in the dedup redo path: items[MaxIndexTuplesPerPage] is filled by a while (datalen > 0) loop over trusted WAL data, with nitems++ and no bound check. A prefix-compressed dedup result page can hold more coded items than MaxIndexTuplesPerPage. Allocate from the record size instead of a fixed stack array.
| if (spagenumber != P_NONE) | ||
| { | ||
| right_hikey = (IndexTuple) datapos; | ||
| datapos += MAXALIGN(IndexTupleSize(right_hikey)); | ||
| datalen -= MAXALIGN(IndexTupleSize(right_hikey)); | ||
| } |
There was a problem hiding this comment.
Unaligned IndexTuple access during redo. The primary logs the right half's prefix as rightprefixlen raw bytes with no MAXALIGN padding (see nbtinsert.c: roff += rightprefixlen before copying the high key/items). rightprefixlen is the number of shared first-column bytes from _bt_prefix_choose() and is an arbitrary value, not a multiple of MAXALIGN. Here datapos += xlrec->rightprefixlen then casting to IndexTuple and calling IndexTupleSize(right_hikey) (and likewise for each rightitems[] entry below) dereferences t_info at an unaligned address. On strict-alignment platforms (ARM, s390x, etc.) this is undefined behavior / a crash. Either MAXALIGN-pad the prefix on the primary before the high key/items, or memcpy the tuples into aligned storage in redo before reading them.
| if (xlrec->flags & XLH_DEDUP_NOT_RIGHTMOST) | ||
| { | ||
| hikey = (IndexTuple) datapos; | ||
| datapos += MAXALIGN(IndexTupleSize(hikey)); | ||
| datalen -= MAXALIGN(IndexTupleSize(hikey)); | ||
| } |
There was a problem hiding this comment.
Same unaligned IndexTuple access in the prefix-dedup redo path. The primary packs prefixlen raw (unpadded) prefix bytes before the high key and deduped items (see nbtdedup.c: off = dedup_prefixlen with no MAXALIGN). After datapos += xlrec->prefixlen, casting to IndexTuple and calling IndexTupleSize(hikey) / IndexTupleSize(it) reads t_info at a non-MAXALIGN'd address whenever prefixlen % MAXALIGN != 0, which crashes on strict-alignment architectures. Pad the prefix to MAXALIGN on the primary, or copy tuples into aligned buffers in redo.
| IndexTuple itup; | ||
| BTItemBuf itupbuf; |
There was a problem hiding this comment.
BTItemBuf is PGAlignedBlock, i.e. a full BLCKSZ-sized buffer (8KB by default, up to 32KB). Declaring it as a local in _bt_compare() -- the single hottest routine in nbtree, called O(log n) times per descent and in every page binary search -- inflates the stack frame by BLCKSZ on every call, including internal-page and non-prefix-index comparisons where _bt_getitem() never decodes (BTP_PREFIX is a leaf-only flag). Since &itupbuf is address-taken, the compiler cannot elide it. Consider having the caller supply the scratch buffer (so it is reserved once per descent rather than per comparison), or otherwise avoid paying this cost on the non-prefix fast path. (performance, moderate confidence)
| Size half = BTTupleStorageSize(scan->indexRelation); | ||
|
|
||
| so->currTuples = (char *) palloc(half * 2); | ||
| so->markTuples = so->currTuples + half; |
There was a problem hiding this comment.
This grows the per-scan index-only tuple workspace from BLCKSZ * 2 (16KB) to BTTupleStorageSize * 2, which is roughly 9x larger (~144KB at default BLCKSZ: 8192 + (8192/16)*128 = 73728 per half). This larger allocation is now paid by every index-only scan on every btree index at btrescan time, even indexes that have never used prefix compression. The nbtree.h comment justifies the worst-case sizing (a surviving BTP_PREFIX page can exist on any index), but the allocation is unconditional rather than gated on the index actually being capable of carrying prefix pages. Worth confirming this per-scan memory regression is acceptable, or sizing it based on whether the index can contain BTP_PREFIX pages. (performance, moderate confidence)
|
|
||
| /* xl_btree_dedup flag bits */ | ||
| #define XLH_DEDUP_NOT_RIGHTMOST 0x01 /* prefix payload includes a high key */ | ||
| #define XLH_DEDUP_RFPI 0x02 /* prefix page carried as a forced image */ |
There was a problem hiding this comment.
XLH_DEDUP_RFPI is write-only dead scaffolding. The primary sets it (nbtdedup.c:387) but no code ever reads it: the dedup redo path relies on XLogReadBufferForRedo() returning BLK_RESTORED for the forced full-page image and short-circuiting before it would inspect this flag. Either drop the flag (and stop setting it), or if it is kept purely as documentation, say so explicitly. As written it is a knob with no caller, which the community rejects (YAGNI). Note this is a separate flag from the already-flagged XLH_SPLIT_LPREFIX_ORIG/RPREFIX_ORIG.
| bool pageprefix = P_HASPREFIX(opaque); | ||
| BTItemBuf curbuf; /* decode scratch for the loop's current item */ | ||
| BTItemBuf basebuf; /* decode scratch kept for the pending base */ | ||
| XLogRecPtr recptr; | ||
| IndexTuple *dedup_plain = NULL; /* deduped plain items, for prefix WAL */ |
There was a problem hiding this comment.
This entire prefix-handling block in _bt_dedup_pass is unreachable. The only caller, _bt_delete_or_dedup_one_page() in nbtinsert.c, now guards the call with !P_HASPREFIX(...), so _bt_dedup_pass() is never entered with a BTP_PREFIX page and pageprefix is always false here. That makes all the if (pageprefix) decode branches, the _bt_prefix_compress_page() re-compression block, the dedup_plain/dedup_hikey/dedup_walbuf packing, and the dedup_islogprefix ? 0 : state->nintervals WAL branch dead code (together with the matching XLOG_BTREE_DEDUP prefix-redo path in nbtxlog.c). This is ~150 lines of complex, untested WAL-logging scaffolding for a path that is deliberately not wired up (the nbtinsert.c comment states "Deduplication is not yet applied to prefix-compressed pages"). Drop it until dedup on prefix pages is actually enabled, per YAGNI. (high confidence)
| if (state->btps_prefix == NULL) | ||
| state->btps_prefix = _bt_prefix_candidate(itup, | ||
| &state->btps_prefixlen); |
There was a problem hiding this comment.
state->btps_prefix is set to a pointer into itup (via _bt_prefix_candidate, which returns VARDATA_ANY(val) of the tuple's first column). But itup here comes from tuplesort_getindextuple(), whose returned tuple is only valid until the next fetch on that sortstate. On the next _bt_buildadd() the previous itup's memory has been reused by the tuplesort, so btps_prefix now dangles and the later _bt_prefix_shared(state->btps_prefix, ...) reads stale/garbage bytes, yielding a meaningless shared estimate (and reading memory this code does not own). It does not corrupt the page (the hard fit check and the recompute in _bt_prefix_compress_page protect correctness), but the fillfactor estimate is effectively random. Copy the candidate prefix bytes into stable storage in BTPageState instead of keeping a pointer into the transient tuple. (medium confidence)
| Deduplication and bottom-up index deletion are not currently applied to | ||
| BTP_PREFIX pages (such a page splits instead): their in-place rewrite of a page | ||
| cannot always be re-compressed within one page, and prefix compression already | ||
| reclaims much of the space they target. Composing them with prefix | ||
| compression is future work. |
There was a problem hiding this comment.
The claim that bottom-up index deletion is not applied to BTP_PREFIX pages contradicts the implementation. In nbtinsert.c (_bt_delete_or_dedup_one_page), only the deduplication pass is gated by !P_HASPREFIX(...); the bottom-up deletion pass (_bt_bottomupdel_pass) runs unconditionally, and its own comment states "Bottom-up deletion above is safe on prefix pages -- it only removes items, never rewrites survivors." So bottom-up deletion IS applied to prefix pages; only deduplication is skipped. Reword to scope the "not currently applied / splits instead" statement to deduplication alone.
| With the prefix_compression storage parameter (default off) a leaf page may | ||
| front-code the first key column of each of its data tuples against a per-page | ||
| prefix, which shrinks indexes on keys that share long leading bytes (URLs, |
There was a problem hiding this comment.
This opening description omits that prefix compression applies only when the first key column is of a varlena (variable-length) type. _bt_prefix_enabled() gates on TupleDescAttr(...,0)->attlen == -1, and nbtree.h documents the same requirement ("whose first key column is of a varlena type"). As written, the README implies any first key column is front-coded, which is misleading for an index whose leading column is fixed-length (it is silently never compressed). (moderate confidence)
| SET enable_indexscan = off; | ||
| SET enable_indexonlyscan = off; | ||
| SET enable_seqscan = on; |
There was a problem hiding this comment.
enable_indexonlyscan is set off here but never reset, so the session ends with a non-default GUC. Every other GUC block in this file restores what it changes (see the reset enable_seqscan/indexscan/bitmapscan at the earlier tests), and this block itself resets enable_seqscan/enable_bitmapscan at lines 615-616 and resets enable_indexscan to its default via SET ... = on at line 614. Leaving enable_indexonlyscan permanently off is inconsistent and sloppy. Add RESET enable_indexonlyscan; alongside the other resets (and prefer RESET enable_indexscan; over SET enable_indexscan = on; for consistency with the file's idiom).
| SET enable_indexscan = off; | |
| SET enable_indexonlyscan = off; | |
| SET enable_seqscan = on; | |
| SET enable_indexscan = off; | |
| SET enable_indexonlyscan = off; | |
| SET enable_seqscan = on; |
| * Leaf halves are built as whole pages now, so they can be prefix- | ||
| * compressed from the start (Strategy 1: a page is faulted into the | ||
| * BTP_PREFIX format only when it is written whole). Both halves of a | ||
| * prefix split are restored from full-page images in redo (their coded | ||
| * physical layout would not be reproduced by item replay): the left half | ||
| * (block 0) is FPI-logged whenever the original page was prefix-compressed | ||
| * and redo detects the image via BLK_RESTORED, so it needs no record flag; | ||
| * the right half (block 1) records XLH_SPLIT_RPREFIX so redo restores it | ||
| * from its forced image rather than rebuilding it from logged tuples. | ||
| * dataitem copies and the lefthighkey copy are freed via the short-lived | ||
| * insert context. |
There was a problem hiding this comment.
This comment describes an abandoned FPI-based design and flatly contradicts the code and the redo path. In reality neither half is restored from a full-page image in the normal case: block 0 is registered with plain REGBUF_STANDARD (no forced image) and btree_xlog_split_prefix() rebuilds the left half from the pre-split page on BLK_NEEDS_REDO; the right half is carried as its logged plain items (rightwalbuf) and laid out coded by _bt_split_leaf_layout(), only falling back to an FPI under XLH_SPLIT_RFPI. The flag named here, XLH_SPLIT_RPREFIX, does not exist (the real flags are XLH_SPLIT_LEAFPREFIX and XLH_SPLIT_RFPI; XLH_SPLIT_RPREFIX_ORIG is defined but unused). This stale comment will badly mislead anyone auditing WAL/redo correctness and should be rewritten to match the tuple-replay design actually implemented (and correctly described near the XLogInsert call below).
|
📊 OCR posted 16/17 inline comment(s). 1 could not be posted
|
There was a problem hiding this comment.
🔍 OCR found 23 issue(s).
- 22 inline, 1 in summary
📄 src/backend/access/nbtree/README
This WAL paragraph contradicts the shipped implementation; it describes a design that was not the one committed. The code does NOT use a full-page image as the primary redo mechanism for prefix splits/dedup:
- Left half (block 0) is registered with plain REGBUF_STANDARD (nbtinsert.c _bt_split, XLogRegisterBuffer(0, buf, REGBUF_STANDARD)) -- there is no forced FPI. Redo rebuilds the left half from the original page's items via _bt_split_leaf_collect + _bt_split_leaf_layout (btree_xlog_split, nbtxlog.c).
- Right half (block 1) is registered REGBUF_WILL_INIT and rebuilt from logged PLAIN items; REGBUF_FORCE_IMAGE is used only in the XLH_SPLIT_RFPI overflow fallback, not 'whenever it ends up prefix-compressed'.
- A dedup rewrite of a prefix page is likewise carried as the prefix + plain items (xl_btree_dedup prefixlen payload), with an FPI only via the XLH_DEDUP_RFPI fallback.
The nbtxlog.h header comment states exactly the opposite of this paragraph ('avoiding a full-page image', 'logs no full-page image: redo rebuilds each half the way the primary did'). Rewrite this paragraph to describe item-replay as the primary path with FPI only as a bounded (>BLCKSZ payload) fallback, consistent with XLH_SPLIT_RFPI / XLH_DEDUP_RFPI.
| * high key (nbtpage.c's _bt_mark_page_halfdead). Recorded in the vacuum | ||
| * record so redo does the same. |
There was a problem hiding this comment.
This comment is inaccurate. Nothing about the PREFIX drop is "recorded in the vacuum record": xl_btree_vacuum only carries ndeleted/nupdated plus the offset arrays (see nbtxlog.h), and no field is added here. Redo (btree_xlog_vacuum, nbtxlog.c) independently re-derives the same condition from the page state after replaying the deletions (P_HASPREFIX && PageGetMaxOffsetNumber == BTPageGetPrefixOff). Stating it is recorded is misleading and would steer a future maintainer wrong if the record format is changed. Reword to match the redo path, e.g. "Redo re-derives this from the replayed page state in btree_xlog_vacuum."
| * high key (nbtpage.c's _bt_mark_page_halfdead). Recorded in the vacuum | |
| * record so redo does the same. | |
| * high key (nbtpage.c's _bt_mark_page_halfdead). btree_xlog_vacuum | |
| * re-derives this from the replayed page state, so no field is recorded. |
| IndexTuple itup; | ||
| BTItemBuf itupbuf; |
There was a problem hiding this comment.
BTItemBuf is PGAlignedBlock, i.e. a full BLCKSZ (8KB default) stack buffer. _bt_compare is the hottest comparison routine in nbtree, invoked repeatedly inside the binary-search loops (_bt_binsrch, _bt_binsrch_insert) at every level of a descent. This adds ~8KB to its stack frame on every call, paid unconditionally even for the vast majority of indexes that are not prefix-compressed. Worth confirming the added per-call frame size is acceptable on the descent hot path (stack depth, not just CPU); the other new BTItemBuf locals in nbtreadpage.c are per-page rather than per-comparison, so this call site is the most sensitive one.
| if (xlrec->rightprefixlen > 0) | ||
| { | ||
| rightprefix = datapos; | ||
| datapos += xlrec->rightprefixlen; | ||
| datalen -= xlrec->rightprefixlen; | ||
| } | ||
| if (spagenumber != P_NONE) | ||
| { | ||
| right_hikey = (IndexTuple) datapos; | ||
| datapos += MAXALIGN(IndexTupleSize(right_hikey)); | ||
| datalen -= MAXALIGN(IndexTupleSize(right_hikey)); | ||
| } |
There was a problem hiding this comment.
Unaligned memory access on strict-alignment platforms (ARM/SPARC/PPC, etc.).
The right-half payload is laid out as [rightprefix bytes][right_hikey][items...], and the prefix bytes are logged WITHOUT MAXALIGN padding (see nbtinsert.c: roff = rightprefixlen; ... roff += MAXALIGN(...)). XLogRecGetBlockData only MAXALIGNs the start of the block data. After datapos += xlrec->rightprefixlen (rightprefixlen can be 1..126, including odd values), datapos is no longer aligned, so (IndexTuple) datapos and IndexTupleSize(right_hikey) / the subsequent item loop dereference t_info (a uint16) at an unaligned address. _bt_prefix_encode/PageAddItem then read the whole tuple header from an unaligned source.
The pre-existing _bt_restore_page deliberately avoids this by using memcpy into an aligned IndexTupleData and a void *items[] array (see its comment about not assuming alignment). This new path regresses. Either MAXALIGN the prefix length in the logged payload on both the write and read sides, or memcpy the header out before calling IndexTupleSize. The left-half path (block 0) happens to be safe only because the prefix is logged last there.
| datapos += xlrec->prefixlen; | ||
| datalen -= xlrec->prefixlen; | ||
| if (xlrec->flags & XLH_DEDUP_NOT_RIGHTMOST) | ||
| { | ||
| hikey = (IndexTuple) datapos; | ||
| datapos += MAXALIGN(IndexTupleSize(hikey)); | ||
| datalen -= MAXALIGN(IndexTupleSize(hikey)); | ||
| } | ||
| while (datalen > 0) | ||
| { | ||
| IndexTuple it = (IndexTuple) datapos; | ||
| Size sz = MAXALIGN(IndexTupleSize(it)); | ||
|
|
||
| items[nitems++] = it; | ||
| datapos += sz; | ||
| datalen -= sz; | ||
| } |
There was a problem hiding this comment.
Same unaligned-access hazard as the split right-half path. The dedup payload is [prefix bytes][hikey][items...] with the prefix logged unpadded (nbtdedup.c packs off += dedup_prefixlen with no MAXALIGN). After datapos += xlrec->prefixlen with an odd prefixlen, (IndexTuple) datapos on the hikey and every item in the loop is an unaligned IndexTuple dereference (IndexTupleSize reads t_info). This will SIGBUS/UB on strict-alignment architectures. Pad the prefix to MAXALIGN on the write side (and mirror it here), or memcpy each tuple header out before use, consistent with _bt_restore_page.
| rightitems[0] = NULL; /* not reused below */ | ||
| { | ||
| IndexTuple dummyright[MaxIndexTuplesPerPage]; | ||
| int dummynr; | ||
|
|
||
| _bt_split_leaf_collect(origpage, xlrec->firstrightoff, | ||
| xlrec->newitemoff, newitem, newitemonleft, | ||
| origpagepostingoff, nposting, | ||
| leftitems, &nleft, dummyright, &dummynr); | ||
| } |
There was a problem hiding this comment.
This rightitems[0] = NULL; followed by a nested block declaring a throwaway dummyright[MaxIndexTuplesPerPage] only to discard the collected right half is an awkward workaround. It burns an extra MaxIndexTuplesPerPage-entry stack array (~ several KB) purely to satisfy the _bt_split_leaf_collect signature. Consider letting _bt_split_leaf_collect accept NULL right arrays (skip the right side) so redo can collect only the half it needs, rather than reusing rightitems and a dummy array.
| while (datalen > 0) | ||
| { | ||
| IndexTuple it = (IndexTuple) datapos; | ||
| Size sz = MAXALIGN(IndexTupleSize(it)); | ||
|
|
||
| rightitems[nright++] = it; | ||
| datapos += sz; | ||
| datalen -= sz; | ||
| } |
There was a problem hiding this comment.
The while (datalen > 0) loop appends to rightitems[] (sized MaxIndexTuplesPerPage) with no bound check on nright. A corrupt record with an oversized datalen would overrun the on-stack array. The pre-existing _bt_restore_page has the same unchecked pattern, so this matches established nbtree-redo convention, but given this runs on standbys where a corrupt record is possible, consider an Assert(nright < MaxIndexTuplesPerPage) (and likewise in the dedup path) to fail loudly rather than smash the stack.
|
📊 OCR posted 24/25 inline comment(s). 1 could not be posted
|
652159b to
ca1c9c0
Compare
Add leaf key prefix compression to nbtree, controlled by a new per-index storage parameter, prefix_compression, that is off by default. When a leaf page is written as a whole -- during an index build, as a split half, or as the result of a deduplication pass -- and its data items share a leading byte-prefix of their first key column, the page is rebuilt in the BTP_PREFIX format: a single PREFIX item after the high key holds the shared bytes, and each data item is stored "coded" (its copy of the shared prefix replaced by a one-byte length). Reads of a leaf data item funnel through _bt_getitem(), which decodes on demand; high keys and internal-page downlinks are never coded and are read with the existing PageGetItem(). The feature is deliberately a per-page opt-in with no on-disk version change: BTREE_VERSION and the catalog version are untouched. A page is converted only when it is next written whole, never by an insert, VACUUM or key-range change, so compression "faults forward" on an existing index. Old-format and new-format pages coexist in the same index, which keeps pg_upgrade, physical replication, and standbys working with no REINDEX: a replica that has not yet seen a page's conversion still reads it correctly, and turning the parameter off (ALTER INDEX ... SET) stops new conversions while leaving existing coded pages readable. Prefix compression is a separate mechanism from deduplication, not an extension of it: deduplication collapses tuples whose whole key is equal, while prefix compression factors out a leading run shared by distinct keys (a URL or jsonb-path column is the motivating case, where deduplication does not help). The two compose on one page -- a build deduplicates first and then codes the survivors, so a duplicate-heavy prefix index is no larger than a plain one. Bottom-up index deletion runs on coded pages unchanged, since it only removes items. A runtime deduplication pass on a coded page decodes first, and because a coded page can hold more logical data than fits plain, the pass aborts cleanly and the page splits instead when the decoded set would overflow. WAL replay reproduces the coded bytes without resorting to full-page images in the common paths. A Relation-free layout routine, _bt_split_leaf_layout(), together with a single item-collection routine, _bt_split_leaf_collect(), are shared by the primary (_bt_split) and redo (btree_xlog_split) so both derive the same plain item sequence and code it against the same logged prefix, emitting identical bytes. An in-place insert logs the coded tuple; a split logs each half's chosen prefix plus the right half's plain items; a prefix deduplication logs the chosen prefix plus the deduped plain items. A bounded fallback carries the page as a forced full-page image only when a pathologically compressible page's plain payload would exceed one block; realistic data never reaches that threshold, so the nbtree-prefix-force-rfpi injection point and a dedicated test in src/test/modules/nbtree exercise the fallback deterministically. A scan reading a coded leaf page skips decoding (and the key comparison that would read the key) for any data item whose key it does not need: when the scan is not an index-only scan and every scan key is already known satisfied for the whole page, _bt_readpage() takes the heap TID (and posting list, both stored uncoded) straight from the on-page tuple. This recovers most of the decoding cost that prefix compression adds to bitmap index scans and plain index scans over a matching range (-15% to -20% on a cached 3M-row index of distinct long-prefixed keys); index-only scans, which must return the key, decode as before. amcheck decodes every leaf data item, including under heapallindexed, and checks the BTP_PREFIX structural invariants (flag only on leaf pages, a well-formed PREFIX item at the expected offset); its test suite covers a prefix-compressed index. The create_index documentation and the nbtree README describe the parameter, the on-disk format, and the relationship to deduplication. Tested with wal_consistency_checking=all on a primary and a streaming standby across insert, split, deduplication (including the forced-image fallback), delete and vacuum workloads, through crash recovery, and across a pg_upgrade that carries both page formats.
| if (codeditup != itup) | ||
| pfree(codeditup); | ||
|
|
||
| /* split the buffer into left and right halves */ |
There was a problem hiding this comment.
Memory leak in the split path: codednposting (allocated above when pageprefix && nposting != NULL) is not freed here before _bt_split(). Only codeditup is freed. The non-split branch correctly does if (codednposting != NULL && codednposting != nposting) pfree(codednposting); (line ~1501), so this is an asymmetry. A posting-list split that then triggers a page split leaks the re-coded posting tuple into CurrentMemoryContext. Bounded and reclaimed at context reset, but it should mirror the other branch.
| if (codeditup != itup) | |
| pfree(codeditup); | |
| /* split the buffer into left and right halves */ | |
| if (codeditup != itup) | |
| pfree(codeditup); | |
| if (codednposting != NULL && codednposting != nposting) | |
| pfree(codednposting); | |
| /* split the buffer into left and right halves */ |
| SET enable_indexonlyscan = on; | ||
| SELECT md5(string_agg(u, '|' ORDER BY u)) AS ios_digest | ||
| FROM btree_prefix WHERE u IS NOT NULL \gset | ||
| SET enable_indexscan = off; |
There was a problem hiding this comment.
The digest checks advertise an "index-only scan" but no plan is pinned, so the returned-value decode path they intend to cover is not guaranteed to run.
At line 606 the ios_digest capture runs with both enable_indexscan and enable_indexonlyscan on (only seqscan/bitmapscan are off). If the planner picks a plain Index Scan instead of an Index Only Scan, the u column is fetched from the heap, not decoded from the coded index tuple. post_churn_ok then compares heap-vs-heap and would still report t even if value decoding on coded leaves were broken -- i.e. it passes with the decode logic regressed. The same applies to ios_ok (line 575), bottomup_ok and runtime_dedup_ok, which only set enable_seqscan = off.
Force the index-only scan exclusively for the capture (disable enable_indexscan so a plain index scan cannot be chosen) and/or add an EXPLAIN (costs off) asserting Index Only Scan for the digest query, as already done for the equality query at line 554. Otherwise these are the only checks that read the key back from the index, and their coverage is not robust. [medium confidence]
| SET enable_indexonlyscan = on; | |
| SELECT md5(string_agg(u, '|' ORDER BY u)) AS ios_digest | |
| FROM btree_prefix WHERE u IS NOT NULL \gset | |
| SET enable_indexscan = off; | |
| SET enable_indexonlyscan = on; | |
| SET enable_indexscan = off; | |
| EXPLAIN (costs off) | |
| SELECT md5(string_agg(u, '|' ORDER BY u)) FROM btree_prefix WHERE u IS NOT NULL; | |
| SELECT md5(string_agg(u, '|' ORDER BY u)) AS ios_digest | |
| FROM btree_prefix WHERE u IS NOT NULL \gset |
|
|
||
| /* xl_btree_dedup flag bits */ | ||
| #define XLH_DEDUP_NOT_RIGHTMOST 0x01 /* prefix payload includes a high key */ | ||
| #define XLH_DEDUP_RFPI 0x02 /* prefix page carried as a forced image */ |
There was a problem hiding this comment.
XLH_DEDUP_RFPI is written by the primary (_bt_dedup_pass) but never read anywhere in redo: a forced full-page image on block 0 makes XLogReadBufferForRedo return BLK_RESTORED, so btree_xlog_dedup short-circuits and the flag is never consulted. This is a dead flag bit that is nonetheless baked into the on-disk WAL format. Either drop it (and the primary's assignment) per YAGNI, or document explicitly that it exists only for pg_waldump/diagnostics and is intentionally not read by redo.
| #define XLH_SPLIT_LEAFPREFIX 0x01 /* leaf split, rebuild halves coded */ | ||
| #define XLH_SPLIT_RFPI 0x08 /* right half carried by a full-page image |
There was a problem hiding this comment.
The 0x02 and 0x04 bit values are skipped between XLH_SPLIT_LEAFPREFIX (0x01) and XLH_SPLIT_RFPI (0x08) with no explanation. Since flags is a newly added field (xl_btree_split had no prior flags), there are no legacy bits to avoid colliding with. Either make the values contiguous (0x02) or add a comment explaining why the gap is intentional, so a future reader does not assume removed flags once lived there.
| PageSetLSN(rightpage, lsn); | ||
| } | ||
| MarkBufferDirty(rbuf); |
There was a problem hiding this comment.
In the RFPI branch rbuf comes from XLogReadBufferForRedo() returning BLK_RESTORED, and the restore path (XLogReadBufferForRedoExtended) has already called MarkBufferDirty() on it. Calling MarkBufferDirty(rbuf) again here for the restored buffer is redundant and non-idiomatic (compare ginRedoSplit, which only unlocks/releases a BLK_RESTORED buffer). It is only needed for the freshly XLogInitBufferForRedo'd page built in the else branch. Consider moving MarkBufferDirty(rbuf) into the non-RFPI branch after PageSetLSN.
| SELECT (SELECT md5(string_agg(u, '|' ORDER BY u)) | ||
| FROM btree_prefix WHERE u IS NOT NULL) = | ||
| md5(string_agg(x, '|' ORDER BY x)) AS ios_ok |
There was a problem hiding this comment.
The comment claims this is an "index-only scan over the prefix index", but no plan is pinned here: only enable_seqscan/enable_bitmapscan are off at this point, while enable_indexscan and enable_indexonlyscan are both at their defaults (on). The planner is free to pick a plain Index Scan, in which case u is fetched from the heap and the index's decode path is never exercised. A decode regression would then still produce ios_ok = t, so this assertion does not reliably test what its comment claims. Force the intended plan (e.g. SET enable_indexscan = off; SET enable_indexonlyscan = on;) around the check, and ideally verify the plan with EXPLAIN. (high confidence)
| SELECT (SELECT md5(string_agg(u, '|' ORDER BY u)) FROM btree_prefix_bu) = | ||
| md5(string_agg(x, '|' ORDER BY x)) AS bottomup_ok |
There was a problem hiding this comment.
Comment says this compares "an index-only scan of the key column" to the oracle, but only enable_seqscan is off; enable_bitmapscan was RESET at line 616 and enable_indexscan/enable_indexonlyscan are on. The planner may use a bitmap or plain index scan that fetches u from the heap, bypassing the coded-leaf decode/deletion code this section is meant to verify. As written, bottomup_ok stays t even if index decoding is broken. Pin an index-only scan (disable seqscan+bitmapscan+indexscan, enable indexonlyscan) and confirm with EXPLAIN. (high confidence)
| SELECT (SELECT md5(string_agg(u, '|' ORDER BY u)) FROM btree_prefix_dd) = | ||
| md5(string_agg(x, '|' ORDER BY x)) AS runtime_dedup_ok |
There was a problem hiding this comment.
Same gap as the other digest checks: this is meant to exercise reading back the re-coded, deduplicated prefix page, but only enable_seqscan is disabled here (bitmapscan was RESET at line 616, indexscan/indexonlyscan are on). A bitmap or plain index scan satisfying u from the heap would make runtime_dedup_ok report t even with a broken posting-list decode. Pin an index-only scan and verify the plan so the check actually covers the new page format. (high confidence)
| -- Confirm the index really is used (and is prefix-compressed), so the queries | ||
| -- below exercise the coded page format rather than silently falling back. |
There was a problem hiding this comment.
The EXPLAIN only proves the index is chosen; it cannot show the leaves are in the coded BTP_PREFIX form. The comment "(and is prefix-compressed)" overstates what this check verifies -- correctness here is not evidence the page format is actually exercised. Reword to claim only what the plan demonstrates. (moderate confidence)
| INSERT INTO btree_prefix_dd | ||
| SELECT 'https://example.com/path/to/resource/' || lpad((g % 200)::text, 8, '0') | ||
| FROM generate_series(1, 40000) g; |
There was a problem hiding this comment.
This block adds a lot of churn to a core parallel regression test (20000 + 10000 + 5000 build/retail rows in btree_prefix, 4000 rows plus three full-table UPDATEs in btree_prefix_bu, and 40000 single-row inserts in btree_prefix_dd). The 40000 one-at-a-time inserts in particular are slow and inflate btree_index.sql runtime for every make check. Consider whether smaller counts still fill enough leaves to force splits/dedup, to keep the suite fast. (moderate confidence)
| else | ||
| { | ||
| _bt_dedup_finish_pending(newpage, state); | ||
| _bt_dedup_finish_pending(newpage, state, false); | ||
| if (pageprefix) |
There was a problem hiding this comment.
The overflowok argument to _bt_dedup_finish_pending is hardcoded false here (and at the final call below), but the primary passes pageprefix (see _bt_dedup_pass in nbtdedup.c, calls with overflowok=pageprefix).
When the source page is prefix-compressed (pageprefix == true here) the state's phystupsize is the DECODED size of the merged tuples, which the code explicitly allows to exceed BLCKSZ (see the comment and relaxed assertion in _bt_dedup_finish_pending: Assert(spacesaving > 0 && (overflowok || spacesaving < BLCKSZ))). For such a legitimately-produced non-prefix dedup record (prefixlen == 0, prefix source, re-compression declined), redo will trip that assertion because it forces overflowok=false while the primary used true.
Pass pageprefix so redo matches the primary. (high confidence)
| else | |
| { | |
| _bt_dedup_finish_pending(newpage, state); | |
| _bt_dedup_finish_pending(newpage, state, false); | |
| if (pageprefix) | |
| else | |
| { | |
| _bt_dedup_finish_pending(newpage, state, pageprefix); | |
| if (pageprefix) |
| _bt_dedup_finish_pending(newpage, state, false); | ||
| Assert(state->nintervals == xlrec->nintervals); |
There was a problem hiding this comment.
Same overflowok mismatch as the in-loop call: the primary uses pageprefix for the final _bt_dedup_finish_pending (nbtdedup.c), but redo hardcodes false. On a prefix source page a merged interval's decoded spacesaving can be >= BLCKSZ while the coded form fit, tripping the relaxed assertion in _bt_dedup_finish_pending. Pass pageprefix here too. (high confidence)
| _bt_dedup_finish_pending(newpage, state, false); | |
| Assert(state->nintervals == xlrec->nintervals); | |
| _bt_dedup_finish_pending(newpage, state, pageprefix); | |
| Assert(state->nintervals == xlrec->nintervals); |
| if (state->btps_prefix == NULL) | ||
| state->btps_prefix = _bt_prefix_candidate(itup, | ||
| &state->btps_prefixlen); |
There was a problem hiding this comment.
Use-after-free: _bt_prefix_candidate(itup, ...) returns a pointer into itup, which is the tuple returned by tuplesort_getindextuple() in _bt_load(). That tuple lives in the tuplesort memory context and is invalidated by the next tuplesort_getindextuple() call. On the following _bt_buildadd() call, _bt_prefix_shared(state->btps_prefix, ...) dereferences this now-stale pointer (it reads prefix[shared] in _bt_prefix_plan), reading freed/reused memory. The comment claims the prefix points "into the page's first data item", but the code captures the transient caller tuple, not the on-page copy that _bt_sortaddtup() just wrote. Capture the pointer from the on-page copy instead (e.g. PageGetItem(npage, PageGetItemId(npage, last_off))) so it stays valid for the lifetime of the page being filled.
| if (!_bt_pgaddtup(leftpage, newitemsz, newitem, afterleftoff, | ||
| false)) | ||
| { | ||
| false)) | ||
| elog(ERROR, "failed to add new item to the left sibling" |
There was a problem hiding this comment.
Unnecessary diff churn on the internal-page loop: these hunks drop the braces around the single-statement elog(ERROR, ...) blocks and re-indent the _bt_pgaddtup() continuation line (e.g. afterrightoff == minusinfoff)) gains extra tabs). The logic is unchanged -- only the loop was moved under else -- so this is whitespace/brace reformatting of otherwise-untouched code. The odd continuation indentation also won't survive pgindent. Keep the original bracing/indentation to minimize the diff. Also, the else for (...) form on the preceding line is hard to read and pgindent-fragile; consider wrapping the internal-page branch in a braced block.
|
📊 OCR posted 14/15 inline comment(s). 1 could not be posted
|
f28763c to
61bfc57
Compare
nbtree: opt-in leaf key prefix compression
Adds a
prefix_compressionindex storage parameter (default off) thatfront-codes the first key column of each non-pivot leaf tuple against a
per-page prefix. Keys that share long leading bytes — URLs,
jsonbkeys withcommon paths, zero-padded codes — then cost little more than their unshared
tail.
Opened against this fork's
masterto exercise CI and the OCR review agent.Posted to pgsql-hackers (CF 7523); this PR tracks that series.
Design
On-disk format (per-page, opt-in, no version bump)
A leaf whose index has
prefix_compression = onand whose first key column isa variable-length type may carry a new page flag
BTP_PREFIX (1 << 9). Such aleaf holds one extra item, the PREFIX item, at the offset where its first
data item would otherwise be (
P_HIKEYon a rightmost page, elseP_FIRSTKEY);data items start one offset later, which
P_FIRSTDATAKEY(opaque)accounts for.The PREFIX item is an
IndexTupleDataheader followed by 1..126 prefix bytes,shaped like a tuple so code that walks items by
IndexTupleSizesteps over it.On a
BTP_PREFIXleaf every non-pivot tuple (plain or posting) whose first keycolumn is non-NULL stores that column coded, as a varlena whose payload begins
with a
uint8 shared:shared <= 126means the value is the page prefix'sfirst
sharedbytes followed by the unshared remainder;shared == 127(RAW)stores the value verbatim. Pivots, NULL first columns, and the high key are
always stored plain. Coding/decoding depend only on
(page prefix, tuple),never the catalog, so WAL redo repeats them and decoding reproduces the plain
tuple byte for byte.
Reader funnel
Every read of a leaf data item goes through a new inline
_bt_getitem()that returns the on-page tuple on a normal page and the decoded tuple (into a
caller
BTItemBuf) on aBTP_PREFIXpage. High keys and internal-page pivotsstay on bare
PageGetItem(). Comparisons always see the real datum, so everycollation and operator class behaves identically with the feature on or off.
Fault-forward, never a forced rewrite
A page is compressed only when it is written as a whole: a finished leaf
during a build, either half of a split, or a deduplication rewrite. No insert,
VACUUM, or key-range change ever rewrites a page to keep a prefix valid.
Because the format is a per-page flag with no metapage version change, pages
without the flag read exactly as before, so pg_upgrade and every form of
replication keep working and existing indexes need no REINDEX.
ALTER INDEX ... SET (prefix_compression=on)rewrites nothing; each leaf converts the nexttime it is written whole, and old/new pages coexist indefinitely.
Deduplication composes with it
Prefix compression is a separate mechanism from deduplication, not an extension
of it: deduplication collapses tuples whose whole key is equal, prefix
compression factors out a leading run shared by distinct keys (a URL or
jsonb-path column is the motivating case, where deduplication does not help).
The two compose on one page — a build deduplicates first and then codes the
survivors, so a duplicate-heavy prefix index is no larger than a plain one.
Bottom-up index deletion runs on coded pages unchanged (it only removes items).
A runtime deduplication pass on a coded page decodes first; because a coded page
can hold more logical data than fits plain, the pass aborts cleanly and the page
splits instead when the decoded set would overflow.
WAL (no full-page-image tax in the common paths)
Redo reconstructs the coded bytes rather than carrying a full-page image. A
Relation-free layout routine
_bt_split_leaf_layout(), together with oneitem-collection routine
_bt_split_leaf_collect(), is shared by the primary(
_bt_split) and redo (btree_xlog_split) so both derive the same plain itemsequence and code it against the same logged prefix, emitting identical bytes.
An in-place insert logs the coded tuple; a split logs each half's chosen prefix
plus the right half's plain items; a prefix deduplication logs the chosen prefix
plus the deduped plain items. A bounded fallback carries the page as a forced
full-page image only when a pathologically compressible page's plain payload
would exceed one block; realistic data never reaches that threshold, so a
nbtree-prefix-force-rfpiinjection point and a dedicated test insrc/test/modules/nbtreeexercise the fallback deterministically.amcheck, docs, tests
amcheck decodes every leaf data item (including under
heapallindexed) andchecks the
BTP_PREFIXstructural invariants; its suite now covers aprefix-compressed index.
create_index.sgmland the nbtree README describe theparameter, the on-disk format, and the relationship to deduplication.
Regression coverage in
btree_index.sqlplus the injection-point module test.Validation
Fedora debug build (
cassert,injection_points):regress+isolation+amcheck+pageinspect+pgstattuple+src/test/modules/nbtreeall green;recovery/027_stream_regresswithPG_TEST_EXTRA=wal_consistency_checking; aprimary+standby run with
wal_consistency_checking=alloverbuild/split/dedup (including the forced-image fallback)/delete/vacuum replays
byte-exact; crash recovery and a pg_upgrade carrying both page formats pass.
-Werrorclean.Benchmarks (release, c7i.4xlarge, fsync/synchronous_commit off)
Size win shows up where keys are distinct-but-prefixed. One row at a time into
an existing URL index (37-byte shared prefix, distinct suffixes):
TPS within noise. Read-side (prefix off vs on, warm, 3M distinct URL keys):
point lookup ~1% (noise), COUNT(*)/range scan ~+6–7% decode cost that scales
with tuples traversed.
Status / open items from review
cost (measured above), skipping decode when only the heap TID is needed
(plain IndexScan/BitmapIndexScan/COUNT — a planned follow-up, partially
already handled by
_bt_saveitem), multi-column keys, and per-page prefixboundaries (both deferred to a future multi-prefix format).
degenerate-split guard; empirically this cannot be made unconditional or
relocated to
_bt_splitwithout a plain-page regression, so it stays as asingle well-commented guard.
BTREE_VERSION/catalog version bump.