Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,53 @@ true until the next version shipped.

### Fixed

- The arm named "the visibility-only caller decodes nothing" passed on a tree
where it decoded (#1077 sweep).

Two arms in `native_fetch_projection.sh` counted a CALL SITE, with its argument
text, pinned at a literal `1`:

grep -c 'PgColumnarRowIsLive(rel, snap, baseRow)' "1"
grep -c 'PgColumnarReadRowByNumberCols(rel, snap, baseRow' "1"

That asserts a particular call is still written the way it was written, which is
not the property either name claims. Plant the regression they exist to catch --
a full `PgColumnarReadRowByNumber` beside the liveness check in the visibility
path -- and both stay green, because the call they count is still there and the
decode added next to it is invisible to them:

state live cols full | OLD1 OLD2 | NEW
clean 1 1 0 | PASS PASS | PASS
REGRESSED, full decode added 1 1 1 | PASS PASS | RED
honest 2nd narrow caller 2 1 0 | PASS PASS | PASS
empty file 0 0 0 | RED RED | premises RED

Unlike the `allColumns` pair repaired in `8e88f42`, these fail in ONE direction
only. A realistic second caller uses different variable names, so the exact-text
count stays at 1 and the old arm is BLIND rather than falsely alarmed. Row 3 is a
pass for the old arms by accident, not by correctness.

THE PROPERTY IS A ZERO. Three entry points exist and only one decodes every
column, so the projection path is asserted never to call it. More correct callers
of the two narrow entry points move nothing; any full decode moves it off zero --
the opposite failure direction from a count pinned at 1.

Three premises keep the zero from being vacuous: each narrow entry point is
called at all, and that `PgColumnarReadRowByNumberCols(` does not match
`PgColumnarReadRowByNumber(`. That third one DOCUMENTS the assumption rather than
providing the guarantee, which the first version of its comment got wrong: a
broken prefix would make `full >= cols`, and `cols >= 1` with `full == 0` is a
contradiction, so the main arm reddens rather than hiding. Corrected in review by
@OffgridwithJD.

Removal proof, against the real suite: md5 `c8324ce4f486` -> `a2c26702a9cc`, full
decodes 0 -> 1, `16 passed + 1 failed + 0 unrunnable + 0 skipped = 17`, one red
naming `got [1 full decode(s)] want [0 full decode(s)]`, all three premises green,
restored with an empty diff.

The fourth and fifth arm in this file repaired for counting a string across a
whole file; `8e88f42` did the second and third and the `deltuples` comment
records the first.
- The docs said `pgcolumnar.compression` picks a codec. It also picks encodings
(#1076), and two things they told users about `none` are measurably wrong
(#1074).
Expand Down
57 changes: 53 additions & 4 deletions test/native_fetch_projection.sh
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,60 @@ check "and its values are right with nothing decoded from the base" \
# every check above while giving back what the change was for.
SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src"

check "the visibility-only caller decodes nothing" \
"$(grep -c 'PgColumnarRowIsLive(rel, snap, baseRow)' "$SRC/columnar_projection.c")" "1"
# THE ARM NAMED "DECODES NOTHING" COULD NOT SEE A DECODE. Both of these counted a
# CALL SITE, with its argument text, and pinned it at a literal 1 -- so they
# asserted that a particular call is still written the way it was written, which is
# not the property either name claims. Measured: plant the regression these arms
# exist to catch, a full `PgColumnarReadRowByNumber` beside the liveness check in
# the visibility path, and BOTH arms stay green, because the call they count is
# still there and the decode added next to it is invisible to them.
#
# The property is a ZERO, not a one. There are three entry points and only one of
# them decodes every column:
#
# PgColumnarRowIsLive answers visibility, decodes nothing
# PgColumnarReadRowByNumberCols decodes a given set of columns
# PgColumnarReadRowByNumber decodes EVERY column -- the thing to stay out
#
# So assert the file never calls the full decode. A want-zero count is also the
# shape a second honest caller cannot break: more correct callers of the two narrow
# entry points move nothing, while any full decode moves it off zero. That is the
# opposite failure direction from a count pinned at 1, which reddens on correct
# additions and stays green on wrong ones.
#
# The two premises keep the zero from being vacuous: a file that called NOTHING
# would also report zero full decodes.
_np_live="$(grep -c 'PgColumnarRowIsLive(' "$SRC/columnar_projection.c")"
_np_cols="$(grep -c 'PgColumnarReadRowByNumberCols(' "$SRC/columnar_projection.c")"
_np_full="$(grep -c 'PgColumnarReadRowByNumber(' "$SRC/columnar_projection.c")"

check "premise: the visibility-only entry point is called at all" \
"$([ "${_np_live:-0}" -ge 1 ] && echo yes || echo no)" "yes"

check "premise: the column-set entry point is called at all" \
"$([ "${_np_cols:-0}" -ge 1 ] && echo yes || echo no)" "yes"

# `PgColumnarReadRowByNumberCols(` does not match `PgColumnarReadRowByNumber(`, so
# the narrow caller is not counted as a full decode.
#
# THIS PREMISE DOCUMENTS THE INTENT; IT IS NOT WHAT PROVIDES THE GUARANTEE, and
# the first version of this comment said it was. A broken prefix relationship
# cannot pass silently, because the other two arms already contradict each other
# under it: if `Cols(` matched the wide pattern then every narrow call would be
# counted twice, so `full >= cols`, and `cols >= 1` with `full == 0` is a
# contradiction. The arm reddens rather than hiding.
#
# cols=1, prefix intact -> full=0 arm PASS
# cols=1, prefix broken -> full=1 arm RED
# cols=3, prefix broken -> full=3 arm RED
#
# Kept because a reader should not have to derive that, and because it names the
# assumption a future rename would break. Correction from @OffgridwithJD's review.
check "premise: the column-set caller is not counted as a full decode" \
"$(printf 'PgColumnarReadRowByNumberCols(a, b)\n' | grep -c 'PgColumnarReadRowByNumber(')" "0"

check "the reconstruct caller asks only for uncovered columns" \
"$(grep -c 'PgColumnarReadRowByNumberCols(rel, snap, baseRow' "$SRC/columnar_projection.c")" "1"
check "neither caller decodes every column: no full decode in the projection path" \
"$_np_full full decode(s)" "0 full decode(s)"

# Scoped to the function rather than counting a string across the file: the
# string appears legitimately elsewhere now that the index fetch also asks only
Expand Down
Loading