Skip to content

Chunk-group skipping: the <= boundary is pinned only incidentally, and a pruning regression that loses no rows is nearly invisible #831

Description

@OffgridwithJD

What I did

You are reviewing for bugs. A bug review is only as good as the tests that pin the
fixes, so I measured what this suite set can actually catch on the surface your
review is most likely to touch: chunk-group skipping.

Method: plant one plausible-wrong line in native_zone_excludes
(src/columnar_reader.c:1344-1372), run the full pg18 matrix, record which
suites notice. Four mutations, each an exact pruning boundary. Predictions written
into the log before any run. Control: main at 808cd46 is 231 PASS / 0 FAIL /
2 SKIP on the same box, same day.

Each mutation asserted it applied (git diff --numstat exactly 1 1 src/columnar_reader.c) before its matrix ran — one attempt did not apply and was
refused rather than reported as a survivor.

Results

# branch change semantics suites that caught it
M1 BTLessEqual c1 > 0 -> c1 >= 0 skips a group whose min == const; loses rows 6
M2 BTGreater c2 <= 0 -> c2 < 0 conservative; loses only pruning 1
M3 BTEqual || -> && conservative; disables equality pruning 2
M4 BTGreaterEqual c2 < 0 -> c2 <= 0 skips a group whose max == const; loses rows 12

Every M1 and M4 catch is genuine row loss against a heap comparison, not a
coincidence:

M1 [bloom_lazy]            a range predicate still agrees with the heap: got [100000] want [120000]
   [write_minmax_fastpath] every column returns the same rows as heap under range predicates:
                           got [i4_bound(columnar=2 heap=3) ...] want []
   [unique_conc]           3 both distinct keys present: got [10] want [10,20]
M4 [phase6]                min/max filtered: got [30001|40000] want [30000|40000]
   [preimage_rewrite]      [ts_trunc >= c] returns the heap's answer: got [190001] want [190002]

The finding: <= is the thin side, and it is thin by 2:1

M1 and M4 are the same defect mirrored. M4 is caught by twice as many suites,
and the reason is visible in the corpus rather than inferred from the run.

differential catches M4 and misses M1. That is the suite whose own header
calls the heap mirror a generic catcher of "encode/decode and skipping bugs".

Counting WHERE <col> <op> across test/*.sh at 808cd46:

  • >= — 18 suites, including differential, native_zonemap, native_skip,
    native_exact_selection, native_saop_pushdown, preimage_rewrite.
  • <= — 16 suites, and not one of the skipping suites above. The only
    skipping-adjacent entry is native_zonemap_session, which passed M1 — so its
    <= is not positioned at a group's minimum.

preimage_rewrite alone fires 14 boundary checks under M4, all >= / >. There
is no <= counterpart anywhere.

So M1 survives every suite named for the surface it breaks
(native_zonemap, native_zonemap_narrow, native_zonemap_session,
native_skip, zonemap_cost, zonemap_estimate_sample, native_exact_selection,
native_saop_pushdown, native_bloom, differential, concurrent_diff) and is
caught only by suites that reach it incidentally.

Stated precisely, because it matters: this is not a coverage hole. The gate
went red on both row-losing mutations, so a <= boundary regression would be
caught today. It is a fragility finding — the coverage is incidental for M1 and
intentional for M4. Six incidental catches can become zero through fixture edits
that no one would think to review as a coverage change.

What I suggest

A <= / < boundary arm mirroring preimage_rewrite's >= arms: a fixture with
a group whose minimum is exactly the constant, queried with col <= K, checked
against the heap mirror by ROWS, not by count. A wrong scan key only loses rows if
it prunes a whole group, so the fixture has to put the boundary value on a group
edge deliberately.

Cheapest home is differential, since it already carries the >= half and the
oracle.

The other half: pruning effectiveness is pinned by exactly three assertions

M2 and M3 cannot lose a row. A && B implies A || B, and both mutations only
ever exclude a subset of what the original excluded, so every correctness oracle
in the matrix is blind to them by construction. They were caught anyway, and by
assertions that measure whether pruning still prunes:

M2 [zonemap_estimate_sample] (the cost-model estimate moves)
M3 [bloom_lazy]  a scan that keeps every group reads more: got [no] want [yes]
M3 [native_skip] bigint equality vs integer literal skips groups (#477): got [no] want [yes]

That is three assertions, in three suites, out of 231. It is not nothing — I
predicted these two would survive outright and they did not, which is to your
credit. But it is thin, and it is the only thing standing between you and a
pruning regression that returns perfectly correct answers while reading the whole
table.

Reproducing

Everything needed is above: the four edits are one line each in the
native_zone_excludes() switch, src/columnar_reader.c:1344-1372. For each,
apply the single substitution, assert it landed
(git diff --numstat must be exactly 1 1 src/columnar_reader.c, because a
no-match and a no-op are indistinguishable otherwise), rebuild, and run
bash test/run_all_versions.sh <pg_config>.

Two traps I hit, in case you script it:

  • sed -i "s|old|new|" breaks on M3, whose pattern contains ||. The delimiter
    collides, sed errors, and the mutation silently does not apply. Without the
    numstat assert that reads as "SURVIVED — no suite noticed", which would have been
    a spectacular false finding.
  • grep -c "^ FAIL " on a matrix log overcounts by one per failing version: the
    summary line FAIL PG18 (231 ran...) matches too. My first count said 7 for
    M1; it is 6.

My driver scripts and the four matrix logs are on my audit container rather than
anywhere you can reach, so I have not linked them; ask if you want any of them
pasted.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions