From 6e2680dabe3e144ad0edc6ab23431e028c09747f Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:33:39 -0600 Subject: [PATCH] docs: fix code comments and docs that contradict the code (#291) Audit of the source comments and user-facing docs for issue #291. 38 verified defects fixed, each grounded in the contradicting code: - comments describing the wrong function or return type (columnar.h: ColumnarComputeFullyDeletedGroups carried the all-visible function's comment) - docstrings naming a non-existent "columnar." schema when the SQL functions live in "pgcolumnar." (export/import_arrow, export/import_parquet, read_parquet, parquet_schema, add/drop/read/reconstruct_projection, vm_selftest, GUC names) - a security-relevant header claiming the FDW partition reader takes the value text literally, when the code percent-decodes it - stale "stubbed for later phases" or "phase 1 uncompressed" file headers for callbacks and formats that are now implemented (tableam, reader, write_state, projection) - a comment claiming VACUUM is a no-op when it marks the VM and retires groups - an orphaned doc block for a deleted function (columnar_run_agg) and a vestigial decompressed-chunk-cache setup comment - groupagg_max_groups documented as a plan-time gate when it is an execution cap - two docs claims the code contradicts (bloom collation eligibility, Arrow type list) in ARCHITECTURE.md, and an empty column-cache section in configuration.md One user-facing error message that named a non-existent object ("columnar.storageid_seq") is corrected to "pgcolumnar.storageid_seq". Comments and docs only; no code logic changed. Five stale "require superuser" claims this audit also found are fixed in the separate server-file docs PR. STE and docs_style gates pass. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UX1jrWiQsJJA1t4pkmkb4T --- docs/ARCHITECTURE.md | 6 ++++-- docs/configuration.md | 5 ----- src/columnar.h | 33 ++++++++++++++++++--------------- src/columnar_arrow.c | 18 ++++++++++-------- src/columnar_delete_vector.c | 11 +++++------ src/columnar_encoding.c | 4 ++-- src/columnar_metadata.c | 13 +++++++------ src/columnar_parallel_copy.c | 4 ++++ src/columnar_parquet.c | 4 ++-- src/columnar_parquet_codec.c | 4 ++-- src/columnar_parquet_reader.c | 28 ++++++++++------------------ src/columnar_projection.c | 19 ++++++++++--------- src/columnar_reader.c | 7 ++++--- src/columnar_tableam.c | 13 ++++++------- src/columnar_vacuum.c | 2 +- src/columnar_vector.c | 33 +++++++++++---------------------- src/columnar_visibilitymap.c | 4 ++-- src/columnar_write_state.c | 13 +++++++------ 18 files changed, 105 insertions(+), 116 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 224e83a..2d1093c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -151,7 +151,8 @@ compression-block run iterator (`ColumnarBlockReader`) that exposes a chunk as ### columnar_bloom.c Per-chunk bloom filters for equality chunk-group skipping. The writer hashes each -non-null value (hashable, non-collatable columns only) and builds a filter per +non-null value (hashable columns whose collation is non-collatable or +deterministic; nondeterministic collations are left unbloomed) and builds a filter per chunk; the reader probes it for an equality predicate the min/max range could not rule out, skipping the group when the value is provably absent. Never a false negative, so results are unaffected. @@ -247,7 +248,8 @@ FlatBuffers builder emits the Schema and RecordBatch messages (MetadataVersion V5); rows are read in physical order via the scalar reader and buffered one RecordBatch at a time (validity bitmap, then values, with utf8/binary offsets). No libarrow dependency. Supported types are int2/int4/int8, float4/float8, bool, -text/varchar, and bytea; other types are rejected. Little-endian hosts only. +text/varchar, bytea, date/time/timestamp/timestamptz, uuid, numeric, and +json/jsonb; other types are rejected. Little-endian hosts only. ### columnar_parquet.c Parquet export (`pgcolumnar.export_parquet`, gap 27). A self-contained Thrift diff --git a/docs/configuration.md b/docs/configuration.md index f41c6f1..bf66ac4 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -67,11 +67,6 @@ disk. It never changes the values that a table returns. | `pgcolumnar.enable_index_only_scan` | boolean | `on` | Allow index-only scans on columnar tables, served by the columnar visibility-map fork. Set to `off` to force a plain index scan. | | `pgcolumnar.enable_projection_scan` | boolean | `on` | Let the planner scan a covering projection instead of the base table when one serves the query better. | -### Column cache - -| Setting | Type | Default | Description | -| --- | --- | --- | --- | - ### Maintenance and disk reclaim | Setting | Type | Default | Description | diff --git a/src/columnar.h b/src/columnar.h index 27138b6..81b516e 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -396,8 +396,8 @@ typedef struct ColumnarRowRange uint64 rowCount; } ColumnarRowRange; -/* all-visible chunk-group row ranges: stripe committed past the horizon and no - * deletes (committed or in-progress). Returns a List of ColumnarRowRange *. */ +/* row groups every one of whose rows is deleted as-of oldestXmin. Returns a + * List of palloc'd uint64 group numbers. */ extern List *ColumnarComputeFullyDeletedGroups(uint64 storageId, TransactionId oldestXmin); extern void ColumnarRetireGroup(uint64 storageId, uint64 groupNumber); @@ -409,6 +409,8 @@ extern bool ColumnarTrailingFreeSpaceSafe(uint64 storageId, uint64 liveEnd, TransactionId oldestXmin); extern void ColumnarDeleteFreeSpaceAtOrAbove(uint64 storageId, uint64 liveEnd); extern void ColumnarReconcileFreeList(Relation dataRel); +/* all-visible chunk-group row ranges: stripe committed past the horizon and no + * deletes (committed or in-progress). Returns a List of ColumnarRowRange *. */ extern List *ColumnarComputeAllVisibleGroups(uint64 storageId, TransactionId oldestXmin); @@ -564,11 +566,6 @@ extern bool ColumnarReadNextRow(ColumnarReadState *readState, extern void ColumnarRescanRead(ColumnarReadState *readState); extern void ColumnarEndRead(ColumnarReadState *readState); -/* - * Parallel scan (gap 23): point the read state at a shared atomic that hands out - * stripe indices, so several workers scanning the same relation each claim - * distinct stripes. Set by the custom scan's DSM init callbacks. - */ /* * Restrict a scan to a set of row groups (issue #149). Groups outside the set * are skipped without their bytes being read. Must be called before the first @@ -586,6 +583,11 @@ extern int64 ColumnarWriteParquetFile(Relation rel, Snapshot snapshot, int nRestrictGroups); extern void ColumnarParquetCheckExportable(Relation rel); +/* + * Parallel scan (gap 23): point the read state at a shared atomic that hands out + * stripe indices, so several workers scanning the same relation each claim + * distinct stripes. Set by the custom scan's DSM init callbacks. + */ extern void ColumnarReadSetParallelCounter(ColumnarReadState *readState, pg_atomic_uint32 *counter); @@ -599,12 +601,6 @@ extern void ColumnarReadStats(ColumnarReadState *readState, uint64 *groupsTotal); extern uint64 ColumnarVectorsSkipped(ColumnarReadState *readState); -/* - * Fetch a single row by its 1-based row number (spec 6), for the table AM's - * fetch-by-tid callback used by UPDATE. Fills values/nulls (by-reference values - * are allocated in the current memory context) and returns true when the row - * exists and is not marked deleted in the delete vector. - */ /* cached base-liveness for a projection scan (gap 26): build once per scan, * probe per row with a binary search instead of a per-row catalog scan */ typedef struct ColumnarLivenessCache ColumnarLivenessCache; @@ -613,6 +609,12 @@ extern ColumnarLivenessCache *ColumnarBuildLivenessCache(Relation rel, extern bool ColumnarLivenessCacheIsLive(ColumnarLivenessCache *cache, uint64 rowNumber); extern void ColumnarFreeLivenessCache(ColumnarLivenessCache *cache); +/* + * Fetch a single row by its 1-based row number (spec 6), for the table AM's + * fetch-by-tid callback used by UPDATE. Fills values/nulls (by-reference values + * are allocated in the current memory context) and returns true when the row + * exists and is not marked deleted in the delete vector. + */ extern bool ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, Datum *values, bool *nulls); @@ -818,8 +820,9 @@ extern void ColumnarVectorInit(void); * padding, so a four-byte varlena header starts wherever the previous value * ended. VARSIZE_ANY reads that header by casting to varattrib_4b and loading a * uint32, which is undefined behaviour on an unaligned address: it happens to - * work on x86_64 and is a SIGBUS on a strict-alignment target, which - * docs/limitations.md promises to support. + * work on x86_64 and is a SIGBUS on a strict-alignment target; unaligned reads + * are covered by the sanitizer gate, though non-x86_64 architectures are + * untested (docs/limitations.md). * * This is not a crafted-input problem. An ordinary INSERT of low-cardinality * text reports it six times under UBSAN, because encode_dict walks exactly such diff --git a/src/columnar_arrow.c b/src/columnar_arrow.c index a7d54fb..81ebcdb 100644 --- a/src/columnar_arrow.c +++ b/src/columnar_arrow.c @@ -3,7 +3,7 @@ * columnar_arrow.c * Arrow IPC stream export for pgColumnar (gap 27, piece 1). * - * columnar.export_arrow(rel regclass, path text) writes a columnar table + * pgcolumnar.export_arrow(rel regclass, path text) writes a columnar table * to an Apache Arrow IPC *stream* file: a Schema message, one RecordBatch * message per ARROW_BATCH_ROWS rows, and an end-of-stream marker. The * writer is self-contained -- it hand-builds the FlatBuffers metadata and @@ -11,9 +11,11 @@ * or run-time dependency. Rows are read in physical order via the scalar * reader; deleted rows are skipped by the reader. * - * First slice type mapping: int2/4/8, float4/8, bool, text/varchar (Utf8), - * bytea (Binary). Any other column type is rejected. Little-endian hosts - * only (the Arrow body mirrors native scalar bytes). + * Type mapping: int2/4/8, float4/8, bool, text/varchar (Utf8), bytea + * (Binary), date/time/timestamp/timestamptz, uuid, numeric, and json/jsonb; + * 1-D arrays (List) and composites (Struct) are also exported. A scalar type + * with no mapping is rejected. Little-endian hosts only (the Arrow body + * mirrors native scalar bytes). * * Independent MIT implementation built from the Apache Arrow columnar format * and IPC specifications (Schema.fbs, Message.fbs, encapsulated message format) @@ -939,7 +941,7 @@ write_record_batch(FILE *f, ArrowCol *cols, int ncols, int64 nrows) /* * columnar_export_arrow - * SQL: columnar.export_arrow(rel regclass, path text) -> bigint. + * SQL: pgcolumnar.export_arrow(rel regclass, path text) -> bigint. * Write a columnar table to an Arrow IPC stream file; returns the number * of rows written. */ @@ -1139,9 +1141,9 @@ columnar_export_arrow(PG_FUNCTION_ARGS) } /* ========================================================================= - * Arrow IPC stream import: columnar.import_arrow(rel regclass, path text). + * Arrow IPC stream import: pgcolumnar.import_arrow(rel regclass, path text). * - * Reads an Arrow IPC *stream* file (as columnar.export_arrow writes, and as + * Reads an Arrow IPC *stream* file (as pgcolumnar.export_arrow writes, and as * pyarrow writes for non-dictionary arrays) and inserts its rows into an * existing columnar table whose column types match the file's schema, using * the reverse of the export type mapping. Uncompressed bodies only; a @@ -1725,7 +1727,7 @@ imp_check_bounds(ImpNode *n, const uint8 *body, const int64 *bufOff, /* * columnar_import_arrow - * SQL: columnar.import_arrow(rel regclass, path text) -> bigint. + * SQL: pgcolumnar.import_arrow(rel regclass, path text) -> bigint. * Insert the rows of an Arrow IPC stream file into a columnar table; * returns the number of rows inserted. */ diff --git a/src/columnar_delete_vector.c b/src/columnar_delete_vector.c index 41a6eb2..5637442 100644 --- a/src/columnar_delete_vector.c +++ b/src/columnar_delete_vector.c @@ -135,8 +135,8 @@ delete_vector_get_buffer(Relation rel, uint64 storageId) /* * delete_vector_find_row_group - * Native (PGCN v1) analog of delete_vector_find_stripe: return the row group that - * contains rowNumber, rebuilding the cache from the catalog on a miss. + * Native (PGCN v1): return the row group that contains rowNumber, + * rebuilding the cache from the catalog on a miss. */ static NativeRowGroupMetadata * delete_vector_find_row_group(DeleteVectorBuffer *buf, uint64 rowNumber) @@ -226,10 +226,9 @@ delete_vector_get_chunk(DeleteVectorBuffer *buf, uint64 stripeId, int chunkId, * ColumnarMarkRowDeleted * Record that the row with the given 1-based row number is deleted, by * setting its bit in the in-memory delete buffer for its chunk group. - * Chunk-group boundaries are computed arithmetically: every chunk group - * but the last in a stripe holds exactly chunkRowCount rows (the writer - * fills a group before starting the next), so no chunk_group catalog read - * is needed here. + * The mark targets the whole enclosing row group as one bitmap (chunk id + * 0), sized to the row group's rowCount, found via + * delete_vector_find_row_group using its firstRowNumber and rowCount. */ void ColumnarMarkRowDeleted(Relation rel, uint64 rowNumber) diff --git a/src/columnar_encoding.c b/src/columnar_encoding.c index b9a4ae6..ca43719 100644 --- a/src/columnar_encoding.c +++ b/src/columnar_encoding.c @@ -2205,11 +2205,11 @@ ColumnarEncodeChunk(const char *raw, uint32 rawLen, Form_pg_attribute att, /* * Choose the candidates to apply. With sampling on and a chunk big enough to - * sample, each candidate is measured on a strided sample and only the best + * sample, each candidate is measured on a windowed sample and only the best * two are applied to the whole chunk; two rather than one because the sample * ranks closely-matched candidates unreliably, and the second application is * cheap next to the three it replaces. With sampling off, or a chunk too - * small for a stride, every candidate is applied as before. + * small to sample, every candidate is applied as before. */ if (w > 0 && columnar_encoding_sample_rows >= ENCODE_SAMPLE_MIN) { diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 8815677..9e77359 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -2,9 +2,10 @@ * * columnar_metadata.c * Access to the "columnar" metadata catalog tables and the storage-id - * sequence (spec 7). Metadata are ordinary heap tables keyed by storage - * id; we read and write them with direct catalog access so we do not - * depend on SPI reentrancy. + * sequence (spec 7). Most metadata are ordinary heap tables keyed by + * storage id (the options and projection_declaration tables are keyed by + * relation OID instead); we read and write them with direct catalog access + * so we do not depend on SPI reentrancy. * *------------------------------------------------------------------------- */ @@ -161,7 +162,7 @@ columnar_index_oid(const char *name) /* * ColumnarNextStorageId - * Draw the next value from columnar.storageid_seq (spec 3, 7.6). + * Draw the next value from pgcolumnar.storageid_seq (spec 3, 7.6). */ uint64 ColumnarNextStorageId(void) @@ -173,7 +174,7 @@ ColumnarNextStorageId(void) if (!OidIsValid(seqOid)) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("columnar.storageid_seq does not exist"))); + errmsg("pgcolumnar.storageid_seq does not exist"))); value = nextval_internal(seqOid, false); return (uint64) value; @@ -488,7 +489,7 @@ insert_free_space_row(Relation rel, TupleDesc td, uint64 storageId, * to a whole page), so free ranges tile the file page-aligned and a split * remnant or a coalesced union stays page-aligned. * - * When columnar.reclaim_coalesce is on, the new range is merged with any + * When pgcolumnar.reclaim_coalesce is on, the new range is merged with any * immediately adjacent existing free range (left neighbor ending at this * offset, or right neighbor starting at this range's end) before insertion, * so a later request larger than either neighbor can still be satisfied from diff --git a/src/columnar_parallel_copy.c b/src/columnar_parallel_copy.c index e148b34..2cf794f 100644 --- a/src/columnar_parallel_copy.c +++ b/src/columnar_parallel_copy.c @@ -19,6 +19,10 @@ * PREPAREDs them all, so a failure in any range leaves no partial load. Distinct * partitions means distinct storage, which is what makes this parallel (no * shared per-storage write lock) and 2PC-safe (no deadlock). + * - the single (non-partitioned) columnar table shape: N loaders write ONE shared + * storage concurrently. Here parallelism does not come from distinct storage; + * it comes from the coordinator pre-creating and committing the storage row so + * the loaders skip its creation lock, each writing via columnar_bulk_parallel_writer. * - a standalone byte splitter (columnar_file_split_offsets) exposed to SQL: N+1 * line-aligned offsets, a diagnostic the parallel load itself no longer calls. * Text format only for now, numeric/date-time partition keys only (their text form diff --git a/src/columnar_parquet.c b/src/columnar_parquet.c index 3799ddd..663b524 100644 --- a/src/columnar_parquet.c +++ b/src/columnar_parquet.c @@ -3,14 +3,14 @@ * columnar_parquet.c * Parquet file export for pgColumnar (gap 27, piece 2). * - * columnar.export_parquet(rel regclass, path text) writes a columnar table + * pgcolumnar.export_parquet(rel regclass, path text) writes a columnar table * to a Parquet file. The writer is self-contained -- it emits the Thrift * compact-protocol metadata and PLAIN-encoded, UNCOMPRESSED data pages * directly -- so there is no libparquet build or run-time dependency. Rows * are read in physical order via the scalar reader; one row group is * emitted per PARQUET_ROWGROUP_ROWS rows, with one DATA_PAGE per column. * - * First-slice type mapping (matches columnar.export_arrow): int2/int4 -> + * First-slice type mapping (matches pgcolumnar.export_arrow): int2/int4 -> * INT32 (int2 tagged INT_16), int8 -> INT64, float4 -> FLOAT, float8 -> * DOUBLE, bool -> BOOLEAN, text/varchar -> BYTE_ARRAY (UTF8), bytea -> * BYTE_ARRAY. All columns are OPTIONAL; nulls are carried in definition diff --git a/src/columnar_parquet_codec.c b/src/columnar_parquet_codec.c index c5a2222..661bc95 100644 --- a/src/columnar_parquet_codec.c +++ b/src/columnar_parquet_codec.c @@ -126,8 +126,8 @@ snappy_raw_uncompress(const uint8 *in, size_t inlen, StringInfo out) * * On success *out / *outlen point at the decompressed bytes: either straight into * `src` (uncompressed), or into `scratch` (any real codec). `usize` is the - * uncompressed size from the page header, which zstd and lz4_raw require up front - * (they do not self-describe the output length the way Snappy and gzip do); pass + * uncompressed size from the page header, which zstd, lz4_raw and gzip require up + * front (only Snappy self-describes its output length, via a leading varint); pass * the value portion's uncompressed size for a v2 data page, where the levels are * stored uncompressed ahead of the compressed values. * diff --git a/src/columnar_parquet_reader.c b/src/columnar_parquet_reader.c index 2ab187b..0051a69 100644 --- a/src/columnar_parquet_reader.c +++ b/src/columnar_parquet_reader.c @@ -1,7 +1,7 @@ /*------------------------------------------------------------------------- * * columnar_parquet_reader.c - * Parquet file import: columnar.import_parquet(rel regclass, path text). + * Parquet file import: pgcolumnar.import_parquet(rel regclass, path text). * * A self-contained Parquet reader with no libparquet/libarrow dependency. It * parses the Thrift compact-protocol file metadata, decompresses Snappy (and @@ -9,7 +9,7 @@ * (RLE_DICTIONARY / PLAIN_DICTIONARY) encodings from both DATA_PAGE (v1) and * DATA_PAGE_V2 pages -- the combination pyarrow writes by default. Rows are * inserted into an existing target table (its tuple descriptor defines the - * expected columns and types), mirroring columnar.import_arrow. + * expected columns and types), mirroring pgcolumnar.import_arrow. * * Independent MIT implementation built from the Apache Parquet format and Thrift * compact-protocol specifications, the Snappy format description, and the public @@ -2330,14 +2330,6 @@ build_imp_targets(TupleDesc tupdesc, PqFile *pf, return tops; } -/* - * Read an entire server-side Parquet file into a palloc'd buffer and parse its - * footer metadata into *pf. On success the file bytes are returned in *bufOut - * (length *lenOut), allocated in the caller's memory context. This never returns - * on failure: any open/size/read/format/metadata error is reported with ereport, - * so the caller does not need to check a return value or clean the buffer up. - * The caller is responsible for any privilege check before calling. - */ /* strcmp comparator for list_sort over a List of cstrings */ static int pq_list_str_cmp(const ListCell *a, const ListCell *b) @@ -2890,7 +2882,7 @@ pq_read_file_into(const char *path, TupleDesc tupdesc, TupleTableSlot *slot, } /* - * columnar.import_parquet(rel regclass, path text) -> bigint + * pgcolumnar.import_parquet(rel regclass, path text) -> bigint */ Datum columnar_import_parquet(PG_FUNCTION_ARGS) @@ -2958,7 +2950,7 @@ columnar_import_parquet(PG_FUNCTION_ARGS) } /* - * columnar.read_parquet(path text) returns setof record + * pgcolumnar.read_parquet(path text) returns setof record * * Stream a server-side Parquet file's rows in place, without importing. The caller * supplies a column definition list: @@ -3036,7 +3028,7 @@ columnar_read_parquet(PG_FUNCTION_ARGS) } /* - * columnar.parquet_schema(path text) + * pgcolumnar.parquet_schema(path text) * -> table(column_name text, data_type text, nullable bool) * * Read a server-side Parquet file's footer and report its leaf columns with the @@ -3331,9 +3323,9 @@ pq_percent_decode(const char *src, const char *file) * function, so the declared type decides what a directory name means, and a value * that will not convert raises through the normal input-function error. * - * The value text is taken literally. Hive percent-encodes characters that cannot - * appear in a path component, and that decoding is deliberately not done here; - * see the limitation in the docs. + * The value text is percent-decoded (see pq_percent_decode). Hive percent-encodes + * characters that cannot appear in a path component, so a value written as a%3Db + * is read as the string "a=b". */ static void pqfdw_partition_values(const char *root, const char *file, TupleDesc tupdesc, @@ -4101,8 +4093,8 @@ pgcolumnar_parquet_fdw_handler(PG_FUNCTION_ARGS) } /* - * Option validator: the only accepted option is "path" on a foreign table. Server, - * wrapper, and user-mapping objects take no options. + * Option validator: the accepted options are "path" and "partition_columns" on a + * foreign table. Server, wrapper, and user-mapping objects take no options. */ Datum pgcolumnar_parquet_fdw_validator(PG_FUNCTION_ARGS) diff --git a/src/columnar_projection.c b/src/columnar_projection.c index bb9f2bc..b60f838 100644 --- a/src/columnar_projection.c +++ b/src/columnar_projection.c @@ -5,13 +5,14 @@ * * A projection is a named, ordered subset of a table's columns stored as its * own columnar storage, sorted on its sort key, sharing the table's row-number - * identity space (the C-Store model; see design/gaps/26-*). Phase 1 provides the - * catalog (columnar.projection) and the add/drop DDL only: declaring a - * projection allocates its storage id and records the row, but no data is - * written to a projection's storage yet (write fan-out is phase 2). + * identity space (the C-Store model; see design/gaps/26-*). The catalog + * (pgcolumnar.projection) and the add/drop DDL are provided here: declaring a + * projection allocates its storage id, records the catalog row, and back-fills + * the projection's storage from existing rows (ColumnarBackfillProjection). Read + * paths (read_projection, reconstruct_via_projection) are also provided. * * projection_id 0 is the implicit base projection (all live columns, insert - * order). A table with no columnar.projection rows has a single implicit base + * order). A table with no pgcolumnar.projection rows has a single implicit base * projection, so pre-existing and 2.0/2.1 tables are unaffected. The base row is * recorded lazily the first time a projection is added. * @@ -139,7 +140,7 @@ record_base_projection(Relation rel, uint64 storageId, List *existing) } /* - * columnar.add_projection(rel, name, columns text[], sort_key text[]) + * pgcolumnar.add_projection(rel, name, columns text[], sort_key text[]) * Declare a projection: a named column subset sorted on sort_key. */ Datum @@ -264,7 +265,7 @@ columnar_add_projection(PG_FUNCTION_ARGS) } /* - * columnar.drop_projection(rel, name) + * pgcolumnar.drop_projection(rel, name) * Drop a declared projection. The base projection cannot be dropped. */ Datum @@ -341,7 +342,7 @@ columnar_drop_projection(PG_FUNCTION_ARGS) } /* - * columnar.read_projection(rel, name) -> setof text + * pgcolumnar.read_projection(rel, name) -> setof text * Debug/verification reader for a projection's storage (gap 26, phase 2). * Scans the projection's stripes (in stored sort order), skips rows whose * base row number is deleted or invisible per the base delete_vector/visibility, @@ -490,7 +491,7 @@ columnar_read_projection(PG_FUNCTION_ARGS) } /* - * columnar.reconstruct_via_projection(rel, name) -> setof text + * pgcolumnar.reconstruct_via_projection(rel, name) -> setof text * Read every live row through a projection and reconstruct the full base * row (gap 26, phase 3): columns stored in the projection come from the * projection's own storage, and any remaining table columns are fetched diff --git a/src/columnar_reader.c b/src/columnar_reader.c index ca22581..3169401 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -5,9 +5,10 @@ * stripes and reconstructs rows (spec 4, 6). Also holds the value-stream * codec shared with the writer. * - * Phase 1 stores value streams uncompressed. Each chunk carries an exists - * (null bitmap) stream of one byte per row; present rows draw their value - * from the value stream in order. + * The native format encodes value streams per vector and optionally block- + * compresses them; the reader reverses both. Each chunk carries an exists + * (null bitmap) stream of one bit per row (a validity bitmap); present rows + * draw their value from the value stream in order. * *------------------------------------------------------------------------- */ diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 388eda0..83524a2 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -4,10 +4,10 @@ * Table access method handler for pgColumnar and extension glue: * GUCs, the pre-commit flush hook, and drop-time metadata cleanup. * - * Implements the subset of TableAmRoutine built through phase 3: create, bulk - * insert, sequential scan, delete and update via the delete vector, fetch by tid, - * size estimation, and non-transactional truncate. Index, vacuum, and sample - * callbacks are stubbed for later phases. + * Implements the TableAmRoutine callbacks: create, bulk insert, sequential + * scan, delete and update via the delete vector, fetch by tid, size + * estimation, non-transactional truncate, index fetch/build, and lazy VACUUM. + * Only the TABLESAMPLE (sample) callbacks are stubbed for later phases. * *------------------------------------------------------------------------- */ @@ -1040,7 +1040,8 @@ columnar_scan_analyze_next_tuple(COLUMNAR_ANALYZE_NEXT_TUPLE_ARGS) } } -/* VACUUM: nothing to do in phase 1 (delete vector / compaction arrive later) */ +/* VACUUM: mark all-visible groups in the VM fork and retire fully-deleted + * groups online, both under ShareUpdateExclusiveLock */ static void columnar_relation_vacuum(Relation rel, COLUMNAR_VACUUM_PARAMS params, BufferAccessStrategy bstrategy) @@ -2405,8 +2406,6 @@ _PG_init(void) /* install the vectorized-aggregate upper-path hook (spec 9) */ ColumnarVectorInit(); - /* set up the optional decompressed-chunk cache (spec 8.3) */ - /* register the unique-index cache invalidation callback (issue #5) */ ColumnarUniqueInit(); } diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index e6d4af3..0c3f328 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -109,7 +109,7 @@ uint64_cmp(const void *a, const void *b) * conflict check in ColumnarUpsertDeleteVector. * ------------------------------------------------------------------------- */ -/* the relation's ready+valid indexes, opened once for a rewrite pass */ +/* the relation's ready indexes, opened once for a rewrite pass */ /* * Rewrite one group's live rows into a fresh group and retire the old group. * Returns the number of live rows moved. Caller holds ShareUpdateExclusiveLock diff --git a/src/columnar_vector.c b/src/columnar_vector.c index bdd9f9f..60d1096 100644 --- a/src/columnar_vector.c +++ b/src/columnar_vector.c @@ -81,9 +81,9 @@ bool columnar_enable_vectorization = true; /* * GUC: extend the vectorized aggregate to GROUP BY (#289). Default off while the * grouped path is built out incrementally; the ungrouped path is unaffected. - * groupagg_max_groups caps the plan-time group estimate the grouped path will - * accept, so a high-cardinality grouping routes to the spillable core HashAgg - * rather than this (non-spilling) path. + * groupagg_max_groups caps the ACTUAL group count at execution time; exceeding + * it raises an error (see columnar_groupagg_lookup) rather than routing to core + * HashAgg. It is not a plan-time gate. */ bool columnar_enable_group_vectorization = false; int columnar_groupagg_max_groups = 1000000; @@ -587,7 +587,7 @@ typedef struct ColumnarGroupAggScanState ColumnarGroupEntry *entries; /* open-addressing table (power-of-two) */ int capacity; int nGroups; - int maxGroups; /* GUC cap (planner guard) */ + int maxGroups; /* GUC cap enforced at execution (columnar_groupagg_lookup) */ MemoryContext keyContext; /* copied key Datums */ MemoryContext specContext; /* per-group specs + running min/max/numeric */ @@ -735,8 +735,8 @@ ColumnarCreateUpperPaths(PlannerInfo *root, UpperRelationKind stage, } /* - * Every restriction clause must convert to a predicate we evaluate exactly, - * so the vectorized filter is the complete WHERE. Otherwise fall back. + * The ungrouped path supports no residual filter: collect any restriction + * clauses so that with any WHERE we fall back to the ordinary Agg. */ quals = extract_actual_clauses(input_rel->baserestrictinfo, false); @@ -890,8 +890,9 @@ ColumnarCreateUpperPaths(PlannerInfo *root, UpperRelationKind stage, * Add a grouped vectorized aggregate path (#289) when the query is one we * can answer exactly: a single columnar base relation, an optional WHERE, * every output entry either a supported aggregate or a bare reference to a - * supported GROUP BY key, and an estimated group count within the cap. On - * anything unsupported it adds nothing and the ordinary Agg plan runs. + * supported GROUP BY key. On anything unsupported it adds nothing and the + * ordinary Agg plan runs. The group-count cap is enforced only at + * execution (columnar_groupagg_lookup). */ static void ColumnarTryGroupAggPath(PlannerInfo *root, RelOptInfo *input_rel, @@ -1253,8 +1254,8 @@ columnar_float4_pl(float4 a, float4 b) /* * columnar_apply_one * Fold one value (or a null) into an aggregate accumulator. This is the - * reference per-row semantics, shared by the vectorized per-row path and - * the run path's fallback and min/max handling. + * reference per-row semantics, shared by the ungrouped scan path + * (columnar_native_scan_agg) and the grouped path (columnar_groupagg_build). */ static void columnar_apply_one(MemoryContext resultContext, ColumnarAggSpec *spec, @@ -1441,18 +1442,6 @@ columnar_apply_one(MemoryContext resultContext, ColumnarAggSpec *spec, } -/* - * columnar_run_agg - * Scan the base relation once and fold every chunk group into the aggregate - * accumulators. The reader (ColumnarBeginRead) applies min/max chunk-group - * skipping and the delete vector. With no pushed-down predicates and fixed-width - * aggregate columns, groups are folded run-at-a-time over the value stream - * (I3 compressed execution); otherwise, and for groups with deletes, the - * per-row vectorized path is used. Returns the read state so the caller can - * read skip counters for EXPLAIN before ending it. - */ - - /* * columnar_agg_finalize * Turn one accumulator into its output Datum, reproducing PostgreSQL's diff --git a/src/columnar_visibilitymap.c b/src/columnar_visibilitymap.c index 8a342dc..5070850 100644 --- a/src/columnar_visibilitymap.c +++ b/src/columnar_visibilitymap.c @@ -22,7 +22,7 @@ * stock visibilitymap_get_status. This decouples the write from any heap * page and from the per-version visibilitymap_set signature. * - * Phase 1 exposes columnar.vm_selftest(rel, blk) to prove empirically that + * Phase 1 exposes pgcolumnar.vm_selftest(rel, blk) to prove empirically that * a bit written here is read back by visibilitymap_get_status on a columnar * relation. Later phases wire set-in-vacuum and clear-on-write. * @@ -52,7 +52,7 @@ PG_FUNCTION_INFO_V1(columnar_vm_is_visible); /* * Visibility-map on-disk layout. These mirror the private macros in * src/backend/access/heap/visibilitymap.c; the two-bits-per-heap-block layout - * has been stable since PostgreSQL 9.6, and columnar.vm_selftest verifies at + * has been stable since PostgreSQL 9.6, and pgcolumnar.vm_selftest verifies at * run time that a bit written with this layout is read back by the backend's * own visibilitymap_get_status (which uses the real macros), so any divergence * would surface immediately in the matrix rather than silently. diff --git a/src/columnar_write_state.c b/src/columnar_write_state.c index 0f6330e..b16f6d6 100644 --- a/src/columnar_write_state.c +++ b/src/columnar_write_state.c @@ -896,12 +896,13 @@ ColumnarBufferedRowByNumber(Relation rel, uint64 rowNumber, /* * columnar_flush_row_group * Native-format (PGCN v1) flush. Lay out the accumulated rows as one row - * group: each column is a column chunk of [validity bitmap][uncompressed - * values], where the validity bitmap is one bit per row (LSB-first) and the - * values are the concatenated present-value streams. Write the bytes to the - * relation file and record the native catalog rows (storage, row_group, - * column_chunk). Phase D2b baseline: the encoding is uncompressed; the - * cascade and zone maps arrive in D4/D5 and the native reader in D3. + * group: each column is a column chunk of [validity bitmap][values], where + * the validity bitmap is one bit per row (LSB-first) and the values are the + * concatenated present-value streams encoded per-vector via the adaptive + * cascade and then optionally block-compressed. Compute per-vector and + * whole-chunk zone maps plus a per-chunk bloom filter. Write the bytes to + * the relation file and record the native catalog rows (storage, row_group, + * column_chunk, zone_map, bloom). */ static void columnar_flush_row_group(ColumnarWriteState *writeState)