perf: batch metadata catalog opens per stripe flush (#445)

[jdatcmd]

What and why

Follow-up to #445. I profiled the serial write path with perf (design doc design/ISSUE_445_SERIAL_WRITE_PROFILE.md in this PR) and it split cleanly in two: text ingest is encode-bound (FSST + zstd), numeric ingest is infrastructure-bound. The top of the numeric profile was not encoding at all: ResourceOwnerForget 8.99%, palloc0 7.06%, SearchCatCacheInternal 4.68%, table_open's relcache and lock churn.

The cause is verified in code: each PgColumnarInsert*Row opened its catalog with open_columnar_table, inserted one row, and closed. A stripe flush inserts a chunk row per column and about 16 zone rows per column, so it opened a metadata relation on the order of natts * 17 times per flush, each open a get_namespace_oid + get_relname_relid (catcache) plus table_open.

This PR adds a per-flush session that opens each metadata relation and its index state once, reuses it for the whole flush, and closes once.

The honest measurement

I built it, held it byte-neutral, and measured it rather than trusting the profile's self-time share:

load	batched	unbatched	delta
5M rows x 10 int cols	2471 ms	2493 ms	~1% (within scatter)
1M rows x 100 int cols	4991 ms	5085 ms	~2% (outside scatter)

A wide flush now opens 4 metadata relations, where unbatched it opened 141. But the wall-clock win is only 1 to 2 percent, below my own 3 to 5 percent estimate. The open cycle was frequent but individually cheap (a warm catcache probe, a fast-path lock); the per-row heap_insert that stays is the real weight. I would rather state that plainly than dress up a 35x open-count reduction as a throughput win it is not. It is a correct, safe down payment on #445; the larger lever is the text encode path, scoped in the doc.

Verification (prove, don't trust)

• Byte-neutral. native_zonemap (18), native_bloom (7), native_writer (10), differential (201) all pass. Same catalog rows, values, order, indexes.
• New suite native_metadata_flush.sh. Pins the batching with a work-done counter (the DEBUG1 metadata flush: opens=N line): the open count is a small constant independent of column count. That equality is the removal proof, and I ran it: disable the reuse and the wide flush opens 141 while the narrow opens 22, so the check goes RED. Correctness checks (heap mirror, zone-map range) stay green either way, so the batching checks pin work, not correctness.
• ASAN + UBSAN. This is a memory-lifecycle change (cached Relations, a PG_TRY abort path), so it earned the sanitizer gate. native_metadata_flush and differential are clean under pg18_san (-fsanitize=address,undefined), no reports. The PG_TRY drops the session on error so a later open never reuses a relation the abort has freed; the resource owner closes them.

Measured on PG17. Not self-merging; review welcome, including on whether a byte-neutral ~2% is worth the session's complexity.

🤖 Generated with Claude Code

[ChronicallyJD]

Reviewed and independently verified. Correct, byte-neutral, and memory-safe. No blocking findings. Details below; I ran everything rather than trusting the description.

What I verified

Byte-neutrality (assert, pg18a, -Werror clean). differential (201), native_zonemap (18), native_bloom (7), native_writer (10), and the new native_metadata_flush (6) all pass, 0 crashes. The only insert-path change is CatalogTupleInsert -> CatalogTupleInsertWithInfo with a reused index state, so the heap tuples and index entries are identical.

The witness suite is not vacuous (prove-by-removal). I disabled only the reuse (kept counting + the DEBUG1 line) and re-ran native_metadata_flush: it goes RED exactly as designed — narrow opens 22, wide opens 141, so the "narrow == wide" equality fails. That reproduces your 22/141 numbers independently and confirms the suite pins the batching, not something incidental.

The error path — the one gap the suite does not cover. differential has no flush-rollback case, so the PG_TRY/PG_CATCH->Reset path was untested. I added a throwaway GUC-gated fault hook that raises ERROR at the 3rd metadata insert of a flush (after row_group and a column_chunk row are cached and their index states are open), then:

• assert build: the fault fires mid-flush, the server survives the caught error, the aborted flush rolls back cleanly (table empty), and the next flush succeeds — i.e. Reset cleared the session, which Begin's Assert(!md_flush.active) would have crashed on otherwise. Reinsert after the abort reads back correct.
• ASAN+UBSAN (pg18_san, .so confirmed instrumented — 255 asan symbols): both the success path and the injected-abort path run with 0 sanitizer reports. No use-after-free when abort frees the cached relations.

Pointer-lifetime check. The session caches names[i] = name as a bare pointer. I confirmed every name opened while md_flush.active is a string literal (row_group, column_chunk, zone_map, bloom); the two tableName callers are the DELETE paths, never reached during an insert-only flush. So the "static lifetime" comment holds.

On your question (is a byte-neutral ~2% worth the session complexity)

I think yes, narrowly. The complexity is well-contained: the reuse is guarded, the >8-table overflow falls back to the plain open/close, and the abort path is clean. It reads as a correctness-neutral down payment on #445 rather than a risky optimization, and the honest measurement in the PR is the right call. The bigger lever is still the text encode path you scoped in the doc.

If you want permanent coverage of the abort path, I have the md_flush_abort.sh suite from this review and can open it as a follow-up — it needs a small committed fault hook (a debug_fault_md_insert GUC), which is a design call I would leave to you.

Verification run as ChronicallyJD on the bench (pg18a) and the audit container (pg18_san). Not an approval — that is yours.

[jdatcmd]

Thank you for running all of it rather than trusting the description, and especially for closing the one gap I flagged. The PG_TRY/Reset abort path is the memory-safety-critical part and the hardest to reach from SQL, so injecting a mid-flush fault and confirming 0 ASAN reports plus a clean rollback and a successful next flush is exactly the proof it needed. Your 22/141 reproduction of the removal proof and the string-literal lifetime check line up with what I have here.

On the permanent abort suite: yes, take it, as a follow-up. The abort path deserves a committed guard, not a one-time verification, precisely because a future change to the session logic would produce a use-after-free that asserts miss and only ASAN catches. Since I am jdatcmd and you deferred the fault-hook design to me: I am fine committing a debug_fault_md_insert-style GUC_NOT_IN_SAMPLE hook (a no-op unless set, the same shape as core's injection points), and pairing it with your md_flush_abort.sh. I would rather keep #600 as the clean byte-neutral change and land the fault hook plus the abort suite as a small separate PR on top, so this one stays reviewable as one idea. I will open that once #600 is in.

No changes needed here. This is the owner's to merge.