fix: probe options and projection catalogs through their primary keys

[linuxhikerpm]

Summary

• Planning one columnar table sequentially scanned pgcolumnar.options and pgcolumnar.projection. options_pkey is (regclass) and projection_pkey leads with storage_id, which is what those lookups ask for, and both scans passed InvalidOid.
• PgColumnarReadOptions, PgColumnarReadTtl, PgColumnarReadSortBy, PgColumnarDeleteOptions, PgColumnarListProjections, and PgColumnarDeleteProjectionRow now pass those indexes, with the same OidIsValid fallback the other catalogs use. The pgcolumnar.storage lookup by relation_oid stays sequential: storage_pkey is on storage_id.
• I will not approve or merge this.

Test plan

Independent twins test/catalog_plan_index.sh and test/pytest/test_catalog_plan_index.py. Same public seam (pg_stat_all_tables after one filtered scan). Different tables, row counts, and aggregates. The pytest scan runs on a second connection.

Unfixed .so 472bd6ec8301, source 8dbf8d2fd6fe, PG18:

Shell:

-- options idx_scan=0 seq_scan=2
FAIL  planning probed pgcolumnar.options through options_pkey: got [0] want [1]
FAIL  planning did not sequentially scan pgcolumnar.options: got [2] want [0]
-- projection idx_scan=0 seq_scan=2
FAIL  planning probed pgcolumnar.projection through projection_pkey: got [0] want [1]
FAIL  planning did not sequentially scan pgcolumnar.projection: got [2] want [0]

Pytest:

-- options idx_scan=0 seq_scan=1 projection idx_scan=0 seq_scan=1
AssertionError: planning probed pgcolumnar.options through options_pkey: got 0, wanted at least 1

Fixed .so a88744fa6d65, source 966700aa9e49:

-- options idx_scan=2 seq_scan=0
-- projection idx_scan=2 seq_scan=0
accounting: 6 passed + 0 failed

Pytest on that same .so: idx_scan=1 seq_scan=0 on both catalogs, 6 pass.

Causation, InvalidOid put back on the two planner scans, .so 215ab2396eaa:

-- options idx_scan=0 seq_scan=2
-- projection idx_scan=0 seq_scan=2

Pytest on that .so: options idx_scan=0 seq_scan=1 projection idx_scan=0 seq_scan=1.

Shell green on PG 15, 16, 17, 18, and 19, source 966700aa9e49, 6 passed on each. Ledger rows carry 15;16;17;18;19. checks_never_observed_red re-counted at 1455. suites_not_covered unchanged. cluster_tests re-derived by collection at 464.

Made with Cursor

[jdatcmd]

Reviewed e8285ba, built and measured in pgcolumnar-dev on PG 18.4. The change is right, the causation proof is the right instrument, and the index choices check out against the install script. Three things: one is red in CI, one is a site the change's own argument covers and skipped, and one is a number I measured that belongs in the record.

CI is red, and the cause is precise

Three pytest legs fail on the same thing:

AssertionError: every pair in the tree is declared, so none is silently ungraded:
    got 'catalog_plan_index' want 'none'
AssertionError: premise: every pair on disk is declared somewhere, so this loop
    and the INCOMPLETE list account for all of them: got 37 want 38

test/catalog_plan_index.sh and test/pytest/test_catalog_plan_index.py are a new PAIR, and compare_to_bash.py declares neither. It needs to go in the COMPLETE list if the two grade one for one, or the INCOMPLETE list with the reason if they do not. Nothing else is failing.

Worth noticing what that guard is: it is the one that refuses a silently ungraded pair, and it fired the moment a pair appeared. It is silent when a suite ships with NO twin at all, because then there is no pair to grade.

One site the change's own argument covers

options_pkey is (regclass), the same column this key names is true of six call sites, and five of them moved:

line	function	index
3086	PgColumnarReadOptions	options_pkey
3308	PgColumnarReadTtl	options_pkey
3355	PgColumnarReadSortBy	options_pkey
3409	PgColumnarDeleteOptions	options_pkey
1988	PgColumnarRenameDeclaredSortByColumn	InvalidOid

It is the same shape as PgColumnarReadSortBy line for line: same Anum_options_regclass key, same BTEqualStrategyNumber, same single-row if (HeapTupleIsValid(...)) read, and like PgColumnarDeleteOptions it takes a RowExclusiveLock with a NULL snapshot after a CommandCounterIncrement().

The cost is small and that is not the reason to fix it. It runs on ALTER TABLE ... RENAME COLUMN, not on a plan, so no measurement will show it. The reason is that the population here was defined by a list of six function names rather than by what the code does, and a list is what silently regrows. Defining it as "every scan whose key column is the index's column" finds this one and finds the next one.

Four more of the same shape, measured, and NOT on the plan path

projection_declaration_pkey is (rel, name), and four scans key on Anum_projection_declaration_rel, which is that leading column: PgColumnarListProjectionDeclarations, PgColumnarRenameProjectionDeclarationColumn, PgColumnarDeleteProjectionDeclaration, PgColumnarDeleteProjectionDeclarationsForRel.

I measured before saying it mattered, and it does not, on this path:

reset; a backend that plans and runs one columnar query; read pg_stat_all_tables

 relname                | idx_scan | seq_scan
------------------------+----------+----------
 options                |        1 |        0     <- your fix
 projection             |        1 |        0     <- your fix
 projection_declaration |        0 |        0     <- never reached
 storage                |        0 |        2

A first pass of mine read projection_declaration seq_scan=1 and I nearly reported it as a live defect; resetting immediately before the measured backend instead of before the DDL showed the scan belonged to CREATE TABLE, not to planning. So these four are in kind with the one above and cost DDL time only. Your call whether they belong in this change.

The measured remainder, which is not yours to fix here

pgcolumnar.storage is sequentially scanned twice per planned query and never through an index, in the control above, on your build. Your note is correct about why: storage_pkey is (storage_id) and the lookup is by relation_oid, and I confirmed the install script gives pgcolumnar.storage that one index and no other.

So the headline defect is two thirds fixed, and the remaining third is the one that scans twice. Closing it needs a new index on relation_oid, which is a catalog change in pgcolumnar--1.0-alpha4--1.0-alpha5.sql and in the base script, plus the convergence check. That is a different change and I am not asking for it here. I am asking that the number appear somewhere, because "stays sequential" reads like a small residue and it is the largest one left.

The control that makes those numbers mean something

The zero is real. A backend that runs SELECT 1 after the same reset reports 0 | 0 on all four catalogs, so the counts above are what one planned columnar query costs and not what a connection costs.

What I verified rather than took on trust

claim	how
options_pkey is on (regclass)	read from the install script, not from the PR body
projection_pkey is (storage_id, projection_id)	same, and storage_id leads, so the one-key ListProjections scan uses it
pgcolumnar_index_oid is cheap enough for the plan path	it is get_relname_relid, a syscache lookup, and the pattern predates this change
the OidIsValid fallback matches the house pattern	the other catalogs already do exactly this
deleting while scanning the index is safe	PgColumnarDeleteOptions and PgColumnarDeleteProjectionRow do what core does in RemoveAttributeById
the pytest twin's stats are not read before they are flushed	pg_stat_force_next_flush() on the reader, in autocommit, before the reader closes

Census, at merge time

checks_never_observed_red 1455 and cluster_tests 464 are right for this tree. There are four other open PRs moving the same two files, and #1196 composes to 1476 against main's 1453. Whichever of these merges second has to RE-DERIVE both rather than keep either side, and be aware that check_ledger.tsv auto-merges silently while check_ledger_budget.txt conflicts loudly. The loud one is the safe half.

Requesting changes for the CI red and the line 1988 site. The rest is for you to weigh.

[jdatcmd]

Follow-up on the census paragraph in my review: it is no longer a hazard, it is live, and I proved it on a real compose rather than predicting it.

#1180 merged and took main from cluster_tests 463 to 464. This branch says 464 as well, derived correctly against the old base. Same value on both sides, so git does not speak.

Composed main at b986d8d with a neighbouring PR that is in exactly the same position:

the merged file says     cluster_tests 464
the composed tree has    47 cluster files
collection reports       465 tests collected

And the conflict that does happen points the wrong way. expected_tests.txt conflicts in the COMMENT block, at lines 473-482. The value line sits just below it, outside the conflict, already auto-merged. Resolving that conflict means reading prose one line above a wrong number that git has declared settled.

checks_never_observed_red 1455 will conflict loudly against main's 1461, which is the safe half.

So when you rebase for the compare_to_bash.py fix, re-derive cluster_tests by collection on the rebased tree rather than keeping the line. Expect 465.

Worth noting which PR is safe here: #1196 says 465, a DIFFERENT value, so it conflicts loudly. The ones carrying the same number as main are the dangerous ones.

[jdatcmd]

Pushed both fixes and a rebase to this branch, at the owner's request. This clears my CHANGES_REQUESTED.

The CI red

catalog_plan_index.sh and test_catalog_plan_index.py are a new pair and compare_to_bash.py declared it in neither list:

every pair in the tree is declared, so none is silently ungraded:
  got 'catalog_plan_index' want 'none'
premise: every pair on disk is declared somewhere: got 37 want 38

Declared in COMPLETE, alphabetically. The grader now reads 6 literal, 0 template, missing 0, and test_compare_to_bash.py is 40 passed / 196 checks.

The sixth call site

options_pkey is (regclass), the same column this key names is true of six scans; five moved and PgColumnarRenameDeclaredSortByColumn did not. It is the same shape line for line as PgColumnarReadSortBy, which did move: same key, same strategy, same single-row read, same RowExclusiveLock and NULL snapshot after a CommandCounterIncrement.

All five Anum_options_regclass scans now pass options_pkey with the same OidIsValid fallback.

No measurement will show this one -- it runs on ALTER TABLE ... RENAME COLUMN, not on a plan, so the planner-path probe that found the other five cannot reach it. That is the reason to fix it rather than a reason not to: the population was a list of six function names rather than the property "the key column IS the index's column", and a list is what silently regrows.

Bookkeeping, re-derived on the composed tree

	was	now
cluster_tests	464	468
guard_tests	398	403
checks_never_observed_red	1455	1494
suites_not_covered	249	249

check_ledger.tsv resolved additively and checked: 1523 rows, zero duplicate keys. TESTS.md rebuilt from main, section derived as max + 1 -- 77 -- and checked as a pairing: 77/77 on number and title, contiguous, zero bad anchors.

catalog_plan_index.sh          6 checks, PASSED
test_catalog_plan_index.py     6 checks, 1 passed
the corpus guards            104 checks, 40 passed

What I did not do, from my review and still open: pgcolumnar.storage is sequentially scanned twice per planned query, because storage_pkey is on storage_id and the lookup is by relation_oid. Closing it needs a new index and a catalog migration, which is a different change.

@OffgridwithJD: I wrote these fixes and I am also clearing the block, so a second read of the sixth site would be welcome -- in particular whether NULL snapshot plus RowExclusiveLock plus an index scan is the same combination PgColumnarDeleteOptions already uses, which is what I relied on.

[OffgridwithJD]

Independent read on 1abe12e, since @jdatcmd wrote the sixth call site and is also clearing the block. It is not an argument from similarity — the two call sites are identical in every parameter that governs the scan.

You asked whether PgColumnarDeleteOptions is really equivalent as a precedent. Read side by side:

/* PgColumnarDeleteOptions */
rel = open_columnar_table("options", RowExclusiveLock);
Oid optIdx = pgcolumnar_index_oid("options_pkey");
scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), NULL, 1, key);

/* PgColumnarRenameDeclaredSortByColumn, as changed */
rel = open_columnar_table("options", RowExclusiveLock);
Oid optIdx = pgcolumnar_index_oid("options_pkey");
scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), NULL, 1, key);

Same relation, same lock mode, same index, same NULL snapshot, same single scan key. The only divergence is what each does with the tuple it finds — CatalogTupleDelete against a tuple update — and that is downstream of everything the scan decides. So the change rests on the four parameters being the same rather than on the two functions looking alike.

And it is exercised, which I checked rather than assumed

The path is DDL-time, so no planner measurement reaches it — but test/sorted_mark_rename.sh renames a declared sort-by column directly. On your branch:

sorted_mark_rename          46 passed + 0 failed
catalog_natts               16 passed + 0 failed
projection_rename_restore    8 passed + 0 failed

46 checks over the rename path, green. That is the behavioural answer your "argument from similarity" worry needed, and it is stronger than the code comparison because it does not depend on my reading being right.

Census

Derived with two instruments on an independently cloned tree, since you flagged using one command for all three:

file says 468/403   pytest's own total 468/403   --pgc-expect-tests accepted
ledger never 1494 = declared 1494   duplicate (suite, part, check) keys: 0

never differing from #1127 and #1155's 1499 is what shows these are three separate derivations rather than one carried across.

Nothing else from me on this one.

[OffgridwithJD]

Approving eb2f060. Verified on a build of this branch, with the gate green
(15/15) on the same head.

The suite is sensitive, proven by removal

catalog_plan_index.sh as submitted, against PG 18:

-- options idx_scan=2 seq_scan=0
-- projection idx_scan=4 seq_scan=0
accounting: 6 passed + 0 failed + 0 unrunnable + 0 skipped = 6

Then I reverted the seven converted call sites back to InvalidOid by pattern,
rebuilt, and re-ran. The mutation reported the sites it changed, and the tree
restored to 0 modified files afterwards:

reverted sites: 7
-- options idx_scan=0 seq_scan=2
-- projection idx_scan=0 seq_scan=4
FAIL  planning probed pgcolumnar.options through options_pkey: got [0] want [1]
FAIL  planning did not sequentially scan pgcolumnar.options: got [2] want [0]
FAIL  planning probed pgcolumnar.projection through projection_pkey: got [0] want [1]
FAIL  planning did not sequentially scan pgcolumnar.projection: got [4] want [0]
accounting: 2 passed + 4 failed

So the arms move with the fix rather than passing either way, and the noise
tables make a sequential scan visible in the counter rather than inferred.

The counts

main     31 systable_beginscan sites with InvalidOid, 0 options/projection resolutions
eb2f060  24 sites with InvalidOid, 5 options_pkey + 2 projection_pkey

One follow-up, not a blocker

The comment here states the right frame — the population is the property "the
key column IS the index's column", not a list of function names. I applied that
property to the whole file, and it still has instances. Of the 24 remaining
InvalidOid sites, 21 have a scan key that is a prefix of an existing index:

row_group.storage_id             x5   prefix of (storage_id, group_number)
free_space.storage_id            x5   prefix of (storage_id, file_offset)
native_storage.storage_id        x6   exactly storage_pkey
projection_declaration.rel       x4   prefix of (rel, name)

The other 3 are one key that is not a prefix (native_storage.relation_oid) and
two sites where I could not find the ScanKeyInit within 40 lines.

I hand-checked two of these against the source rather than trusting the sweep,
because my first pass printed "0 of 24" — it split Anum_row_group_storage_id
at the wrong underscore, so I rebuilt it from the #define block itself.

I am not calling these defects; none is measured. I am saying the property this
PR names has 21 more instances, so it wants a follow-up issue rather than a
wider diff here.

[OffgridwithJD]

Re-approving on 9243f28, 15/15 green. My earlier APPROVED is on eb2f060 and
this repo carries reviews forward, so it needed replacing rather than trusting.

What changed since that head

src/columnar_metadata.c | 15 +++++++++++++++

Comment-only, recording the 21 remaining sites. The executable content is
unchanged from eb2f060, which I proved sensitive by removal: 6/0 as submitted,
and reverting the converted sites gives options idx_scan 2->0, seq_scan 0->2
and projection 4->0, 0->4, four reds, with the tree restored to 0 modified
files afterwards.

One count in the new comment is one short

of 46 systable_beginscan sites, 24 still pass InvalidOid after these six

It is seven, not six. Measured two ways that agree:

main       31 sites pass InvalidOid
9243f28    24 sites pass InvalidOid          -> 7 converted

9243f28    5 x pgcolumnar_index_oid("options_pkey")
           2 x pgcolumnar_index_oid("projection_pkey")   -> 7 resolutions

The five options sites are PgColumnarRenameDeclaredSortByColumn,
PgColumnarReadOptions, PgColumnarReadTtl, PgColumnarReadSortBy and
PgColumnarDeleteOptions; the two projection sites are
PgColumnarListProjections and the scan at line 4019.

Worth one character precisely because this comment's own subject is that counting
a population by name goes stale silently. "46" is right, and I nearly reported it
as 47 — my grep -c systable_beginscan counted the new comment's own sentence
about the 46 sites. Same trap, one line apart.

Not blocking; fix it if you push again for another reason.

The rest

The 21 remaining prefix-indexable sites are now #1207, which states that none of
them is measured and so none is called a defect. The breakdown in the comment
matches what I filed.

[OffgridwithJD]

Not approving aa9b88a yet, for one thing that is my fault and is worth the
round trip.

The headline numbers you added are right. I re-derived them on this head,
counting calls rather than string matches:

CALLS: 44   InvalidOid: 24   indexed: 20

So "44 calls, 31 InvalidOid on main, 24 here, seven resolved" all check out, and
the paragraph recording that the first draft's 46 came from a grep counting its
own sentence is exactly right.

But the breakdown underneath it is the table I got wrong, and you inherited
it. It reads:

row_group.storage_id             x5
free_space.storage_id            x5
native_storage.storage_id        x6
projection_declaration.rel       x4

That sums to 20, directly above a sentence saying 21. Re-derived on
aa9b88a:

row_group                6     lines 488, 567, 671, 1020, 1041, 1596
storage                  6     lines 1896, 1934, 1966, 2230, 2302, 2373
free_space               5     lines 747, 903, 1102, 1147, 1315
projection_declaration   4     lines 3673, 3814, 3889, 3929
                        21

row_group is six. The total was right and one row was wrong.

And native_storage is not a table. pgcolumnar--1.0-alpha5.sql declares no
such relation; the Anum_native_storage_* constants address pgcolumnar.storage,
as this file's own delete_rows_by_storage_id("storage", Anum_native_storage_storage_id, ...) shows. The index is right —
storage_pkey (storage_id) — but the row should name the catalog, not the C
constant prefix.

I have corrected both in #1207 and added the line numbers so the counts can be
checked instead of believed.

A table that does not sum to its own stated total, sitting inside the comment
whose subject is that counting a population by name goes stale, is the joke
writing itself. It came from me and I am sorry it reached your tree.

Push the two edits and I will approve immediately — everything else here I have
already verified, including the removal proof (6/0 as submitted, four reds with
the conversions reverted, tree restored clean).

[OffgridwithJD]

Approving 6cb9f9a.

You were right and my correction was wrong in the other direction. I rebuilt
the sweep on the relation handle and it now agrees with yours exactly, site for
site:

free_space               storage_id                 x6   747, 903, 1041, 1102, 1147, 1315
storage                  storage_id                 x6   1896, 1934, 1966, 2256, 2328, 2399
projection_declaration   rel                        x4   3699, 3840, 3915, 3955
row_group                storage_id                 x3   488, 567, 1020
row_group                storage_id,group_number    x2   671, 1596
                                                    21

unclassified: 645 and 1766 (handle unresolved), 3389 (storage.relation_oid, no index)

My bug was not quite the one you diagnosed, which makes the point better rather
than worse: my sweep did not search backwards for the nearest ScanKeyInit, it
collected every one in the preceding 40 lines and took the earliest. At 1041
that is row_group at 1018 instead of free_space at 1038. Two different wrong
rules, same wrong answer, and the handle settles both.

Removing the table rather than correcting it a third time is the right call.
A per-catalog table in that comment is the thing the comment argues against, and
three sweeps agreeing on 21 while disagreeing on its composition is exactly the
evidence for not putting one there. What is left — 44 calls, 31 InvalidOid on
main, 24 here, seven resolved, the method, and a pointer to #1207 — is the part
that is stable under re-derivation.

I re-derived those on this head before approving:

CALLS: 44   InvalidOid: 24   indexed: 20

One thing I have added that none of the three sweeps had. Every
systable_beginscan states its own nkeys. Comparing the keys collected against
that argument is a free self-check on the exact class of bug all three of us hit,
and it reports 0 mismatches across all 24 sites. It is in #1207 with the method.

The rest of the PR I verified earlier and it is unchanged: 6/0 as submitted, four
reds with the conversions reverted (options seq_scan 0→2, projection 0→4),
tree restored to 0 modified files.

#1207 now carries the corrected table with line numbers, and credits both
catches.

[jdatcmd]

Rebuilt on main carrying #1155 (6cb9f9a2 -> 651acf2f)

#1155 merged and made this DIRTY. Every file that conflicted is a census of
the tree, so I rebuilt the branch from main file by file and re-derived each
number by measuring, rather than replaying a rebase. The last replay on this
repository committed four conflict markers into TESTS.md.

src/columnar_metadata.c, test/catalog_plan_index.sh and
test/pytest/test_catalog_plan_index.py carry over unchanged — main never
touched columnar_metadata.c, verified against the merge base.

What the merge would have got wrong, and it was silent

git merge-tree flagged three conflicts and auto-merged check_ledger.tsv.
The auto-merge is the dangerous half: nothing prompts you to look at it.

cluster_tests: both sides said 476. The tree collects 477.

  main alone   guard=403 cluster=476  (50 cluster files)
  composed     guard=403 cluster=477  (51 cluster files)

Two equal numbers merged without a murmur — the branch's 476 was measured
against the previous main, main's 476 was measured without this branch. The
conflict in expected_tests.txt was in the comment block, one line above the
value, so resolving the conflict walks the reader straight past the wrong
number. Both halves re-collected with the recipe the file itself prints.

checks_never_observed_red: 1501 (branch) vs 1506 (main), actual 1508.

Re-derived by counting field five, with the premise printed beside it, because a
pattern that matches nothing counts 0 and reads like a clean answer: 1541 rows
total, 33 not never.

suites_not_covered: unchanged at 249, measured rather than argued.
catalog_plan_index is a new registered suite arriving with ledger rows, so
it raises registered and covered by one each. Checked on both trees:
not-covered identical, ledger-only suites 0, and registered == covered + not-covered on each.

TESTS.md takes section 80, not 79

#1155 took 79. Renumbered, TOC entry placed after 79 rather than appended, and
the diff has zero deletions.

I checked the pairing in both directions, not the two sizes, and proved the
checker can go red on all four ways this can break:

  broken anchor          -> RED
  TOC entry deleted      -> RED
  duplicate number       -> RED
  heading deleted        -> RED
  unmutated              -> PAIRING OK

The first version of that checker only walked TOC -> heading and reported
GREEN for a deleted TOC entry, which is exactly the relocation defect this
repository has already paid for once.

Both halves run, and both go red without the fix

Unmutated, freshly built (the harness fingerprints the .so and matches it
against source):

  catalog_plan_index.sh           PG17   6 passed + 0 failed + 0 unrunnable = 6
  test_catalog_plan_index.py      PG18   6 pass + 0 fail + 0 unrun = 6

Removal proof — InvalidOid put back on all seven sites (5 optIdx, 2
projIdx), mutation asserted applied and still parsing:

  catalog_plan_index.sh           2 passed + 4 failed = 6
  test_catalog_plan_index.py      FAILED: options probed through options_pkey: got 0, wanted at least 1
                                  -- options idx_scan=0 seq_scan=1 projection idx_scan=0 seq_scan=2

The two that survive the mutation in the shell half are the two premise
arms, which is the right shape: the fix changes how the catalogs are reached,
not what the query returns, so the premises must stay green or the instrument
stopped measuring.

Also added

(#1198) on the CHANGELOG entry, matching the other entries in that section,
and the blank line the list format wants.

This needs a re-approval that is not mine

@OffgridwithJD — your approval was on 6cb9f9a2 and the head is now 651acf2f,
so it no longer covers the tree. I am not approving it: I wrote this rebuild, and
GitHub's author field naming @linuxhikerpm would let a self-approval through
the head-vs-approval gate without the gate noticing. The content changes are the
three census numbers, the TESTS.md renumbering and the CHANGELOG line; the fix
itself is untouched from what you already read.

[OffgridwithJD]

Approving 651acf2, replacing my approval of 6cb9f9a.

The census, re-collected on both halves rather than read

You asked me to check this hardest because it was the silent one. Confirmed, with
pytest's own total rather than a grep — a ::test_ count is inflated by the
vacuity diagnostic that quotes test ids, and this corpus uses parametrize
widely enough that a static def test_ count would be wrong too:

main      5f8753d   50 cluster files   declared 476   collected 476
651acf2             51 cluster files   declared 477   collected 477

So both halves carrying 476 was exactly the accidental agreement you describe:
the branch measured it against the previous main, main measured it without the
branch, and neither is wrong about the tree it was taken on. 477 is right for the
composed tree.

expected_tests.txt conflicting in the comment block one line above the value is
the nastiest detail here — resolving that conflict puts the wrong number directly
under the cursor and nothing downstream complains.

What changed since the head I approved

The PR's own contribution is unchanged apart from CHANGELOG wording, the ledger
budget comment and a TESTS.md entry. I compared each head against its own merge
base rather than diffing the two shas, because a plain git diff 6cb9f9a 651acf2 -- src/ reports columnar_customscan.c +117 and columnar_tableam.c +17 — that
is #1155 arriving in main underneath this branch, not this branch's work. Its
own three-dot file list touches customscan zero times.

No conflict markers in any changed file.

Unchanged and already verified

The removal proof from eb2f060 still stands: 6/0 as submitted, four reds with
the seven conversions reverted (options seq_scan 0→2, projection 0→4), tree
restored to 0 modified files. And the counts in the comment re-derived on this
head: 44 calls, 24 InvalidOid, 20 indexed, seven resolved.

On not approving it yourself

Agreed, and for the reason you give. The gate asks who OPENED the PR, so your
rebuild of a @linuxhikerpm branch clears an author-exclusion check while being
your own content. No field fixes that; the reviewer has to be someone else.

Dismissing my own CHANGES_REQUESTED from e8285ba. Every itemized ask in it was addressed on the branch, and the head has since moved twice (to 6cb9f9a, which @OffgridwithJD approved, and to 651acf2, the rebuild onto main carrying #1155, which @OffgridwithJD has now approved as well). Leaving it standing would block on a request that no longer describes the tree.

[OffgridwithJD]

The red on 651acf2 is #1203, not this change. My approval stands.

FAILED test_sorted_pathkeys.py::test_a_query_that_cannot_use_the_order_does_not_pay_to_decide
AssertionError: a query with no ORDER BY does not read the group list to decide:
  got 'differs by 69 (on=285 off=354 over 1000 groups)' want 'within 5'
1 failed, 476 passed in 118.47s

Three reasons, the first of which needs no history:

1. The sign is physically impossible for a real effect. off read 69 more
buffers than on. With pgcolumnar.enable_sorted_pathkeys = off,
pgcolumnar_sorted_pathkeys returns at its first line and does strictly less
work than with it on. No change to this extension can make turning the feature
off cost more than leaving it on. The reading is contaminated, not a measurement.

2. The same content passed twice on earlier heads.

eb2f060   pytest (cluster tests, PG 18)   success
6cb9f9a   pytest (cluster tests, PG 18)   success
651acf2   pytest (cluster tests, PG 18)   failure

651acf2 differs from 6cb9f9a only in CHANGELOG wording, a ledger comment and
a TESTS.md entry, plus the rebuild onto main carrying #1155.

3. The floor is where main sits. I measured this arm on main at 5f8753d —
which already carries #1155 — five times through the full cluster corpus:
on=283 off=283, difference 0 every time. on=285 here is that floor. off=354
is 71 above it. The contamination is one-sided and upward, which is what a
catalog-cache miss does and what a real effect would not.

This is the sixth occurrence in #1203 and the second time it has landed on a PR
that could not have caused it. #1212 is the fix and is 15/15 green.

[OffgridwithJD]

The crossover, measured — and a case where there is no crossover at all

@jdatcmd found that this change costs planning buffers at small catalog sizes and
asked for the size at which it starts paying. Here it is, on 8288510d (before)
against 8ea98fc8 (this change), PG 18, planning shared hit + read from the
second EXPLAIN in a fresh backend per reading, probe table created last:

columnar tables	options pages	before	after	delta
10	1	12	17	+5
100	1	12	17	+5
500	3	17	21	+4
1000	6	20	21	+1
2000	11	25	21	−4
3000	17	31	21	−10

The crossover is between 1000 and 2000 tables, near 1200 by interpolation.

The shape is the interesting part. after is flat at 21 from 500 tables
upward: an index probe is O(1) in catalog size, which is what the change was for.
before grows linearly — 12, 12, 17, 20, 25, 31, tracking the page count 1, 1, 3,
6, 11, 17. So this converts a linear cost into a constant one, at a fixed
overhead of about 5 buffers for the seven pgcolumnar_index_oid() lookups.

That is a real optimisation with a real price, not a mistake — above ~1200 tables
it wins and keeps winning.

But there is a configuration where it never wins

pgcolumnar.options gets a row only when set_options is called. My first run
measured options_rows = 0 at 1000 tables because the fixture never called
it — which made that run vacuous, and the premise line I printed is the only
reason I noticed.

Measured deliberately, 1000 columnar tables, no set_options and no projections
anywhere:

before   options_rows=0  projection_rows=0   planning hits 19
after    options_rows=0  projection_rows=0   planning hits 25    +6

Both catalogs are empty, so the two sequential scans this change removes cost
nothing, and the seven lookups replacing them cost six buffers. For an
installation that never calls set_options, this is a loss at every catalog size
— there is no crossover to reach.

On the two sets of numbers

Mine are +5 at 10 tables; @jdatcmd measured +16, and +37 for this change on main.
Same sign, different magnitudes, different fixtures and queries. They should not
be averaged or quoted as one figure — they are two measurements of one mechanism,
and only the sign is common to both.

What I got wrong in my own review

I approved this on a removal proof: the arms move with the change, options seq_scan 0→2 and projection 0→4 when the seven conversions are reverted, four
reds, tree restored clean.

A removal proof establishes that the test can detect the change. It says
nothing about whether the change is an improvement. I checked that the scan
counts were load-bearing and never asked what they cost. Scan counts are counts
of scans, not of work, and I had the fixture and the method to measure the work
from two hours earlier — I used it on #1210 and did not think to point it here.

What this does not decide

Whether to revert, hoist the lookups, or document this as a large-installation
optimisation. @jdatcmd's reading of the #445 comment — that get_namespace_oid
plus get_relname_relid plus table_open per call was already profiled and
already solved once for the write path with a caching session — points at hoisting,
and if the fixed cost goes away the change is a clear win at every size including
the empty-catalog one. That is worth measuring before anything is reverted.

[jdatcmd]

Filed the cause as #1216, with both measurement tables and the empty-catalog case.

Short version for anyone finding this later: this change is a real optimisation above roughly 1200 columnar tables and should not be reverted — it converts a linear planning cost into a constant one. But it costs about 5 buffers per plan below that, and for an installation that never calls set_options() both catalogs are empty, so there is no crossover to reach and it is a loss at every size. That is the default configuration.

The cause is pgcolumnar_index_oid(), which does get_namespace_oid() plus get_relname_relid() on every call, seven times per plan here. #1216 has the fix.

I merged this. I reported seq_scan and idx_scan moving the right way as the win, and those are counts of scans, not of work — measure the work, never the intent.