Repository navigation
Conversation
📜 Change history & discussion (Agora / pg.ddx.io)I have enough to map the BARK commit groups to real, author-confirmed threads and commitfest entries. No evidence BARK itself was ever posted as a thread — it's the author's integration vehicle. Writing up. 🧵 Related discussionBARK is an out-of-tree research branch by Greg Burd (confirmed: the CI commits reference
🔗 Related commits / prior art
📋 CommitfestThe BARK branch is not a single CF entry. Its decomposed pieces are tracked separately (all Greg Burd, CF 62 unless noted):
No commitfest entry titled "BARK" exists. 🧭 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}
There was a problem hiding this comment.
🔍 OCR found 88 issue(s).
- 25 inline, 63 in summary (inline capped at 25)
⚠️ 1 warning(s) during review
📄 contrib/amcheck/t/007_subattr_proof.pl
File-number collision: this test is 007_subattr_proof.pl, but contrib/amcheck/t/007_verify_bark.pl is also added in this change (both are listed in meson.build). Two TAP tests sharing the 007_ prefix in one directory breaks the project convention of unique numeric prefixes and is confusing to the TAP runner / prove. This looks like an unintended collision between the subattr and bark work streams. Renumber one of them -- since 007/008/009 form the contiguous bark series, this subattr test should move to the next free number (e.g. 010_subattr_proof.pl). (high confidence)
📄 contrib/amcheck/sql/check_bark_oversized.sql (L64-L64)
This predicate does not isolate the oversized keys as the comment claims. bark_big holds 40 'small-...' rows plus the 30 'K...' and 40 'M...' oversized rows. Lowercase 's' (0x73) sorts after 'K' (0x4B) and 'M' (0x4D) under the default/C collation, so k >= 'K' matches all 110 rows, not just the 70 oversized ones. The expected output confirms this: idx_oversized is 110, identical to idx_total, so the "oversized count" assertion is meaningless and gives false confidence that oversized-key filtering works. Tighten the predicate to actually select the oversized keys (e.g. k LIKE 'K%' OR k LIKE 'M%', or length(k) > <threshold>) so idx/seq agreement proves something. As a side effect the current form also silently depends on collation (the hardcoded 110 would change under a locale that orders lowercase before uppercase).
📄 contrib/amcheck/sql/check_bark_oversized.sql (L20-L21)
string_agg(..., '') is evaluated without an ORDER BY, yet the function is marked IMMUTABLE and the whole test relies on it producing byte-identical output across every call (e.g. payload = bark_bigstr(k, 32000), k = bark_bigstr(2, 65000)). PostgreSQL does not guarantee the input order of an aggregate absent ORDER BY; under parallel aggregation or a reordered input path the concatenation order can differ between the INSERT-time evaluation and the later comparison, breaking the equality checks non-deterministically. The comment even promises the string is "repeatable". Add ORDER BY g to make the ordering explicit and the IMMUTABLE marking truthful.
💡 Suggested change
Before:
$$ SELECT substr(string_agg(md5(s::text || g::text), ''), 1, n)
FROM generate_series(1, (n + 31) / 32) g $$;
After:
$$ SELECT substr(string_agg(md5(s::text || g::text), '' ORDER BY g), 1, n)
FROM generate_series(1, (n + 31) / 32) g $$;
📄 contrib/amcheck/sql/check_bark_skip.sql (L59-L64)
bark_skip_searches relies on EXPLAIN ANALYZE's exact Index Searches metric (the expected output pins bark_skip_ab: 12 searches) but, unlike its sibling bark_skip_stats (line 45) and the referenced bark_xt_buffers in src/test/regress/sql/bark.sql, it does not disable parallel query. Under debug_parallel_query = on (which the buildfarm exercises) the scan runs in a parallel worker and/or the work is split across workers, so the per-scan search count changes and this test will fail nondeterministically. The call site at lines 85-86 only turns off seqscan/bitmapscan, which does not prevent a parallel plan. Add the same guard the other helpers use.
Confidence: high.
💡 Suggested change
Before:
CREATE FUNCTION bark_skip_searches(q text) RETURNS text LANGUAGE plpgsql AS $$
DECLARE
plan json;
node json;
BEGIN
EXECUTE 'EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, FORMAT JSON) ' || q INTO plan;
After:
CREATE FUNCTION bark_skip_searches(q text) RETURNS text LANGUAGE plpgsql AS $$
DECLARE
plan json;
node json;
BEGIN
SET LOCAL debug_parallel_query = off; -- as in bark_xt_buffers
EXECUTE 'EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, FORMAT JSON) ' || q INTO plan;
📄 contrib/amcheck/sql/check_bark_split.sql (L42-L45)
bark_pfxfast (and its index bark_pfxfast_idx) is the only object created in this file that is never dropped; every other table/function has a matching DROP. Since amcheck regression tests share one database, this leaves residue behind for subsequent tests in the schedule (e.g. it would surface in a whole-database pg_amcheck/dump). Add a DROP TABLE bark_pfxfast; after this block to keep the test self-contained.
💡 Suggested change
Before:
SELECT count(*) AS rng_seq FROM bark_pfxfast WHERE k >= repeat('A', 9000);
+RESET enable_seqscan;
+RESET enable_indexscan;
+RESET enable_bitmapscan;
After:
SELECT count(*) AS rng_seq FROM bark_pfxfast WHERE k >= repeat('A', 9000);
+RESET enable_seqscan;
+RESET enable_indexscan;
+RESET enable_bitmapscan;
+DROP TABLE bark_pfxfast;
📄 contrib/amcheck/sql/check_bark_skip.sql (L128-L128)
The header comment states these cases "check the index with bark_index_check() after the changes they make, so they live here rather than in the core regression suite" -- that is the stated reason this file lives in amcheck at all. But of the four indexes built (bark_skip_ab, bark_skip_tb, bark_skip2_ab, bark_skip2_tb), only bark_skip2_ab is ever passed to bark_index_check(). The primary skip-scan subject bark_skip_ab and both DESC/text variants go unverified. Sibling files (check_bark_tidorder.sql, check_bark_opclass.sql, check_bark_rowcmp.sql) verify essentially every index they create. Add bark_index_check() calls for the remaining indexes (at minimum bark_skip_ab), otherwise the file's justification for living outside core regress does not hold and structural corruption from skip-scan activity on these indexes would go undetected. (moderate confidence)
💡 Suggested change
Before:
SELECT bark_index_check('bark_skip2_ab');
After:
SELECT bark_index_check('bark_skip_ab'), bark_index_check('bark_skip_tb');
SELECT bark_index_check('bark_skip2_ab'), bark_index_check('bark_skip2_tb');
📄 contrib/amcheck/sql/check_bark_tidorder.sql (L35-L37)
This size comparison uses a zero-margin <= against btree, unlike every other size check in this file and its siblings (lines 91-92 use * 1.2, lines 125-126 use * 1.1, and check_bark_split.sql/check_bark_rowcmp.sql all keep a 1.1-1.25x slack). A margin-less equality-boundary comparison of raw byte sizes against btree is the most buildfarm-flake-prone form: btree deduplication, page-level alignment/padding, and MAXALIGN differ across 32/64-bit and architectures, so bark delta landing even one block above btree delta flips the result. Give this check the same slack as the others (confidence: moderate).
💡 Suggested change
Before:
SELECT pg_relation_size('bark_hk_bark') - bark0 <=
pg_relation_size('bark_hk_bt') - bt0 AS bark_grows_no_more
FROM bark_hk_size;
After:
SELECT pg_relation_size('bark_hk_bark') - bark0 <=
(pg_relation_size('bark_hk_bt') - bt0) * 1.1 AS bark_grows_no_more
FROM bark_hk_size;
📄 contrib/amcheck/sql/check_bark_tidorder.sql (L357-L359)
This EXPLAIN (COSTS OFF) commits a specific non-parallel plan shape (Aggregate -> Index Scan) to the expected output, but parallelism is not disabled here. On buildfarm animals that run the regression suite with debug_parallel_query = regress (or with lowered min_parallel_*_size), the planner can pick a parallel aggregate (Finalize Aggregate -> Gather -> Partial Aggregate -> Partial Aggregate -> Index Scan), which will not match the expected output and fail the test. Either SET max_parallel_workers_per_gather = 0 (or SET LOCAL) around this EXPLAIN, or drop the EXPLAIN since the result-equivalence SELECTs on lines 359/365 already cover the correctness intent (confidence: moderate).
💡 Suggested change
Before:
EXPLAIN (COSTS OFF)
SELECT count(*), count(DISTINCT k), sum(v) FROM bark_fp WHERE k >= '';
SELECT count(*), count(DISTINCT k), sum(v) FROM bark_fp WHERE k >= '';
After:
SET max_parallel_workers_per_gather = 0;
EXPLAIN (COSTS OFF)
SELECT count(*), count(DISTINCT k), sum(v) FROM bark_fp WHERE k >= '';
RESET max_parallel_workers_per_gather;
SELECT count(*), count(DISTINCT k), sum(v) FROM bark_fp WHERE k >= '';
📄 contrib/amcheck/sql/check_bark_split.sql (L40-L42)
This block is meant to compare an index scan against a sequential scan, but it only disables enable_indexscan and enable_bitmapscan while leaving enable_indexonlyscan on. Since the query selects count(*) and references only the indexed column k, the planner can still satisfy it with an index-only scan on bark_pfxfast_idx. So rng_seq is not guaranteed to be a seqscan, and the "index vs. seqscan" check it is supposed to provide can silently degrade to index-vs-index. Disable enable_indexonlyscan here too (as the random/duplicate blocks at lines 210-216 and 292-301 correctly do). The same defect exists in the bark_pfxcomp (line ~63), bark_ff_wide (line ~133), and bark_ff_room (line ~157) "seq" blocks.
💡 Suggested change
Before:
SET enable_seqscan = on;
+SET enable_indexscan = off;
+SELECT count(*) AS rng_seq FROM bark_pfxfast WHERE k >= repeat('A', 9000);
After:
SET enable_seqscan = on;
SET enable_indexscan = off;
SET enable_indexonlyscan = off;
SELECT count(*) AS rng_seq FROM bark_pfxfast WHERE k >= repeat('A', 9000);
📄 contrib/amcheck/sql/check_bark_split.sql (L63-L65)
Same issue as the bark_pfxfast block: to force a sequential scan you must also disable enable_indexonlyscan. The query selects only count(*) over the indexed column t, so an index-only scan on bark_pfxcomp_idx remains eligible and seq_count may not be a seqscan, defeating the index-vs-seq comparison.
💡 Suggested change
Before:
SET enable_indexscan = off;
SET enable_bitmapscan = off;
SELECT count(*) AS seq_count FROM bark_pfxcomp WHERE t >= '0300';
After:
SET enable_indexscan = off;
SET enable_indexonlyscan = off;
SET enable_bitmapscan = off;
SELECT count(*) AS seq_count FROM bark_pfxcomp WHERE t >= '0300';
📄 contrib/amcheck/sql/check_bark_split.sql (L133-L137)
Same issue: enable_indexonlyscan is left enabled, so this "seq" query can be answered by an index-only scan on bark_ff_wide_idx (it only needs indexed column t for count(*)), and is not guaranteed to run as a sequential scan.
💡 Suggested change
Before:
SET enable_indexscan = off;
SET enable_bitmapscan = off;
SELECT count(*) FROM bark_ff_wide WHERE t >= '150';
RESET enable_indexscan;
RESET enable_bitmapscan;
After:
SET enable_indexscan = off;
SET enable_indexonlyscan = off;
SET enable_bitmapscan = off;
SELECT count(*) FROM bark_ff_wide WHERE t >= '150';
📄 contrib/amcheck/sql/check_bark_split.sql (L157-L161)
Same issue: without disabling enable_indexonlyscan, this count(*) over indexed column t can still use an index-only scan on bark_ff_room_idx, so seq_count is not reliably a sequential scan.
💡 Suggested change
Before:
SET enable_indexscan = off;
SET enable_bitmapscan = off;
SELECT count(*) AS seq_count FROM bark_ff_room WHERE t >= '00015000';
RESET enable_indexscan;
RESET enable_bitmapscan;
After:
SET enable_indexscan = off;
SET enable_indexonlyscan = off;
SET enable_bitmapscan = off;
SELECT count(*) AS seq_count FROM bark_ff_room WHERE t >= '00015000';
📄 contrib/amcheck/sql/check_bark_unique.sql (L84-L84)
This filter is endianness-dependent and will fail on big-endian buildfarm members (s390x, PPC64BE). bark_page_items.data is produced by bark_hex_bytes(), which dumps the raw stored key bytes in memory order (sprintf("%02x", ptr[off])). The key here is int4 4999 = 0x00001387; its little-endian layout is 87 13 00 00, but on big-endian it is 00 00 13 87. On a big-endian host i.data LIKE '87 13 00 00 %' matches zero rows, so run_spans_leaves becomes false/NULL and coalesced becomes 0, diverging from the expected output (t | 0). The test must not key off the byte order of a stored integer. Consider selecting the run by something order-independent (e.g. matching on key value via a scan, or restricting by itemoffset/shape without a raw-byte LIKE), or make the pattern explicitly cover both byte orders.
📄 contrib/amcheck/sql/check_bark_unique.sql (L70-L70)
This claim (and the run_spans_leaves = t assertion it supports) is block-size dependent. At a non-default BLCKSZ (e.g. 16K/32K), 400 entries of one key plus a 100-byte INCLUDE value may well fit on a single leaf, making count(DISTINCT s.blkno) > 1 false and the expected output wrong. If this file is intended to pass under non-default block sizes, the leaf-spanning condition should be derived rather than assumed; otherwise document that it only holds at the default 8K block size.
📄 contrib/amcheck/sql/check_bark_vacuum.sql (L152-L155)
These exact page counts (19/7/13/25/32) only hold at the default BLCKSZ=8192. The assertion itself is portable (it compares against the actual built_pages with a fixed +10 slack), but the prose hard-codes block-size-specific numbers that are wrong under a non-default BLCKSZ. Consider rewording to describe the invariant (refill reuses the deleted pages rather than extending the index) without citing absolute page counts, so the comment does not drift from behavior on 4kB/16kB/32kB builds.
📄 contrib/amcheck/sql/check_bark_vacuum.sql (L30-L34)
The seq_gap query is meant to be a true sequential scan that independently cross-checks the index count (idx_gap), per the comment "the index counts must equal a sequential scan's". But only enable_indexscan and enable_bitmapscan are turned off here; enable_indexonlyscan is left at its default (on). For a bare count(*), the planner will happily serve this from an index-only scan over bark_vgap_idx, so seq_gap can be computed from the same index as idx_gap rather than the heap, defeating the cross-check. Every sibling test that wants a genuine seqscan (e.g. check_bark_markpos.sql, check_bark_opclass.sql, check_bark_tidorder.sql) also disables enable_indexonlyscan. Add SET enable_indexonlyscan = off; here (and a matching RESET below).
💡 Suggested change
Before:
SET enable_indexscan = off;
+SET enable_bitmapscan = off;
+SELECT count(*) AS seq_gap FROM bark_vgap WHERE a BETWEEN 50000 AND 150010;
+RESET enable_indexscan;
+RESET enable_bitmapscan;
After:
SET enable_indexscan = off;
+SET enable_bitmapscan = off;
+SET enable_indexonlyscan = off;
+SELECT count(*) AS seq_gap FROM bark_vgap WHERE a BETWEEN 50000 AND 150010;
+RESET enable_indexscan;
+RESET enable_bitmapscan;
+RESET enable_indexonlyscan;
📄 contrib/amcheck/t/007_subattr_proof.pl (L100-L100)
This case is a false negative. The per-statement analysis that emits the "cannot change index" DEBUG1 lines (ExecInitUpdateUntouched) is triggered from ExecUpdateAct only on the second updated row (ri_UpdateCount reaches 2). With WHERE false no rows are updated, so ExecUpdateAct never runs, the analysis never executes, and the log is empty regardless of how a NULL path element is handled. The expected '' therefore passes vacuously and would not catch a regression in the null-path logic this case claims to cover. To actually exercise the NULL path element, the UPDATE needs to touch >=2 rows; since jsonb_set(d, ARRAY[NULL, 'x'], ...) errors at run time on a real row, consider a construct that reaches the analysis without erroring (or verify via the companion SQL that this path is exercised elsewhere). (high confidence)
📄 contrib/amcheck/t/007_verify_bark.pl (L19-L19)
Duplicate TAP test number: contrib/amcheck/t/ already contains 007_subattr_proof.pl (also added in this change), so this file reuses the 007_ prefix. PostgreSQL convention is a unique, sequential NNN_ prefix per directory; the duplicate makes the suite ambiguous and is registered twice at the same number in meson.build. Renumber this test to the next free slot (e.g. 010_verify_bark.pl, since 008/009 are taken by the bark parent/chains tests) and update the Makefile/meson registration accordingly. (high confidence)
📄 contrib/amcheck/t/007_verify_bark.pl (L168-L168)
The item() helper extracts ItemIdData.lp_off as the low 15 bits of the 32-bit line pointer (g32(...) & 0x7fff), and maxoff()/the opaque offsets build on native-endian scalar reads. The native-endian pack/unpack of plain integer fields is fine (PostgreSQL stores page integers in native byte order, so the round-trip matches the backend on any one platform). The lp_off bitfield extraction, however, depends on the compiler packing the first-declared bitfield into the low-order bits, which is true for GCC/Clang on little-endian but not guaranteed on big-endian ABIs (s390x, PPC64 BE). On such a platform item() would compute the wrong byte offset and silently corrupt the wrong bytes, turning a targeted corruption test into a spurious pass/fail. Portability across both endiannesses is a hard gate for PostgreSQL; either document that this test is little-endian-only and skip elsewhere, or derive lp_off in an endian-independent way. (medium confidence)
📄 contrib/amcheck/t/008_verify_bark_parent.pl (L269-L269)
Portability (moderate confidence): $maxitem hardcodes 40 for MAXALIGN(SizeOfPageHeaderData + 3*sizeof(ItemIdData)) and & ~7 for MAXALIGN_DOWN, both of which assume MAXALIGN == 8 (64-bit). On a 32-bit build MAXALIGN is 4, so BarkMaxItemSize (bark.h:433) uses MAXALIGN(36)=36 and MAXALIGN_DOWN(x)=x & ~3, making the computed $maxitem disagree with the C-side value quoted in the "larger than the %zu-byte limit" / "should be OVERSIZED" error messages, and the i_toobig/i_shouldovf regexes would not match. The special-space term is read from the page (robust), but the page-header term is not. Consider deriving the alignment from a known quantity rather than hardcoding 40/~7.
📄 contrib/amcheck/t/008_verify_bark_parent.pl (L308-L308)
Portability (moderate confidence): byte 5 of the sbm wire header is hardcoded to 1 (SBM_WIRE_FLAG_LE), but sbm_deserialize() rejects a wire image whose endianness flag disagrees with the host (wire_is_le != host_is_le returns NULL; sbm.c:10329-10335). On a big-endian build (s390x, PPC64BE) sbm_host_is_little_endian() is false, so this image fails to deserialize and i_post1 is not a "valid sbm of one member" -- the expected has an invalid posting set (1 members) message (line 467) is never produced and the test fails. Since Perl runs on the same host as the server, derive the flag from the actual byte order (e.g. pack('L',1) eq "\1\0\0\0" ? 1 : 0) rather than hardcoding little-endian. The magic and body words already use native-order pack, which is correct; only this flag is wrong.
📄 contrib/amcheck/t/007_verify_bark.pl (L116-L116)
overflow_blkno is NULL for non-OVERSIZED shapes (see barkfuncs.c), yet this loop stores the parsed value into a single shared $c->{ovf} on every iteration. It happens to work only because OVERSIZED is iterated last in qw(LIST POSTING OVERSIZED), so for the t_big* indexes (which carry only OVERSIZED entries) ovf ends up correct. For any index that also had a LIST/POSTING entry, those iterations would clobber ovf with the empty string. Key the overflow block to the shape (e.g. off_$shape/ovf_$shape) so correctness does not depend on the qw() ordering.
💡 Suggested change
Before:
($c->{"off_$shape"}, $c->{ovf}) = split /\|/, $r if $r ne '';
After:
($c->{"off_$shape"}, $c->{"ovf_$shape"}) = split /\|/, $r if $r ne '';
📄 contrib/pageinspect/Makefile (L29-L32)
The REGRESS list order here diverges from the regress sql list in contrib/pageinspect/meson.build. meson runs page, btree, bark, bark_pages, brin, gin, gist, hash, checksum, ..., whereas this Makefile runs page, bark, bark_pages, brin, btree, checksum, gin, gist, hash, .... The test set matches, and since only page creates the extension there is no functional dependency broken, so both pass today. But the two build systems should list the serial schedule in the same order to stay in sync and avoid confusing divergence (low confidence / low severity). Consider ordering the new entries identically in both files.
📄 contrib/pageinspect/barkfuncs.c (L446-L457)
Hardening gap on the bytea (raw-page) path. bark_check_entry_bounds validates that the coded LIST's nbytes delta stream fits within the tuple, but it does not validate the entry's locator count (BarkListGetCount) against that stream. For LIST/POSTING the decode runs later via bark_entry_get_tids -> bark_coded_body_get_tids, which loops n = BarkListGetCount(itup) times calling bark_decode_varbyte(&ptr) and only guards the stream end with an Assert(ptr < endptr) (compiled out in production builds). A crafted bytea page whose count exceeds the deltas actually encoded in nbytes will read past the stream (and potentially past the tuple/page) in a non-assert build. Since bark_check_entry_bounds was written specifically to make the bytea path safe against arbitrary input, it should also bound the decoded count against nbytes (e.g. require that n deltas cannot exceed nbytes bytes, 1 byte minimum per delta: nbytes >= n - 1). Confidence: moderate (superuser-gated, same trust model as other raw-page functions, but the stated contract of bark_check_entry_bounds is not fully met).
📄 contrib/pageinspect/sql/bark_pages.sql (L128-L131)
The OVERSIZED-pivot section hard-codes a 5000-byte key and asserts a fixed two-level structure (:level = 1), a fixed shape table, and downlinks_ok/chains_ok/pivot_htids_ok. These assume the default 8 kB page: BarkMaxItemSize is MAXALIGN_DOWN((BLCKSZ - ...) / 3) capped at INDEX_SIZE_MASK (see src/include/access/bark.h), so it scales with BLCKSZ. On a non-default BLCKSZ=16384 build BarkMaxItemSize is roughly 5400 bytes and a 5000-byte key fits inline -- no OVERSIZED entry is produced, and every assertion in this block (two_levels, the shape grouping, downlinks_ok, chains_ok, the leaf-high-key case) diverges from the expected output and fails. PostgreSQL's buildfarm runs non-default block sizes, so this test is not portable. The sibling tests already parameterize on block size (contrib/pageinspect/sql/bark.sql line 165 SHOW block_size); this section needs to either size the key off current_setting('block_size') to stay above BarkMaxItemSize, or guard/skip when block_size is not 8192.
📄 contrib/pageinspect/sql/hot_updates.sql (L93-L97)
This test's expected output (contrib/pageinspect/expected/hot_updates.out) hardcodes physical tuple offsets, e.g. the HOT successor lands at (0,4). That exact offset depends on the heap tuple size (the non_indexed_col text value plus alignment/padding) and the page free-space layout at fillfactor = 50, so it is sensitive to toast/alignment behavior and could diverge on some buildfarm animals. Notably the sibling src/test/regress/hot_updates test deliberately avoids printing physical CTIDs and prints only the HOT counts. Consider making the assertion layout-independent (e.g. check the chain shape / that the successor's lp differs from the head and t_ctid self-points, without asserting the literal offset) to keep this portable. (moderate confidence)
📄 contrib/pgstattuple/Makefile (L11-L11)
The new upgrade script is inserted in the middle of the DATA list, breaking the otherwise descending --X--Y.sql ordering (1.4.sql, then 1.5--1.6, then 1.4--1.5, 1.3--1.4 ...). While DATA order is functionally irrelevant, this reads oddly and produces a larger-than-necessary diff. For a minimal, consistent diff that matches meson.build's grouping (base .sql last, upgrades ascending), place the new file with the other upgrade scripts rather than between 1.4.sql and 1.4--1.5.sql. Low confidence / style.
💡 Suggested change
Before:
DATA = pgstattuple--1.4.sql pgstattuple--1.5--1.6.sql pgstattuple--1.4--1.5.sql \
After:
DATA = pgstattuple--1.4.sql pgstattuple--1.4--1.5.sql pgstattuple--1.5--1.6.sql \
📄 contrib/pgstattuple/pgstattuple--1.5--1.6.sql (L23-L23)
pgstatbarkindex is declared PARALLEL RESTRICTED, but the sibling index-stat functions (pgstatindex, pgstathashindex, pgstatginindex) are all PARALLEL SAFE, and this function does the same read-only buffer scan. If the restriction is deliberate (e.g. the ReadStream usage), a brief rationale would help; otherwise PARALLEL SAFE keeps it consistent with the other functions in this extension. Low confidence / not a correctness issue.
💡 Suggested change
Before:
LANGUAGE C STRICT PARALLEL RESTRICTED;
After:
LANGUAGE C STRICT PARALLEL SAFE;
📄 contrib/pageinspect/t/001_bark_pages.pl (L56-L56)
The row count (10000 int8 values) is hardcoded while the leaf-count assertion is(scalar @leaves, 2, ...) is a hard requirement. The expected number of leaves depends on block_size: the test reads $blksz but never uses it to scale the generate_series range, so a server built with a non-default block size (e.g. --with-blocksize=1 or 32) can yield a different leaf count and fail at setup rather than exercising the half-dead / dead-line-pointer paths. Either scale the row count to $blksz, or LIMIT 2 and drop the exact-count assertion to a >= 2 check so the two patched leaves ($leaves[0], $leaves[1]) are still guaranteed to exist.
📄 contrib/pgstattuple/pgstatindex.c (L945-L948)
The corruption and pivot error messages report the loop counter blkno rather than the actual block number of the buffer returned by the read stream. The model function pgstathashindex uses BufferGetBlockNumber(buf) here (see the errmsg("index \"%s\" contains corrupted page at block %u", ...) and unexpected page type paths). While the read stream delivers blocks in order so the two agree in practice, deriving the block number from the buffer is the idiomatic and robust choice and keeps this consistent with the hash path. Consider BufferGetBlockNumber(buffer) in both error sites.
💡 Suggested change
Before:
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
errmsg("index \"%s\" contains corrupted page at block %u",
RelationGetRelationName(rel), blkno)));
After:
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
errmsg("index \"%s\" contains corrupted page at block %u",
RelationGetRelationName(rel), BufferGetBlockNumber(buffer))));
📄 contrib/pgstattuple/pgstatindex.c (L899-L901)
The metapage is read and its fields (version, level, root) are trusted without any validation of the special-area size or page id, unlike the per-block data pages below (which check PageGetSpecialSize/bark_page_id). BarkPageGetMeta is a bare cast with no checks. For a corruption-reporting tool a corrupt/zeroed metapage would silently yield garbage version/level/root values instead of raising ERRCODE_INDEX_CORRUPTED. Consider validating the metapage (e.g. PageIsNew / special size / BarkPageIsMeta) before reading its fields.
📄 contrib/pageinspect/t/001_bark_pages.pl (L123-L125)
The range-scan assertion depends on the physical layout of the corrupted leaves, which this test does not control. $leaves[0] (patched half-dead) and $leaves[1] (first line pointer set LP_DEAD) are selected by ORDER BY blkno, but block order has no relation to key order in a BARK index. The comment claims the scan covers "the leaves not patched below," yet nothing guarantees keys 5000..9000 avoid the two patched leaves. If values in that range happen to live on a corrupted leaf, count(*)/sum(a) will not equal 4001|28007000, making this a flaky/latently-wrong assertion. Pick a value range that is provably disjoint from the patched leaves (e.g. derive the patched leaves' key ranges from bark_page_items before stopping the server), or scan a range that cannot intersect them.
📄 contrib/pageinspect/t/001_bark_pages.pl (L54-L55)
$segblks is read without an int() wrapper, unlike the sibling test contrib/amcheck/t/007_verify_bark.pl (lines 25-26) which wraps the identical query in int(...). While pg_settings.setting for a GUC_UNIT_BLOCKS variable currently returns a bare numeric string that Perl coerces in the int($blk / $segblks) / $blk % $segblks arithmetic in seg(), relying on that implicit coercion is fragile and inconsistent with the sibling test. Wrap in int() for a deterministic integer.
💡 Suggested change
Before:
my $segblks = $node->safe_psql('postgres',
"SELECT setting FROM pg_settings WHERE name = 'segment_size'");
After:
my $segblks = int($node->safe_psql('postgres',
"SELECT setting FROM pg_settings WHERE name = 'segment_size'"));
📄 contrib/pageinspect/t/001_bark_pages.pl (L38-L39)
$ret and $out are assigned from the psql() list return but never used; only $err is consumed by the following like(). Assigning $ret to the catch-all is fine, but naming two unused lexicals is dead clutter. Use undef placeholders to make the intent explicit: my (undef, undef, $err) = $node->psql(...).
💡 Suggested change
Before:
my ($ret, $out, $err) =
$node->psql('postgres', "SELECT * FROM bark_metap('$tmpidx')");
After:
my (undef, undef, $err) =
$node->psql('postgres', "SELECT * FROM bark_metap('$tmpidx')");
📄 contrib/pgstattuple/sql/bark.sql (L10-L12)
This query emits raw tree_level, internal_pages, and overflow_pages directly, whose values depend on how many entries fit per page and therefore on BLCKSZ. The committed expected output hardcodes tree_level=1, internal_pages=1 (and later overflow_pages=4, leaf_pages=1 for bark_stat3), which only hold for the default 8kB block size. The buildfarm builds with BLCKSZ of 4kB/16kB/32kB, where a 10000-entry index has a different number of levels/internal pages, so this test will fail there. Note the sibling pgstattuple.sql test deliberately avoids this by dividing index_size by block_size and testing only an empty index (tree_level=0). Make these assertions BLCKSZ-independent (e.g. compare derived booleans, or restrict exact counts to block-size-invariant quantities such as single_entries).
📄 contrib/pgstattuple/sql/bark.sql (L41-L42)
overflow_pages (expected 4) and leaf_pages (expected 1) are direct page counts that scale inversely with BLCKSZ. The number of overflow pages needed to hold an oversized key is BLCKSZ/keysize-dependent, so this row will not match on non-default block sizes. Prefer a block-size-independent check (e.g. overflow_pages > 0, oversized_entries = 2).
📄 contrib/pgstattuple/sql/bark.sql (L19-L20)
The cross-AM density comparison uses a fixed 2-percentage-point tolerance. avg_leaf_density is a percentage of per-page fill, and the per-page fixed overhead (page header, special area) differs in relative terms as BLCKSZ changes, so the BARK-vs-btree density gap is not bounded by the same constant across block sizes. The leaf_pages difference bound of 1 is likewise fragile on small block sizes where many leaves are involved. Consider widening/guarding these tolerances or restricting this comparison to the default block size.
📄 flake.nix (L30-L36)
eachDefaultSystem advertises these outputs for every default system, including x86_64-darwin and aarch64-darwin. However shell.nix unconditionally builds all of these shells from Linux-only package sets: patchedGlibc (a glibc override) and pkgs.pkgsMusl.*, with no stdenv.isLinux guard. On a Darwin machine, evaluating/building any of these devShells (and nix flake check) will fail because glibc/pkgsMusl are not available there. Either restrict the flake to Linux systems (e.g. eachSystem ["x86_64-linux" "aarch64-linux"]) or conditionalize the Linux-only shells so every advertised system resolves.
📄 flake.nix (L42-L42)
environment.localBinInPath is a NixOS module option, not a flake output schema key. Placed here it becomes an inert per-system attribute that does nothing (it is not consumed by any evaluator in this context). Remove it to avoid implying a behavior that is not wired up.
📄 pg-aliases.sh (L1-L1)
This entire file is developer-personal workflow tooling (build/test/debug/benchmark/flame-graph helpers driven by Nix env vars such as $PG_BUILD_DIR / $PG_DATA_DIR). It is not part of the PostgreSQL source tree or build system -- it is sourced only from shell.nix's dev-shell hook (source ./pg-aliases.sh), and nothing in the meson/make build references it. The top-level .gitignore does not exclude it, and the project explicitly states that "auxiliary files from local workflows ... should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." Committing this (together with .clangd, .envrc, .gdbinit, flake.nix, shell.nix and the fork-* workflows) bloats the diff and violates the minimal-diff rule; a patch destined for pgsql-hackers/commitfest would be rejected on sight. Move it to the developer's dotfiles / local git-exclude instead of the repository.
📄 pg-aliases.sh (L82-L82)
Destructive data-loss footgun: this expands to rm -rf "" if PG_DATA_DIR is unset/empty. The variable is defined only by shell.nix's dev-shell hook; if this script is sourced outside that shell (it is committed to the repo and trivially source-able standalone), unset variables cause rm -rf "" / wiping of the current directory tree. There is no guard verifying the variable is non-empty before the destructive expansion. Add an explicit non-empty check, e.g. [ -n "$PG_DATA_DIR" ] || { echo "PG_DATA_DIR not set" >&2; return 1; }, before any rm -rf.
📄 pg-aliases.sh (L79-L79)
Same unguarded-variable data-loss hazard: if PG_BUILD_DIR and/or PG_INSTALL_DIR are unset this becomes rm -rf "" "", deleting the current working directory's contents. Guard that both variables are non-empty before running rm -rf.
📄 pg-aliases.sh (L86-L86)
If PG_DATA_DIR is unset/empty the pattern collapses to postgres.*-D.*, which matches every postgres ... -D ... process on the machine -- pkill will then terminate unrelated PostgreSQL instances. On a shared or multi-instance dev box this is an operational hazard. The same applies to the pgrep-based helpers (pg-status, pg-attach-*, pg-perf-record, flame/bench helpers) that reuse this pattern. Guard that PG_DATA_DIR is non-empty before building the pattern.
📄 pg-aliases.sh (L7-L7)
$CC is unquoted inside the command substitution. If CC contains more than one word (a common setup, e.g. CC="ccache gcc") or a path with spaces, basename receives multiple arguments and either prints the wrong value or fails, so current_compiler becomes garbage and the compiler-change detection below misbehaves (spurious or missing build-dir wipes). Quote the expansion.
💡 Suggested change
Before:
local current_compiler="$(basename $CC)"
After:
local current_compiler="$(basename "$CC")"
📄 pg-aliases.sh (L508-L508)
modified_files is assigned without local, so it leaks into the caller's shell environment as a global. The sibling helpers pg-tidy and pg-spell correctly declare local files; make this consistent to avoid polluting the interactive shell and to match the file's own convention.
💡 Suggested change
Before:
modified_files=$(git diff --name-only "${since}" | grep -E "\.c$|\.h$")
After:
local modified_files=$(git diff --name-only "${since}" | grep -E "\.c$|\.h$")
📄 shell.nix (L77-L83)
These Linux-only packages (strace, ltrace, perf-tools, linuxPackages.perf, bpftrace, rr, iotop, sysstat, and below numactl/liburing/libselinux) are added unconditionally to getPostgreSQLDeps, which every devShell shares. flake.nix exposes these shells via flake-utils.lib.eachDefaultSystem, so it advertises devShells for x86_64-darwin and aarch64-darwin as well. On those systems these attributes do not exist in nixpkgs, so nix develop (and nix flake check) will fail to evaluate. Either gate the Linux-only inputs behind pkgs.stdenv.isLinux, or restrict the flake to Linux systems instead of eachDefaultSystem.
📄 shell.nix (L735-L737)
The musl cross target is hardcoded to x86_64-linux-musl in CFLAGS/CXXFLAGS/LDFLAGS. Combined with the fact that the system function argument is accepted but never used anywhere in the body, these musl shells silently cross-compile to x86_64 even on an aarch64 host. Derive the target triple from the system argument (which is otherwise dead) so the musl shells are correct on non-x86_64 hosts.
📄 shell.nix (L335-L335)
flameGraphScript and pgbenchScript are exposed as flake packages outputs in flake.nix for every default system (via flake-utils.lib.eachDefaultSystem). But this derivation hardcodes ${pkgs.linuxPackages.perf} (and ${pkgs.flamegraph} below), which do not exist on Darwin. On x86_64-darwin/aarch64-darwin, nix flake check, nix build .#flameGraphScript, and even nix develop will fail at evaluation because linuxPackages.perf cannot be resolved. This is a distinct evaluation path from the devShell buildInputs issue: these are standalone flake outputs advertised for all systems. Either restrict these packages/devShells to Linux systems (e.g. use flake-utils.lib.eachSystem with Linux-only systems, or gate on pkgs.stdenv.isLinux), or resolve perf/flamegraph lazily so non-Linux evaluation does not force them.
📄 src/backend/access/bark/README (L911-L912)
This "Bounded streams, next step" passage documents unshipped work ("both are left for later", "About 600 lines with the tests ... not started") inside a README whose opening states it "describes the code as built". A future-work roadmap with line-count estimates is aspirational content that will bit-rot and belongs in the design doc (BARK-Design.mediawiki) or a -hackers TODO, not in the access-method README that documents current on-disk/runtime contracts. Per the project's comment-accuracy discipline (no "left for later"/"not started" future-tense in committed source), move or drop this section.
📄 src/backend/access/bark/README (L420-L423)
Hard performance numbers are quoted as bare facts throughout the README (here "46% slower with io_method=worker (3115 against 2140 ms) and 25% with sync"; elsewhere "1.42-1.58x at 96-768" on m7i.metal-48xl, "5.6x as long with the validator (39.5 s against 7.1 s)", "5.2 s under cassert and 28 s with gcov"). Per the project's discipline, performance claims need a reproducible benchmark reference (a -hackers Message-Id / design-doc pointer); raw numbers with no reproducer embedded in the AM README will bit-rot and are unverifiable by a reader. Either cite the benchmark/thread that produced them or move the quantified comparisons to the design document and keep only the qualitative conclusion here.
📄 src/backend/access/bark/bark.c (L2798-L2800)
Memory leak: keyinfo is palloc'd here via bark_build_keyinfo() (which uses palloc0 in the current/planner memory context), but it is never freed on any return path of bark_estimate_qual_selectivity -- neither the mid-loop return -1; (when sample->nkeys == 0 && !exact) nor the final return Min(selec, 1.0);. Compare the VACUUM path, which pfrees its keyinfo (pfree(vstate.keyinfo)). While the planner context is eventually reset, repeated planning of the same query accumulates these allocations. Add pfree(keyinfo); before every return after this allocation.
📄 src/backend/access/bark/README (L12-L14)
The "being built now" qualifier on multikey directly contradicts the very next sentence ("This README describes the code as built") and the rest of the file, which documents multikey as a complete, shipped feature: there is a full ~140-line "Multikey keys" section, a "Versioned multikey scans" section, cost-estimate handling, amcheck coverage, and named regression/TAP test suites (bark_multikey, bark_wildcard, test_versioned_tableam, etc.). Per the standard that comments must describe what the code does now (flag aspirational/in-progress wording), either multikey is built -- drop "being built now" -- or it is not, in which case the extensive sections presenting it as working are misleading. Make the opening consistent with the implemented state.
💡 Suggested change
Before:
+prefix compression, nearest-neighbor (KNN) ordering, and, being built now,
+operator classes that extract several keys from one value (multikey).
+This README describes the code as built: the on-disk format defined in
After:
+prefix compression, nearest-neighbor (KNN) ordering, and
+operator classes that extract several keys from one value (multikey).
+This README describes the code as built: the on-disk format defined in
📄 src/backend/access/bark/barkscan.c (L4673-L4676)
This DEBUG1 fires on every scan start (every bark_first for a multikey index), building a message string each time. It is developer tracing noise on a hot path and should be removed before this is commit-ready; pgsql-hackers reviewers will flag a per-scan DEBUG elog. (low confidence that it is intended to stay)
📄 src/backend/access/bark/barkscan.c (L6446-L6449)
Unlike the three other bark_seen_find call sites (bark_mk_seen, bark_mk_seen_add, bark_mk_admit) which all NULL-check the returned word, this undo-replay path dereferences it unconditionally. The invariant that mk->seen != NULL and the window exists whenever nundo > 0 does hold today (undo entries are only appended right after the bit is set, and bark_mk_clear_seen is never reached while marked), so this is not a live crash. But it is a fragile asymmetry: a future change to the mark/clear ordering would turn it into a NULL/garbage dereference silently. Add a defensive Assert(word != NULL) (or guard the deref) to match the other call sites and document the invariant. (moderate confidence)
💡 Suggested change
Before:
uint64 *word = bark_seen_find(mk->seen, key >> 6);
*word &= ~(UINT64CONST(1) << (key & 63));
mk->nseen--;
After:
uint64 *word = bark_seen_find(mk->seen, key >> 6);
Assert(word != NULL);
*word &= ~(UINT64CONST(1) << (key & 63));
mk->nseen--;
📄 src/backend/access/bark/barkscan.c (L3293-L3296)
This trusts the amrescan nscankeys argument and memcpys that many ScanKeyData into scan->keyData, which was sized by the nkeys passed to bark_beginscan (RelationGetIndexScan allocates exactly nkeys slots). The relaxation vs. index_rescan's Assert(nkeys == scan->numberOfKeys) is only safe in one direction: nscankeys <= beginscan nkeys. Nothing here enforces that, so a wrapper AM that asks for fewer keys at beginscan than it passes to amrescan causes a heap-buffer-overflow write. Since BARK deliberately removes the safety assert to support the DocumentDB wrapper, it should keep a bound of its own, e.g. record the beginscan nkeys in BarkScanOpaque and Assert(nscankeys <= so->maxKeys) (or at least document that the contract is unchecked and misuse corrupts memory). (moderate confidence)
📄 src/backend/access/bark/barkscan.c (L3217-L3222)
bark_mk_begin() is called at line 3220, but so->scanCxt is only assigned at line 3222 (after the call). Since so was palloc0'd, so->scanCxt is NULL inside bark_mk_begin, so the contexts created there with so->scanCxt as parent -- mk->seenCxt, mk->groupCxt, and mk->hitupCxt -- get a NULL parent and become top-level children of TopMemoryContext rather than children of the scan context.
Consequences:
mk->groupCxtis never explicitly deleted inbark_endscan; it relies on being a child ofscanCxtto be reclaimed. With a NULL parent it leaks on every multikey scan, including the normal path.mk->seenCxt/mk->hitupCxtare only reclaimed via the explicitMemoryContextDeleteinbark_endscan; on any error path that tears down the executor's context without callingbark_endscan, they leak for the life of the session (they are not in the scan's context subtree).
(The ginCxt/ginChunkCxt created with CurrentMemoryContext happen to work because that equals the eventual scanCxt, but that too is fragile.)
Move so->scanCxt = CurrentMemoryContext; above the bark_mk_begin() call so the child contexts are attached to the scan context tree.
💡 Suggested change
Before:
so->keyinfo = bark_build_keyinfo(index);
scan->opaque = so;
if (bark_index_extracted_column(index) > 0)
so->mk = bark_mk_begin(scan, bark_index_extracted_column(index));
so->firstCall = true;
so->scanCxt = CurrentMemoryContext;
After:
so->keyinfo = bark_build_keyinfo(index);
scan->opaque = so;
so->scanCxt = CurrentMemoryContext;
if (bark_index_extracted_column(index) > 0)
so->mk = bark_mk_begin(scan, bark_index_extracted_column(index));
so->firstCall = true;
📄 src/backend/access/bark/barkvacuum.c (L119-L122)
These three functions index vacuums[MyProcNumber] with no bound check. The array is sized BARK_VAC_SLOTS = MaxBackends + NUM_AUXILIARY_PROCS, which is exactly FIRST_PREPARED_XACT_PROC_NUMBER; a ProcNumber at or above that (a 2PC dummy proc) would write out of bounds and corrupt shared memory. Real callers (regular backends and autovacuum workers via VACUUM, inserters via split) all have MyProcNumber < MaxBackends, so this is currently safe, but there is no guard enforcing it. Core per-proc code that indexes arrays of this exact size asserts the bound first (e.g. xlogwait.c: Assert(MyProcNumber >= 0 && MyProcNumber < MaxBackends + NUM_AUXILIARY_PROCS)). Add the same assertion in each of bark_vacuum_cycleid, bark_start_vacuum and bark_end_vacuum to document the invariant and catch any future caller from an invalid-ProcNumber context.
💡 Suggested change
Before:
BarkOneVacInfo *vac = &barkvacinfo->vacuums[MyProcNumber];
BarkCycleId result;
LWLockAcquire(BarkVacuumLock, LW_EXCLUSIVE);
After:
BarkOneVacInfo *vac = &barkvacinfo->vacuums[MyProcNumber];
BarkCycleId result;
Assert(MyProcNumber >= 0 && MyProcNumber < BARK_VAC_SLOTS);
LWLockAcquire(BarkVacuumLock, LW_EXCLUSIVE);
📄 src/backend/access/bark/barkvalidate.c (L494-L495)
This sentence is wrapped prematurely: "A BARK class in a btree" is far shorter than the surrounding fill width, leaving an orphaned short line mid-sentence. This reads as an unfinished edit and won't survive a reflow. Re-fill the paragraph to the normal ~78-column comment width.
💡 Suggested change
Before:
* A BARK class in a btree
* family must not get any of them: such a member would make btvalidate
After:
* A BARK class in a btree family must not get any of them: such a member
* would make btvalidate
📄 src/backend/access/bark/barkverify.c (L636-L640)
This overflow-chain walk has no CHECK_FOR_INTERRUPTS(). The loop bound got < fulllen uses fulllen = ref->fulllen, a uint32 read from a potentially-corrupt on-disk entry (up to ~4 GB). With BarkOverflowChunkSize ~8 kB and a cyclic chain (A->B->A...), each iteration still advances got by a chunk but re-reads and re-locks the same buffers, so a crafted/corrupt entry can drive ~500k ReadBuffer/LockBuffer/UnlockReleaseBuffer iterations with the backend uninterruptible. Since amcheck runs on untrusted/corrupt indexes, this is a real DoS/hang hazard. The counterpart walks in verify_bark.c (bark_check_levels, bark_check_downlinks) all call CHECK_FOR_INTERRUPTS() in their long loops; this loop should too. [high confidence]
💡 Suggested change
Before:
while (chainblk != BARK_P_NONE && got < fulllen)
{
Buffer cbuf;
Page cpage;
BarkPageOpaque copaque;
After:
while (chainblk != BARK_P_NONE && got < fulllen)
{
Buffer cbuf;
Page cpage;
BarkPageOpaque copaque;
CHECK_FOR_INTERRUPTS();
📄 src/backend/access/bark/barkverify.c (L169-L170)
This assertion guards keyinfo for ORDER and CONTENTS but not index (rel). Yet the CONTENTS-only helpers dereference v->rel without a NULL check: bark_verify_marker calls bark_index_extracted_column(v->rel), index_getprocid(v->rel, ...) and index_getattr(..., RelationGetDescr(v->rel), ...), and bark_verify_chain calls RelationGetNumberOfBlocks(v->rel)/ReadBuffer(v->rel, ...). The documented contract is that CONTENTS requires a non-NULL index, but nothing enforces it; a caller passing relname+NULL index with CONTENTS set would crash. Tighten the assertion to make the contract explicit. [medium confidence]
💡 Suggested change
Before:
Assert(keyinfo != NULL ||
(flags & (BARK_VERIFY_ORDER | BARK_VERIFY_CONTENTS)) == 0);
After:
Assert(keyinfo != NULL ||
(flags & (BARK_VERIFY_ORDER | BARK_VERIFY_CONTENTS)) == 0);
Assert(index != NULL || (flags & BARK_VERIFY_CONTENTS) == 0);
📄 src/backend/access/bark/barkverify.c (L732-L735)
bark_verify_meta is defined inside the #ifdef USE_ASSERT_CHECKING block (opened at line 720, closed by #endif /* USE_ASSERT_CHECKING */ at line 919), but it is an extern function declared unconditionally in bark.h (lines 1812-1813) and called unconditionally from contrib/amcheck/verify_bark.c:253. amcheck is built in all configurations, not just cassert builds, so a production (--disable-cassert) build will fail to link the amcheck extension with an undefined reference to bark_verify_meta.
The file header itself states this function is "shared by amcheck's bark_index_check and bark_index_parent_check and by the page validator", so it must be available in all builds. Move bark_verify_meta (and its doc comment) out of the USE_ASSERT_CHECKING block so it is always compiled. Note the static helper bark_verify_other_page, used only from the assert-only validator paths, can stay gated. (high confidence)
📄 src/backend/access/heap/heapam_handler.c (L23-L23)
Include out of alphabetical order. access/amlocator.h is inserted after access/genam.h, breaking the alphabetical ordering of this include group (genam, amlocator, heapam). PostgreSQL convention sorts include groups alphabetically; move it above access/genam.h.
Confidence: high (style nit; verifiable against the diff).
💡 Suggested change
Before:
+#include "access/amlocator.h"
After:
#include "access/amlocator.h"
#include "access/genam.h"
📄 src/backend/access/nbtree/nbtvalidate.c (L239-L239)
Minor readability nit (low confidence): the mask (1 << (BTMaxStrategyNumber + 1)) - 1 is an int expression used to mask a uint64 field. The value (63) is well within range so the comparison is correct, but consider matching the ((uint64) 1) idiom used in identify_opfamily_groups() for consistency and to make the width explicit, e.g. (((uint64) 1 << (BTMaxStrategyNumber + 1)) - 1). Functionally equivalent here; purely cosmetic.
💡 Suggested change
Before:
if ((thisgroup->operatorset & ((1 << (BTMaxStrategyNumber + 1)) - 1)) !=
After:
if ((thisgroup->operatorset & (((uint64) 1 << (BTMaxStrategyNumber + 1)) - 1)) !=
| source_up | ||
| watch_file flake.nix | ||
| use flake | ||
| pg_cflags | ||
| use project_steering postgresql aws c | ||
| use git_policy allow-force-push | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
This entire file is personal developer/direnv tooling and must not be committed to a PostgreSQL patch. It is not part of the stated change and will be rejected on pgsql-hackers as an unrelated/local artifact:
- Line 1 hardcodes a machine-specific absolute path (
~/ws/postgres/.envrc) viasource_up, which only works on the author's machine. - The helpers
use flake,pg_cflags,use project_steering,use git_policy allow-force-push, anduse project_mcpare not defined anywhere in the tree; they come from an external, local direnv/nix layout, so this file is non-functional for anyone else. use git_policy allow-force-pushin particular encodes a force-push workflow that is inappropriate to ship in the repository.
Drop this file from the patch (and add it to a local, uncommitted .gitignore if needed). Also note: .envrc is not listed in the repo's .gitignore, so it should not be tracked.
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This .clangd file is personal editor/LSP tooling configuration, not part of the PostgreSQL source tree. Committing it into a patch destined for pgsql-hackers violates the minimal-diff discipline: it is unrelated scaffolding that has nothing to do with the stated purpose of the change, will cause needless merge conflicts, and imposes one contributor's toolchain preferences on everyone. PostgreSQL does not ship editor configs in-tree. Drop this file from the patch and, if desired, add .clangd to a personal (global) gitignore or .git/info/exclude instead.
| - -I. | ||
| - -I../../../../src/include |
There was a problem hiding this comment.
The hardcoded relative include path -I../../../../src/include only resolves correctly when clangd is invoked from a directory exactly four levels below the repo root (e.g. src/backend/access/bark/). From the repo root, src/, or most other directories it points outside the tree and will silently fail to find PostgreSQL headers. This fragile, location-dependent assumption is another reason this file should not be tracked; the CompilationDatabase: build/ entry (compile_commands.json) already supplies correct per-file include paths, making this hand-written -I both redundant and incorrect.
| jobs: | ||
| check: | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
There was a problem hiding this comment.
No permissions block is declared. The header comment states this workflow "opens/updates one issue" in this repo, which requires issues: write. With no top-level permissions key the GITHUB_TOKEN falls back to the repo/org default, which is typically broad (often read/write on many scopes). The sibling fork-sync-upstream.yml added in this same change correctly declares least-privilege permissions: contents: read; this file should follow suit and grant only what the called reusable workflow needs (e.g. issues: write plus id-token: write for the AWS OIDC auth mentioned in the comments). Note a reusable workflow cannot elevate beyond the token it is handed, so least-privilege must be set here in the caller. (high confidence)
| jobs: | |
| check: | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 | |
| permissions: | |
| contents: read | |
| issues: write | |
| id-token: write | |
| jobs: | |
| check: | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
|
|
||
| jobs: | ||
| check: | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
There was a problem hiding this comment.
The reusable workflow is pinned to the mutable tag @v1 on a third-party repo (gburd/ci-workflows). Per the review checklist, third-party actions/workflows should be pinned to a full commit SHA, since tags can be force-moved and hijacked. This matters more here because the called workflow is granted the AWS OIDC role (AWS_ROLE_ARN); a compromised tag would run with those cloud credentials. Consider pinning to a commit SHA (...@<sha>). (moderate confidence — this is consistent with the sibling fork-* workflows, so may be an accepted fork convention.)
| CREATE FUNCTION bark_index_parent_check(index regclass, | ||
| heapallindexed boolean DEFAULT false) |
There was a problem hiding this comment.
DEFAULT false here is asymmetric with the sibling bark_index_check(regclass, boolean) just above (no DEFAULT) and with the upstream bt_index_parent_check convention, where the one-argument behavior is provided by a separate base overload (from 1.0) rather than a parameter default. Since no bark_index_parent_check(regclass) base form exists, this DEFAULT is what makes bark_index_parent_check('idx') (relied on by check_bark.sql) work. That is a legitimate design, but please make the two bark functions consistent: either give bark_index_check the same DEFAULT treatment, or add a one-argument base form and drop the DEFAULT, so the overload story reads uniformly. (moderate confidence)
| CREATE FUNCTION bark_chk_bigstr(s int, n int) RETURNS text | ||
| LANGUAGE sql IMMUTABLE AS | ||
| $$ SELECT substr(string_agg(md5(s::text || g::text), ''), 1, n) | ||
| FROM generate_series(1, (n + 31) / 32) g $$; | ||
| CREATE TABLE bark_check_inc (k int, payload text); |
There was a problem hiding this comment.
The bark_chk_bigstr(int, int) helper is created here (line 112) with a body byte-for-byte identical to the one defined earlier for the oversized-key section and dropped at line 106. Dropping and immediately re-creating the same function is unnecessary churn. Consider defining it once near the first use and dropping it once after the last consumer (the INCLUDE-payload section), rather than tearing it down and rebuilding it.
| DROP EXTENSION amcheck; | ||
| CREATE EXTENSION amcheck VERSION '1.6'; | ||
| SELECT bark_index_check('bark_check_empty_idx'); | ||
| ALTER EXTENSION amcheck UPDATE TO '1.7'; |
There was a problem hiding this comment.
This block leaves global extension state mutated and not restored: check.sql installs amcheck at the latest version (1.8), but check_bark.sql ends by dropping it and re-creating at 1.6, then only upgrading to 1.7. The extension is left at 1.7 (confirmed by expected/check_bark.out tail). This works today only because the next REGRESS test, check_bark_unique.sql, happens to open with ALTER EXTENSION amcheck UPDATE TO '1.8'; -- a hidden cross-test coupling. Running check_bark in isolation, or reordering the REGRESS list, would leave downstream tests on 1.7 and break the ones that need the 1.8 overloads. A test that downgrades a shared fixture should restore it; add ALTER EXTENSION amcheck UPDATE TO '1.8'; at the end of this test. (moderate confidence)
| DROP EXTENSION amcheck; | |
| CREATE EXTENSION amcheck VERSION '1.6'; | |
| SELECT bark_index_check('bark_check_empty_idx'); | |
| ALTER EXTENSION amcheck UPDATE TO '1.7'; | |
| DROP EXTENSION amcheck; | |
| CREATE EXTENSION amcheck VERSION '1.6'; | |
| SELECT bark_index_check('bark_check_empty_idx'); | |
| ALTER EXTENSION amcheck UPDATE TO '1.7'; |
| SELECT bark_wide_shapes() AS two_rows; | ||
| INSERT INTO bark_wide_dup SELECT bark_wide_key(1), g FROM generate_series(3, 20) g; | ||
| INSERT INTO bark_wide_dup SELECT bark_wide_key(2), g FROM generate_series(1, 1000) g; | ||
| SELECT bark_wide_shapes() <@ '{LIST,POSTING}' AS coalesced; |
There was a problem hiding this comment.
The <@ (is contained by) check is a weak assertion: an empty array is contained by any array, so if bark_wide_shapes() ever returned {} (e.g. the p.type IN ('leaf','root')/shape <> 'PIVOT' filter matched nothing, or a shape literal changed), this would still report t and silently mask a real coalescing failure instead of detecting it. Consider also asserting the result is non-empty, e.g. bark_wide_shapes() <@ '{LIST,POSTING}' AND bark_wide_shapes() <> '{}' (or array_length(bark_wide_shapes(), 1) > 0). The preceding two_rows check returns a non-empty {LIST}, so the pipeline works today, but the final assertion itself is self-masking.
| -- The widest keys that stay inline coalesce as any other: rows of one key | ||
| -- make a LIST (forming one tries the coded form first), and a thousand | ||
| -- LIST or POSTING entries (which, depends on the block size), each with | ||
| -- the whole key as its key part. The key | ||
| -- is a quarter of a page, up to 6000 bytes, so that with 32 kB blocks the | ||
| -- entry's body starts past 4095 bytes. |
There was a problem hiding this comment.
This comment block is garbled and ungrammatical: the clause "a thousand LIST or POSTING entries (which, depends on the block size), each with the whole key as its key part. The key" is a dangling/incomplete sentence that does not parse. PostgreSQL review standards require comments that clearly describe current behavior. Please rewrite to state plainly: two rows of the same wide key coalesce into a LIST entry; many duplicates coalesce into LIST or POSTING depending on block size; the key is a quarter of a page (capped at 6000 bytes) so its body starts past offset 4095 on 32 kB blocks.
|
Addressed in the v1 series now on branch Fixed, among others:
Rejected (examples): |
dcfecd6 to
1c00723
Compare
There was a problem hiding this comment.
🔍 OCR found 45 issue(s).
- 25 inline, 20 in summary (inline capped at 25)
⚠️ 2 warning(s) during review
📄 src/backend/lib/sbm.c
Misleading comment: these comparators are claimed to be called from "the planner's relid set-algebra, where these comparators run millions of times." The planner uses Bitmapset for relid sets, not Sbm, and a codebase-wide search finds zero callers of sbm_equals/sbm_overlap/sbm_subset_compare/sbm_compare/sbm_is_subset anywhere (backend, contrib, bark AM, or test modules). The performance justification for the small/small fast path is aspirational. Please correct the comment to reflect that these paths currently have no in-tree caller, or defer the comparator API until a consumer exists (YAGNI).
📄 src/test/modules/test_versioned_tableam/sql/bark_mk_versioned.sql
Same hardcoded 8192 block-size assumption as above; breaks on non-default BLCKSZ builds. Use current_setting('block_size')::int.
📄 src/test/modules/test_versioned_tableam/sql/bark_mk_versioned.sql
Same hardcoded 8192 block-size assumption in tmk_state(); breaks on non-default BLCKSZ builds. Use current_setting('block_size')::int.
📄 src/test/modules/bark/sql/bark_btree_impl.sql (L401-L401)
REPACK (VERBOSE) emits an INFO line that includes the heap page count (the committed expected output has ... 10000 nonremovable row versions in 84 pages). That page count depends on heap tuple packing and is MAXALIGN-sensitive, so it will diverge on platforms where MAXALIGN is 4 (32-bit) vs 8, breaking the regression diff. This is why the core cluster/vacuum regression tests never emit a successful VERBOSE page count. The meaningful assertion here is which scan method REPACK chose (using sequential scan and sort vs using index scan), not the page count. Consider dropping VERBOSE and instead asserting the scan path another way (e.g. a filtered message or a verdict function), or wrapping the REPACK so the variable page-count line is suppressed/normalized. The same applies to the second REPACK (VERBOSE) below.
📄 src/backend/nodes/tidbitmap.c (L728-L731)
This comment is factually wrong for this tree. It claims the hook "is a lock-free VM probe for heap", but heap's table AM handler sets bitmap_and_inexact = NULL (heapam_handler.c), so heap never takes this union-and-recheck path and performs no VM probe. No AM anywhere in the change set installs a non-NULL bitmap_and_inexact/bitmap_or_inexact, so the entire inexact-combine mechanism currently has no provider. Either correct the comment to describe the actual (NULL-for-heap) behavior, or defer this scaffolding until the AM that consumes it lands.
📄 src/backend/lib/sbm.c (L7887-L7888)
Large stack frame on a path advertised as hot. Each SbmRunIter embeds two uint64[SBM_CHUNK_MAX_CAPACITY/2 + 1] (1025-element) buffers, ~16 KB each, so declaring two iterators here yields a ~32 KB stack frame. sbm_equals, sbm_compare, sbm_subset_compare, sbm_overlap and sbm_run_pair_counts all do this. PostgreSQL backends run on a bounded stack (max_stack_depth, typically 2 MB with a safety margin), so ~32 KB frames on comparator paths meant to be called "millions of times" are a real concern. Consider heap-allocating the run buffers (palloc in a short-lived context) or bounding them to a far smaller fixed size with spill handling.
📄 src/test/modules/bark/sql/bark_scan_members.sql (L296-L296)
This file leaves its session-level helper functions (sm_cmp, sm_plan, sm_entries, sm_fetch, sm_drain) undropped, whereas the sibling bark_lpdead.sql carefully drops every helper it creates at the end. Not a correctness or cross-test issue (each regress .sql runs in its own session against a throwaway temp-install, and the REGRESS schedule is serial with runningcheck: false), but it is a consistency wart vs. the project convention of cleaning up what a test creates. Consider adding a matching DROP FUNCTION list before/after the final DROP TABLE.
💡 Suggested change
Before:
DROP TABLE sm, sm_outer, sm_want, sm_want3, sm_leaves;
After:
DROP FUNCTION sm_cmp(text), sm_plan(text), sm_entries(), sm_fetch(int, text),
sm_drain();
DROP TABLE sm, sm_outer, sm_want, sm_want3, sm_leaves;
📄 src/test/modules/bark/sql/bark_knn_inttypes.sql (L35-L37)
This test's data comparison does not actually exercise the KNN index path, so a wrong KNN index result could go undetected.
Flow: line with INTO knn (the only execution under enable_seqscan=off, enable_sort=off) is a dead store — knn is overwritten below. The two SET LOCAL ... = on statements re-enable seqscan and sort before both ref and the final knn are computed. Neither ref nor the final knn query forces the index, so on these small tables the planner will almost certainly serve both via seqscan + top-N sort. The final comparison (knn IS NOT DISTINCT FROM ref) then compares two seqscan+sort results, which agree regardless of whether the BARK KNN index scan is correct.
The EXPLAIN check above only verifies plan shape for q, not the result values. Net: this test would still pass with the int2/int8 KNN index logic broken — defeating its purpose (per the user rules, a test that passes with the feature reverted is worthless).
Fix: keep seqscan/sort disabled while computing the index-scan result used in the comparison (compare the line-35 knn against ref), or force the index for the result query, and drop the redundant first INTO knn.
📄 src/backend/lib/sbm.c (L8828-L8841)
DoS/performance asymmetry: sbm_remove_range iterates bit-by-bit over [lo, hi), making it O(hi - lo). sbm_add_range was deliberately rewritten (see its header comment: the old per-bit loop was "O(hi - lo) -- a [0, 1e6) add cost ~14 ms") to be O(chunks) via the run emitter, precisely to avoid this. On an RLE-compressed map a huge range here can loop up to ~2^64 times from a tiny input -- an amplification hazard with untrusted spans. Rewrite to operate at chunk/run granularity like sbm_add_range (difference against the literal run), or at minimum document the cost in sbm.h. Confidence: high.
📄 src/backend/lib/sbm.c (L9775-L9790)
Same O(hi - lo) amplification hazard as sbm_remove_range, aggravated by an extra sbm_contains lookup per bit. sbm_add_range avoids this with a run/chunk-level rewrite; sbm_flip_range still touches every bit, so a large range on a compressed map loops up to ~2^64 times. Rewrite to operate at run granularity (flip = xor against the literal run), or document the linear cost in sbm.h. Confidence: high.
📄 src/backend/lib/sbm.c (L2187-L2188)
Redundant arithmetic: 63 - (63 - pg_leftmost_one_pos64(w[i])) simplifies to pg_leftmost_one_pos64(w[i]). The double subtraction obscures intent with no effect. Same pattern also appears in sbm_try_demote and sbm_prev_member. Confidence: high.
💡 Suggested change
Before:
return (uint64) i * 64 + (63 -
(63 - pg_leftmost_one_pos64(w[i])));
After:
return (uint64) i * 64 + (uint64) pg_leftmost_one_pos64(w[i]);
📄 src/test/modules/test_sbm/test_sbm--1.0.sql (L260-L260)
test_sbm_benchmark is a manual micro-benchmark SRF that is never invoked by sql/test_sbm.sql or any TAP test in the tree (only referenced from BARK-Design.mediawiki). As committed it is speculative scaffolding: ~200 lines of C plus the Tuplestore/instr_time/TIDBitmap/Bitmapset machinery it drags in, exposed as a SQL function with no automated caller. Performance claims in its header comment have no reproducible harness in the suite. Consider dropping it from the test module (a benchmark belongs in a throwaway/manual tool, not the committed regression surface), or at minimum justify why it ships here. (confidence: high)
📄 src/test/modules/test_sbm/test_sbm.c (L1839-L1840)
This detailed benchmark header comment (lines ~1839-1861) is orphaned: it documents test_sbm_benchmark, whose definition lives ~2800 lines below (near line 4674), with unrelated code in between. A reader here sees a long comment describing build/scan curves followed immediately by the unrelated 'Encoding-transition' section, with no function. If the benchmark is kept, move this comment to sit directly above the function body; if the benchmark is dropped (recommended, see the SQL declaration), remove this comment too. Also note the doubled blank line at the end of the block. (confidence: high)
📄 src/test/modules/test_sbm/test_sbm.c (L223-L225)
Inconsistent handling of invalid (negative) input: test_sbm_contains silently returns false for idx < 0, whereas the mutator wrappers (test_sbm_add, test_sbm_remove, test_sbm_assign, etc.) elog(ERROR, ...) for the same negative-member case. The SQL test SELECT test_sbm_contains('{1,3,5}', -1); depends on this quiet-false behavior. Treating the same invalid input two different ways across one API is a POLA violation and makes the negative-index test expectations harder to reason about. Consider erroring consistently (and dropping the negative-returns-false probe), or document why contains intentionally differs. (confidence: medium)
📄 src/test/modules/test_versioned_tableam/t/004_bark_vacuum.pl (L39-L39)
This reads $ENV{enable_injection_points} without a defined-ness guard, unlike the sibling files in this same module (002_bark_tags.pl line 318 and 008_bark_mktags.pl line 211 both use ($ENV{enable_injection_points} // '') eq 'yes'). Under the standard Make (export enable_injection_points) and meson harnesses the variable is always defined ('yes'/'no'), so this is safe there; but because the file uses use warnings FATAL => 'all', running the test directly via prove without the harness setting the env var makes undef eq 'yes' a fatal "Use of uninitialized value" error, aborting the whole test. Match the sibling files for robustness and consistency.
💡 Suggested change
Before:
my $inj = $ENV{enable_injection_points} eq 'yes';
After:
my $inj = ($ENV{enable_injection_points} // '') eq 'yes';
📄 src/test/modules/test_sbm/test_sbm.c (L347-L351)
test_sbm_assign never exercises sbm_assign(map, idx, true). For value=true it calls sbm_add_grow and only calls sbm_assign when value=false. The SQL test SELECT test_sbm_assign('{1,3}', 2, true) therefore does not test the set-bit path of sbm_assign at all — a bug in that path would pass. Either route the value=true case through sbm_assign (growing the buffer first so it has headroom, then sbm_assign(map, idx, true)), or drop the misleading claim that this wrapper covers sbm_assign. (moderate confidence)
📄 src/test/modules/test_sbm/test_sbm.c (L258-L264)
test_sbm_fill_factor is dead code: it is a byte-for-byte duplicate of test_sbm_fill_factor_val (both just call sbm_fill_factor), and only _val is invoked by sql/test_sbm.sql. The test_sbm_fill_factor(bigint[]) SQL declaration is likewise never called. Drop this wrapper and its SQL CREATE FUNCTION (keep _val), per the YAGNI/minimalism discipline. (high confidence)
📄 src/test/modules/test_sbm/test_sbm--1.0.sql (L30-L32)
Dead declaration: test_sbm_fill_factor(bigint[]) is never called by sql/test_sbm.sql (only test_sbm_fill_factor_val is), and its C body is an exact duplicate of test_sbm_fill_factor_val. Remove this CREATE FUNCTION together with the unused C wrapper. (high confidence)
📄 src/test/regress/sql/opr_sanity.sql (L1260-L1261)
The original single-line comment -- amopmethod must match owning opfamily's opfmethod was left in place while the expanded comment was added below it, leaving a stale duplicate (both lines now appear in expected/opr_sanity.out at 1982-1987). Compare with the opcmethod block above, where the original one-liner was correctly replaced. Remove the leftover line so only the new comment remains. [high confidence]
💡 Suggested change
Before:
-- amopmethod must match owning opfamily's opfmethod, except that a
+-- btree-compatible access method may add its own ORDER BY (KNN) operators to
After:
-- amopmethod must match owning opfamily's opfmethod, except that a
-- btree-compatible access method may add its own ORDER BY (KNN) operators to
📄 src/test/modules/test_versioned_tableam/sql/bark_mk_versioned.sql (L404-L407)
Hardcoded block size 8192 assumes the default BLCKSZ. On a non-default build (e.g. --with-blocksize=16), pg_relation_size('tmd_doc') / 8192 overcounts the number of blocks, so the LATERAL bark_page_items(...) call will be handed a block number past the end of the relation and raise an error, failing the test. The same file already uses current_setting('block_size')::int at line 140, so these three occurrences (lines 404, 411, 499) are inconsistent. Use the GUC here too.
💡 Suggested change
Before:
FROM tmd t, generate_series(1, pg_relation_size('tmd_doc') / 8192 - 1) b,
LATERAL bark_page_items('tmd_doc', b) i
WHERE t.id = 1 AND i.htid = t.ctid;
RESET test_versioned_tableam.stale_tags;
After:
FROM tmd t, generate_series(1, pg_relation_size('tmd_doc') / current_setting('block_size')::int - 1) b,
LATERAL bark_page_items('tmd_doc', b) i
WHERE t.id = 1 AND i.htid = t.ctid;
RESET test_versioned_tableam.stale_tags;
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
This GDB script hardcodes absolute source line numbers that are already wrong against the current tree. For example break heapam.c:3210 is annotated "Main entry: heap_update", but line 3210 of heapam.c is actually the tail of heap_delete() (if (old_key_tuple != NULL && old_key_copied) ... return TM_Ok;). Line-number breakpoints rot on every edit; combined with the fact that pruneheap.c / heapam_xlog.c are not part of this change set, these breakpoints are a correctness/maintainability footgun. More fundamentally, this is personal debugging scaffolding that should not be committed to a PostgreSQL patch (pgsql-hackers/commitfest) — it belongs in a local ignore, not the tree.
| # ========================================================================= | ||
|
|
||
| # Predict augmented tuple size (returns 0 if t_hoff would overflow) | ||
| break heap_hot_indexed_tuple_size |
There was a problem hiding this comment.
Several symbol breakpoints reference functions that do not exist anywhere in this tree/change set: heap_hot_indexed_create_tuple, heap_hot_indexed_tuple_size, heap_hot_indexed_serialize_bitmap (and heap_xlog_indexed_update below). Sourcing this file will emit "Function ... not defined" errors. The only symbol that actually resolves is heap_hot_search_buffer. This reinforces that the script is stale/personal and should not be committed.
| CompileFlags: | ||
| CompilationDatabase: build/ # Search build/ directory for compile_commands.json |
There was a problem hiding this comment.
.clangd is personal editor/LSP configuration with a hardcoded build path (CompilationDatabase: build/) and an include path tailored to one layout (-I../../../../src/include). This is author-specific scaffolding that does not belong in a PostgreSQL patch destined for pgsql-hackers/commitfest; it should live in a local (untracked) ignore rather than the committed tree.
| pg_cflags | ||
| use project_steering postgresql aws c | ||
| use git_policy allow-force-push | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
This direnv file depends on an external parent config (source_up) and non-standard custom stdlib helpers (pg_cflags, use project_steering ..., use git_policy ..., use project_mcp ...) that exist only on the author's machine; it will fail on any other checkout. Note also the apparent typo on the last line: use project_mcp postgresq (missing trailing l?), which makes it broken even for the author. Machine-specific developer config like this should not be committed.
| --dlpath="$bdir/src/test/regress" \ | ||
| --outputdir="$outdir" \ | ||
| --temp-instance="$outdir/tmp" \ | ||
| --port=40099 \ |
There was a problem hiding this comment.
pg-test-one hardcodes --port=40099 and --temp-instance, so two concurrent invocations (or a run overlapping with another checkout's regress) will collide on the port. Prefer letting pg_regress pick a free port rather than a fixed one. More broadly this is personal dev tooling that does not belong in a committed PostgreSQL patch.
| /* | ||
| * Without a tuple, _bt_mkscankey() reads only relcache data built from | ||
| * the index's btree operator classes and pg_index row, never the index's | ||
| * pages, so this works for a btree index whatever code implements it. | ||
| */ | ||
| indexScanKey = _bt_mkscankey(indexRel, NULL); |
There was a problem hiding this comment.
This is a pure comment addition on an otherwise-unchanged line. The justification it adds ("this works for a btree index whatever code implements it") is only meaningful once alternative btree implementations exist (the bark feature). In a minimal, focused plumbing patch this reads as documentation churn for an unrelated feature. Consider dropping it here and introducing it with the patch that actually adds alternative btree implementations. [low confidence; minimal-diff discipline]
| * If the table's locator is versioned (see amlocator.h), an index that can | ||
| * store tags (index_can_version()) puts each entry's version tag in its | ||
| * TM_IndexStatus, and an | ||
| * entry is deletable when no snapshot can see a version for which the entry |
There was a problem hiding this comment.
This added comment block is not pgindent-clean: lines 257-259 wrap awkwardly ("...version tag in its / TM_IndexStatus, and an / entry is deletable...") and the line ending "...offers a TID once. The tag is in TM_IndexStatus, not in" runs well past the usual comment width while the following lines are short. Run this through pgindent so the block reflows consistently before posting.
| afterTriggers.maximages = Max(64, afterTriggers.maximages * 2); | ||
| afterTriggers.images = afterTriggers.images ? | ||
| repalloc_huge(afterTriggers.images, | ||
| afterTriggers.maximages * sizeof(AfterTriggerImage)) : | ||
| MemoryContextAllocHuge(afterTriggers.event_cxt, | ||
| afterTriggers.maximages * sizeof(AfterTriggerImage)); |
There was a problem hiding this comment.
The images[] metadata array grows unbounded via repalloc_huge and is never spilled, even after tuples start spilling to the BufFile. For a large bulk INSERT/UPDATE on an in-place AM, this holds one AfterTriggerImage (~24 bytes) per row in memory for the whole transaction, defeating the work_mem spill that only applies to the tuple bodies. The memory cap (work_mem) bounds image_mem but not the index array, so a multi-billion-row statement can still OOM on the metadata alone. Additionally, maximages * 2 is uint32 arithmetic: once maximages exceeds 2^30 the doubling overflows uint32 and Max(64, ...) can yield a value <= nimages, leading to an out-of-bounds write at images[nimages]. Consider bounding/spilling the index array too, or at least computing the new size in Size and guarding against overflow.
| appendPQExpBuffer(&buf, " a.amname, c2.relname, " | ||
| "pg_catalog.pg_get_expr(i.indpred, i.indrelid, true)\n" | ||
| "pg_catalog.pg_get_expr(i.indpred, i.indrelid, true),\n"); |
There was a problem hiding this comment.
This appendPQExpBuffer call no longer has any format arguments (the %s that previously followed moved into the separate call on line 2259). Using the printf-style appendPQExpBuffer with a format-free literal is inconsistent with the surrounding conventions (the adjacent new calls at lines 2254/2257 correctly use appendPQExpBufferStr). Switch to appendPQExpBufferStr here.
| appendPQExpBuffer(&buf, " a.amname, c2.relname, " | |
| "pg_catalog.pg_get_expr(i.indpred, i.indrelid, true)\n" | |
| "pg_catalog.pg_get_expr(i.indpred, i.indrelid, true),\n"); | |
| appendPQExpBufferStr(&buf, " a.amname, c2.relname, " | |
| "pg_catalog.pg_get_expr(i.indpred, i.indrelid, true),\n"); |
| * intersect exactly, as always. The hook is a lock-free VM probe | ||
| * for heap and NULL for every table without selective-indexed | ||
| * indexes, so the common path is unchanged. |
There was a problem hiding this comment.
This comment is factually wrong for the tree it ships in. It claims "The hook is a lock-free VM probe for heap," but heap's table AM sets bitmap_and_inexact = NULL (heapam_handler.c), and no AM in this changeset provides a non-NULL implementation. For heap the if condition is always false and the exact-intersect path is always taken; there is no VM probe. A comment describing behavior that does not exist in the tree will mislead maintainers. Describe the actual contract (NULL hook => exact intersection, always, for heap) or drop the heap-specific claim until an AM that provides the hook lands.
Confidence: high.
There was a problem hiding this comment.
🔍 OCR found 47 issue(s).
- 25 inline, 22 in summary (inline capped at 25)
⚠️ 1 warning(s) during review
📄 src/backend/utils/adt/jsonfuncs.c (L6281-L6290)
Out-of-bounds args read for variadic callers. jsonb_extract_path/jsonb_extract_path_text are variadic (provariadic => 'text' in pg_proc.dat). A call with zero variadic arguments, e.g. jsonb_extract_path(col), is legal and produces a FuncExpr whose args list has length 1 (the parser only wraps variadic args into an array when nvargs > 0; see parse_func.c). The executor's ask_support() (execSubattr.c) hands this FuncExpr's args straight to this support function with no arity check, so lsecond(args) here dereferences list_nth_cell(l, 1) on a 1-element list: an Assert failure in assert-enabled builds and an out-of-bounds read in production. Guard on list_length(args) before using lsecond().
📄 src/backend/utils/adt/jsonfuncs.c (L6350-L6353)
Same out-of-bounds hazard for the variadic jsonb_delete. F_JSONB_DELETE_JSONB__TEXT maps to jsonb_delete(jsonb, _text) (oid 3343), which is variadic. jsonb_delete(col) with zero variadic args yields a 1-element args list, so lsecond(args) reads past the list end (Assert trip / OOB read). Note the non-variadic F_JSONB_DELETE_JSONB_TEXT shares this line and is safe, but the variadic variant is not. Check list_length(args) before lsecond() (or only run this branch for the two-element shape).
📄 src/backend/utils/misc/guc_tables.c (L32-L33)
Unnecessary include of access/bark.h. guc_tables.c references no symbol from bark.h: the new default_btree_implementation variable is declared in access/amapi.h (already included just above) and its check hook check_default_btree_implementation is in utils/guc_hooks.h. Pulling a new-AM header into this core (partly generated) file is an over-broad dependency and extra diff noise; drop it unless a concrete symbol requires it.
📄 src/include/lib/sbm.h (L607-L608)
The usage example does not match the declared signature. sbm_add_grow_embedded takes four parameters (void *outer, size_t sbm_offset, size_t data_offset, uint64 idx) (lines 615-618), but the example passes five arguments, with a spurious &node->sbm. The sbm_offset already locates the embedded Sbm, so there is no separate map-pointer argument. As written the example would not compile; drop the &node->sbm argument.
💡 Suggested change
Before:
* node = sbm_add_grow_embedded(node, offsetof(Node, sbm),
* offsetof(Node, data), &node->sbm, x);
After:
* node = sbm_add_grow_embedded(node, offsetof(Node, sbm),
* offsetof(Node, data), x);
📄 src/bin/pg_amcheck/pg_amcheck.c (L2181-L2183)
Unlike the primary index CTE (which gained AND i.indimplam IN (%s) at the end), this toast_index CTE still filters only on c.relam = BTREE_AM_OID and hardcodes is_bark = false, without applying the implams filter. In practice this is safe because TOAST indexes are always system-created plain nbtree indexes (indimplam = 0), so the gap is not reachable today. Noting it for consistency: if a future path ever allowed a TOAST index implemented by an AM amcheck cannot read, it would be dispatched to bt_index_check() here. Consider mirroring the implam filter for symmetry, or add a comment explaining why it is intentionally omitted. [low confidence, maintainability]
📄 src/backend/lib/sbm.c (L6489-L6496)
Performance: sbm_append_rle_chunk walks the entire result chunk stream from the head on every call just to locate the last chunk for inline coalescing (for (i = 0; i < count; i++) ... last_p = p). This is O(chunks) per append. It is called once per emitted RLE chunk inside the sbm_union / sbm_intersection / sbm_emit_rle merge loops, which can produce O(chunks) RLE output chunks, so a sequence of N appends is O(N^2). This directly contradicts the "two-pointer chunk merge walk for O(chunks) performance" goal documented on sbm_union/sbm_intersection, and sbm_append_sparse_chunk (just above) has no such walk. Cache the byte offset of the last-appended chunk in the result map (or in the SbmEmitter) and resume from there, mirroring how SbmCursor already caches a chunk offset, so coalescing is O(1) per append.
📄 src/test/regress/pg_regress.c (L1246-L1246)
This execl->execlp change looks unrelated to the test-module additions in this PR and should be split into its own focused commit (minimal-diff discipline).
More substantively, it is also a no-op in every standard build while introducing a latent footgun: shellprog is SHELLPROG, which is $(SHELL) from the Makefiles and /bin/sh from meson -- always an absolute path. When the program name contains a slash, execlp performs no PATH search and behaves identically to execl, so this buys nothing today. But should $(SHELL) ever resolve to a bare name, execlp would silently switch to a PATH-dependent lookup of the shell binary -- a portability/security behavior change. If there is a concrete reason the shell must be resolved via PATH, state it in the commit message; otherwise drop this hunk.
💡 Suggested change
Before:
execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
After:
execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
📄 src/test/modules/bark/sql/bark_walinspect.sql (L32-L34)
The logged column of this check (expected t for every type except CREATE_ROOT) depends on the workload actually producing every WAL record type, which in turn depends on BARK's page-fill math and split heuristics. setseed() makes this deterministic across platforms for a given server version, so this is not a portability bug. However, the comment itself cites concrete, version-fragile constants ("300-byte keys fit about 25 to a page") that any future change to BARK's on-page layout, MAXALIGN handling, or split thresholds can silently break, flipping a logged value to f. That is an acceptable failure mode (it fails loudly, not vacuously), but please make sure the design thread notes that this test is intentionally tied to internal layout and must be rebalanced when those heuristics change. No code change required.
📄 src/test/modules/bark/sql/bark_walinspect.sql (L193-L193)
The INSERT_SWAP regex is anchored (...\)$) and does not permit the optional , divide for tag: %u suffix that barkdesc.c appends when an INSERT_SWAP record carries a version tag (bark_divide_entry in barkinsert.c). That is correct here because the only INSERT_SWAP producer in this workload is the plain-heap table wi_hkp, which never registers a tag, so the branch is unreachable. Worth a one-line note in the comment that the tagged INSERT_SWAP variant (the divide for tag suffix) is deliberately NOT covered by this file, so a future reader does not assume the regex validates the full description.
📄 src/test/modules/Makefile (L38-L39)
test_versioned_tableam is out of alphabetical order. This SUBDIRS list is maintained alphabetically (test_tidstore, test_wait_lsn, ...), and test_versioned_tableam belongs between test_tidstore (line 61) and test_wait_lsn (line 62), not here after test_index_impl. Placing it here hurts maintainability and will draw a pgindent/review nit. (test_index_impl itself is correctly placed.)
💡 Suggested change
Before:
test_index_impl \
test_versioned_tableam \
After:
test_index_impl \
📄 src/test/modules/bark/sql/bark_btree_impl.sql (L401-L401)
REPACK (VERBOSE) with client_min_messages = info captures the line found 0 removable, 10000 nonremovable row versions in NN pages into the expected output (expected/bark_btree_impl.out lines 906/919 show 84 pages). That page count is a function of BLCKSZ and heap tuple packing, so the expected output diverges on non-default block sizes. The core regression suite deliberately never captures this VERBOSE line for exactly this reason. Prefer dropping VERBOSE and relying on the physically_ordered() helper already used here to prove the sort- vs index-scan repack ordering, or filter the page-count line, so the output stays portable.
📄 src/test/modules/bark/sql/bark_walinspect.sql (L204-L204)
vacuum_image does not verify what the header comment claims ("a VACUUM record that has a full-page image instead of them"). barkdesc.c only appends , deleted: when XLogRecHasBlockData(record, 0) is true, i.e. when the offsets array was logged. But bark_delitems_vacuum is also invoked with ndelete == nupdated == nuntagged == 0 purely to clear the cycle id (bark.c, the clearcycleid branch of the ndelete > 0 || nupdated > 0 || nuntagged > 0 || clearcycleid call site), and such a record registers no block data and so also omits , deleted:. So description !~ ', deleted:' matches a cycle-id-only VACUUM record just as well as an FPI one. The assertion therefore stays t even if FPIs were never logged (e.g. under full_page_writes = off), so it cannot catch the regression it is meant to. Tighten it to actually require an FPI, e.g. correlate with pg_get_wal_block_info / pg_get_wal_fpi_info for the record, or at minimum restrict the match to records whose ndeleted/nupdated are non-zero yet still lack the offset arrays.
📄 src/test/modules/bark/t/009_validate_single_user.pl (L82-L82)
$ret is captured but never used; only $stdout and $stderr are read afterward. Drop the unused variable to keep the diff minimal and avoid a dead binding.
(low confidence this matters functionally; style/maintainability only.)
💡 Suggested change
Before:
my ($ret, $stdout, $stderr) = $node->psql('postgres',
After:
my ($stdout, $stderr) = ($node->psql('postgres',
📄 src/test/modules/bark/t/009_validate_single_user.pl (L82-L82)
Correction to my earlier note: $node->psql returns (ret, stdout, stderr), so the first element must be kept positionally. Use undef rather than removing the slot, otherwise $stdout would capture the return code. The $ret binding is only unused, not harmful.
my (undef, $stdout, $stderr) = $node->psql('postgres', ...);
(low confidence; style only.)
📄 src/test/modules/bark/t/010_bottomup_wal_level.pl (L47-L49)
bark_index_check raises an ERROR on corruption (verify_bark.c uses ereport(ERROR)), so safe_psql already fails the whole test file if the index is corrupt -- the detection is real. But the following pass() always records a passing assertion regardless of what safe_psql returned, which is misleading: it suggests the assertion verified something when it is unconditional. Consider asserting the result is empty instead, e.g.
is($node->safe_psql('postgres', "SELECT bark_index_check('t_k', true), bark_index_check('t_v', true)"), '|', "$what: bark_index_check");
so the test line actually checks the return value rather than relying solely on die-on-error. (low confidence; test clarity.)
📄 src/test/modules/bark/sql/bark_scan_members.sql (L298-L298)
sm_got is created as a TEMP table (line 138) but is not dropped at the end, while the sibling temp tables sm, sm_outer, sm_want, sm_want3, and sm_leaves are explicitly dropped on line 298 (and sm_alt on line 250). Being a TEMP table it is auto-reaped at session end, so this is not a leak, but it is an inconsistency with the test's own cleanup convention ("DROP objects the test creates"). Add it to the final DROP TABLE list for consistency.
💡 Suggested change
Before:
DROP TABLE sm, sm_outer, sm_want, sm_want3, sm_leaves;
After:
DROP TABLE sm, sm_outer, sm_got, sm_want, sm_want3, sm_leaves;
📄 src/test/modules/bark/t/012_codedlist_recovery.pl (L113-L113)
This bark_index_check call is unwrapped: unlike the pass() at line 73, it produces no test point. It only guards against regressions by virtue of safe_psql dying (bailing out the whole TAP file) if amcheck raises an ERROR. That is an inconsistent and opaque signal. Prefer an explicit is($node->safe_psql(... bark_index_check('clp_k', true)), '', 'crash 1: clp_k passes amcheck') so a corruption surfaces as a clean failing assertion rather than a test-file bail-out. (low confidence it hides a real bug; it is a test-quality issue.)
💡 Suggested change
Before:
$node->safe_psql('postgres', "SELECT bark_index_check('clp_k', true)");
After:
is($node->safe_psql('postgres', "SELECT bark_index_check('clp_k', true)"),
'', 'crash 1: clp_k passes amcheck');
📄 src/test/modules/bark/t/010_bottomup_wal_level.pl (L45-L46)
The is($idx, $seq) / is($bmp, $seq) assertions can pass vacuously. SET enable_seqscan = off only penalizes the seqscan cost; it does not guarantee an index/bitmap plan is actually chosen. If BARK's scan path is broken or disabled, the planner can fall back to a seqscan and $idx/$bmp would trivially equal $seq, so the test still passes without exercising the index. The crash/recovery siblings (003-008) guard this with an EXPLAIN (COSTS OFF) ... =~ qr/Index Scan/ check for exactly this reason. Add an EXPLAIN plan guard here so these assertions provably run an index/bitmap scan.
📄 src/test/modules/bark/t/001_empty_index_race.pl (L90-L90)
Misleading capture. BackgroundPsql::query() in list context returns ($output, $ret) where $ret is a 0/1 status flag, not the stderr text. So $err here is the status integer, not the error string, and the comment "collect its error" is inaccurate. Both $out and $err are unused -- the following like() reads $s1->{stderr} directly. Call $s1->query('SELECT 1') in void/scalar context (or drop the bindings) to avoid the misleading names.
💡 Suggested change
Before:
my ($out, $err) = $s1->query('SELECT 1');
After:
$s1->query('SELECT 1');
📄 src/test/modules/bark/t/016_version2.pl (L41-L41)
$blksz is computed but never used. The meta-page seek below uses the hardcoded 24 + 4 offset rather than this value. Remove the unused variable (and the SHOW block_size round-trip) to keep the test minimal.
📄 src/test/modules/bark_multikey/sql/bark_multikey_ginmerge.sql (L12-L13)
This data generator feeds absolute result counts into the committed expected output: the n column of the gm_res comparison (e.g. 6466, 7768, 2211, 85) is cardinality(seq) over rows built from floor(200 * random() ^ 3). While setseed()+random() is deterministic across platforms, the ^ operator maps to libm pow(), which is not guaranteed bit-identical across architectures/libc. A one-ULP difference in pow() can push floor(200 * x^3) across an integer boundary for a few rows, shifting the committed n values and breaking the buildfarm. The bark_ok/gin_ok booleans are self-consistent and safe, but the exact n values are not portable. Consider generating the array elements with integer/multiply-only arithmetic (as bark_multikey_sim.sql does with (random() * 40)::int4), or drop the exact n column from the committed output. Same issue at line 94 (floor(10 * random() ^ 2)) feeding the gt/tsvector counts.
📄 src/test/modules/bark_multikey/sql/bark_multikey_ginscan.sql (L86-L87)
Same portability risk as in bark_multikey_ginmerge.sql: the gm table's elements are built via floor(200 * random() ^ 3), and gs_check('gm', ...) commits the absolute n counts (cardinality of each result) into the expected output. ^ maps to libm pow(), which is not bit-reproducible across platforms/libc, so floor(200 * x^3) can land on a different integer for a few rows on some arch, shifting the committed n values and failing on the buildfarm. The per-platform boolean comparisons (index vs seqscan) stay stable, but the pinned counts do not. Prefer integer/multiply-only element generation, or avoid committing exact counts derived from float exponentiation. Same at line 132 (floor(10 * random() ^ 2)).
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This file (along with .envrc, .gdbinit, pg-aliases.sh, flake.nix, shell.nix) is personal developer scaffolding and must not be part of a PostgreSQL patch destined for pgsql-hackers/commitfest. Verified: no committed build/test file (meson.build, Makefile) references any of these, so they are purely local tooling. The tree's own root .gitignore explicitly states that "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude" -- exactly these files. Per minimal-diff hygiene they pollute the diff and should be dropped from the commit and kept in a personal gitignore. (high confidence)
| source_up | ||
| watch_file flake.nix | ||
| use flake | ||
| pg_cflags | ||
| use project_steering postgresql aws c | ||
| use git_policy allow-force-push | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
Beyond being local-only scaffolding that should not be committed, this file hardcodes the author's private environment: it sources ~/ws/postgres/.envrc (an absolute path to the author's home) and invokes undeclared/private direnv libraries (use project_steering, use project_mcp, use git_policy, pg_cflags) that are not present in this tree. Any other developer running direnv allow here will fail. Drop from the patch. (high confidence)
| break heap_hot_indexed_tuple_size | ||
|
|
||
| # Create augmented tuple with embedded modified-column bitmap | ||
| break heap_hot_indexed_create_tuple |
There was a problem hiding this comment.
These symbolic breakpoints reference functions that do not exist anywhere in the tree -- heap_hot_indexed_create_tuple, heap_hot_indexed_tuple_size, heap_hot_indexed_serialize_bitmap, and the entire heap_hot_indexed_* bitmap family defined below are found ONLY in this .gdbinit (verified via tree-wide search). GDB will reject them as unresolved. This confirms the file is a stale throwaway debugging aid, not patch content; it should not be committed. (high confidence)
| # WAL replay for XLOG_HEAP2_INDEXED_UPDATE | ||
| break heap_xlog_indexed_update |
There was a problem hiding this comment.
heap_xlog_indexed_update and log_heap_update (and the pruneheap.c symbol in the comment below) do not exist in this change set -- no matches tree-wide outside this file. These breakpoints will not resolve. Combined with the hardcoded line-numbered breakpoints into pruneheap.c (lines 1802/1836/1863/1287/2936) -- a file this series does NOT modify -- the line numbers are already drifted and point at the wrong code. Brittle and non-shippable. (high confidence)
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
Hardcoded source line numbers as breakpoints (heapam.c:3210/4019/4024/4033/4101/4147) are extremely brittle: they drift the instant any referenced file changes. These line-number breakpoints are an unmaintainable, local-only debugging artifact and should not be part of the patch. (high confidence)
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
Unrelated change to a project-owned tooling script bundled into a feature branch. The established tree-wide convention in src/tools/ is #!/usr/bin/perl (copyright.pl, git_changelog, mark_pgdllimport.pl all use it -- verified). Switching to env perl makes the resolved interpreter PATH-dependent and can pick up a user perl lacking required modules. This is a separate concern that should not ride along with this patch; revert it. (high confidence)
| alias pg-full-clean='trash "$PG_BUILD_DIR" "$PG_INSTALL_DIR" 2>/dev/null || rm -rf "$PG_BUILD_DIR" "$PG_INSTALL_DIR"; echo "Build and install directories cleaned"' | ||
|
|
||
| # Database management | ||
| alias pg-init='trash "$PG_DATA_DIR" 2>/dev/null || rm -rf "$PG_DATA_DIR"; "$PG_INSTALL_DIR/bin/initdb" --debug --no-clean "$PG_DATA_DIR"' |
There was a problem hiding this comment.
Destructive/privileged footguns that should not ship in a committed patch: pg-init and pg-full-clean run rm -rf on $PG_DATA_DIR / $PG_BUILD_DIR (values derived from $PWD), pg-stop/pg-restart/pg-status use an unanchored pkill -f "postgres.*-D.*$PG_DATA_DIR" that can match unintended processes, and pg-enable-cores/pg-disable-cores write /proc/sys/kernel/core_pattern via sudo. These belong in a personal environment, not a committed file. (moderate confidence)
| SELECT pg_relation_size('bark_dpg_idx') / current_setting('block_size')::int < built_pages + 10 | ||
| AS refill_reused_pages | ||
| FROM bark_dpg_sz; |
There was a problem hiding this comment.
The + 10 page threshold is hard-coded for the default 8kB BLCKSZ (the comment above spells out "on 8kB pages the index has 19 pages, the VACUUM deletes 7, and the refill needs 13 ... 25 with reuse and 32 without"). PostgreSQL's buildfarm runs non-default block sizes (e.g. --with-blocksize=4). At BLCKSZ=4096 the index has more pages and the refill can exceed built_pages + 10 even with reuse, so refill_reused_pages turns false and the test fails; at BLCKSZ>=16384 the whole index is a handful of pages and the check passes trivially, no longer proving reuse. Either skip this assertion unless block_size = 8192, or express the threshold relative to block size so the reuse property is actually tested across all supported BLCKSZ. (moderate confidence)
| (SELECT string_agg(md5(g::text || i::text), '') FROM generate_series(1, 30) i) | ||
| FROM generate_series(1, 200) g; | ||
| CREATE INDEX bark_ff_wide_idx ON bark_ff_wide USING bark (t) WITH (fillfactor = 10); | ||
| SELECT pg_relation_size('bark_ff_wide_idx') / current_setting('block_size')::int < 200 AS two_per_page; |
There was a problem hiding this comment.
This < 200 pages threshold assumes 8kB pages: a ~1kB key at fillfactor 10 fits roughly two per 8kB leaf, giving ~100 leaf pages. With BLCKSZ=4096 a 4kB leaf holds only ~2 such keys at 100% fill and far fewer at fillfactor 10, so the page count can approach or exceed 200 and two_per_page may become false. The comment "must take fewer than 200 pages" is only valid for the default BLCKSZ. Guard this (and the spans_leaves > 2 / oversized > 500 checks in check_bark_tidorder.sql) on block_size = 8192, or normalize the bound to block size. (moderate confidence)
| FROM generate_series(1, 63) g; | ||
| END LOOP; | ||
| END $$; | ||
| SELECT pg_relation_size('bark_hku_idx') / current_setting('block_size')::int > 2 AS spans_leaves; |
There was a problem hiding this comment.
> 2 AS spans_leaves hard-codes an 8kB page assumption. The run of a key with a 2000-byte INCLUDE column is sized to span a leaf boundary only on 8kB pages; at BLCKSZ=4096 it spans even more leaves (test still passes but loosely), while at BLCKSZ>=16384 the whole run may fit in <=2 pages and spans_leaves becomes false, so the "dead versions span a leaf boundary" precondition this case exists to exercise is no longer met and the test silently stops testing it. Guard on block_size = 8192 or size the data relative to block size. (moderate confidence)
| FROM generate_series(0, 499) g; | ||
| CREATE INDEX bark_sk_big_idx ON bark_sk_big USING bark (k, n); | ||
| VACUUM ANALYZE bark_sk_big; | ||
| SELECT pg_relation_size('bark_sk_big_idx') / current_setting('block_size')::int > 500 AS oversized; |
There was a problem hiding this comment.
> 500 AS oversized assumes 8kB pages (500 entries over 500 pages implies roughly one OVERSIZED entry per 8kB leaf). At a larger BLCKSZ more entries fit per leaf, so the index takes fewer than 500 pages and oversized becomes false; the OVERSIZED-from-overflow-chain path this case means to exercise then goes unverified. Normalize to block size or guard on block_size = 8192. (moderate confidence)
| my $maxitem = | ||
| int(($blksz - maxalign(24 + 3 * 4) - ($blksz - $special)) / 3) & | ||
| ~($maxalign - 1); |
There was a problem hiding this comment.
This $maxitem computation diverges from the C BarkMaxItemSize macro (src/include/access/bark.h), which is Min(MAXALIGN_DOWN(((BLCKSZ - ... ) / 3)), MAXALIGN_DOWN(INDEX_SIZE_MASK)). The Perl here reproduces only the first term and omits the Min(..., MAXALIGN_DOWN(INDEX_SIZE_MASK)) clamp (INDEX_SIZE_MASK = 0x1FFF = 8191, MAXALIGN_DOWN = 8184).
At the default 8 kB block size the first term (~2712) is well below the clamp, so the two agree and the test passes. But with --with-blocksize=32, the first term (~10904) exceeds 8184, so the backend's limit is 8184 while $maxitem is 10904. The i_toobig/i_shouldovf corruptions are then sized against the wrong limit and the like() assertions on "larger than the $maxitem-byte limit" can never match, giving a spurious failure on large-block builds.
The comment also overstates the fidelity ("as bark.h computes it"). Either apply the same clamp here, e.g. $maxitem = min($maxitem, $maxalign_down_of_0x1fff), or scope the comment to note the default-block-size assumption. Confidence: moderate.
| oldextversions \ | ||
| hot_updates | ||
|
|
||
| TAP_TESTS = 1 |
There was a problem hiding this comment.
hot_updates is a heap HOT-chain test (it uses heap_page_items, get_raw_page, tid_block/tid_offset, pg_stat_* only) with zero dependency on BARK. Bundling it into the BARK pageinspect patch violates the one-thing-per-patch / minimal-diff discipline that -hackers enforces; it should be split into its own commit. Confidence: high.
|
|
||
| -- oldextversions, which runs just before this, drops the extension; this | ||
| -- creates it again and drops it at the end, leaving things as they were | ||
| CREATE EXTENSION pageinspect; |
There was a problem hiding this comment.
This test has a hidden ordering dependency on the REGRESS schedule: it relies on oldextversions (which runs immediately before it) having dropped the extension, so it CREATE EXTENSION pageinspect at the top and DROP EXTENSION at the end. If the schedule is reordered, or another extension-dependent test is inserted after this one, the suite breaks non-obviously. The coupling is undocumented here. Consider making the test self-contained (e.g. CREATE EXTENSION IF NOT EXISTS and not dropping it, or a clear comment pinning the required position relative to oldextversions). Confidence: high.
| SELECT pg_column_compression(t) IS NOT NULL AS t_compressed, | ||
| pg_column_size(e) > 2500 AS e_out_of_line | ||
| FROM hot_bt_image WHERE id = 1; |
There was a problem hiding this comment.
t_compressed assumes TOAST compresses t, which only happens when the whole heap tuple exceeds TOAST_TUPLE_THRESHOLD. That threshold scales with BLCKSZ (roughly BLCKSZ/4): on 8kB/4kB pages this ~3000-byte row exceeds it and t is compressed, but on 16kB/32kB pages (the buildfarm runs --with-blocksize=32) the ~3kB value plus a short row stays well under the ~8kB threshold, so no out-of-line/compression pass runs. t then stays inline uncompressed and pg_column_compression(t) is NULL, making t_compressed false and the expected output diverge. e_out_of_line is likewise at risk since STORAGE EXTERNAL only pushes e out of line once the tuple crosses the threshold. Force compression/toasting independently of block size (e.g. make the values large enough to exceed the threshold at 32kB, or assert the precondition in a block-size-independent way).
|
|
||
| my $sroot = $info{i_toobig}{root}; | ||
| my ($mroot, $L0, $L1) = ($info{i_tidhikey}{root}, @{ $info{i_tidhikey}{kids} }); | ||
| my $nkids = scalar @{ $info{i_tidhikey}{kids} }; |
There was a problem hiding this comment.
$nkids is assigned here but never used anywhere else in the file (verified by search). This is dead code. Either drop the line, or if it was intended as a guard it should be referenced (e.g. in a cmp_ok assertion like the t_multi has 4+ leaves check that already exists). Note $I1 on the next line is also unused, though as a list-unpack tail that is a more defensible idiom.
| page \ | ||
| bark \ | ||
| bark_pages \ | ||
| brin \ | ||
| btree \ | ||
| checksum \ |
There was a problem hiding this comment.
The REGRESS list here and the regress list in contrib/pageinspect/meson.build are out of sync, and the existing tests have been needlessly reordered. meson.build lists page, btree, bark, bark_pages, brin, gin, gist, hash, checksum, oldextversions, hot_updates, whereas this Makefile lists page, bark, bark_pages, brin, btree, checksum, gin, gist, hash, oldextversions, hot_updates. The two build systems must run the same tests in the same order. Also, inserting bark/bark_pages ahead of the pre-existing brin/btree/checksum entries is churn on untouched lines; append the new tests to preserve a minimal diff and keep both files identical.
| ereport(info->message_level, | ||
| (errmsg("index \"%s\": untagged %lld entries in place, kept %lld tagged whose page's lower bound may carry a tag and %lld kept (dup, AM gave LIVE to an older entry), after lowering %lld lower bounds", |
There was a problem hiding this comment.
This is a developer-facing diagnostic emitted through errmsg(), which marks the string for translation. The parenthetical "(dup, AM gave LIVE to an older entry)" is internal jargon that does not belong in a user-translatable string, and the message mixes several plural quantities without errmsg_plural. Since this is a diagnostic rather than operational VACUUM output, prefer errmsg_internal() to avoid burdening translators (consistent with how other internal-only diagnostics are emitted). Low confidence on severity; style/convention nit.
| ereport(info->message_level, | |
| (errmsg("index \"%s\": untagged %lld entries in place, kept %lld tagged whose page's lower bound may carry a tag and %lld kept (dup, AM gave LIVE to an older entry), after lowering %lld lower bounds", | |
| ereport(info->message_level, | |
| (errmsg_internal("index \"%s\": untagged %lld entries in place, kept %lld tagged whose page's lower bound may carry a tag and %lld kept (dup, AM gave LIVE to an older entry), after lowering %lld lower bounds", |
| typedef struct BarkPageOpaqueData | ||
| { | ||
| BlockNumber bark_prev; /* left sibling, or BARK_P_NONE if leftmost */ | ||
| BlockNumber bark_next; /* right sibling, or BARK_P_NONE if rightmost */ | ||
| uint16 bark_level; /* tree level; zero for leaf pages */ | ||
| uint16 bark_cycleid; /* vacuum cycle ID of latest leaf split */ | ||
| uint16 bark_flags; /* flag bits, see below */ | ||
| uint16 bark_page_id; /* BARK_PAGE_ID, for tool identification */ | ||
| } BarkPageOpaqueData; |
There was a problem hiding this comment.
The comment asserts a size invariant ("the struct stays at 16 bytes") for an on-disk struct, but nothing enforces it. For on-disk/WAL formats, portability is a hard gate and such layout invariants should be compile-time checked, as the rest of the tree does (e.g. StaticAssertDecl on struct sizes/offsets). Recommend adding a StaticAssertDecl(sizeof(BarkPageOpaqueData) == 16, ...) (and similarly for BarkCodedListHdr/BarkTagTrailer/BarkMetaPageData field offsets, e.g. offsetof(BarkMetaPageData, bark_tid_stride)) so a future compiler/field change that silently alters the layout fails the build rather than corrupting readable-across-versions pages. The bit-overlap invariants are already guarded this way (lines 837/862); the size invariants are not.
| uint16 bark_tid_stride; /* the heap's largest locator offset at | ||
| * CREATE INDEX (0: MaxHeapTuplesPerPage) */ | ||
| uint32 bark_flags; /* BARK_META_* bits */ |
There was a problem hiding this comment.
The on-disk compatibility of bark_tid_stride relies entirely on it "sit[ting] in what was the padding after bark_allequalimage, so the struct keeps its size". Nothing enforces this at compile time: the field's offset and the overall struct size depend on compiler struct-padding placement, which is exactly the kind of assumption the portability gate requires to be checked for an on-disk format (same class of issue as the confirmed BarkPageOpaqueData finding). Readers (bark_index_tid_stride, the barkutils.c versioning checks) compute offsets from the struct itself, which is safe within one build, but a silent layout change (e.g. a future field reorder, or a compiler/ABI that pads bark_allequalimage differently) would corrupt reads of existing indexes with no warning. Add a StaticAssertDecl pinning offsetof(BarkMetaPageData, bark_tid_stride) and sizeof(BarkMetaPageData) so any layout drift fails the build rather than silently misreading on-disk meta pages.
An index AM that can store version tags needs to be handed the tag of each entry it makes, and must hand it back when a scan returns the entry, so that a table AM with a versioned locator can tell which row version the entry was made for. Add IndexInfo.ii_VersionTag for the first direction. Whoever hands a row to the index sets it: the executor before index_insert(), always to 0, since a new row (INSERT, COPY, a moved row) has no history; and a table AM with a versioned locator before each callback of its index build and validation scans. An index AM with amcanversion reads it in aminsert and in its build callback. Passing it in the IndexInfo rather than as an argument leaves aminsert and IndexBuildCallback, which every index AM implements, unchanged. Add IndexScanDescData.xs_heaptag for the other direction. An index AM with amcanversion sets it with xs_heaptid in amgettuple; for every other AM it stays 0 from RelationGetIndexScan. The table AM's xs_getnext_slot reads it, which covers plain and index-only scans alike, since both already ask the table AM about visibility. A TID bitmap has no room for tags, so a bitmap scan of a versioned table relies on the table AM to recheck rows whose locator a bitmap may hold through a stale entry. Document the table AM's side: the validity rule applies only to entries of indexes that store tags, since the executor keeps every other index's entries true for every version their locator reaches; its build and validation scans must tag rows that have history; and a build from the current version sets ii_BrokenHotChain when an older version a snapshot may see differs, as heap does. Index AMs recognise the inserting row in a unique check by locator alone, so stale entries of the same row never conflict. Heap neither sets nor reads a tag, so nothing changes for it. (cherry picked from commit bde9da5)
Simple and bottom-up index deletion ask the table AM which entries are dead by TID. For a table whose locator names row versions as (TID, tag), a TID's verdict is still safe: "deletable" means every version at that locator is dead to every snapshot. But it cannot reclaim the stale entries of a row that is still live, which is the version churn bottom-up deletion exists for. Add the entry's tag to TM_IndexStatus, so that an index AM that stores tags can pass it and the table AM can judge (TID, tag). It goes in TM_IndexStatus, reached through the entry's id and never sorted, rather than in TM_IndexDelete, so that the deltids array the table AM sorts stays 8 bytes. TM_IndexStatus grows from 6 to 12 bytes. nbtree and the generic shim in genam.c set it to 0. Heap ignores it: its locator is not versioned, so its verdict for a TID holds for every entry that names it. (cherry picked from commit 83c1e5b)
With version tags in place, an UPDATE of a table whose locator is versioned no longer has to move the row just because an indexed column changed, as long as every index that needs a new entry can store a tag: its new entry then names the new version, and its old entry names the old one. The executor decides. ExecUpdateMayKeepLocator() is true when the table's locator is versioned and every non-summarizing index whose attributes overlap the update's modified indexed attributes has amcanversion and is ready for inserts; a not-ready index forces a move, as heap's HOT rule does, while a ready but invalid index (left behind by a failed CREATE INDEX CONCURRENTLY) does not, since it is maintained as usual. It passes TABLE_UPDATE_MAY_KEEP_LOCATOR in table_tuple_update's options. An AM that keeps the locator returns the new version's tag through a new IndexVersionTag *version_tag out-parameter, with the new TU_Affected; ExecUpdateIndexTuples() then inserts entries with that tag into the indexes whose attributes overlap the modified attributes and into the summarizing ones, and no other. It forms the new row's datums as ExecInsertIndexTuples() does, so with EIIT_NEW_DATUMS it reuses those ExecUpdateModifiedIdxAttrs() saved. Otherwise update_indexes means what it always has. An exclusion constraint's index that amcomparedatums said keeps its entries gets no new entry under TU_Affected either, so its constraint is checked after the update by ExecCheckHotExclusion(), as after a HOT update: the callers already call it whenever update_indexes is not TU_All. Its old entries lead to the row at its unchanged locator, so the check finds the row as self, and a concurrent inserter's check finds the new version through them. A noted index that gets an entry after all, because another index kept one of its attributes in the modified set, is checked when the entry is inserted, and ExecUpdateIndexTuples() drops it from ri_HotExclusionCheck. "The attributes an index reads" must be the same set on both sides, or the table AM's validity check and the executor's choice of indexes would disagree, so this adds RelationGetIndexedAttrs(), a relcache-cached bitmap of one index's key, INCLUDE, expression and predicate attributes, built like INDEX_ATTR_BITMAP_INDEXED from the stored trees (adapted from the tepid patch set). ExecUpdate and MERGE, logical replication apply and REPACK (CONCURRENTLY) all go through the same two functions, so a versioned table is maintained the same way on each path; simple_table_tuple_update_ext() grows options and version_tag arguments for apply, and simple_table_tuple_update(), which passes no options, keeps its signature. New rows stay untagged: INSERT and COPY entries carry tag 0, which is valid because a new row has no history, so tuple_insert needs no out-parameter. Heap's locator is not versioned: the executor never passes the option, heap never reports TU_Affected and always reports tag 0, and the index entries it makes are exactly as before. (cherry picked from commit 1c8cac3)
A unique check that finds an equal key asks the table AM, through table_fetch_tid(), whether any version reachable from the entry's TID is live or in progress. For a table whose locator is versioned that is the wrong question: the row at that locator may be live with a different key, reached through a stale entry. The question is whether a version for which this entry is valid is visible, and only the table AM, given the entry's tag and its index, can answer it. Add an optional fetch_tid_version(rel, tid, tag, index, snapshot, all_dead) callback, required of AMs whose locator is versioned (checked when the relcache first fetches the locator), and a table_fetch_tid_version() wrapper that falls back to fetch_tid for every other AM. An index AM that stores tags calls it from its unique check. nbtree and the other in-core AMs keep calling table_fetch_tid(): their entries are untagged and, since the executor moves a row whenever an update affects them, describe every version their locator reaches, so the locator alone answers the question. Exclusion constraints and ON CONFLICT arbiters find conflicts through an index scan, which already passes the tag to the table AM's xs_getnext_slot, so they need nothing new. The tepid patch set's fetch_tid_check, which also fills a slot and says whether the caller must recheck the key, answers the same question for heap with HOT-indexed chains; the two would merge into one callback that takes the tag and the index, with the slot and recheck outputs optional. For heap the wrapper is table_fetch_tid(), so nothing changes. (cherry picked from commit 36106c8)
A table AM that keeps history for in-place versions can discard part of a row's history only once no index entry depends on it, as heap turns LP_DEAD into LP_UNUSED only after the index pass. Its index pass needs to judge each entry as (TID, tag), and must be able to say that an entry is live but its tag is no longer needed. Keep IndexBulkDeleteCallback as it is, so that no index AM has to change, and add a second callback that only an index AM with amcanversion uses: IndexBulkDeleteVersionCallback(tid, tag, state) returns an IndexEntryVerdict, INDEX_ENTRY_DEAD, INDEX_ENTRY_LIVE or INDEX_ENTRY_LIVE_UNTAG. The last lets the index AM rewrite the entry with tag 0, "older than the row's base"; the table AM gives it only for an entry whose meaning that leaves unchanged. The table AM passes it in new IndexVacuumInfo fields, version_callback and version_state, from its own VACUUM. When set, the index AM asks it about every entry, tagged or untagged, instead of the plain callback. Every in-core caller sets them to NULL: heap's lazy vacuum, parallel vacuum, ANALYZE and validate_index. (cherry picked from commit 4d012cd)
CREATE INDEX CONCURRENTLY's validation pass collects the TIDs already in the new index and lets the table AM insert every row version visible to the reference snapshot whose TID is missing. For a table whose locator is versioned, a row's TID can be in the index through an entry made for an older version, so a TID alone cannot say whether the visible version is indexed; the table AM must judge (TID, tag) pairs with its validity rule. When the table's locator is versioned and the index AM can store tags, validate_index() sets IndexVacuumInfo.version_callback and collects one (TID, tag) pair per entry, both as int8, into a tuplesort of heap tuples sorted by TID then tag. ValidateIndexState says which it holds (versioned) and with what descriptor (tupdesc), and tableam.h and tableam.sgml document what index_validate_scan receives. An index whose AM stores no tags is collected by TID as before, since its entries are untagged. Heap takes the int8 path exactly as before, and asserts so. (cherry picked from commit aa16bd4)
amcanversion says an index AM may store version tags. An AM can support them for only some of its indexes, for instance by the format an index was built in, or by the kind of its operator classes, and then the executor must treat each other index as one that stores no tags: move the row whenever an update affects it, and give it untagged entries. Add an optional IndexAmRoutine callback, amindexcanversion, and index_can_version(), which is true when the AM has amcanversion and amindexcanversion is absent or says so for the index. Use it wherever core code asked amcanversion about one index: ExecUpdateMayKeepLocator, the tag the executor hands each index (an index that cannot store tags gets 0), and validate_index's choice of (TID, tag) pairs. The docs for table AMs say the validity rule applies to entries of indexes for which index_can_version() is true and to no others, and that VACUUM's version_callback is for those indexes. amcanversion stays the AM-level flag that pg_indexam_has_property reports as can_version. [rebase: moved here, before the BARK series, from after BARK's format version 3 commits; it touches no BARK file, and nothing in the BARK series before it depended on it.]
An index scan's index AM learns nothing about what the table AM did with the entry it returned: whether a row came back through it, or the entry reached no visible version. An index that holds several entries per row and deduplicates by locator needs that answer when the table's locator is versioned, because an entry may then be valid for no version of the row while another entry of the same row is valid. Skipping a row's later entries because an earlier one was returned is right only if the earlier one actually returned the row. Add IndexScanDesc.xs_entry_verdict (UNKNOWN, VALID, INVALID). tableam_index_getnext_tid resets it to UNKNOWN after each amgettuple call, and index_rescan and index_restrpos reset it too. The table AM's xs_getnext_slot sets it before it next asks for a TID: VALID when it returns a tuple through the entry, INVALID when it moves on without one. On a HOT chain walked under a non-MVCC snapshot (xs_heap_continue) the verdict concerns the entry, so it stays VALID once a member was returned. The index AM reads it at the start of its next amgettuple call. amlocator.h and tableam.h document it as an obligation of a table AM with a versioned locator, for indexes for which index_can_version() is true. heapam sets it for plain and index-only scans. Its locator is not versioned, so it never rejects an entry by tag; the verdict says only whether a visible version was found. heap_versioned's index-scan wrapper sets INVALID for an entry the validity rule rejects, and test_versioned_btree reads and counts the verdicts; the regression test shows them for an index scan, an index-only scan, rows not visible to the snapshot, an entry rejected by tag, and the exclusion check's dirty-snapshot scan that continues along a HOT chain. The first consumer is to be versioned multikey indexes, whose scan deduplicates a row's entries by locator; RUM is the expected second.
index_can_version() answered two questions: whether an index's entries carry tags (so the table AM judges them by the validity rule), and whether an update that needs a new entry in it may keep the row's locator. The first must not change for the life of the relfilenode, since tagged entries already on disk need judging. The second can change: after a constraint is added that the index's checks cannot serve under a versioned locator, the index needs updates to move the row, while its existing entries still need the rule. Add an optional amindexmaykeeplocator to IndexAmRoutine and index_may_keep_locator() beside index_can_version(). It is false unless index_can_version() is true; NULL means the answer of amindexcanversion. ExecUpdateMayKeepLocator asks it. ii_VersionTag stays keyed on index_can_version(): once a row moves, its new entries describe the version at their locator either way. test_versioned_btree refuses for an index whose name ends in _nokeep. versioned.sql shows an update affecting such an index moving the row, its new entry still tagged, and an update that does not affect it keeping the locator.
No in-core table AM has a versioned locator, and no in-core index AM stores tags, so nothing in the regular tests reaches the code that carries version tags between them. test_versioned_tableam is a test module for that API, as dummy_index_am is for the index AM API: a pair of test access methods that deliberately use heap's storage. It is built and installed only for the tests (src/test/modules), never by a normal install. heap_versioned is heap whose locator says it is versioned. Its tag for a row version is the version's raw xmin. It still moves a row on every update that changes an indexed column, so every entry describes the version its TID names and heap's visibility answers stay right. That makes the module a test of the plumbing, not of in-place semantics: wherever core hands heap_versioned a (TID, tag), from an index scan or index-only scan, fetch_tid_version, index_delete_tuples, the VACUUM version callback, or a CIC validation scan, it raises an error unless the tag is 0 or the raw xmin of the tuple at that TID, and it counts what it saw. A GUC, keep_locator = pretend, makes it report that it kept a moved row's locator, so that the executor's TU_Affected path, and its refusal of TU_Affected with tag 0, can be tested; that leaves the unaffected indexes without entries for the moved row, so the tests use it only on scratch tables, which it corrupts on purpose. test_versioned_btree is nbtree with amcanversion, implementing btree indexes (heap_versioned asks for it through index_implementation). It stores no tag on disk: it records (index, TID) -> tag in a shared dshash from aminsert and heap_versioned's build scan, reports it in xs_heaptag, and clears or drops it from the version callback in its bulk delete. The record lives in dynamic shared memory, so it is shared by backends but lost at restart. The tests cover INSERT, UPDATE, DELETE, unique violations, ON CONFLICT, plain, index-only and bitmap scans, CREATE INDEX, REINDEX, REPACK, VACUUM FULL, VACUUM with the version callback, bottom-up deletion, CREATE INDEX CONCURRENTLY with concurrent updates (isolation), an index left ready but invalid by a failed CREATE UNIQUE INDEX CONCURRENTLY that is still maintained and does not force rows to move, and logical replication apply into a heap_versioned table (TAP). REPACK (CONCURRENTLY) refuses any routine but heap's, so it is tested only as a refusal. [rebase: moved here, before the BARK series; it touches no BARK file. On CF 5556 v43 nbtree has no amcomparedatums, and the executor treats the same value stored differently as unchanged for every index. So tx_a is now partial, which makes the executor evaluate it rather than keep k modified for the hash index (which stores no tags, and would make the row move); and tz's compressed-to-plain write-back changes no index at all, so only tz_p gets a (tagged) entry and the constraint is not checked again, where v42 gave both indexes entries and checked once.]
BARK (CREATE INDEX ... USING bark) is a new index access method: a
Lehman and Yao B-tree [1] over btree's operator families, written to
nbtree's algorithms. This first step is the base AM: the page format,
CREATE INDEX by bulk load, inserts with tree descent, move-right and
page splits, ordered and bitmap scans, recovery of an interrupted split,
index-only scans, page-level predicate locks, unique indexes, backward
scans, INCLUDE columns, VACUUM, and amcheck's structural checks. The
later steps add what nbtree cannot store (keys of any size, sorted value
sets, multikey operator classes) and bring it to parity with nbtree's
optimizations.
The page structure is nbtree's: right-linked pages with a high key, a
meta page naming the root, pivots truncated to the attributes that
separate them. Index entries use IndexTupleData with the alternate-TID
bit in t_tid, as nbtree's posting lists do, so that later entry shapes
fit without a format change. Operator classes are btree's: BARK's are
placed in btree's operator families (ambtreeopfamilies), so the planner
sees one family per type.
BARK-Design.mediawiki at the top of the tree records the contracts and
the roadmap; src/backend/access/bark/README describes the code as built;
doc/src/sgml/bark.sgml is the user manual.
[1] P. L. Lehman, S. B. Yao, "Efficient Locking for Concurrent
Operations on B-Trees", ACM TODS 6(4), 1981,
https://doi.org/10.1145/319628.319663
Duplicates of a key are kept in one entry as a value set, in one of two
shapes chosen per entry: a sorted array of heap TIDs (LIST, Berkeley
DB's sorted duplicates [1]) or a compressed bitmap of them (POSTING,
using the sbm sparse bitmap set). Bitmap scans read both shapes and
combine under BitmapAnd and BitmapOr.
A key larger than the item limit is stored on a chain of overflow pages
(OVERSIZED), with an inline prefix compared first; INCLUDE values may be
oversized too, and index-only scans return them in full. Nearest
neighbor scans (ORDER BY col <~> const) walk outward from the constant
in key order (amcanorderbyop).
The write path gains what nbtree has: parallel builds and scans,
speculative insertion (ON CONFLICT), ScalarArrayOp keys, split points
chosen by bytes and fillfactor, lock coupling child before parent, the
finishing of incomplete splits by later writers, VACUUM in one physical
pass with cycle IDs and cleanup locks, page deletion that keeps a
deleted page's links and recycles it only when no scan can hold a link
to it (safexid), page-at-a-time leaf reads with nbtree's pin rule, and a
WAL resource manager of its own instead of generic WAL, with hot standby
conflict handling.
[1] M. A. Olson, K. Bostic, M. Seltzer, "Berkeley DB", USENIX ATC
1999, FREENIX track.
Scans gain mark/restore, skip scan over leading-column groups with skip support, required array keys on any column, row-comparison positioning, the per-page "every entry satisfies this key" test, and a start past a strict bound. The root is cached in rd_amcache as nbtree caches its meta page. Equal keys are ordered by heap TID, as nbtree's are since PostgreSQL 12, so that inserts and deletions find an entry directly. Writes gain bottom-up deletion [1], a merge of a leaf's equal-key entries before it splits (nbtree's deduplication pass), the cached rightmost-leaf fast path for ascending inserts, logging only the right half of a split, and UNIQUE NULLS NOT DISTINCT. Leaf prefix compression (the prefix_compression reloption) stores the bytes a leaf's keys share once per page, in the tradition of Bayer and Unterauer's prefix B-trees [2]. BARK gets an operator class for every btree operator class, a cost estimate built on btcostestimate's (correlation, descent cost), and nbtree's tools: pageinspect functions, pgstatbarkindex, and amcheck's heapallindexed and parent checks, reachable through pg_amcheck. [1] P. Geoghegan, "Bottom-up index deletion", commit d168b66 [2] R. Bayer, K. Unterauer, "Prefix B-Trees", ACM TODS 2(1), 1977, https://doi.org/10.1145/320521.320530
An operator class may extract several keys from one value (support
procedures 7 and 8, extractValue and extractQuery), so one row has an
entry per key, as in GIN, but in an ordered tree that also holds scalar
columns. A scan returns each row once, deduplicating through a seen set
of the TIDs it returned. Classes may instead use GIN's query interface
(procedures 10-13: extractValue, extractQuery, consistent,
triConsistent), so a GIN operator class ports with little change, and
RUM's ordering functions [1] for ORDER BY on an extracted column. Unique
indexes over extracted keys and ON CONFLICT on them are supported,
unique being "no two rows share a key".
Equal keys of a GIN-interface column coalesce into value sets by GIN's
rule, and LIST entries may be coded as delta+varbyte, as GIN's posting
lists are, when that is smaller. A level-1 page cache starts read and
write descents below the root. src/test/modules/bark_multikey and
bark_wildcard provide array, prefix and jsonb path ("wildcard") operator
classes, used by the tests.
Also: the user manual (bark.sgml), crash-recovery TAP tests for every
split and insert record, and amcheck's checkunique.
[1] https://github.com/postgrespro/rum
BARK answers amcomparedatums for columns it cannot return, so an update that leaves an index's entries unchanged stays HOT. It can implement btree indexes: with default_btree_implementation = bark, or a table AM whose index_implementation callback asks for it, CREATE INDEX ... USING btree builds a BARK index, recorded in pg_index.indimplam, that the planner, pg_dump and pg_amcheck treat as a btree index. The TID stride is recorded in the meta page so that tables whose locator offsets exceed heap's can be indexed. Scans mark the entries of dead rows LP_DEAD, and a leaf deletes them before it splits (nbtree's simple deletion). In cassert builds every page is checked after each change and each redo, with the checks that amcheck runs (bark_page_verify), bounded to the entries a small change touched. VACUUM reads its blocks through a read stream.
Format version 3 adds a 4-byte version tag to leaf entries and pivots, stored only in indexes built for a table whose locator names row versions (BARK_META_VERSIONED), so heap's indexes are unchanged. An entry is then (key, locator, tag), ordered by all three. BARK stores the tags the table AM hands to inserts and builds, returns each entry's tag on every scan path, judges entries through the version-aware bulk-delete callback in VACUUM (untagging only in place), and passes tags to table_index_delete_tuples. CREATE INDEX sorts tagged rows rather than inserting them after the load. VACUUM lowers a tagged lower bound when it unlinks a page, so that it can untag every entry. amcheck follows overflow chains and reports orphaned pages, and VACUUM and the insert fast path free the chains they give up. Exclusion constraints may use an extracted column. The tests use src/test/modules/test_versioned_tableam.
The selectivity of a qual on an extracted column is estimated from the index's own separators: exact counts for points and narrow ranges, a memoized leaf-density probe for wide ones. A TID is added to a coded LIST without decoding it. A scan returns a leaf's LIST and POSTING rows one entry at a time, with an items array allocated once and kept under AllocSet's chunk limit, so LIMIT 1 on a duplicate key costs what it does on nbtree. OVERSIZED entries are compared with search keys by their inline prefix. A plain multikey index on a versioned table stores tags (BARK_META_MKTAGS) and returns each row through the entry the table AM judges valid (xs_entry_verdict). An update that keeps a row's locator inserts the new value's full key set, tagged. Such an index made a constraint's index after its build moves rows from then on (amindexmaykeeplocator).
A bitmap scan added TIDs to its TIDBitmap one call per TID, and tbm_add_tuples() reuses a heap page's entry only within one call (commit f5ae3ba), so every TID paid a hash lookup. Collect a scan's single TIDs into a fixed batch per recheck flag, allocated once per scan under AllocSet's chunk limit, and add each batch in one call; the members of a LIST or POSTING entry go in as the array they are. This is the idea of the "Batch B-tree TIDs when building a bitmap" patch for nbtree [1]. A bitmap scan of 10M rows takes 111 ms in its Bitmap Index Scan node instead of 157 (nbtree 151, nbtree with that patch 104). A scan of a GIN-interface column read each query key's boundary into an exact TID array allocated huge and doubled with repalloc_huge, merged the streams into another huge array, and intersected keys into a third, so its memory grew with the result (134 MB for one stream over 10M rows) and every query did large malloc/free cycles. Now a boundary in TID order (a non-NULL point on the leading column) is read into a stream of fixed chunks under 8 kB, reused from a free list, and any other boundary into a TIDBitmap of its own, bounded by work_mem and lossy beyond it, as ginget.c's collectMatchBitmap does. The streams are merged a heap page at a time following keyGetItem's lossy rule, with ginlogic.c's shim for classes without triConsistent. No stream keeps a leaf position, so no new pin or lock rule is needed. BARK's own allocations per query fall from 134 MB to under 8 kB each; a two-key AND over 10M rows takes 47 ms instead of 198, and the 2-key shape at 16 clients gains 3% tps with 15% fewer page faults. [1] https://postgr.es/m/CAEEBa3U7QDChb%3DPjzQGHoie-tOnfr7dumhO12Tp%3D5o2iT4t_ug%40mail.gmail.com
A multikey index on a table whose locator is versioned stores tags (BARK_META_MKTAGS) and lets updates keep a row's locator. Such an index can later become unique or an exclusion constraint's index (DocumentDB's prepareUnique does that with only AccessShareLock on the table), and an update whose statement began before the change can still keep its locator. While it is in progress a dirty snapshot sees two versions at one locator, each reached only through its own entries, and a check that kept one row per locator could miss the conflicting version and commit a duplicate. Until the conversion has settled, a non-MVCC scan of such an index judges every entry by itself and rechecks every row, keeping one row per locator only for the row its own transaction inserted last. The meta page records the conversion (CONVERTING, then SETTLED once a VACUUM has waited out every writer that could still keep a locator; an autovacuum worker does not block), inserts store untagged entries from SETTLED on, and a VACUUM that finds no tagged entry left clears the bits and MKTAGS, leaving the index a REINDEX would build. amcheck and the page validator check the bits. test_versioned_tableam gains real in-place updates on a HOT chain (keep_locator = inplace), and the isolation test bark_mk_convert shows the check waiting for the in-place update and then failing or succeeding by its outcome.
There was a problem hiding this comment.
🔍 OCR found 53 issue(s).
- 25 inline, 28 in summary (inline capped at 25)
⚠️ 2 warning(s) during review
📄 src/backend/lib/sbm.c (L10274-L10275)
Portability hard gate: the serialized body is a raw host-endian dump of the internal encoding (memcpy(... sbm_get_data(map) ...)), with only a 1-byte LE/BE flag in the header. These bytes are persisted to disk via BARK posting entries, so the on-disk format depends on host byte order. PostgreSQL forbids on-disk/wire formats that depend on endianness, struct padding, or word width. A binary copy / replication / shared storage between a little-endian and big-endian host produces a map that cannot be read back (sbm_deserialize returns NULL on endianness mismatch), i.e. silent unavailability of persisted data. The format must be serialized field-by-field in a fixed byte order (e.g. pg_bswap helpers), not memcpy'd. Confidence: high.
📄 src/backend/lib/sbm.c (L10308-L10309)
Future-tense comment on an on-disk limitation that has already shipped. The OCR/-hackers convention forbids "not yet"/aspirational comments for behavior that is committed. More importantly, this is not a cosmetic nit: rejecting cross-endian input means a BARK posting entry written on one architecture is unreadable on another, which is a shipped on-disk format defect rather than a pending feature. Either make the wire format endian-neutral (see the body memcpy above) or state plainly that the format is host-endian-only and why. Confidence: high.
📄 src/backend/lib/sbm.c (L1880-L1880)
Read path mutates a logically-const map: sbm_get_size_impl rewrites the on-disk chunk count via sbm_set_chunk_count(unconstify(Sbm *, map), ...) when it detects truncation. sbm.h documents that "concurrent readers are safe only when no writer is active" -- but this makes two concurrent readers of a corrupt/adversarial buffer both writers, a data race with no lock. Two separate hazards: (1) the unsynchronized write breaks the stated concurrency contract; (2) silently truncating the chunk count on corrupt input masks data loss (a read that quietly drops chunks). A validation/read routine should report corruption (return the valid prefix without persisting a mutation), not rewrite the buffer in place. Confidence: high.
📄 src/backend/lib/sbm.c (L252-L252)
Buffer-overrun mitigation relies on documented +SBM_SIZEOF_OVERHEAD slack rather than fixing the root cause. The comment itself states sbm_insert_data's memmove length over-counts by SBM_SIZEOF_OVERHEAD under one calling convention, and that fixing it directly "breaks the alternate convention used by sbm_anchor()-without-clear callers." This is a footgun: the safety of every mutation path now depends on which of two offset conventions its caller used, enforced only by this slack constant. Prefer a single offset convention and a correct memmove length so the +8 slack is unnecessary; a future caller using the other convention overruns by 8 bytes with no diagnostic. Confidence: moderate.
📄 src/backend/lib/sbm.c (L8316-L8316)
Inline performance claims with specific timings ("~37 s for 50k inserts into a 50k-chunk map", "~13 ms", "~2750x", "~4x per doubling") are unverifiable from the diff and belong in the commit message / -hackers design thread, not the source. Comments should explain why the algorithm is structured this way, not cite benchmark numbers that will silently rot. Keep the big-O rationale; drop the measured constants. Confidence: moderate.
📄 src/include/lib/sbm.h (L130-L130)
This ~12,900-line general-purpose ADT (set algebra, Jaccard index, sqrt-n locators, 8-way MRU cache, embedded ops, serialization) overlaps heavily with existing in-tree infrastructure the header itself cites: Bitmapset (nodes/bitmapset.c), integerset (lib/integerset.c), and the TID bitmap. Per DRY/YAGNI review discipline, much of this API surface (jaccard, locators, cursor caches, in-place set-op synonyms sbm_or/and/andnot) appears to be speculative scaffolding unless BARK actually calls it. Please justify why an existing ADT (integerset for uint64 sets, Bitmapset for dense small sets) cannot serve, and strip any API with no in-tree caller. A 12.9k-line reinvention of core infrastructure is a top rejection reason on -hackers. Confidence: moderate.
📄 src/backend/executor/execSubattr.c (L318-L324)
read_walker() recurses through itself via expression_tree_walker() (and indirectly via accessor_path()), but unlike accessor_path() and collect_modifies() it has no check_stack_depth() guard at its own entry. expression_tree_walker() does not perform the check itself. Index expressions and predicates (ii_Expressions / ii_Predicate) that this walks can be arbitrarily deeply nested, so a pathological tree can recurse without a stack guard. Per project convention (e.g. clauses.c walkers), add check_stack_depth() at the top of read_walker().
💡 Suggested change
Before:
read_walker(Node *node, SubattrWalk *walk)
{
List *paths;
bool subvalue;
if (node == NULL)
return false;
After:
read_walker(Node *node, SubattrWalk *walk)
{
List *paths;
bool subvalue;
if (node == NULL)
return false;
check_stack_depth();
📄 src/backend/executor/execSubattr.c (L453-L459)
This heuristic's safety is directional and corruption-critical: paths_may_overlap() treats a key-vs-position element pair as disjoint when key_may_be_position() returns false. If this function ever returns false for a key that jsonb would in fact read as an array index, an index that reads that element could be wrongly proven untouched, dropping a needed index entry (silent index corruption / lost tuple). The whitespace handling here uses locale-dependent isspace(), which is coupled by hand to jsonb's internal strtol-style subscript parsing; if that internal parsing ever diverges (different whitespace set, overflow/range handling of very long digit runs, etc.) the proof becomes unsafe in the corruption direction rather than merely over-conservative. This fragile coupling to an implementation detail of another module is worth a stronger note in the comment, and ideally the key-as-position test should share jsonb's own parsing routine rather than reimplementing it.
📄 src/backend/nodes/tidbitmap.c (L729-L731)
This comment is inaccurate and will mislead readers. It claims the hook "is a lock-free VM probe for heap", but heap's bitmap_and_inexact is set to NULL (heapam_handler.c and the default in tableam.h). Heap never does a VM probe here; it takes the NULL/common path. The VM-probe implementation belongs to a versioned (in-place-update) AM, not heap. Reword to avoid attributing VM-probe behaviour to heap, e.g. state the hook is NULL for heap (so the common exact-intersection path is unchanged) and that an AM with selective-indexed updates supplies a probe.
📄 src/backend/executor/execSubattr.c (L527-L530)
substitute_params() recurses via expression_tree_mutator() without a check_stack_depth() guard. It runs on the SET expression before collect_modifies() (which does guard) ever sees it, so a deeply-nested SET expression could overflow the stack here first. eval_const_expressions(), which this function explicitly models itself on, calls check_stack_depth() for exactly this reason. Add the guard at function entry for consistency with the other walkers in this file (accessor_path/collect_modifies already have it).
💡 Suggested change
Before:
substitute_params(Node *node, ParamListInfo params)
{
if (node == NULL)
return NULL;
After:
substitute_params(Node *node, ParamListInfo params)
{
if (node == NULL)
return NULL;
check_stack_depth();
📄 src/backend/lib/sbm.c (L11882-L11887)
This ENOSPC path violates the documented "both maps are unchanged" contract (sbm.h sbm_split: "SBM_IDX_MAX with errno = ENOSPC if either buffer is too small (in which case both maps are unchanged)"), and contradicts this function's own comment just above ("refuse up front, leaving both maps untouched").
By the time this check runs, the in_middle phase at lines 11794-11822 has already mutated BOTH maps: sbm_map_set(other, j, false, ...) populated other with the split-off middle chunk, and sbm_map_unset(map, j, false) removed bits [idx, ...) from map's middle chunk. Returning SBM_IDX_MAX here leaves map with lost bits and other non-empty. A caller that follows the header's advice to grow and retry after ENOSPC then operates on corrupted maps -- silent data loss. The up-front refusal must total the bytes for the whole move (and the middle split) BEFORE the in_middle mutation, not after.
📄 src/backend/utils/cache/relcache.c (L5408-L5415)
Memory leak (moderate confidence): the TextDatumGetCString(datum) strings and the node trees from stringToNode() are palloc'd in the caller's current memory context and never freed. The sibling RelationGetIndexExpressions() deliberately pfree()s the CString after stringToNode() for exactly this reason. Although rd_indattr caches the result so this only leaks on first use per relcache entry, the leftover node trees land in whatever context the caller runs (here the executor UPDATE path via IndexNeedsEntry), which is inconsistent with the established pattern. Capture the CString into a local, pfree it after pull_varattnos, and free the parsed trees (or do the parsing in a short-lived context).
📄 src/bin/pg_dump/pg_dump.c (L18263-L18266)
The fout parameter is never used in appendIndexImplementationReset (unlike the Set variant, which uses fout->dopt and fout->remoteVersion). It appears to be kept only for signature symmetry. Since pg_dump is also built as part of libpgcommon/libpgport-adjacent code and some buildfarm animals enable -Wunused-parameter, consider dropping the unused parameter to avoid a warning. Low confidence / low severity — if symmetry is the deliberate intent, this is acceptable.
💡 Suggested change
Before:
appendIndexImplementationReset(Archive *fout, PQExpBuffer q,
const IndxInfo *indxinfo)
{
if (indxinfo->indimplam)
After:
appendIndexImplementationReset(PQExpBuffer q, const IndxInfo *indxinfo)
{
if (indxinfo->indimplam)
📄 src/include/access/relscan.h (L136-L142)
The new IndexScanEntryVerdict enum is inserted between the IndexScanDescData struct's doc comment ("We use the same IndexScanDescData structure...") and the struct itself, leaving two back-to-back block comments and detaching the struct's documentation from its definition. Move the enum (and its comment) above the IndexScanDescData doc comment so the latter sits directly on typedef struct IndexScanDescData. (low confidence, style/maintainability)
📄 src/include/access/relscan.h (L142-L142)
IndexScanEntryVerdict is missing from src/tools/pgindent/typedefs.list, while the sibling new typedefs added in this change (IndexBulkDeleteVersionCallback, IndexEntryVerdict, IndexVersionTag) were all registered there. The tell is the closing line's formatting: a registered enum is closed } TypeName; (single space, as ScanDirection in sdir.h), but this one uses tabs (} IndexScanEntryVerdict;) -- exactly how pgindent lays out a } followed by an unregistered identifier. Add IndexScanEntryVerdict to typedefs.list so the tree stays pgindent-clean. (high confidence)
📄 src/include/access/tableam.h (L1565-L1568)
This parenthetical references an external, unshipped patch set ("the tepid patch set's fetch_tid_check") and describes speculative future work that does not exist in this tree ("they would merge into one callback ... with the slot and recheck outputs optional"). Per the project's comment-accuracy rule, committed comments must describe what the code does now, not aspirational/future behavior or code that is not present. This belongs in the -hackers thread, not in a committed header. Remove the parenthetical.
Confidence: high.
📄 src/test/modules/bark/sql/bark_bottomup.sql (L133-L134)
bbu_plain and bbu_uct are the only physical-layout-dependent tables in this file that do not set autovacuum_enabled = off (every sibling table above does: bbu_big, bbu_post, bbu_mk, bbu_run). This test relies on the fillfactor-100 leaves staying full so the second UPDATE's insert takes the split path and runs bark_bottomup_delete (barkinsert.c only reaches it when the leaf would split, the key is unchanged, and the row is a new version). If autovacuum fires on these WAL-logged, non-temp tables between the inserts and the UPDATEs, it can vacuum the index / prune the heap, so the leaf no longer needs to split and the bottom-up path is silently skipped. The output compared here (bbu_same index-vs-seqscan agreement + bark_index_check) stays green either way, so the test would pass without exercising the targeted code — the classic "passes with the feature reverted" hazard. Add autovacuum_enabled = off to match the sibling tables. (moderate confidence)
💡 Suggested change
Before:
CREATE TABLE bbu_plain (id int, k int, v int);
CREATE TABLE bbu_uct (id int, k int, v int) WITH (user_catalog_table = true);
After:
CREATE TABLE bbu_plain (id int, k int, v int) WITH (autovacuum_enabled = off);
CREATE TABLE bbu_uct (id int, k int, v int)
WITH (user_catalog_table = true, autovacuum_enabled = off);
📄 src/test/modules/bark/sql/bark_walinspect.sql (L31-L31)
This script reduces a page-size-sensitive workload (a 3-level tree, multi-page OVERFLOW, a full-page-image VACUUM, specific split variants "300-byte keys fit about 25 to a page", "432 where MAXALIGN is 8") down to boolean logged/described assertions that are all expected t. Unlike its sibling bark_zerofree.sql -- which the same authors guard with a block_size <> '8192' OR max_data_alignment <> 8 skip plus a _1.out alternate -- this file has no block_size/MAXALIGN guard. On a build configured with a non-default block size (--with-blocksize) or non-8 MAXALIGN, record types such as vacuum_image, overflow_multi, newroot_level2, unlink_lowers_flag, or split_replace may not be produced, silently flipping a boolean to f and yielding an opaque diff with no indication that the cause is page geometry. (medium confidence) Either add the same block_size/MAXALIGN skip guard used by bark_zerofree.sql, or document in the header comment that the workload assumes 8 kB pages and 8-byte MAXALIGN.
📄 src/test/modules/bark/t/001_empty_index_race.pl (L2-L2)
The copyright header uses a sole future year (2026); the real-world authoring date is 2025. PostgreSQL convention for a new file is the current year (or a range like 2025-2026 for files spanning years, as in src/test/modules/test_misc/t/008_replslot_single_user.pl). A bare future year will be flagged on pgsql-hackers. Same issue in 002-010 (and the rest of the series). Low severity; committer-adjustable.
📄 src/test/modules/bark/t/009_validate_single_user.pl (L87-L87)
These two magic counts (16 client-side INFO reports, 3 invalid opclasses) are tightly coupled to the exact number and wording of barkvalidate.c's ereport(INFO) emissions across the three crafted opclasses. Any future change to a validation rule silently breaks this with an opaque count mismatch, and a wrong count here yields a false sense of coverage rather than a clear failure. Consider asserting on the distinct message content (e.g. counting specific errmsg substrings) in addition to the raw total, so a drift points at the rule that changed. The rule-by-rule coverage already lives in sql/bark_validate.sql per the file comment, so this total is somewhat redundant.
📄 src/test/modules/bark/sql/bark_lpdead.sql (L434-L436)
This prints pg_relation_size() as raw byte counts, which the expected output hardcodes as 16384 (2 x 8kB pages). On builds with a non-default block_size (PostgreSQL supports --with-blocksize), these absolute values differ and the test diverges from the expected file.
The upstream src/test/regress/sql/mvcc.sql case this is modeled on deliberately avoids this by printing rows only on mismatch:
SELECT :clean_aborted_self_key_before AS size_before, pg_relation_size('clean_aborted_self_key') size_after
WHERE :clean_aborted_self_key_before != pg_relation_size('clean_aborted_self_key');Adopt the same block-size-independent comparison (keeping the implam() check separate), so the output stays stable across page sizes.
💡 Suggested change
Before:
SELECT implam('clean_aborted_self_key'::regclass) AS impl,
:clean_aborted_self_key_before AS size_before,
pg_relation_size('clean_aborted_self_key') AS size_after;
After:
SELECT implam('clean_aborted_self_key'::regclass) AS impl;
SELECT :clean_aborted_self_key_before AS size_before,
pg_relation_size('clean_aborted_self_key') AS size_after
WHERE :clean_aborted_self_key_before != pg_relation_size('clean_aborted_self_key');
📄 src/test/modules/bark_multikey/sql/bark_multikey_sim.sql (L114-L114)
This statement references a column i that does not exist in FROM sm (the alias is sm), so it fails at runtime. The committed expected output (expected/bark_multikey_sim.out:198) enshrines ERROR: column "i" does not exist, which means this INSERT never runs and the intended "re-insert copies of existing rows" DML coverage is silently dropped while the test still "passes". The column was almost certainly meant to be id.
high confidence.
💡 Suggested change
Before:
+INSERT INTO sm SELECT 7500 + i, a FROM sm WHERE id % 13 = 0 AND id <= 6000;
After:
+INSERT INTO sm SELECT 7500 + id, a FROM sm WHERE id % 13 = 0 AND id <= 6000;
📄 src/test/modules/bark_multikey/sql/bark_multikey_gin.sql (L40-L41)
The mg table is populated with random() after setseed(0.42) (lines 17-18), yet this query prints cardinality(mg_run(NULL, q)) AS n, and the committed expected file (expected/bark_multikey_gin.out) bakes in the concrete counts (135, 1905, 539, ...). setseed/random() output is explicitly documented as not stable across major versions or platforms, and the PRNG implementation changed in PG16. Any assertion embedding a literal count derived from seeded-random data is a buildfarm/portability hazard. The bark_ok/bark11_ok/gin_ok boolean comparisons are robust and sufficient; drop the raw n column (or compute it against a deterministic dataset).
📄 src/test/modules/bark_multikey/sql/bark_multikey_gin.sql (L65-L66)
Same seeded-random stability problem as the earlier query: mg_saop scans the random()-generated mg table, so printing cardinality(mg_saop(NULL)) AS n pins a concrete count into the expected output that depends on random()/setseed behavior, which is not stable across PG versions or platforms. Keep only the bark_ok/gin_ok seq-vs-index comparisons; the raw count adds no coverage and is a portability footgun.
📄 src/test/modules/bark_multikey/sql/bark_multikey_ginmerge.sql (L56-L56)
gm.a is generated with random() after setseed(0.7), so printing cardinality(seq) AS n here records concrete, random-derived counts in the committed expected output. setseed/random() is not guaranteed stable across PG major versions or platforms (the PRNG changed in PG16), making these n values fragile. The bark_ok/bark11_ok/bark_g_ok/gin_ok comparisons already prove correctness independent of the exact cardinality; drop the raw n.
📄 src/test/modules/bark_multikey/sql/bark_multikey_ginco.sql (L138-L138)
The gn table uses round(floor(40 * random() ^ 2)::numeric, ...) (seeded by setseed(0.31)), so cardinality(seq) AS n emits random-derived counts into the committed expected output. These will diverge whenever random()/setseed behavior changes across PG versions or platforms. The bark_ok/bark_g_ok/bark_n_ok/gin_ok seq-vs-index comparisons are the meaningful checks and are stable; remove the raw n column to avoid a non-portable expected file.
📄 src/test/modules/bark_multikey/sql/bark_locator.sql (L28-L29)
These expectations are BLCKSZ-dependent: the committed expected output records stride 291 (MaxHeapTuplesPerPage at 8192) for the heap rebuild, 2048 (MaxOffsetNumber) for the wide AM, and the error text stride must be 0 or between 291 and 2048. Both MaxHeapTuplesPerPage and MaxOffsetNumber change with a non-default block size, so this test (and its expected .out) will fail on any non-8192 build. The comment itself concedes "At BLCKSZ 8192". The module Makefile does not gate this test on BLCKSZ. Either derive the stride numbers from catalog/SQL-visible constants instead of hardcoding, or document/gate the test as 8192-only.
📄 src/test/modules/bark_multikey/sql/bark_locator.sql (L45-L46)
bark_test_tid_mappings(291) and bark_test_tid_mappings(0) produce per-check ntids counts in the expected output (1455, 4830, 10240, ...) that are computed from the 291/2048 stride geometry, i.e. from MaxHeapTuplesPerPage/MaxOffsetNumber. On a non-default BLCKSZ these counts (and the magic stride argument 291) change, breaking the expected file. Same portability concern as the stride assertions above.
| patchedGlibc = pkgs.glibc.overrideAttrs (oldAttrs: { | ||
| patches = (oldAttrs.patches or []) ++ [ | ||
| ./glibc-no-fortify-warning.patch | ||
| ]; | ||
| }); |
There was a problem hiding this comment.
./glibc-no-fortify-warning.patch is referenced here but does not exist anywhere in the repository. Any evaluation that forces the patchedGlibc derivation (e.g. nix flake check, entering the default devShell) will fail with a missing-path error. Either add the patch file to the change set or drop the patchedGlibc override. More fundamentally, this entire Nix dev-environment scaffolding does not belong in a PostgreSQL patch destined for pgsql-hackers (minimal-diff discipline).
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
This is the only Perl script in the tree that uses #!/usr/bin/env perl; all ~35 other Perl tools (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, etc.) use the fixed #!/usr/bin/perl. Switching to env perl makes the resolved interpreter PATH-dependent and less reproducible, diverging from the established project convention. If this change is genuinely wanted it must be a standalone, justified patch with -hackers discussion, not bundled with unrelated dev-tooling. As-is it is an inconsistent one-off.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
| break heap_prune_chain | ||
|
|
||
| # Chain collapsing: collect bitmaps from dead INDEXED_UPDATED intermediates | ||
| break pruneheap.c:1802 |
There was a problem hiding this comment.
These hardcoded absolute line-number breakpoints are already stale and point at the wrong code. For example line 1802 is annotated "Chain collapsing: collect bitmaps from dead INDEXED_UPDATED intermediates", but the actual pruneheap.c:1802 is prstate->processed[offnum] = true;. Line-number breakpoints drift the instant any line is added/removed above them, silently breaking debugging. This personal debug script (tied to an unrelated 'HOT indexed updates'/bark series) should not be committed to a PostgreSQL patch at all; prefer symbol-based breakpoints if kept locally.
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
Same line-number-drift problem throughout this block (heapam.c:3210/4019/4024/4033/4101/4147). These are fragile against any future edit and tie this committed file to an unrelated patch series. Personal GDB scaffolding does not belong in a pgsql-hackers submission.
| alias pg-full-clean='trash "$PG_BUILD_DIR" "$PG_INSTALL_DIR" 2>/dev/null || rm -rf "$PG_BUILD_DIR" "$PG_INSTALL_DIR"; echo "Build and install directories cleaned"' | ||
|
|
||
| # Database management | ||
| alias pg-init='trash "$PG_DATA_DIR" 2>/dev/null || rm -rf "$PG_DATA_DIR"; "$PG_INSTALL_DIR/bin/initdb" --debug --no-clean "$PG_DATA_DIR"' |
There was a problem hiding this comment.
trash/rm -rf on $PG_DATA_DIR, $PG_BUILD_DIR, $PG_INSTALL_DIR is a destructive footgun: if any of these vars is unset or empty, the fallback rm -rf "$PG_DATA_DIR" becomes rm -rf "" and, combined with other unquoted expansions elsewhere, can delete unintended paths. trash is also a non-standard external tool. Beyond the footgun, this personal alias file should not be part of a PostgreSQL contribution.
| REGRESS = \ | ||
| page \ | ||
| bark \ | ||
| bark_pages \ | ||
| brin \ | ||
| btree \ | ||
| checksum \ | ||
| gin \ | ||
| gist \ | ||
| hash \ | ||
| oldextversions | ||
| oldextversions \ | ||
| hot_updates |
There was a problem hiding this comment.
The REGRESS test order here diverges from contrib/pageinspect/meson.build, which lists page, btree, bark, bark_pages, brin, gin, gist, hash, checksum, oldextversions, hot_updates. This Makefile lists page, bark, bark_pages, brin, btree, checksum, gin, gist, hash, oldextversions, hot_updates -- btree/checksum are in different positions relative to bark/bark_pages. The two build systems must run the same tests in the same order (the suite relies on ordered side effects: page creates the extension, and hot_updates depends on oldextversions having dropped it). Reorder one to match the other. (moderate confidence)
| /* As in pgstathashindex, relation_open refuses partitioned indexes. */ | ||
| rel = relation_open(relid, AccessShareLock); |
There was a problem hiding this comment.
This comment is inaccurate. relation_open() opens anything with a pg_class entry and does not refuse partitioned indexes (see relation.c: "The caller is expected to check whether the relkind is something it can handle"). What actually rejects a partitioned index here is the subsequent bark_relation_is_bark() check, which requires relkind == RELKIND_INDEX ('i'); a partitioned index has RELKIND_PARTITIONED_INDEX ('I') and so fails it. Note the pgstathashindex comment this references is careful to attribute the refusal to the relkind check, not to relation_open. Reword to describe what actually forbids them.
| /* As in pgstathashindex, relation_open refuses partitioned indexes. */ | |
| rel = relation_open(relid, AccessShareLock); | |
| /* | |
| * Use relation_open(), not index_open(): the latter accepts partitioned | |
| * indexes, which the relkind check in bark_relation_is_bark() below | |
| * rejects (as pgstathashindex does). | |
| */ | |
| rel = relation_open(relid, AccessShareLock); |
| OUT posting_entries BIGINT, | ||
| OUT oversized_entries BIGINT) | ||
| AS 'MODULE_PATHNAME', 'pgstatbarkindex' | ||
| LANGUAGE C STRICT PARALLEL RESTRICTED; |
There was a problem hiding this comment.
pgstatbarkindex is declared PARALLEL RESTRICTED, but every other page-scanning function in this extension -- pgstatindex, pgstathashindex, pgstatginindex -- is PARALLEL SAFE. The function does exactly what those do (read-only buffer access under share locks via a read stream, ereport on corruption) and reads no parallel-unsafe state, so there is no apparent justification for restricting it. This inconsistency needlessly prevents parallelizing queries that call pgstatbarkindex where the equivalent btree/hash/gin calls can parallelize. Use PARALLEL SAFE to match. (moderate confidence)
| LANGUAGE C STRICT PARALLEL RESTRICTED; | |
| LANGUAGE C STRICT PARALLEL SAFE; |
| * unique index whose column holds several keys per row (an operator | ||
| * class that indexes an array's elements, say) is unique over the | ||
| * keys and has none. |
There was a problem hiding this comment.
The trailing clause "is unique over the keys and has none" reads as incomplete/confusing. The intent is that such an index enforces uniqueness over the individual keys, not over whole rows, so it is unusable for match-merge. Consider rewording for clarity, e.g. "... is unique over the individual keys, not over rows, so it is unusable here."
| if (afterTriggers.image_file == NULL && | ||
| afterTriggers.image_mem + tuple->t_len <= (Size) work_mem * 1024L) | ||
| { | ||
| img->tuple = tuple; | ||
| afterTriggers.image_mem += tuple->t_len; |
There was a problem hiding this comment.
The spill-to-disk logic only counts tuple bytes (image_mem += tuple->t_len) against work_mem, but the images array itself is never charged against the budget. One AfterTriggerImage entry (24 bytes on LP64) is appended per saved image for the whole transaction, even after tuples spill to the temp file. A large bulk UPDATE (or INSERT) on an in-place-updating AM with AFTER ROW triggers therefore still accumulates an unbounded in-memory array in event_cxt (e.g. ~24 MB per million rows), which contradicts this function's stated goal that "a large bulk update does not hold every row in memory". Compare tuplestore.c, which charges both the tuple and the memtuples array against availMem. Consider including the array growth in the memory accounting, or at least documenting that per-image index overhead is held in memory for the whole transaction. Confidence: moderate.
6f4c64e to
4831504
Compare
There was a problem hiding this comment.
🔍 OCR found 55 issue(s).
- 25 inline, 30 in summary (inline capped at 25)
⚠️ 2 warning(s) during review
📄 src/backend/access/bark/bark.c (L209-L212)
This right-walk loop lacks CHECK_FOR_INTERRUPTS(). It exits on rightmost/ignore/self-loop (bark_next == leftblk), but a corrupt index with a longer right-link cycle (A->B->A is caught, but A->B->C->A is not) would loop uninterruptibly, holding and releasing buffer locks each iteration. nbtree's sibling walks check interrupts; mirror that here.
📄 src/backend/access/bark/bark.c (L1883-L1884)
This stack buffer fill relies only on the Assert(PageGetMaxOffsetNumber(page) <= MaxIndexTuplesPerPage) above for its bound. Unlike bark_vacuum_page, which has a runtime ereport(ERRCODE_INDEX_CORRUPTED) guard before filling its same-sized stack arrays, this second pass re-reads the page under a fresh cleanup lock after bark_lower_bound and only asserts the item count. In a non-assert build, a page that grew past MaxIndexTuplesPerPage (corruption, or a concurrent insert/split between passes) would overflow untag/from/to/items. Add a runtime corruption check here rather than trusting the Assert.
📄 src/backend/access/bark/barksort.c (L100-L103)
BarkPageState.prevblk and prevbuf are dead fields: they are written in bark_pagestate (init) and in bark_flush_page (st->prevblk = flushedblk; st->prevbuf = flushedbuf;), but never read anywhere. The right-sibling links are stamped directly on the page opaque at flush time (BarkPageGetOpaque((Page) flushedbuf)->bark_next = fresh->blkno; and ...->bark_prev = flushedblk;), so these state fields serve no purpose. Per the minimalism discipline, drop both fields and their assignments to avoid unused state.
📄 src/backend/access/bark/barkscan.c (L5300-L5301)
Exception-safety / lifecycle defect in the GIN chunk cache. bark_gin_chunks_take (called from bark_mk_begin at beginscan) sets bark_gin_cache = NULL and reparents the cached context to so->scanCxt for the whole scan lifetime. The cache is only returned via bark_gin_chunks_keep, which runs exclusively from bark_endscan. On any elog(ERROR) during the scan (and GIN-interface scans call user procs and CHECK_FOR_INTERRUPTS throughout bark_mk_gin_items/bark_mk_gin_key), the per-query scanCxt is torn down by the resource-owner/memory-context reset without bark_endscan ever running; the reparented chunk context is destroyed with it, and bark_gin_cache stays NULL permanently for the rest of the backend's life. There is no abort/resource-release hook resetting these statics. Result: the documented cache is silently and permanently disabled after the first error in a GIN-interface query -- a persistent performance regression. More broadly, a process-global mutable static holding a pointer into (transiently) a per-query context is an unusual and fragile pattern for a PG backend; consider keeping the cache context permanently parented under TopMemoryContext (never reparenting it into scanCxt), or registering a cleanup so the global cannot be left stale.
📄 src/backend/access/bark/barkscan.c (L1146-L1146)
User-facing error text spells the access-method name in all caps ("BARK index"). PostgreSQL convention is a lowercase AM name in messages (e.g. "btree index", "gist index"). Same inconsistency appears in the other user-facing errmsg strings in this file (ordering-operator check, cannot-change-direction, and the GIN-interface MVCC/parallel messages). Lowercase the AM name in these errmsg texts for consistency with message-style guidelines.
📄 src/backend/access/bark/barkscan.c (L3222-L3226)
so->keyCmp is allocated sized to so->numberOfKeys but guarded by the one-shot keyCmpReady flag, so the array is sized on the first rescan only and never resized. so->numberOfKeys can differ between rescans: the comment on bark_rescan explicitly supports an external AM (DocumentDB) calling amrescan with a varying nscankeys (<= maxKeys). If a later rescan passes more keys than the first, bark_setup_key_procs returns early via keyCmpReady, leaving keyCmp undersized, and later readers index out of bounds -- e.g. bark_key_bounds does so->keyCmp[sk - scan->keyData] and bark_setup_req_keys does so->keyCmp[chosen[k]] for indices up to the new (larger) numberOfKeys. That is a heap buffer overread/overwrite. Size keyCmp to so->maxKeys (the fixed upper bound from beginscan) instead of so->numberOfKeys, or re-populate when the count grows. (high confidence this is a latent OOB; the trigger depends on an external AM varying the key count across rescans, which this code deliberately permits.)
📄 src/backend/access/bark/barkscan.c (L1256-L1274)
bark_mk_seen (TID loop) and bark_mk_seen_add duplicate the identical seen-set logic: compute key/bit via bark_tid_to_key, bark_seen_find/bark_seen_set, bump nseen, and grow/append the undo list while mk->marked. Factor the per-TID insertion into one helper and call it from both to keep the two in sync (DRY).
📄 src/backend/access/bark/barkscan.c (L3271-L3273)
The shortcut: note is an aspirational future-optimization comment ("detoast those too if a keyset scan over OVERSIZED keys turns up in a profile"). Per project comment discipline, comments should describe current behavior, not conditional future work. Drop it or restate it as a plain statement of the current limitation (row-comparison members are not detoasted here).
📄 src/backend/executor/execReplication.c (L992-L992)
Minor: modified_idx_attrs is declared at function scope, but its assignment and all uses live inside the if (!skip_tuple) block (where version_tag, update_indexes, etc. are declared). Moving the declaration into that block tightens its scope and keeps the new locals together, matching the surrounding style. Not a correctness issue (it is never read when skip_tuple is true). [low confidence]
📄 src/backend/commands/trigger.c (L4440-L4448)
The tuple bodies spill to a BufFile once they exceed work_mem, but the images[] index array itself only ever grows in memory (doubling via repalloc_huge) and is never spilled or accounted against work_mem. For a very large bulk UPDATE on an in-place AM, this array (24 bytes/entry on LP64) can grow to hundreds of MB independently of work_mem, which partly defeats the point of spilling the tuple bodies. Worth a comment acknowledging the index array is bounded only by the number of row events, or capping/accounting it.
📄 src/backend/lib/sbm.c (L10054-L10060)
The serialized body is the map's internal encoding copied verbatim in host byte order (chunk-count header, 8-byte chunk-start offsets, sparse/RLE descriptor words, payload vectors are all native-endian), and sbm_deserialize rejects any stream whose byte order differs from the host (returns NULL). This output is stored verbatim inside a BARK POSTING IndexTuple (barkutils.c sbm_serialize(map, BarkPostingGetData(entry), ...)), which is written to an index page and WAL-logged. That makes the on-disk/WAL format endianness-dependent: a big-endian standby replaying a little-endian primary's WAL, or pg_upgrade/base-backup across differing endianness, will fail to decode postings. This violates the portability gate and directly contradicts the BARK README's documented contract that the on-disk format is portable across little/big-endian. The GIN posting-list code sets the precedent: encode with the explicit little-endian helpers (pg_htole64/pg_le64toh, pg_htole32/pg_le32toh) so the wire/on-disk form is architecture-independent, rather than refusing cross-endian reads. Confidence: high.
📄 src/include/nodes/tidbitmap.h (L26-L27)
Pulling in utils/relcache.h adds a heavy transitive dependency to tidbitmap.h, which is widely included across the executor and planner. Only the Relation typedef is needed here, for the tbm_set_relation() prototype. amlocator.h in this same series already demonstrates the lighter approach by forward-declaring struct RelationData * rather than including relcache.h. Prefer a forward declaration (struct RelationData; typedef struct RelationData *Relation; is already provided by c.h/postgres.h via utils/relcache.h's own typedef, so just forward-declaring the struct and using struct RelationData * in the prototype, or relying on the existing Relation typedef without the full include) to avoid ballooning compile dependencies and risking include cycles. Confidence: moderate.
📄 src/backend/optimizer/util/plancat.c (L472-L473)
For consistency with the sibling fields reset here (amcanorderbyop, ambtreeopfamilies, amcostestimate, etc.), amorderbyopcolumn should also be explicitly reset to NULL in this non-ordinary-index (partitioned) branch. It is harmless in practice because makeNode(IndexOptInfo) zeroes the struct, but leaving it out breaks the local convention that every amroutine-derived field is explicitly mirrored in both branches, and is a trap if someone later reorders allocation. (low confidence this is a live bug; it is a maintainability nit.)
💡 Suggested change
Before:
info->amcanorderbyop = false;
info->ambtreeopfamilies = false;
After:
info->amcanorderbyop = false;
info->ambtreeopfamilies = false;
📄 src/backend/utils/adt/amutils.c (L196-L197)
Minor style inconsistency: GETSTRUCT(tuple) is cast to Form_pg_index and dereferenced twice on consecutive lines, unlike the sibling block above (line 182) which binds rd_rel = (Form_pg_class) GETSTRUCT(tuple) once. Bind the struct pointer once for readability and to avoid re-evaluating the cast.
💡 Suggested change
Before:
if (OidIsValid(((Form_pg_index) GETSTRUCT(tuple))->indimplam))
amoid = ((Form_pg_index) GETSTRUCT(tuple))->indimplam;
After:
{
Form_pg_index index_rel = (Form_pg_index) GETSTRUCT(tuple);
if (OidIsValid(index_rel->indimplam))
amoid = index_rel->indimplam;
}
📄 src/backend/lib/sbm.c (L10322-L10327)
This comment drifted from the code. It says "the first 4 bytes contain chunk_count", but the chunk-count field is SBM_SIZEOF_OVERHEAD (8) bytes wide -- the comment three lines above correctly states so. It also says "we need to walk to compute total size" via "sbm_open's pattern", yet the code does not walk: it sets m_data_used = body_len and leaves validation to sbm_validate(). Fix the width and drop the description of a walk that does not happen here.
💡 Suggested change
Before:
/*
* Force m_data_used to its expected value: the first 4 bytes contain
* chunk_count, then we need to walk to compute total size. sbm_open's
* pattern handles this.
*/
map->m_data_used = body_len;
After:
/*
* The body is the map's raw encoding: an SBM_SIZEOF_OVERHEAD-byte header
* word followed by the body. sbm_validate() both derives the true used
* size from the chunk walk and rejects malformed input.
*/
map->m_data_used = body_len;
📄 src/include/access/relscan.h (L131-L136)
Style/placement: the new IndexScanEntryVerdict comment+enum is inserted between the IndexScanDescData doc comment ("We use the same IndexScanDescData structure...") and the typedef struct IndexScanDescData it documents. This orphans that doc comment from its struct and reads as if it described the enum. Move the enum (and its comment) above the IndexScanDescData doc comment so the latter sits directly on its struct. Low severity, but worth fixing to keep the header readable and the diff clean.
📄 src/include/access/bark.h (L528-L528)
The comment is inaccurate. 0x5241424B laid out as a big-endian uint32 is the byte sequence 0x52 0x41 0x42 0x4B = "RABK", not "BARK". For the string "BARK" (B=0x42, A=0x41, R=0x52, K=0x4B) as a big-endian uint32 the value would be 0x4241524B. The comparison itself is a plain integer compare and so is endianness-safe and correct; only the comment is misleading (low confidence this matters functionally, but it will mislead anyone verifying the magic by eye or in a hex dump). Either fix the comment to say "RABK" or change the constant to 0x4241524B to actually spell "BARK".
💡 Suggested change
Before:
#define BARK_MAGIC 0x5241424B /* "BARK" as a big-endian uint32 */
After:
#define BARK_MAGIC 0x5241424B /* "RABK" as a big-endian uint32 */
📄 src/test/modules/bark/Makefile (L18-L20)
The Makefile is missing NO_INSTALLCHECK = 1, but the companion meson.build sets 'runningcheck': false for both the regress and isolation suites with the note "Injection points are cluster-wide" / "disable parallelism to keep the local-attach semantics deterministic." Under the Make build, make installcheck will therefore attempt to run these tests against a running cluster where cluster-wide injection points and the non-parallel/local-attach assumptions do not hold, so the two build systems diverge and installcheck can behave unstably. The injection_points module's own Makefile sets NO_INSTALLCHECK = 1 for exactly this reason. Add the same guard here to match meson.
💡 Suggested change
Before:
REGRESS_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf
ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf
TAP_TESTS = 1
After:
REGRESS_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf
ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf
TAP_TESTS = 1
# The injection points are cluster-wide, so disable installcheck
NO_INSTALLCHECK = 1
📄 src/test/modules/bark/sql/bark_subtree_delete.sql (L57-L58)
This test prints raw, geometry-dependent values: select level from bark_metap('sd_idx') (expected 4) and select * from sd_shape('sd_idx') (expected per-level page counts 0|500 1|100 2|20 3|4 4|1, and post-VACUUM -1|448 0|137 ...). These counts follow directly from the "1000-byte keys ... six entries a leaf ... tree of height 4" assumption, which only holds on 8 kB pages with 8-byte MAXALIGN. On a non-default block_size or non-8 MAXALIGN build the tree height and page counts change and the test fails with a spurious diff.
This file guards nothing, yet the sibling bark_zerofree.sql in this same patch gates the identical 8 kB / MAXALIGN=8 assumption with \if :skip_test / \quit (and ships a _1.out). Either add the same skip guard here, or rewrite these assertions as comparisons the way bark_split_moveright.sql (:seq_count >= 2401) and check_bark_anyblock.sql (level >= 2) do -- e.g. assert level = 4 / monotonic shape via thresholds rather than printing exact per-level counts. (high confidence)
📄 src/test/modules/bark/sql/bark_lpdead.sql (L434-L436)
This prints the absolute index sizes (size_before/size_after), and the expected output hardcodes 16384 | 16384, i.e. a 2-page index at the default 8kB BLCKSZ. On a non-default block size (16kB/32kB) both values become 32768/65536, so this test fails even though the behavior under test (the index does not grow because LP_DEAD entries are reclaimed before a split) is correct.
The upstream case this is adapted from (src/test/regress/sql/mvcc.sql) deliberately avoids this by only emitting a row when the sizes differ:
SELECT :clean_aborted_self_key_before AS size_before,
pg_relation_size('clean_aborted_self_key') size_after
WHERE :clean_aborted_self_key_before != pg_relation_size('clean_aborted_self_key');
That produces zero rows (block-size independent) on success. Recommend following the same pattern so the comparison is portable across BLCKSZ. The impl check can be kept as a separate query.
💡 Suggested change
Before:
SELECT implam('clean_aborted_self_key'::regclass) AS impl,
:clean_aborted_self_key_before AS size_before,
pg_relation_size('clean_aborted_self_key') AS size_after;
After:
SELECT implam('clean_aborted_self_key'::regclass) AS impl;
-- Emit a row only if the index grew; nothing on success (block-size independent).
SELECT :clean_aborted_self_key_before AS size_before,
pg_relation_size('clean_aborted_self_key') AS size_after
WHERE :clean_aborted_self_key_before <> pg_relation_size('clean_aborted_self_key');
📄 src/test/modules/bark/sql/bark_subtree_delete.sql (L69-L70)
Like the already-flagged bark_metap/sd_shape prints, these three queries emit raw, tree-geometry-dependent values into the expected output: bt_shape('sd_t_idx') prints nbtree's per-level live page counts; the UNLINK_PAGE query prints the distinct internal levels: N values and a per-group row count; and deleted_pages prints the exact number of deleted pages. All of these follow from the specific key size / fillfactor / tree shape and will drift with any change to page layout, MAXALIGN, block size, or the deletion algorithm, breaking the expected file non-portably. Reduce them to shape-independent assertions (e.g. compare to the btree oracle, or assert boolean relationships) as done elsewhere in this file.
📄 src/test/modules/bark/sql/bark_subtree_delete.sql (L22-L22)
pg_create_physical_replication_slot(name, true) passes true for immediately_reserve; the temporary argument defaults to false, so this creates a permanent slot that reserves WAL. If the test errors out anywhere before the matching pg_drop_replication_slot at the end, the slot survives in the cluster, pins WAL indefinitely, and can break or bloat subsequent runs on the same instance (buildfarm). Pass temporary := true (3-arg form pg_create_physical_replication_slot('bark_subtree_delete', true, true)) so the slot is auto-dropped on session end. Same applies to the bark_walinspect slot in bark_walinspect.sql.
📄 src/test/modules/bark/t/002_standby.pl (L118-L118)
high-confidence robustness concern: this test asserts the "wait then cancel" ordering via check_conflict_record(), which depends on replay staying within max_standby_streaming_delay (5s) of the primary so the startup process logs "recovery still waiting ... CONTEXT: WAL redo ... for Bark/X" before cancelling. As the header comment itself admits, a standby whose replay is more than the delay behind cancels at once without the wait, and under buildfarm load (or with wal_consistency_checking page images) that is a real possibility. Upstream 031_recovery_conflict.pl deliberately avoids asserting this ordering for exactly this reason (it uses a 50ms delay and only checks that the conflict/stat happened). Consider dropping the check_conflict_record() assertions (or making them non-fatal/skippable) so the test cannot become a buildfarm flake, and relying on check_conflict_log()/check_conflict_stat() as 031 does.
📄 src/test/modules/bark/sql/bark_walinspect.sql (L26-L26)
Same issue as the slot in bark_subtree_delete.sql: (name, true) creates a permanent slot (the second arg is immediately_reserve, not temporary). An error before pg_drop_replication_slot('bark_walinspect') leaks a WAL-pinning slot into the cluster. Use the 3-arg form with temporary := true so it is cleaned up automatically if the test aborts; the slot is only needed for the duration of this single session, so a temporary slot suffices.
📄 src/test/modules/bark/t/009_validate_single_user.pl (L86-L87)
moderate-confidence maintainability concern: these hardcoded counts (16 client-side INFO reports, 3 invalid classes) are tightly coupled to the exact set of rules barkvalidate emits; barkvalidate.c currently has 17 distinct ereport(INFO) sites, so the mapping from rules to this 16 is not obvious. Adding or removing any validation rule, or changing which of the three crafted opclasses triggers it, breaks this with an opaque off-by-one failure that gives no hint which rule changed. Consider asserting a lower bound (cmp_ok($reports, '>=', ...)) or documenting inline exactly which rule each of the 16 reports corresponds to, so a future change to barkvalidate.c fails loudly and legibly.
📄 src/test/modules/bark_multikey/sql/bark_multikey_sim.sql (L114-L114)
This INSERT is broken and silently does nothing. The column i does not exist in FROM sm (no alias here; the alias i on the previous line comes from generate_series(...) i). The recorded expected output confirms it fails:
ERROR: column "i" does not exist
Because the statement errors out, the comment's "insert again" / re-inserting existing rows (the dup-TID/LIST-entry path) is never actually exercised, and VACUUM then runs against unchanged data. This looks like a copy-paste slip from the preceding line; it was presumably meant to re-id existing rows via id, e.g. SELECT 7500 + id, a. Recording an ERROR in the expected file hides the fact that this DML path is dead. Fix the statement (and regenerate the expected output) so the intended path is covered.
💡 Suggested change
Before:
+INSERT INTO sm SELECT 7500 + i, a FROM sm WHERE id % 13 = 0 AND id <= 6000;
After:
+INSERT INTO sm SELECT 7500 + id, a FROM sm WHERE id % 13 = 0 AND id <= 6000;
📄 src/test/modules/bark_multikey/sql/bark_locator.sql (L17-L19)
These stride values and the whole file's expected output are hardcoded for BLCKSZ = 8192: 291 is MaxHeapTuplesPerPage and 2048 is MaxOffsetNumber only at the default block size (at 4kB they are ~145/1024; at 16kB ~582/4096). Downstream this file also passes the literal 291 to bark_test_tid_mappings(291) and asserts the exact row counts and error strings (stride must be 0 or between 291 and 2048, exceeds the largest offset 291) that derive from it. The entire test will fail on any non-default-BLCKSZ buildfarm animal. Either derive the expected strides/bounds from current_setting('block_size') and compare symbolically, or gate the file on the default block size, so the expected output stays stable across block sizes.
📄 src/test/modules/bark_multikey/sql/bark_locator.sql (L45-L47)
The synthetic-TID mapping checks pass the literal stride 291 (BLCKSZ-8192 MaxHeapTuplesPerPage) and the expected output asserts exact per-check ntids counts derived from it; the bark_test_tid_mappings(100) call also asserts the literal error text stride must be 0 or between 291 and 2048. All of these are BLCKSZ-dependent and will diverge on non-default block sizes. Prefer computing the stride from block_size rather than embedding 291/2048.
📄 src/test/modules/bark_multikey/sql/bark_multikey_gin.sql (L69-L70)
Directly mutating pg_index.indisvalid to force an index choice is a technique not used anywhere else in the PostgreSQL tree; the established idiom is enable_* GUCs plus dropping competing indexes. The risk here is that these two statements run at top level (outside the exception-guarded plpgsql helpers): if any statement between the disable and the UPDATE ... = true restore raises an error, every index on mg is left indisvalid = false for the remainder of the file, cascading into confusing downstream failures. Consider wrapping disable/restore in a transaction that is guaranteed to roll back, or using the GUC+drop approach the rest of the suite already relies on.
📄 src/test/modules/bark_multikey/sql/bark_multikey_ginbound.sql (L202-L202)
total_bytes <= 64 * 1024 + 8192 for the 'BARK GIN stream chunks' context is an allocator/platform-dependent bound. Each kept chunk is a separate malloc block (above ALLOCSET_SEPARATE_THRESHOLD), so the number of free-list blocks at work_mem=64kB is work_mem*1024 / sizeof(BarkGinChunk), and sizeof(BarkGinChunk) differs between 32- and 64-bit (the next pointer). The retained-bytes math stays under ~64kB, but the per-malloc-block bookkeeping counted in total_bytes scales with the block count; the single 8kB slack may be tight on a build with many small blocks. Confirm this holds on 32-bit/large-alignment animals, or loosen the slack to a clearly safe multiple.
| if git diff --name-only upstream/master...HEAD | | ||
| grep -v '^\.github/workflows/fork-'; then | ||
| echo "::error::master carries non-CI changes (listed above); move them to a branch" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Fail-open guard: this pipeline's exit status is used as the if condition, so set -e correctly does not abort on it -- but that also means a failure of git diff itself is silently swallowed. With set -o pipefail, if git diff errors (e.g. upstream/master not fetched, a transient git failure) its non-zero status combined with grep -v matching nothing (exit 1) makes the pipeline non-zero, the if evaluates false, and the script proceeds straight to git rebase + git push --force-with-lease. A destructive force-push to master should not run when the guard command could not actually be evaluated. Capture the diff into a variable first and check git's own exit status before grepping, so a git error aborts instead of bypassing the guard.
| sync: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
The sync job has no timeout-minutes. It runs on a 3-hour cron and ends in a git push --force-with-lease to master; a hung rebase, a network stall on the upstream fetch, or an interactive git prompt could leave the job running until the account/runner default (up to 6h) elapses, burning runner minutes. The fork's own pg-ci.yml sets timeout-minutes on every job; add an explicit (short) timeout here.
| sync: | |
| runs-on: ubuntu-latest | |
| steps: | |
| sync: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 | |
| steps: |
| jobs: | ||
| review: |
There was a problem hiding this comment.
No permissions block is declared at the workflow or job level. Because the job delegates to a reusable workflow via uses:, the called workflow inherits whatever the repository/org default token scope is -- which may be read/write on contents, issues, PRs, etc. The fork's pg-ci.yml explicitly pins permissions: contents: read for least privilege; this AI-review caller should likewise declare exactly the scopes the review needs (e.g. pull-requests: write, issues: write, contents: read) rather than inheriting a broad default.
| jobs: | |
| review: | |
| permissions: | |
| contents: read | |
| pull-requests: write | |
| issues: write | |
| jobs: | |
| review: |
| (github.event_name == 'issue_comment' && github.event.issue.pull_request && | ||
| (startsWith(github.event.comment.body, '/open-code-review') || | ||
| startsWith(github.event.comment.body, '@open-code-review') || | ||
| startsWith(github.event.comment.body, '/pg-history'))) |
There was a problem hiding this comment.
The issue_comment path gates only on the comment body prefix, not on the commenter's author_association. Any user who can comment on a PR (including outside contributors on a public fork) can invoke this Bedrock-cost review workflow by posting /open-code-review. If authorization is not enforced inside the called gburd/ci-workflows reusable workflow, add an author_association check here (e.g. OWNER/MEMBER/COLLABORATOR) to prevent cost abuse by arbitrary commenters.
| (github.event_name == 'issue_comment' && github.event.issue.pull_request && | |
| (startsWith(github.event.comment.body, '/open-code-review') || | |
| startsWith(github.event.comment.body, '@open-code-review') || | |
| startsWith(github.event.comment.body, '/pg-history'))) | |
| (github.event_name == 'issue_comment' && github.event.issue.pull_request && | |
| contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) && | |
| (startsWith(github.event.comment.body, '/open-code-review') || | |
| startsWith(github.event.comment.body, '@open-code-review') || | |
| startsWith(github.event.comment.body, '/pg-history'))) |
| check: | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
There was a problem hiding this comment.
The check job has no timeout-minutes. A reusable-workflow call that stalls (e.g. waiting on AWS/Bedrock or OIDC) would run until the default maximum, consuming runner minutes on a weekly cron. Add an explicit timeout consistent with the fork's pg-ci.yml convention of setting one per job.
| check: | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 | |
| check: | |
| timeout-minutes: 15 | |
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 |
| SELECT pg_relation_size('bark_dpg_idx') / current_setting('block_size')::int < built_pages + 10 | ||
| AS refill_reused_pages |
There was a problem hiding this comment.
The + 10 here is an absolute page-count margin calibrated to the 8kB geometry spelled out in the comment above ("19 pages ... 25 with reuse ... 32 without"): built_pages ~= 19, so the threshold is ~29, which sits between the reuse (25) and no-reuse (32) sizes. On a non-default BLCKSZ this constant no longer discriminates. With 16kB blocks the index is far smaller (built_pages could be ~5, freed ~2, refill ~3), so built_pages + 10 is trivially satisfied whether or not pages were reused -- the assertion silently stops testing reuse. With 4kB blocks the page counts roughly double and the margin can fall the wrong side, risking a buildfarm failure on the non-8kB members. Consider expressing the bound relative to the no-reuse size (e.g. compare against what a fresh rebuild/non-reuse case produces) rather than a fixed + 10. (medium confidence)
| "SELECT pid FROM pg_stat_activity WHERE wait_event = 'bark-overflow-chain-written'"); | ||
| is(PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid), | ||
| 0, 'killed the inserting backend'); | ||
| $s1->{run}->finish; |
There was a problem hiding this comment.
This reaches into BackgroundPsql's internal {run} IPC::Run handle instead of the public $s1->quit API. quit() appends \q to stdin before calling finish, which here would hit a broken pipe because the backend was just KILLed on line 114 (on_error_stop => 0 session). That is presumably why the internal handle is used directly, but the reasoning is undocumented and the access is fragile against framework changes. Add a one-line comment explaining why quit() is avoided (dead backend), so a future BackgroundPsql refactor does not silently break this. Confidence: low.
| FROM generate_series(1, 63) g; | ||
| END LOOP; | ||
| END $$; | ||
| SELECT pg_relation_size('bark_hku_idx') / current_setting('block_size')::int > 2 AS spans_leaves; |
There was a problem hiding this comment.
This > 2 page-count assertion depends on the default 8kB BLCKSZ. The data is fixed at ~5 dead+live versions of key 1, each a ~2016-byte INCLUDE value (63 md5 strings), i.e. roughly 10kB of index data. On a non-default large BLCKSZ (e.g. 32kB) all those entries fit on a single leaf, so spans_leaves becomes false and the expected output diverges. Sibling tests in this group (e.g. check_bark_anyblock.sql, check_bark_oversized.sql) deliberately scale their payload to current_setting('block_size') to stay block-size independent; consider doing the same here, or scale the INCLUDE width / version count to the block size so the run is guaranteed to span leaves on all supported BLCKSZ.
| bark \ | ||
| bark_pages \ | ||
| brin \ | ||
| btree \ | ||
| checksum \ |
There was a problem hiding this comment.
The REGRESS list order here (page bark bark_pages brin btree checksum gin gist hash oldextversions hot_updates) does not match contrib/pageinspect/meson.build, which orders them page btree bark bark_pages brin gin gist hash checksum oldextversions hot_updates. The btree and checksum positions diverge between the two build systems. The pageinspect regress tests are mostly independent so this won't fail today, but the Makefile and meson.build should stay in sync to avoid surprising divergence (and the oldextversions->hot_updates dependency ordering is the one that actually matters, which both happen to preserve). Low severity. Confidence: high.
| /* Not on this page; follow the right link if the parent split. */ | ||
| if (BarkPageRightmost(popaque)) | ||
| break; |
There was a problem hiding this comment.
This right-sibling walk has no CHECK_FOR_INTERRUPTS() and no cycle guard. nbtree's equivalent right-walks (_bt_getstackbuf, nbtsearch.c) and BARK's own scan right-walks (barkscan.c) all include CHECK_FOR_INTERRUPTS(). A corrupt index with a right-link cycle among non-rightmost, non-ignored parent pages (BarkPageRightmost is the only exit here) would spin uninterruptibly while repeatedly pinning/locking buffers, with no way to cancel VACUUM. Add a CHECK_FOR_INTERRUPTS() at the top of the loop body.
5c98bb7 to
551e9eb
Compare
v1 of the BARK series as it will be posted to pgsql-hackers (54 patches on master, plus the fork's dev commit first).
Each patch builds and passes check-world on its own (Linux, cassert, wal_consistency_checking). CI green: https://github.com/gburd/postgres/actions/runs/40587153948
Draft: for the OCR review only; not for merging.