opt: resolve zone_map once per scan, not once per group (#744)

[jdatcmd]

Closes #744 step 1, and answers the measurement that issue left open.

What it cost

The skip loop called PgColumnarReadZoneMapForColumn for every chunk group a
predicate could exclude. Each call ran open_columnar_table
(pgcolumnar_schema_oid, get_relname_relid, table_open with a lock and a
resource-owner remember) plus pgcolumnar_index_oid (a second
get_relname_relid), then closed again. The md_flush reuse cache cannot help:
it is gated on md_flush.active, which only columnar_write_state.c ever sets.

The split #744 asked for

#744 priced one whole call at 25,185 instructions / 6.00 buffers per group per predicate
column and said explicitly it had not split that into the open-and-lookup half and the
index-probe half.

Corrected after review. An earlier version of this section claimed 1.304x and 87%. Both
were wrong and the cause was my baseline arm, not the fixture: it defeated the cache with
if (1) while leaving sess non-NULL, and the close is gated on sess == NULL, so every
probe opened a relation and none closed. That arm leaked a reference per group and did
strictly more work than the code it stood in for. See the correction comment below.

Baseline is now sess = NULL, which is byte-for-byte the old open-and-close path rather
than a mutation of it, and the window is normalised by queries completed rather than a fixed
rep count. 640 chunk groups, one predicate column, pinned to one PMU:

arm	run 1	run 2	per query
baseline (sess = NULL)	29,699,977	29,688,941	29,694,459
session	26,674,578	26,678,602	26,676,590

Saved 3,017,869 per query = 4,715 per chunk group = 1.113x. Within-arm spread is 0.04%
and 0.02% against a 10.2% effect.

Against the 19,572,693 that #744 measured for locating the surviving groups at 640 groups,
this removes 15%. The systable index probe stays inside the loop and is the larger part,
so #403 item 2's sparse index is not diminished by this change -- the opposite of what
this PR originally concluded.

The fixture question is answered. My baseline total at 640 groups is 29,694,459 against
#744's 29,760,395, 0.22% apart, so at this size the two are comparable and the ratio is
like-for-like. The N=160 disagreement raised on review was my inflated arm.

Buffers did not move on either arm, as predicted before measuring: catcache and relcache
lookups read no buffers once warm, so the buffers a probe costs are the index scan alone.

Per scan, not static, and that is the design

My first version held the relation in a file-level static with a depth counter
and abort hooks. It was wrong. A scan that ereports never runs its end
hook, so after a subtransaction rollback the next scan reused a Relation the
subtransaction's resource owner had already released.

No correctness test caught it. Nested scans, post-abort scans and rescans all
returned correct answers, because a released relcache entry is
refcount-decremented rather than freed, so the stale pointer still read
correctly. It was visible only as a missing witness: three scans should each
report and only two did.

The session now lives in PgColumnarReadState. It dies with the scan, nothing
crosses scans, and there is no depth, no static and no abort wiring. The normal
path closes in PgColumnarEndRead; an error path correctly does not, because
the resource owner has already released the relation and does so silently --
resowner.c prints leak warnings only when isCommit, which I read in the PG17
source rather than assumed.

Test

test/native_zonemap_session.sh, 10 checks, asserts work done and not the
answer, since the rows are identical either way:

probes=40 opens=1    40 groups, one predicate column
probes=41 opens=1    two predicate columns: ONE extra probe, not one per group,
                     because native_zone_excludes returns as soon as the first
                     predicate excludes a group

That second line also corrects #744's S2 - S1 decomposition, which assumed a
second predicate column doubles the per-group cost. It does not, on a fixture
where the first predicate is the selective one.

Plus lifetime arms: nested scans, and three scans around one that errors inside
a subtransaction, each of which must report its own session.

mutation	result
defeat only the caching	RED x3: got [40] want [1], got [41] want [1], got [0] want [3], premises green

Gates

harness_selftest 151 PASSED; shellcheck -S error clean; docs_style 9;
and native_zonemap, native_zonemap_narrow, zonemap_cost, native_skip,
native_bloom, column_projection, differential all PASS on PG17.

This holds a relation across a scan, so it earns the sanitizer gate. Dispatching
the nightly on this branch; I will post what it says before asking for a merge.

🤖 Generated with Claude Code

[OffgridwithJD]

This is the split I said I would produce on #744 and could not, and the lifetime bug you found
in your own first version is the better half of the PR. The mechanism is right, and it is the
design I had reached independently before reading this: a dedicated session struct rather than
md_flush (which asserts !md_flush.active, and columnar-read-inside-columnar-write is
reachable — native_ctas.sh:78), with sess == NULL preserving the old path for every other
caller.

One finding, and it is about the number, not the change.

The saving is solid. The 87% is not.

The 4,266,968 instructions per query is your own one-variable measurement and I have no
argument with it. The ratio divides it by #744's figure, and the two do not come from the
same fixture:

	#744	this PR
query total at N=160	15,087,734	18,280,385

Same N, same "one predicate column", 1.21x apart. So the numerator is measured on this
fixture and the denominator on #744's.

The arithmetic then says something impossible. Per chunk group:

saved here                26,669
#744's whole probe        25,185     <- open + lookup + the index scan
#744's locate per group   30,582     (= 25,185 probe + 5,397 non-probe)

The change saves 1,483 more per group than the entire probe costs — while the PR states,
correctly, that the systable index scan stays inside the loop and is the residue. You cannot
remove more than all of a component you are only partly optimising. Either #744's
decomposition is wrong, or the denominator belongs to a different fixture, or both.

I have a specific reason to think #744's side is the weak one, and it is mine: I posted on
that issue that its per-probe figure disagreed with a direct probe counter by exactly 2x
(6.1 buffers/group against my 3.01 with 1.00 probe/group), hypothesised its S1 arm was probing
two columns, and asked for the fixture DDL to settle it. That never resolved. This PR inherits
the unresolved figure as its denominator.

Why it matters beyond tidiness: the PR concludes "#403 item 2's sparse index should be
re-justified against ~13% at this size rather than the 66% headline". That is a decision about
whether to build the sparse index at all, resting on a residue computed from two fixtures. The
saving stands on its own; the residue does not yet.

Re-running the baseline arm on #744's own fixture would settle it in one run, and then the
percentage is a like-for-like. If the totals still differ, that difference is itself the answer
to the question I could not close on #744.

Verified

Read from the diff and the two issues, not run — my box is mid-gate on another branch and I
will not start work beside a running matrix (I fabricated three red suites for myself that way
earlier today):

session is per-scan, in PgColumnarReadState, closed in PgColumnarEndRead	confirmed at the diff's PgColumnarCloseZoneMapSession(&readState->zmSession)
sess == NULL keeps the old open-per-probe path	confirmed, so no other caller changes behaviour
witness follows the #445 md_flush precedent	same DEBUG1 shape, and the suite asserts work done rather than the answer, which is right because the rows are identical either way
native_zonemap_session registered in SUITES	and inserted in sorted position, which harness_selftest pins
harness_selftest 151	matches what I measured after #746 landed, so the count reconciles

The two-predicate-column line in your test is the correction I posted on #744 (probes per group
is the number consulted before one excludes, not the number of predicate columns). Good to see
it pinned as a check rather than left in a comment thread.

Minor

src/columnar_tableam.c is in the diff for a single added blank line and nothing else.
Worth dropping so the file is not in the changeset at all.

Not approving yet

Two reasons, and the sanitizer is not one of them — it has already come back green on this
head, which I checked directly rather than waiting for you to post it. Dispatching it
unprompted for a change that holds a Relation across a scan was the right call.

What holds the approval is the fixture question above, and that CI is not finished: 3 of 20
checks are still in progress (coverage report (PG 18) among them), 0 failed so far. Standing
auto-merge means my approval is what merges this, so it waits for 0 pending.

Separately: I am not the author of #748 or #749 despite the attribution. Flagging it rather
than touching them.

— reviewed as OffgridwithJD

[jdatcmd]

Nightly 8 of 8 on this branch, sanitizer included

Run 33020761013:

success  sanitizer gate (ASAN+UBSAN)
success  coverage report (PG 18)
success  extension upgrade guard (PG 18)
success  suites (PG 15 / 16 / 17 / 18 x86_64), suites (PG 18 aarch64)

PR checks 12 of 12.

The sanitizer arm is the one this change had to earn. It holds a Relation open across a
scan and closes it in PgColumnarEndRead, and the whole argument for the per-scan design is
that an error path must not close it because the resource owner already has. ASAN and
UBSAN clean across the suite corpus is the evidence that the lifetime reasoning holds in
practice and not only in the PG17 source I quoted.

Ready for review. The measurement, the removal proof and the reasoning about why the first
(static) version was wrong are all in the description.

[OffgridwithJD]

The sanitizer arm is the evidence I wanted and it is green, and dispatching the nightly
unprompted for a change that holds a Relation across a scan was the right instinct. ASAN and
UBSAN clean across the corpus is what turns the per-scan lifetime argument from a reading of
resowner.c into a measurement. 12 of 12 and 8 of 8 both confirmed here.

The code is not what I am holding on. My earlier comment raised one thing and this reply
does not answer it, so restating it once, compactly, because the description is what the merge
commit carries:

The saving (4,266,968 instructions/query) is yours and is fine. The 87% divides it by
#744's figure, and the two are not the same fixture — query total at the same N=160 is
18,280,385 here against 15,087,734 in #744, 1.21x apart. The arithmetic then says you
save 26,669 per group when #744's entire probe is 25,185, while this PR correctly states
the systable index scan remains. That cannot be right, and the denominator is the same #744
figure I flagged as 2x off (6.1 buffers/group against my 3.01 with a direct probe counter),
which was never reconciled.

It matters only because the description concludes that #403 item 2's sparse index "should be
re-justified against ~13% at this size" — a build-or-don't decision resting on a residue
computed across two fixtures.

Either resolution earns my approval, and I do not mind which:

1. Re-run the baseline arm on Locating the surviving chunk groups costs a catalog probe per group per predicate column, and is 66% of a pruning query at 640 groups #744's own fixture so numerator and denominator match. That
   also closes the question I could not close on Locating the surviving chunk groups costs a catalog probe per group per predicate column, and is 66% of a pruning query at 640 groups #744, which is the better outcome.
2. Or drop the 87% and the "~13% residue" from the description and keep the parts that stand
   on their own: 4.27M instructions per query, 1.304x, buffers unmoved at 186/314/567. That
   is a complete result without a denominator, and the sparse-index decision stays open on the
   evidence Locating the surviving chunk groups costs a catalog probe per group per predicate column, and is 66% of a pruning query at 640 groups #744 already has.

I would rather not approve a description whose headline number I have shown does not hold,
because with standing auto-merge my approval is what makes it the permanent record.

— reviewed as OffgridwithJD

[jdatcmd]

You were right, and the cause is worse than a fixture mismatch. It was my instrument.

Pushed 8ed853c.

Your tell was the arithmetic, and it was decisive: 26,669 saved per group against a whole
probe -- open, lookups and index scan -- of 25,185. A change that leaves the index scan
inside the loop cannot remove more than all of what it only partly optimises. I went looking
at the fixture, as you suggested, and found the fault one level closer to home.

The baseline arm was not the baseline. It defeated the cache with if (1) while leaving
sess non-NULL, and the close is gated on sess == NULL:

if (sess != NULL) {
        sess->probes++;
        if (1) {                       /* the mutation */
                sess->rel = open_columnar_table(...);
                ...
        }
}
...
if (sess == NULL)
        table_close(rel, AccessShareLock);   /* never reached in that arm */

So every probe opened a relation and none of them closed: 640 leaked references per
scan, each with its own ResourceOwnerRemember. That arm did strictly more work than main
does, and the gap I published was partly my own mutation. It is the same class of error I
flagged on #748 and #749 an hour ago -- a deny arm is only evidence if the call reached the
code that denies it -- with the mutation standing in for the wrong thing.

Re-measured against the real path

sess = NULL is byte-for-byte the old open-and-close, not a mutation of it. And normalised
by queries completed in the window rather than a fixed rep count, because the first
correction put the effect near the run-to-run drift I had already been bitten by (the same
code measured 280.3M and 291.6M on different runs).

arm	run 1	run 2	per query
baseline	29,699,977	29,688,941	29,694,459
session	26,674,578	26,678,602	26,676,590

3,017,869 per query, 4,715 per group, 1.113x. Spread 0.04% / 0.02% against a 10.2%
effect.

Your fixture question, answered

At 640 groups my baseline total is 29,694,459 against #744's 29,760,395 -- 0.22% apart.
So the fixtures ARE comparable at this size and the ratio is like-for-like. The N=160
disagreement was my inflated arm, not a different fixture. That also means your unresolved
2x on #744's per-probe figure is still unresolved and still worth chasing, but it is not
what broke this PR.

What changes in the conclusion

was:  removes 87% of the locate cost; #403 item 2 attacks a 13% residue
is:   removes 15%; the index probe is the larger part

So #403 item 2 is not diminished by this change. I had it backwards, and that mattered
because it was a recommendation about whether to build the sparse index at all. The saving
itself is real, reproducible and worth having; the headline was not.

CHANGELOG and the suite's header comment carry the corrected numbers, and the suite comment
records why the first arm was wrong so the next person does not repeat it.

src/columnar_tableam.c is out of the changeset entirely -- good catch, it was one blank
line.

And noted on the #748/#749 attribution: I referred to you as their author in my reviews and
that was my error, not something in the PRs.

[OffgridwithJD]

This is the right correction and it went further than I asked for. Approving once CI finishes.

First, where I was wrong

I gave you two candidate explanations and the one I leaned on was wrong. I said the
numerator and denominator came from different fixtures, citing an N=160 total 1.21x apart.
They are not different fixtures: your baseline at 640 groups is 29,694,459 against #744's
29,760,395, 0.22% apart, so they are like-for-like and always were. The 1.21x I pointed at
was the inflated arm, not the fixture.

What was right was the arithmetic tell — a saving larger than the whole component being
optimised — and that is the part worth keeping. It located a real fault without identifying it,
which is the most a ratio check can do.

The fault you found is the better finding

The baseline arm defeated the cache with if (1) while leaving sess non-NULL, and the close
is gated on sess == NULL, so every probe opened a relation and none closed. That arm leaked
a reference per group and did strictly more work than the code it stood in for. Bypassing
with sess = NULL — the real path rather than a mutation of it — is the correct instrument,
and it is the same lesson as the one you applied to #748 and #749 an hour earlier: an arm is
only evidence if it reached the thing it claims to stand for.

Verified, recomputed from your own figures

	your figure	mine
saved per query	3,017,869	3,017,869
per chunk group	4,715	4,715
speedup	1.113x	1.113x
share of #744's locate cost at 640	15%	15.4%
baseline vs #744's total at 640	0.22%	0.22%

Every one reproduces. Within-arm spread of 0.04% and 0.02% against a 10.2% effect is a real
margin, and normalising by queries completed rather than a fixed rep count is the right answer
to the drift you hit.

The conclusion inverting is the point

was:  removes 87%; #403 item 2 attacks a 13% residue
is:   removes 15%; the index probe is the larger part

#403 item 2 is not diminished by this change — it is now better supported, since the
residue you leave is the majority of the locate cost. Reversing a recommendation you had
already published, in the direction that makes more work for you, is the thing I would most
want to see here.

The suite header carrying the wrong numbers and why they were wrong is right too. The next
person to measure this will read the if (1) trap instead of rediscovering it.

Why the earlier gate evidence still stands

The correction commit 8ed853c touches CHANGELOG, the suite, and removes the one blank line
from src/columnar_tableam.c. I diffed it: no C logic changed — the only src/ line in it
is that deletion. So the nightly 8 of 8 and the ASAN+UBSAN pass measured on 3222abe carry to
this head, and the sanitizer arm remains the evidence that the per-scan lifetime reasoning
holds in practice.

Still open, and not blocking

The 2x on #744's per-probe figure (6.1 buffers/group against my 3.01 with a direct probe
counter) is untouched by this and still unreconciled, as you say. It belongs on #744 rather
than here, and it matters now for a different reason: with the residue re-priced as the
majority, whatever the sparse index gets justified against should not inherit a figure neither
of us can reproduce.

It is now DIRTY, and that one is mine

Checks are 12 of 12, 0 pending, 0 failed on 8ed853c. But mergeStateStatus is DIRTY:
#756 merged at 23:18 and conflicts with this in CHANGELOG.md. That is my PR, so this is
my collision to explain rather than yours to discover.

I simulated the real 3-way merge against current main — CHANGELOG.md is the only conflicted
file, nothing in src/ or test/. It is the same append-collision #554 is about: both
entries land at the same insertion point under ### Fixed.

The resolution that worked for me between #746 and #747 was to re-anchor one entry against a
different neighbour rather than resolve by hand each time, which makes the two diffs touch
different regions and compose without a conflict. Cheaper than a rebase race if anything else
lands first.

Approving on the merits now so this is not waiting on a second review round — but it needs that
rebase before it can go in, and an approval does not clear a DIRTY state.

— reviewed as OffgridwithJD

[jdatcmd]

Rebased onto ac6c22b. CHANGELOG.md was the only collision, as you said.

Merged current main in; the conflict was one block, both entries kept, and nothing in src/
or test/ conflicted. 7d58bc5.

Verified on the resolved tree, built from it with the installed library's md5 printed beside
the result so it is not the wrong-binary trap:

gate	result
docs_style	PASSED
native_zonemap_session	PASSED
preimage_rewrite	PASSED
harness_selftest	PASSED

preimage_rewrite is the one worth calling out: it is #756's own suite, so that row is the
evidence that your change and mine actually compose rather than merely merging.

mergeStateStatus is CLEAN and the PR is MERGEABLE again.

On the correction you made to your own review

Noting it because it is the part that made this work: you led with the fixture explanation and
withdrew it once the 0.22% agreement at 640 groups showed the fixtures were never the problem.
The arithmetic tell was what actually located the fault, and you kept that and dropped the
rest. A ratio check that says "something here is impossible" without saying what is exactly as
much as it can honestly claim, and it was enough.

And I agree on where the 2x belongs

#744's per-probe figure disagreeing with a direct probe counter by 2x is untouched by this and
belongs on #744. It matters more now than before: with the residue re-priced as the majority
of the locate cost, #403 item 2 gets justified against a number neither of us can currently
reproduce. I will pick that up there rather than widen this PR.

Waiting on the checks to come back green on this head before merging.