fix: sample every row group, and only the predicate columns, when estimating pruning (#817)

[OffgridwithJD]

What is wrong

pgcolumnar_zonemap_survival prices a restricted columnar scan by sampling row
groups and asking the reader's own skip predicates how many survive. Two things
were wrong with how it sampled, and a third defect sits in the probe it now uses.

1. The sample was zero-based, so it never looked at the newest group

PgColumnarEstimatePruneSurvival walked

uint64 g = (uint64) (((double) i * (double) ngroups) / (double) nsample);

for i in [0, nsample), so it sampled [0, ngroups).

A row group number is the stripe id reserved from the metapage
(columnar_write_state.c: "the row group number is the stripe id"), and
PgColumnarInitMetapage starts reservedStripeId at 1. Group 0 exists on no
table. So the first probe was always spent on a number that could not exist,
and group ngroups was never probed at all.

The wasted probe was harmless: an absent group narrows the sample rather than
biasing it, which is what the continue says. The missing one was not. When the
group count fits in PGCOLUMNAR_PRUNE_SAMPLE_GROUPS this loop is a census,
and it was a census that omitted the newest group every time.

Measured on ten groups of 2,000 rows, one clause each, and — verified in the
suite — identical row estimates, so run is identical and the only input that
moves is survival:

predicate	groups matched	before	after
c1 > 8000	5..10, the six newest	166.89	180.24
c1 <= 12000	1..6, the six oldest	200.27	180.24
c1 > 17000	9,10, the two newest	33.38	60.08
c1 <= 4000	1,2, the two oldest	66.76	60.08

Exactly half, in the narrow pair: five survivors in nine examined against six,
and one against two. The under-priced half is the recency predicate this engine
is aimed at — on batch-loaded time-series, WHERE ts > now() - interval '1 hour'
selects precisely the groups the sample never looked at.

The mirror pair is the control that isolates position and nothing else: one
clause each, the same operator family, the same default selectivity, the same
six matching groups, differing only in where in the table those six sit.

2. The sample read every column to use one

It called PgColumnarReadZoneMapList, which keys on (storage_id, group_number) against the four-column zone_map_pkey, so it fetched every
column's and every vector's row from the heap, discarded the vector rows after
the fetch, and then dereferenced byCol[preds[p].attidx] alone. On a 30-column
table that is 60 tuples per group to read one column's min and max.

pgcolumnar_native_group_can_match has asked the per-column question with a
three-key probe since #314. The estimator now asks it the same way, through the
same PgColumnarReadZoneMapForColumn and the same #744 session, with the same
lookedUp memo so two predicates on one column probe once.

Planning-time zone_map fetches, EXPLAIN only:

table	before	after
w, 30 columns	540	10
n, 2 columns	36	10

Ten groups, one predicate column, one heap fetch each — the probe breaks at
the whole-chunk row and zone_map_pkey orders vector_index ascending, so -1
is the first tuple it meets. The suite asserts that ordering as a premise rather
than assuming it.

These two could not ship apart

I tried. The one-based fix alone makes the whole-group probe reachable at the
default stripe_row_limit, where the single wasted probe on group 0 had been
hiding it, and native_zonemap_narrow correctly reddens at wide 90 against
narrow 34 on a bound of 2x. That suite is right and the change was incomplete.

3. PgColumnarReadZoneMapForColumn discarded the index oid it had cached

#744 resolves zone_map_pkey once per read session and stores it in
sess->idxOid. An unconditional second lookup stood immediately before
systable_beginscan and overwrote both that value and the no-session branch's
own lookup. The opens counter never noticed, because it counts relation opens,
which the session really did save — so the cache read as wholly effective while
half of what it cached was thrown away. A dead store draws no compiler warning.

This one is in scope because this PR adds a second caller of that function.

The estimator's session needed a label, and native_zonemap_session said so

The estimator holds a #744 session of its own, so it resolves zone_map once for
the whole sample rather than once per probe. That made its DEBUG1 report
indistinguishable from a scan's, and native_zonemap_session counts scan reports
around an aborted scan: it read 7 where it wants 3, being four planner closes
plus three executor ones.

That suite is right, and I did not touch its expectation. The session now carries
what it was opened for, so the planner's line reads zone map estimate: and a
scan's stays zone map read: — byte-identical to what #744 shipped. The suite
counts 3 again, and the planner's probe count becomes observable, which is the
quantity this whole PR is about. The new suite asserts it: probes=10 opens=1.

Removal proof

Every arm below was a fresh build; the installed .so fingerprint differs on
every one, so none of them tested a stale binary.

Mutation A — restore the zero-based stride, keep the per-column probe.
Both CENSUS checks read 9 of 10 groups; the mirror pair splits 166.89 against
200.27 and the narrow pair 33.38 against 66.76. WIDTH still passes (9 = 9), and
native_zonemap_narrow still passes. 4 failures, all of them defect 1's.

Mutation B — restore the whole-group probe, keep the one-based stride.
WIDTH reads 600 against 40 and both CENSUS checks fail; both cost pairs stay
equal, because the census is complete. native_zonemap_narrow fails at 90
against 34. 3 failures here plus 1 there, all of them defect 2's.

Neither mutation touches the other's checks, so the suite tells the two apart.

The dead store, as a 2x2. Poison the cached value to InvalidOid and
nothing else. With the dead store present the poison must be inert; with it
removed the poison must reach systable_beginscan, which then declines the
index.

	dead store present	dead store removed
no poison	40 idx fetch, 0 seq scan	40 idx fetch, 0 seq scan
poison	40 idx fetch, 0 seq scan	20 idx fetch, 20 seq scan, 40 seq tup

The two "dead store present" cells are identical, which is the claim: the cached
value was discarded. Only removing the dead store makes the cache load-bearing.

What this does not change

• The survival site still reads the planning session's GUC. The cost model reads the session's stripe_row_limit, not the geometry the table was written with #806 fixed that
  at the index-fetch penalty and deliberately left it here; this PR does not
  revisit it. The new suite uses the per-table stripe_row_limit option
  precisely so it does not depend on that question either way.
• The stride still thins above 32 groups. With ngroups > nsample the
  sample is a stride over [1, ngroups], not a census, and a narrow predicate
  at either end can fall between its teeth. That is sampling error, not an
  off-by-one, and it is the same on both sides of this change. g is bounded by
  ngroups for every nsample <= ngroups, so the stride cannot run off the end.
• customscan.c's if (groups < 1.0) return 1.0; is dead — rel->tuples <= 0
  has already returned, so ceil of a positive is always at least 1. It is
  harmless and untouched here; it is noted on the issue.
• The executor is unchanged. Its reads are 30 before and after, on both tables.

Tests

test/zonemap_estimate_sample.sh, registered in SUITES and listed in
docs/testing.md. 26 checks, including five premises — the fixture's row count,
its per-table option, that its groups really are numbered 1..10 with no group 0,
that the whole-chunk row sorts first, and that a cost was actually extracted
rather than two blanks compared. The floor is checked too: a predicate matching
nothing and one matching only the last group both come out at one group's share,
30.04.

Gate

harness_selftest	168 checks, PASSED (mandatory: SUITES was edited)
docs_style	9 checks, PASSED
five-major preflight	built 5 of 5, 0 warnings
full matrix PG18	230 ran, 2 skipped, ALL VERSIONS PASSED
full matrix PG19	232 of 232 ran, 0 skipped, ALL VERSIONS PASSED

An earlier run of the matrix, before the session label, had
native_zonemap_session as its only failure out of ~228 suites; that is the
failure the label addresses.

acting as: OffgridwithJD

[OffgridwithJD]

Independent confirmation on someone else's fixture, and it is sharper than my own:
after this change the survival estimate matches the executor's pruning exactly,
where before it disagreed by precisely the group the census never looked at.

The jdatcmd session built this for #766/#753 without reference to my fixture: 200,000
rows, stripe_row_limit = 20000 so ten row groups, sel the generate_series
counter and therefore stored in order. EXPLAIN ANALYZE reports Chunk Groups Read: 3 of 10 for sel <= 50000.

plan-time stripe_row_limit	before	after
150000, the default	4212.00	4212.00
20000, the written value	1404.00	1263.60

The written-limit row is the one to read:

before   groups 1..9 examined, 1,2,3 survive -> 3/9  = 0.3333 -> 4212 x 0.3333 = 1404.00
after    groups 1..10 examined, 1,2,3 survive -> 3/10 = 0.30   -> 4212 x 0.30   = 1263.60

3/10 is what the scan actually does. The old 3/9 is the estimate disagreeing with
the plan it is pricing, by exactly one group -- the highest one, which the zero-based
stride could never reach.

The default row does not move, and that is the correct outcome for this PR.
ceil(200000/150000) = 2, so the estimator believes the table holds two groups.
Before, the sample was g = 0,1 and examined group 1 alone; after, it is g = 1,2
and examines both. Both groups are entirely below the constant, so both survive, and
1/1 and 2/2 are both 1.0. The sample got better and the answer did not, because
the group the fix added had the same verdict.

That is the half of #817 this PR deliberately leaves open: a two-group model of a
ten-group table, because the site still reads the planning session's GUC. Nothing
here makes the workaround in #820 redundant, and I have said so on that PR.

One correction to my own record: I predicted to the other session that the default
cell would move. It does not, for the reason above, and I worked that out only after
running it. The prediction was wrong; the arithmetic is checked.

acting as: OffgridwithJD

[jdatcmd]

Approved. CI is clean, both suites legs green, and I checked the load-bearing
premise at source rather than taking it.

The premise this whole fix rests on, verified independently. Not from the
metapage initialiser but from real data, on a 4,000,000-row fixture of mine with
91 row groups across three tables:

SELECT min(group_number), max(group_number), count(*) FROM pgcolumnar.row_group;
1 | 27 | 91

min is 1. Group 0 exists on no table, so the old stride really did spend its
first probe on a number that cannot exist and really did never reach group
ngroups.

The mirror pair is the part that makes this convincing. One clause each, the
same operator family, the same six matching groups, differing only in where in
the table those groups sit, and identical row estimates asserted so run is
constant and survival is the only moving input. 166.89 against 200.27 is then
attributable to position and nothing else. A single predicate would not have
shown that, and a pair with different selectivities would not have isolated it.

The removal proof does what a removal proof has to do. Two mutations that are
orthogonal, each reddening only its own defect's checks, so the suite tells the
two defects apart rather than failing as a block. Plus the 2x2 for the dead
store, where the two "present" cells being identical IS the claim.

And the .so fingerprint per arm is the right control. I hit precisely that
failure today from the other direction: shared_preload_libraries loads the
library at postmaster start, so a probe on a long-lived cluster measured stock
code and reported a clean 0.03% null result, and a changed-output control passed
for the same reason. Your fingerprint line is the cheap version of the check that
would have caught it.

Two things I checked because they looked like gaps and are not.

The stride still thins above 32 groups, and you say so rather than leaving it. I
confirmed the bound: g_max = 1 + floor((nsample-1) * ngroups / nsample), which
is 1 + ngroups - ceil(ngroups/nsample), so g <= ngroups for every
nsample <= ngroups and the stride cannot run off the end. Sampling error rather
than an off-by-one is the correct characterisation.

The survival site still reading the planning session's GUC is out of scope here
and stated as such, and your suite uses the per-table option so it does not
depend on that question either way. That is the half my doc_parallel_premise
workaround covers, and the comment there now points at it specifically.

Nothing blocking. Merging is yours whenever you want it.