From 274f2170fa7dd3fa1cd770e9e3cd494ba9bafacc Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 12 Aug 2026 23:01:14 -0600 Subject: [PATCH] perf: batch metadata catalog opens per stripe flush (#445) Each PgColumnarInsert*Row opened its catalog, inserted one row, and closed, so a flush opened a metadata relation once per inserted row: about 16*natts zone rows plus a chunk row per column. A profile of the numeric write path (#445) put that open and close cycle, not the encoding, at the top of the profile: SearchCatCacheInternal, ResourceOwnerForget, and table_open's relcache and lock churn. A per-flush session caches each metadata relation and its index state on first open and reuses it for the rest of the flush, closing once at the end. A wide flush now opens 4 relations instead of up to 141. It is byte-neutral (same rows, values, order, indexes): native_zonemap, native_bloom, native_writer and differential all pass, and it is clean under address and undefined-behaviour sanitizers. The PG_TRY drops the session on error so a later open never reuses a relation the aborting subtransaction has freed. The measured saving is modest: about 1 percent on a 10-column load (within scatter) and about 2 percent on a 100-column load (outside it). The open cycle was frequent but cheap (a warm catcache probe, a fast-path lock); the per-row heap_insert that stays is the real weight. This is an honest down payment on #445, not a headline. The larger lever is the text encode path, scoped in the design doc added here. The new suite native_metadata_flush.sh pins the batching with a work-done counter: the open count is a small constant independent of the column count. Removal proof: disable the reuse and the wide flush opens 141 while the narrow opens 22, so the equality check goes red. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01WmQJqcXdwyuoAiHHt2znBr --- design/ISSUE_445_SERIAL_WRITE_PROFILE.md | 206 +++++++++++++++++++++++ src/columnar_metadata.c | 198 ++++++++++++++++++++-- src/columnar_metadata.h | 5 + src/columnar_write_state.c | 77 +++++---- test/native_metadata_flush.sh | 99 +++++++++++ test/run_all_versions.sh | 1 + 6 files changed, 543 insertions(+), 43 deletions(-) create mode 100644 design/ISSUE_445_SERIAL_WRITE_PROFILE.md create mode 100755 test/native_metadata_flush.sh diff --git a/design/ISSUE_445_SERIAL_WRITE_PROFILE.md b/design/ISSUE_445_SERIAL_WRITE_PROFILE.md new file mode 100644 index 00000000..8a359b6b --- /dev/null +++ b/design/ISSUE_445_SERIAL_WRITE_PROFILE.md @@ -0,0 +1,206 @@ +# Issue #445: profile of the serial write path, and a scoped reduction plan + +pgColumnar serial `COPY` runs about 2.37x slower than Citus columnar on the +#445 benchmark, on the same file through the same core parser. This document +records a measured profile of the write path, and scopes the reductions that +survive adversarial verification. It ranks them by bounded saving times safety +and names the one change to take first. + +## 1. What was measured + +Two synthetic ingest profiles were taken, one per column family, with `perf` +(`cpu-clock`, DWARF unwind) on a non-assert PostgreSQL 17 build. Each captured +about 12000 samples of a real `INSERT INTO ... SELECT` whose source rows were +materialised outside the timed statement. The two shapes isolate two distinct +cost regimes. + +**Text ingest is encode-bound.** The fixture was `(id bigint, t text)` with +32-character values. Self time concentrates in the FSST and zstd encode path: + +| symbol | self time | object | +| --- | ---: | --- | +| `libzstd` (two frames) | 13.72% | libzstd | +| `encode_fsst_shared` | 10.60% | pgcolumnar | +| `PgColumnarFsstBuildChunkTable` | 4.65% | pgcolumnar | +| `detoast_attr` | 4.23% | postgres | +| `__memmove` / `__memcmp` | ~4.7% | libc | + +**Numeric ingest is infrastructure-bound.** The fixture was ten integer columns. +Self time concentrates in catalog access, resource-owner churn, and allocation, +not in encoding: + +| symbol | self time | object | +| --- | ---: | --- | +| `ResourceOwnerForget` | 8.99% | postgres | +| `palloc0` | 7.06% | postgres | +| `hash_search_with_hash_value` | 4.74% | postgres | +| `SearchCatCacheInternal` | 4.68% | postgres | +| `TupleDescInitEntry` | 3.30% | postgres | +| `LockAcquireExtended` | 2.28% | postgres | +| `llseek` | 2.54% | libc | + +The two profiles are the endpoints. The 105-column ClickBench mix is neither in +isolation: its text columns pay the encode path and its numeric columns pay the +metadata-infrastructure path, so both regimes apply per stripe. The blend sits +between the endpoints and is not yet measured. + +Provenance: the percentages above are from these two real profiles. The +adversarial verifiers in the scoping run did not have the profile file, so they +treated every figure as an unverified ceiling and corrected several savings +downward. That conservatism is kept below. No saving here is a promise until it +is measured on a build. + +## 2. Where the time goes + +### Numeric: a catalog open and lock cycle per metadata row + +This is verified against the code, not inferred. Each metadata row is inserted +by an independent `PgColumnarInsert*Row`. Each such call runs +`open_columnar_table`, which is `get_namespace_oid` plus `get_relname_relid` +(two catcache probes) plus `table_open` (a relcache hash search, a lock, and a +resource-owner remember), then `CatalogTupleInsert`, then `table_close`. + +The per-vector zone map is the row-count multiplier. A flush emits +`(stripe_row_limit / chunk_group_row_limit) + 1` zone rows per column, which is +16 for the default limits. One stripe flush of a four-column table therefore +opens and closes a metadata relation on the order of 70 times, where four opens +would do. + +Code sites confirmed: + +- `src/columnar_metadata.c:137` `open_columnar_table` (the per-call open) +- `src/columnar_metadata.c` the four inserters `PgColumnarInsertRowGroupRow`, + `PgColumnarInsertColumnChunkRow`, `PgColumnarInsertZoneMapRow`, + `PgColumnarInsertBloomRow`, each opening and closing on its own +- `src/columnar_write_state.c:2772-2794` the per-row insert loops in the flush + +### Text: repeated FSST setup and per-byte re-reads + +`encode_fsst_shared` rebuilds an `FsstLookup` on every vector of a chunk, plus +the decide pass. The symbol table is constant across the chunk, so most builds +repeat identical work. `fsst_verdict_cache` (#472) caches the keep-or-drop +decision, not the symbol table, so it does not remove this. `fsst_longest_match` +does one small `memcpy` per candidate length at every input byte. +`PgColumnarEncodeValue` forces every varlena to a full four-byte header even when +the `COPY` input arrived with a short header. + +## 3. Candidate reductions, ranked + +Ranked by bounded saving times safety. Every entry is measure-first: none is a +certain win until profiled on a build. Byte-neutral means no on-disk or metadata +bytes change, so no opt-in GUC is needed. + +### 3.1 Batch metadata catalog inserts per flush (open once, insert all, close once) + +The headline candidate, and the recommended first step. + +- Mechanism. Open each metadata relation once per flush, insert all its rows, + close once. This collapses about 70 relation-open cycles per stripe to about + four. +- Code site. `src/columnar_write_state.c:2772-2794` and the four inserters. +- Byte-identity. Unconditional. Same catalog rows, same values, same order. +- Guarding test. A work-done counter of metadata relation opens per flush, plus + `native_zonemap`, `native_bloom`, `native_writer`, and `differential` as byte + and content pins. +- Measured result (implemented and benchmarked). The batching lands the open + count where the estimate said: a flush now opens each metadata table once + (`opens=4`), independent of the column count, where an unbatched 20-column + flush opened 141 relations and a 100-column flush more. But the wall-clock + saving is smaller than the self-time share suggested. A 5,000,000-row + ten-column load was 2471 ms batched against 2493 ms unbatched, about one + percent and inside the run scatter. A 1,000,000-row 100-column load, where the + open count is highest, was 4991 ms against 5085 ms, about two percent and + outside the scatter. The lesson is that the open cycle was frequent but cheap: + a warm catcache probe and a fast-path lock cost little each, and the per-row + `heap_insert` and index insert that stay are the real weight. The estimate of 3 + to 5 percent was optimistic; the measured figure is 1 to 2 percent, larger on + wider tables. Byte-identity held (`native_zonemap`, `native_bloom`, + `native_writer`, `differential` all pass), and the change is clean under + address and undefined-behaviour sanitizers. +- Verdict. Correct, byte-neutral, low risk, and a real but modest win. Worth + taking as a down payment, but it does not on its own close a meaningful part of + the gap. The larger lever is the text encode path. + +### 3.2 Batch the per-vector zone rows with heap_multi_insert + +Stacks on 3.1. Once the relation is open once, insert its zone rows with +`heap_multi_insert` and one index pass. Residual saving after 3.1 is only the +`heap_insert`-per-tuple amortisation, plausibly sub-one-percent. Byte-neutral to +SQL readers, but it switches to `XLOG_HEAP2_MULTI_INSERT`, an existing core +record. Do not sum its saving with 3.1. + +### 3.3 Skip the storage existence scan after the first flush + +A `bool` on the write state, set after the first ensure, skips the redundant +`PgColumnarInsertNativeStorageRow` existence scan on later flushes of the same +write state. Zero saving on single-flush loads, about 1 to 3 percent on +many-flush loads only. Byte-neutral. Direct precedent: the existing `projInited` +field. + +### 3.4 Hoist the FSST lookup to once per chunk + +Build one `FsstLookup` per chunk and hold it across the vector loop. Byte-neutral. +Bounded by the non-inner-loop share of `encode_fsst_shared`, realistically +sub-one-percent for bulk text. Carries a double-free risk on the abort path that +asserts will not catch, so it earns the `pg18_san` ASAN gate. + +### 3.5 One 8-byte load and mask in fsst_longest_match + +Replace the per-length `memcpy` with a single unaligned little-endian load. +Byte-identical on little-endian; keep the copy path on big-endian. Honest upper +bound about 1 to 2 percent, because the `fsst_lookup_find` hash probes, not the +copy, dominate the inner cost. Watch the `1ULL << 64` undefined-behaviour edge. + +### 3.6 Reuse a persistent ZSTD_CCtx + +Hold a per-process `ZSTD_CCtx` and call `ZSTD_compressCCtx`. Byte-neutral only +with `compressCCtx`, not the `compress2` variant. Under two percent as a ceiling, +likely far less, because context setup amortises to near zero over a large block +compression. Being byte-neutral, only a benchmark can prove it, not a red test. + +### 3.7 Store packed varlena headers (needs an opt-in GUC and a format bump) + +Keep short one-byte varlena headers instead of expanding to four. Saves a palloc +and a copy per short value and shrinks stored files by three bytes per value. +This changes on-disk bytes, including stored min and max, so it is admissible +only behind an opt-in GUC and a format-version stamp, with a full audit of the +27 raw `VARSIZE`/`VARDATA` sites. Format work, not a drop-in. + +### Rejected + +- Caching the metadata OIDs on their own. The probes are warm catcache hits + below the profiler floor, and 3.1 removes them anyway by not re-opening. +- Raising `chunk_group_row_limit` to cut zone rows. Already available as a + `PGC_USERSET` GUC. Tuning and documentation, not a code candidate. +- Packing zone maps into the descriptor region. A coordinated read and write + format change with wrong-skip risk. Defer behind a format version. + +## 4. The recommended first step, and its removal proof + +Take 3.1. It is the highest-leverage byte-neutral change, it needs no GUC, and +it targets the numeric-infrastructure regime that ClickBench's numeric columns +hit. + +Because it is byte-neutral, no data test can go red, so per the house rule +"measure the work, never the intent" the removal proof is a work-done counter, +not a data assertion. Instrument a per-flush count of metadata relation opens. +Assert that one stripe flush of a four-column fixture opens each metadata +relation once. The counter reads about 70 today and about four after the change; +reverting the change restores the ~70 count and the test goes red again. The +byte pins run alongside to prove the catalog content and on-disk bytes did not +move. Only then is the real saving measured with `perf` on the #445 bench. + +## 5. Honest limits + +- The profiles are two synthetic single-family ingests. The 105-column + ClickBench blend is not yet measured, and it decides whether the encode regime + or the infrastructure regime deserves the larger effort. +- Every saving except 3.1 is a bound narrowed by reasoning, not a measured + result. Several were corrected downward during verification. Treat none as a + certain win until measured on a build. 3.1 was measured and came in at 1 to 2 + percent, below its own 3 to 5 percent estimate, which is the caution made + concrete: a large self-time share is not the same as a large recoverable cost. +- The byte-changing candidates (3.7, and the deferred zone-map packing) carry an + opt-in GUC, a format-version bump, a read-site audit, and re-baselined byte + pins. They are not comparable in effort to the byte-neutral set and should not + be scheduled until that set is measured. diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 69f2f2e5..2d9943c1 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -134,11 +134,66 @@ pgcolumnar_schema_oid(void) return get_namespace_oid(COLUMNAR_SCHEMA_NAME, false); } +/* + * #445: a metadata flush session. + * + * Every 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 ran get_namespace_oid plus + * get_relname_relid (catcache probes) and table_open (a relcache lookup, a lock, + * a resource-owner remember), and the matching close undid it. A profile of the + * numeric write path (#445) put that per-row open and close cycle, not the + * encoding, at the top of the profile. + * + * A session caches each metadata relation and its index state the first time the + * flush opens it, and reuses that open for every later row of the same flush. The + * relation is closed once, when the flush ends. The rows written, their values, + * their order, and the indexes updated are all unchanged, so the catalog and the + * on-disk bytes are byte-for-byte identical. This is a scan-local optimisation, + * not a format change. + */ +#define MD_FLUSH_MAX 8 +typedef struct MetadataFlushSession +{ + bool active; + int count; + const char *names[MD_FLUSH_MAX]; + LOCKMODE locks[MD_FLUSH_MAX]; + Relation rels[MD_FLUSH_MAX]; + CatalogIndexState indstates[MD_FLUSH_MAX]; /* lazily opened on first insert */ +} MetadataFlushSession; + +static MetadataFlushSession md_flush = {0}; + +/* + * Count of real metadata relation opens during the active session. It is the + * work-done witness for the #445 batching: with the session off it counts one + * open per inserted metadata row, with it on it counts one per distinct table. + * native_metadata_flush.sh asserts on it via the DEBUG1 line End emits. + */ +static uint64 md_flush_opens = 0; + static Relation open_columnar_table(const char *name, LOCKMODE lockmode) { - Oid nspOid = pgcolumnar_schema_oid(); - Oid relOid = get_relname_relid(name, nspOid); + Oid nspOid; + Oid relOid; + Relation rel; + + /* Reuse a relation already open for this flush session. */ + if (md_flush.active) + { + int i; + + for (i = 0; i < md_flush.count; i++) + if (md_flush.locks[i] == lockmode && + strcmp(md_flush.names[i], name) == 0) + return md_flush.rels[i]; + } + + nspOid = pgcolumnar_schema_oid(); + relOid = get_relname_relid(name, nspOid); if (!OidIsValid(relOid)) ereport(ERROR, @@ -146,7 +201,124 @@ open_columnar_table(const char *name, LOCKMODE lockmode) errmsg("columnar metadata table \"%s.%s\" does not exist", COLUMNAR_SCHEMA_NAME, name))); - return table_open(relOid, lockmode); + rel = table_open(relOid, lockmode); + + /* + * Count every real open reached during a session, whether or not it is + * cached. With reuse on this is one per distinct table; with reuse removed it + * is one per inserted row, which is what the removal proof in + * native_metadata_flush.sh relies on. + */ + if (md_flush.active) + md_flush_opens++; + + /* + * Cache it for the rest of the flush. The names are string literals with + * static lifetime, so storing the pointer is safe. If more than MD_FLUSH_MAX + * distinct tables are ever opened in one flush (there are five), the extra + * relation is left uncached and metadata_flush_close closes it as before, so + * nothing leaks. + */ + if (md_flush.active && md_flush.count < MD_FLUSH_MAX) + { + int i = md_flush.count++; + + md_flush.names[i] = name; + md_flush.locks[i] = lockmode; + md_flush.rels[i] = rel; + md_flush.indstates[i] = NULL; + } + return rel; +} + +/* + * metadata_flush_insert + * Insert one metadata tuple. Under a flush session the relation's index + * state is opened once and reused, so CatalogOpenIndexes does not run per + * row. Off-session it is the plain CatalogTupleInsert, unchanged. + */ +static void +metadata_flush_insert(Relation rel, HeapTuple tuple) +{ + if (md_flush.active) + { + int i; + + for (i = 0; i < md_flush.count; i++) + { + if (md_flush.rels[i] == rel) + { + if (md_flush.indstates[i] == NULL) + md_flush.indstates[i] = CatalogOpenIndexes(rel); + CatalogTupleInsertWithInfo(rel, tuple, md_flush.indstates[i]); + return; + } + } + } + CatalogTupleInsert(rel, tuple); +} + +/* + * metadata_flush_close + * Close a metadata relation, unless the session owns it (then the session + * closes it once at the end). Off-session it is the plain table_close. + */ +static void +metadata_flush_close(Relation rel, LOCKMODE lockmode) +{ + if (md_flush.active) + { + int i; + + for (i = 0; i < md_flush.count; i++) + if (md_flush.rels[i] == rel) + return; /* PgColumnarEndMetadataFlush closes it */ + } + table_close(rel, lockmode); +} + +/* + * PgColumnarBeginMetadataFlush / EndMetadataFlush / ResetMetadataFlush + * Bracket the metadata inserts of one stripe flush so the inserters share + * one open per table. Begin and End are the success path. Reset is the + * error path: the aborting subtransaction closes the relations through the + * resource owner, so Reset only drops the session pointer, so that a later + * open never reads a relation the abort has freed. + */ +void +PgColumnarBeginMetadataFlush(void) +{ + Assert(!md_flush.active); /* nesting would orphan the outer opens */ + md_flush.active = true; + md_flush.count = 0; + md_flush_opens = 0; +} + +void +PgColumnarEndMetadataFlush(void) +{ + int i; + + if (!md_flush.active) + return; + for (i = 0; i < md_flush.count; i++) + { + if (md_flush.indstates[i] != NULL) + CatalogCloseIndexes(md_flush.indstates[i]); + table_close(md_flush.rels[i], md_flush.locks[i]); + } + md_flush.active = false; + if (message_level_is_interesting(DEBUG1)) + elog(DEBUG1, "pgcolumnar metadata flush: opens=%lu tables=%d", + (unsigned long) md_flush_opens, md_flush.count); + md_flush.count = 0; +} + +void +PgColumnarResetMetadataFlush(void) +{ + md_flush.active = false; + md_flush.count = 0; } /* @@ -1646,9 +1818,9 @@ PgColumnarInsertNativeStorageRow(const NativeStorageMetadata *s) nulls[Anum_native_storage_sorted_from - 1] = true; tuple = heap_form_tuple(tupdesc, values, nulls); - CatalogTupleInsert(rel, tuple); + metadata_flush_insert(rel, tuple); heap_freetuple(tuple); - table_close(rel, RowExclusiveLock); + metadata_flush_close(rel, RowExclusiveLock); } /* @@ -1797,9 +1969,9 @@ PgColumnarInsertRowGroupRow(const NativeRowGroupMetadata *rg) PointerGetDatum(construct_empty_array(INT2OID)); tuple = heap_form_tuple(tupdesc, values, nulls); - CatalogTupleInsert(rel, tuple); + metadata_flush_insert(rel, tuple); heap_freetuple(tuple); - table_close(rel, RowExclusiveLock); + metadata_flush_close(rel, RowExclusiveLock); } void @@ -1828,9 +2000,9 @@ PgColumnarInsertColumnChunkRow(const NativeColumnChunkMetadata *cc) values[Anum_column_chunk_page_length - 1] = Int64GetDatum((int64) cc->pageLength); tuple = heap_form_tuple(tupdesc, values, nulls); - CatalogTupleInsert(rel, tuple); + metadata_flush_insert(rel, tuple); heap_freetuple(tuple); - table_close(rel, RowExclusiveLock); + metadata_flush_close(rel, RowExclusiveLock); } /* @@ -1884,9 +2056,9 @@ PgColumnarInsertZoneMapRow(const NativeZoneMapMetadata *z) values[Anum_zone_map_null_count - 1] = Int64GetDatum((int64) z->nullCount); tuple = heap_form_tuple(tupdesc, values, nulls); - CatalogTupleInsert(rel, tuple); + metadata_flush_insert(rel, tuple); heap_freetuple(tuple); - table_close(rel, RowExclusiveLock); + metadata_flush_close(rel, RowExclusiveLock); } /* @@ -1915,9 +2087,9 @@ PgColumnarInsertBloomRow(const NativeBloomMetadata *b) values[Anum_bloom_filter - 1] = PointerGetDatum(filt); tuple = heap_form_tuple(tupdesc, values, nulls); - CatalogTupleInsert(rel, tuple); + metadata_flush_insert(rel, tuple); heap_freetuple(tuple); - table_close(rel, RowExclusiveLock); + metadata_flush_close(rel, RowExclusiveLock); } /* diff --git a/src/columnar_metadata.h b/src/columnar_metadata.h index 4ccfee2c..6a21754c 100644 --- a/src/columnar_metadata.h +++ b/src/columnar_metadata.h @@ -52,6 +52,11 @@ extern void PgColumnarInsertZoneMapRow(const NativeZoneMapMetadata *z); extern void PgColumnarInsertBloomRow(const NativeBloomMetadata *b); +/* #445: bracket a stripe flush's metadata inserts to share one open per table. */ +extern void PgColumnarBeginMetadataFlush(void); +extern void PgColumnarEndMetadataFlush(void); +extern void PgColumnarResetMetadataFlush(void); + extern List *PgColumnarReadColumnChunkList(uint64 storageId, uint64 groupNumber, Snapshot snapshot); diff --git a/src/columnar_write_state.c b/src/columnar_write_state.c index 57659c92..3227edd5 100644 --- a/src/columnar_write_state.c +++ b/src/columnar_write_state.c @@ -2758,40 +2758,57 @@ pgcolumnar_flush_row_group(PgColumnarWriteState *writeState) s.rowGroupLimit = writeState->stripeRowLimit; PgColumnarInsertNativeStorageRow(&s); } + /* + * #445: share one open per metadata table across this flush's inserts. The + * PG_TRY drops the session on error so a later open never reuses a relation + * the aborting subtransaction has freed; the resource owner closes them. + */ + PgColumnarBeginMetadataFlush(); + PG_TRY(); { - NativeRowGroupMetadata rg; - - rg.storageId = writeState->storageId; - rg.groupNumber = groupNumber; - rg.fileOffset = fileOffset; - rg.rowCount = rowCount; - rg.byteLength = dataLength; - rg.firstRowNumber = writeState->stripeFirstRowNumber; - PgColumnarInsertRowGroupRow(&rg); - } - for (c = 0; c < natts; c++) - { - NativeColumnChunkMetadata cc; + { + NativeRowGroupMetadata rg; + + rg.storageId = writeState->storageId; + rg.groupNumber = groupNumber; + rg.fileOffset = fileOffset; + rg.rowCount = rowCount; + rg.byteLength = dataLength; + rg.firstRowNumber = writeState->stripeFirstRowNumber; + PgColumnarInsertRowGroupRow(&rg); + } + for (c = 0; c < natts; c++) + { + NativeColumnChunkMetadata cc; - /* skipped virtual generated column (no chunk written) */ - if (chunkDescriptor[c] == NULL) - continue; + /* skipped virtual generated column (no chunk written) */ + if (chunkDescriptor[c] == NULL) + continue; + + cc.storageId = writeState->storageId; + cc.groupNumber = groupNumber; + cc.columnIndex = c; + cc.valueCount = rowCount; + cc.encodingDescriptor = chunkDescriptor[c]; + cc.encodingDescriptorLen = chunkDescriptorLen[c]; + cc.blockCodec = chunkBlockCodec[c]; + cc.pageOffset = fileOffset + chunkOffset[c]; + cc.pageLength = chunkLength[c]; + PgColumnarInsertColumnChunkRow(&cc); + } + foreach(lc, zoneRows) + PgColumnarInsertZoneMapRow((NativeZoneMapMetadata *) lfirst(lc)); + foreach(lc, bloomRows) + PgColumnarInsertBloomRow((NativeBloomMetadata *) lfirst(lc)); - cc.storageId = writeState->storageId; - cc.groupNumber = groupNumber; - cc.columnIndex = c; - cc.valueCount = rowCount; - cc.encodingDescriptor = chunkDescriptor[c]; - cc.encodingDescriptorLen = chunkDescriptorLen[c]; - cc.blockCodec = chunkBlockCodec[c]; - cc.pageOffset = fileOffset + chunkOffset[c]; - cc.pageLength = chunkLength[c]; - PgColumnarInsertColumnChunkRow(&cc); + PgColumnarEndMetadataFlush(); } - foreach(lc, zoneRows) - PgColumnarInsertZoneMapRow((NativeZoneMapMetadata *) lfirst(lc)); - foreach(lc, bloomRows) - PgColumnarInsertBloomRow((NativeBloomMetadata *) lfirst(lc)); + PG_CATCH(); + { + PgColumnarResetMetadataFlush(); + PG_RE_THROW(); + } + PG_END_TRY(); table_close(rel, RowExclusiveLock); diff --git a/test/native_metadata_flush.sh b/test/native_metadata_flush.sh new file mode 100755 index 00000000..7e06a1b3 --- /dev/null +++ b/test/native_metadata_flush.sh @@ -0,0 +1,99 @@ +#!/usr/bin/env bash +# +# pgColumnar: a stripe flush opens each metadata table ONCE, not once per row (#445). +# +# The four metadata inserters (row_group, column_chunk, zone_map, bloom) each +# used to open their catalog, insert one row, and close. A flush inserts a chunk +# row per column and about one zone row per vector per column, so it opened a +# metadata relation on the order of natts * vectors times, and a profile of the +# numeric write path put that open and close cycle, not the encoding, at the top. +# A per-flush session now caches each metadata relation and reuses it for the +# whole flush. +# +# The witness is the DEBUG1 line the flush emits once per stripe: +# "pgcolumnar metadata flush: opens=N tables=M" +# where N is the number of REAL relation opens during the flush. +# +# This suite pins the property that is the whole point: the open count is a small +# constant, and it does NOT grow with the column count. That equality IS the +# removal proof. Revert the reuse (make open_columnar_table always table_open, or +# drop the session bracket in the flush) and the wide flush opens far more +# relations than the narrow one, so the "same for narrow and wide" check goes RED +# on its own, with no mutation needed. The absolute-bound and heap-mirror checks +# fence it further. +# +# Written fresh for pgColumnar. + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +q() { + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -Atqc "$1" 2>/dev/null +} + +# One full stripe of exactly stripe_row_limit rows flushes once. Small limits +# keep it fast: 2000 rows / 500 per vector = 4 vectors, so 5 zone rows per column. +LIMITS="SET pgcolumnar.stripe_row_limit=2000; SET pgcolumnar.chunk_group_row_limit=500;" + +# opens -> the N from the flush's DEBUG1 witness. +# The SET and the INSERT share one session, so the limits reach the flush. +opens() { # opens + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -c "SET client_min_messages=debug1;" -c "$LIMITS" -c "$1" 2>&1 | + grep -oE 'metadata flush: opens=[0-9]+' | tail -1 | grep -oE '[0-9]+' +} + +# ---- premise: the witness fires at all -------------------------------------- + +q "CREATE TABLE tprobe (a int, b int, c int) USING pgcolumnar;" >/dev/null +probe="$(opens "INSERT INTO tprobe SELECT g, g*2, g%7 FROM generate_series(1,2000) g;")" +check_text "premise: the flush emits the metadata-open witness" \ + "$([ -n "$probe" ] && echo yes || echo no)" "yes" + +# ---- the batched open count is a small constant ----------------------------- + +# A 3-column and a 20-column table, each flushed once. Batched, both open the +# same small set of metadata tables (row_group, column_chunk, zone_map, and bloom +# if built), so the counts are EQUAL and small. Un-batched, the wide table opens +# ~natts more chunk rows and ~natts*vectors more zone rows, so its count is far +# larger than the narrow one and this equality FAILS. +q "CREATE TABLE tnarrow (a int, b int, c int) USING pgcolumnar;" >/dev/null +q "CREATE TABLE twide (c00 int, c01 int, c02 int, c03 int, c04 int, c05 int, + c06 int, c07 int, c08 int, c09 int, c10 int, c11 int, c12 int, c13 int, + c14 int, c15 int, c16 int, c17 int, c18 int, c19 int) USING pgcolumnar;" >/dev/null + +n_narrow="$(opens "INSERT INTO tnarrow SELECT g, g*2, g%7 FROM generate_series(1,2000) g;")" +n_wide="$(opens "INSERT INTO twide SELECT g,g,g,g,g,g,g,g,g,g,g,g,g,g,g,g,g,g,g,g FROM generate_series(1,2000) g;")" + +check_num "narrow (3 col) and wide (20 col) flush open the same number of metadata relations" \ + "$n_narrow" "$n_wide" + +# Absolute bound: batched is the count of distinct metadata tables, a small +# constant near four. Un-batched a 20-column flush opens ~120, so this bound also +# goes RED without the batching. +check_text "a 20-column flush opens a small constant number of metadata relations, not one per row" \ + "$([ -n "$n_wide" ] && [ "$n_wide" -le 6 ] && echo small || echo "big:[$n_wide]")" \ + "small" + +# ---- the writes are correct, batched or not --------------------------------- + +# The batching must not change what is written. Mirror the wide table in heap and +# compare, so a corrupted or dropped metadata row would show as a count or sum +# mismatch. +q "CREATE TABLE twide_h (LIKE twide);" >/dev/null +q "INSERT INTO twide_h SELECT * FROM twide;" >/dev/null + +check_num "the wide table's row count survives the batched flush" \ + "$(q "SELECT count(*) FROM twide")" "2000" +check_num "every column reads back correct (sum over all 20 columns matches the heap mirror)" \ + "$(q "SELECT sum(c00+c01+c02+c03+c04+c05+c06+c07+c08+c09+c10+c11+c12+c13+c14+c15+c16+c17+c18+c19) FROM twide")" \ + "$(q "SELECT sum(c00+c01+c02+c03+c04+c05+c06+c07+c08+c09+c10+c11+c12+c13+c14+c15+c16+c17+c18+c19) FROM twide_h")" + +# A min/max range read exercises the zone-map rows the flush wrote in a batch. +check_num "a zone-map-pruned range read matches heap (the batched zone rows are correct)" \ + "$(q "SELECT count(*) FROM twide WHERE c00 BETWEEN 500 AND 1500")" \ + "$(q "SELECT count(*) FROM twide_h WHERE c00 BETWEEN 500 AND 1500")" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 638ddc37..8472eac1 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -111,6 +111,7 @@ SUITES=( native_ios native_late_materialization native_lazy_slot + native_metadata_flush native_ownership native_parquet_codecs native_parquet_fdw