Skip to content

pgcolumnar_projection_pages: the near-empty-projection return path has no arm #1208

Description

@OffgridwithJD

What

pgcolumnar_projection_pages in src/columnar_customscan.c has three return
paths. The suites added in #1155 reach two of them.

if (projSid == 0)
    return fallbackPages;          /* 1: lookup failed -- REACHED */

pages = (BlockNumber) (bytes / COLUMNAR_BYTES_PER_PAGE);

if (pages < 1)
    pages = 1;                     /* 3: near-empty projection -- NOT REACHED */

return pages;                      /* 2: the covering arm -- REACHED */

Path 3 needs the projection lookup to succeed while its row groups sum to
less than one page. That is a different catalog state from "the projection was
not found": findable but nearly empty, rather than absent.

Proof that it is unreached

Mutating only that line, pages = 1 to pages = 997, and rebuilding:

test/projection_scan_io.sh          9 passed + 0 failed   (unchanged)
test/pytest/test_projection_scan_io.py  9 passed          (unchanged)

Neither harness moves, so no arm's outcome depends on the value that path
produces.

Why it is worth an arm

The pricing arm is named "is not priced as one page". It is guarding against the
value path 3 returns while exercising path 1 — so the name describes path 3 and
the coverage is of the lookup-failure case. Those are different states and they
are reached by different catalog contents.

#1155's own comment now distinguishes them correctly ("This 1 is arithmetic, not
the lookup-failure path above"), which is the right fix for a reader. It does not
give the arm.

What this is not

A coverage gap, not a defect. The clamp is correct; pages of 0 would price a
real projection at free and the 1 prevents that. The behaviour under path 3 has
simply never been observed by a test, so a future change to the division, the
rounding, or COLUMNAR_BYTES_PER_PAGE could alter it silently.

Suggested fixture

A projection that exists and is tiny: one covering projection over a handful of
rows, so sum(COLUMNAR_PAGE_ROUND_UP(byteLength)) < COLUMNAR_BYTES_PER_PAGE
while projSid != 0. Then assert the priced pages, and confirm the fixture is
load-bearing by re-running the 997 mutation against it — it must redden.

Raised while reviewing #1155 (1d127de, still present at 1052309). Agreed with
@jdatcmd at the time that it does not belong in that PR.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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