From 9e6fdb1b2a6328aaa7193c09e3ebacf9cfd13c10 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 5 Aug 2026 19:16:14 -0600 Subject: [PATCH 01/14] refactor: give the Parquet byte source a vtable, ahead of object storage (#393) First unit of M1. No behaviour change, and that is the property under test. Every Parquet read in this reader goes through pq_source_read, a positional read whose callers have already bounded the offset. That is the one seam a remote source has to replace, and it is why object storage touches three functions rather than the reader. Making it dispatch now, on its own, keeps the change that adds a network reviewable. PqSource gains an ops pointer and a priv slot. ops NULL means the local FILE * implementation, which is the code that was already here, renamed to pq_source_read_local and pq_source_close_local and otherwise untouched. Nothing sets ops yet. Verified by running the whole Parquet surface rather than by reading the diff: native_read_parquet, native_parquet_fdw, native_parquet_pushdown, native_parquet_projection, native_parquet_multifile, native_parquet_partition, native_parquet_streaming, native_parquet_codecs, native_parquet_hardening and fuzz_parquet, 190 checks, all green. Builds clean on 15 to 19. The decisions this is built on are recorded on #393: a separate non-preloaded module, OpenSSL for TLS inside it, ambient credentials by default, no FUSE, and v1 scoped to exact object keys so ListObjectsV2 and its XML parser get their own milestone. --- src/columnar_parquet_reader.c | 58 ++++++++++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 5 deletions(-) diff --git a/src/columnar_parquet_reader.c b/src/columnar_parquet_reader.c index ba56caf..f99bc28 100644 --- a/src/columnar_parquet_reader.c +++ b/src/columnar_parquet_reader.c @@ -1415,21 +1415,45 @@ decode_plain_bools(const uint8 *buf, size_t buflen, int n, Datum *out) * scan of this file. Page bytes are read as they are needed, so peak memory does * not scale with the file. */ -typedef struct PqSource +/* + * A Parquet byte source (#393). + * + * Every read of a Parquet file in this reader goes through pq_source_read, which + * is a positional read whose callers have already bounded the offset. That makes + * it the one seam a remote source has to replace, and the reason object storage + * touches three functions rather than the reader. + * + * `ops` dispatches. The local implementation is the code that was here before and + * behaves identically; a remote one lives in a separate, non-preloaded module so + * that nothing an object-store client links is mapped into the postmaster. See + * PqObjStoreApi. + */ +typedef struct PqSource PqSource; + +typedef struct PqSourceOps +{ + const char *name; /* for error messages: "file", "s3", ... */ + void (*read) (PqSource *src, int64 off, void *buf, size_t n); + void (*close) (PqSource *src); +} PqSourceOps; + +struct PqSource { - FILE *f; /* AllocateFile handle (buffered) */ + const PqSourceOps *ops; /* NULL means the local file implementation */ + FILE *f; /* AllocateFile handle (buffered), local only */ + void *priv; /* remote implementation's own state */ const char *path; /* for error messages; palloc'd by the caller */ int64 len; /* file length in bytes */ uint8 *meta; /* serialized footer metadata */ uint32 metalen; -} PqSource; +}; /* * Read `n` bytes at `off` into `buf`. Every caller has already bounded `off` and * `n` against src->len; this reports the I/O failure that is left. */ static void -pq_source_read(PqSource *src, int64 off, void *buf, size_t n) +pq_source_read_local(PqSource *src, int64 off, void *buf, size_t n) { Assert(off >= 0 && n <= (size_t) (src->len - off)); if (fseeko(src->f, (off_t) off, SEEK_SET) != 0) @@ -1453,6 +1477,20 @@ pq_source_read(PqSource *src, int64 off, void *buf, size_t n) } } +/* + * The dispatcher every caller uses. Local sources keep the exact path they had + * before this existed: ops NULL means the FILE * implementation above. + */ +static void +pq_source_read(PqSource *src, int64 off, void *buf, size_t n) +{ + if (src->ops != NULL) + src->ops->read(src, off, buf, n); + else + pq_source_read_local(src, off, buf, n); +} + + /* * Open a Parquet file and parse its footer. Reads the two magics and the footer, * never the body. The error texts match what the whole-file reader raised, so a @@ -1520,7 +1558,7 @@ pq_source_open(const char *path, PqSource *src, PqFile *pf) } static void -pq_source_close(PqSource *src) +pq_source_close_local(PqSource *src) { if (src->f != NULL) { @@ -1529,6 +1567,16 @@ pq_source_close(PqSource *src) } } +static void +pq_source_close(PqSource *src) +{ + if (src->ops != NULL) + src->ops->close(src); + else + pq_source_close_local(src); +} + + /* * Page headers are small thrift structures, but a v2 header can carry column * statistics whose min and max are values from the column, so the size is From d1ed830ac7fe0c856f87a051d8c5b16db72d52d4 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 5 Aug 2026 20:12:28 -0600 Subject: [PATCH 02/14] feat: the object-store module, its ABI and its loader (#393 M1) Second unit of M1. Establishes the packaging and the boundary; carries no protocol yet, by design, so the commit that adds a network is a network change and nothing else. The property the whole design rests on, verified on all five majors: the main library still has exactly 4 NEEDED entries and the module is a separate file depending only on libc. pgColumnar loads through shared_preload_libraries, so anything it links is mapped into the postmaster and inherited by every backend whether or not a query ever reads a remote file. libcurl alone resolves to 30 shared objects, including two TLS implementations. PostgreSQL made the same call for its own libcurl dependency: separate library, runtime dlopen behind a frozen ABI, separate package. The module is built by objstore/Makefile, because PGXS builds one MODULE_big and this must not be that one. The top-level recurses for all, install and clean, without the `-` prefix, so a failure there is not silent. Loading is on the first read of a remote path and never before, through load_external_function with error_on_fail = false. An installation without the module reports an unsupported scheme rather than failing to load, so local files are unaffected either way. The result is cached including the negative, because whether the module is installed cannot change mid-session. The ABI is versioned and a mismatch is refused rather than called through: a stale module and a new main library agree on the symbol and disagree on the struct, and that produces a wrong function through a valid-looking pointer. A remote path now reports an object-storage error from the reader. Before this it reached AllocateFile and reported "No such file or directory", which is true of the filesystem and useless. test/objstore_module.sh asserts the separation directly, because adding one object to the main OBJS list would undo it silently, and that is the failure this design exists to prevent. 12 checks. The Parquet surface is unchanged. --- Makefile | 24 +++++++- objstore/Makefile | 33 +++++++++++ objstore/columnar_objstore_module.c | 73 +++++++++++++++++++++++++ src/columnar_objstore.c | 85 +++++++++++++++++++++++++++++ src/columnar_objstore.h | 81 +++++++++++++++++++++++++++ src/columnar_parquet_reader.c | 29 ++++++++++ test/objstore_module.sh | 51 +++++++++++++++++ test/run_all_versions.sh | 2 +- 8 files changed, 376 insertions(+), 2 deletions(-) create mode 100644 objstore/Makefile create mode 100644 objstore/columnar_objstore_module.c create mode 100644 src/columnar_objstore.c create mode 100644 src/columnar_objstore.h create mode 100755 test/objstore_module.sh diff --git a/Makefile b/Makefile index f206f2b..3f26fcb 100644 --- a/Makefile +++ b/Makefile @@ -27,7 +27,8 @@ OBJS = \ src/columnar_projection.o \ src/columnar_parquet_reader.o \ src/columnar_parallel_copy.o \ - src/columnar_parallel_export.o + src/columnar_parallel_export.o \ + src/columnar_objstore.o EXTENSION = pgcolumnar DATA = pgcolumnar--1.0-alpha.sql pgcolumnar--1.0-dev--1.0-alpha.sql @@ -113,3 +114,24 @@ PG_CFLAGS += -std=gnu17 endif include $(PGXS) + +# The object-store module is a SEPARATE shared library, built and installed +# alongside this one but never linked into it. See src/columnar_objstore.h: this +# extension is preloaded, so anything it links reaches the postmaster. +# +# A build failure there must not be silent, so these do not use the `-` prefix. +OBJSTORE_DIR = $(realpath $(dir $(firstword $(MAKEFILE_LIST))))/objstore + +all: objstore-all +objstore-all: + $(MAKE) -C $(OBJSTORE_DIR) PG_CONFIG=$(PG_CONFIG) all + +install: objstore-install +objstore-install: + $(MAKE) -C $(OBJSTORE_DIR) PG_CONFIG=$(PG_CONFIG) install + +clean: objstore-clean +objstore-clean: + $(MAKE) -C $(OBJSTORE_DIR) PG_CONFIG=$(PG_CONFIG) clean + +.PHONY: objstore-all objstore-install objstore-clean diff --git a/objstore/Makefile b/objstore/Makefile new file mode 100644 index 0000000..2e51d9d --- /dev/null +++ b/objstore/Makefile @@ -0,0 +1,33 @@ +# pgColumnar object-store module (#393). +# +# A SEPARATE shared library on purpose, and not part of MODULE_big. pgColumnar +# loads through shared_preload_libraries, so anything the main library links is +# mapped into the postmaster and inherited by every backend. This one is opened +# with load_external_function on the first read of a remote path and never +# before, so a cluster that reads only local files never maps it. +# +# PostgreSQL packages its own libcurl dependency this way (libpq-oauth): separate +# library, runtime dlopen, separate package. +MODULE_big = pgcolumnar_objstore + +OBJS = columnar_objstore_module.o + +PGFILEDESC = "pgColumnar object-store byte source" + +PG_CPPFLAGS = -I$(realpath $(dir $(firstword $(MAKEFILE_LIST)))../src) + +PG_CONFIG ?= pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) + +# Match the language standard the main library uses, for the same reason: PG19's +# headers are C23 and GCC 13 spells it gnu2x while GCC 14 spells it gnu23. +PG_MAJORVERSION := $(shell $(PG_CONFIG) --version | sed -E 's/[^0-9]*([0-9]+).*/\1/') +ifeq ($(shell test "$(PG_MAJORVERSION)" -ge 19 && echo yes),yes) +C23_STD := $(shell echo 'int main(void){return 0;}' \ + | cc -std=gnu23 -x c -c -o /dev/null - >/dev/null 2>&1 && echo gnu23 || echo gnu2x) +PG_CFLAGS += -std=$(C23_STD) +else +PG_CFLAGS += -std=gnu17 +endif + +include $(PGXS) diff --git a/objstore/columnar_objstore_module.c b/objstore/columnar_objstore_module.c new file mode 100644 index 0000000..0c6753b --- /dev/null +++ b/objstore/columnar_objstore_module.c @@ -0,0 +1,73 @@ +/*------------------------------------------------------------------------- + * columnar_objstore_module.c + * Object-store byte source for pgColumnar (#393). + * + * Loaded on demand by PgColumnarObjStoreGet, never preloaded. See + * src/columnar_objstore.h for why it is a separate library and for the ABI. + * + * This commit establishes the module, its build, and the ABI. The protocol + * itself arrives next: a range GET over HTTP/1.1 driven from a WaitEventSet, then + * SigV4 signing, then TLS. Until then every scheme reports unsupported, which is + * a better failure than the reader silently treating an s3:// URL as a filename. + *------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "fmgr.h" +#include "columnar_objstore.h" + +PG_MODULE_MAGIC; + +PGDLLEXPORT const PgColumnarObjStoreApi *pgcolumnar_objstore_init(void); + +static bool +objstore_handles_url(const char *url) +{ + /* + * Nothing is handled yet. Deliberately not returning true for s3:// before + * the protocol exists: the reader asks this question so it can report an + * unsupported scheme without attempting a connection, and answering yes here + * would turn a clear error into a failure inside open(). + */ + (void) url; + return false; +} + +static PgColumnarObjHandle * +objstore_open(const char *url, int64 *len) +{ + (void) len; + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("columnar: object storage is not implemented yet"), + errdetail("The object-store module is installed but carries no " + "protocol implementation for \"%s\".", url))); + return NULL; /* unreachable */ +} + +static void +objstore_read(PgColumnarObjHandle *h, int64 off, void *buf, size_t n) +{ + (void) h; (void) off; (void) buf; (void) n; + elog(ERROR, "columnar: object-store read reached an unopened handle"); +} + +static void +objstore_close(PgColumnarObjHandle *h) +{ + (void) h; +} + +static const PgColumnarObjStoreApi objstore_api = { + .abi_version = PGCOLUMNAR_OBJSTORE_ABI, + .handles_url = objstore_handles_url, + .open = objstore_open, + .read = objstore_read, + .close = objstore_close, +}; + +const PgColumnarObjStoreApi * +pgcolumnar_objstore_init(void) +{ + return &objstore_api; +} diff --git a/src/columnar_objstore.c b/src/columnar_objstore.c new file mode 100644 index 0000000..7f45a66 --- /dev/null +++ b/src/columnar_objstore.c @@ -0,0 +1,85 @@ +/*------------------------------------------------------------------------- + * columnar_objstore.c + * Loader for the object-store module (#393). + * + * The module is a separate, non-preloaded shared library. See + * columnar_objstore.h for why. This file is the only thing in the main library + * that knows it exists, and it never loads it until a remote path is read. + *------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "fmgr.h" +#include "utils/elog.h" + +#include "columnar.h" +#include "columnar_objstore.h" + +/* + * Cached across the session, INCLUDING the negative result. Whether the module is + * installed is a property of the installation, so a miss will not become a hit + * mid-session, and retrying a dlopen per read would be a per-read cost for + * something that cannot change. + */ +static const PgColumnarObjStoreApi *objstore_api = NULL; +static bool objstore_tried = false; + +bool +PgColumnarPathIsRemote(const char *path) +{ + static const char *const schemes[] = {"s3://", "gs://", "az://", "https://", + "http://", NULL}; + int i; + + if (path == NULL) + return false; + for (i = 0; schemes[i] != NULL; i++) + if (pg_strncasecmp(path, schemes[i], strlen(schemes[i])) == 0) + return true; + return false; +} + +const PgColumnarObjStoreApi * +PgColumnarObjStoreGet(void) +{ + PgColumnarObjStoreInitFn init; + const PgColumnarObjStoreApi *api; + + if (objstore_tried) + return objstore_api; + objstore_tried = true; + + /* + * load_external_function with error_on_fail = false, so an installation + * without the module reports an unsupported scheme rather than failing to + * load. $libdir is resolved by the server, so the module is found wherever + * the main library was installed. + */ + init = (PgColumnarObjStoreInitFn) + load_external_function("$libdir/pgcolumnar_objstore", + "pgcolumnar_objstore_init", false, NULL); + if (init == NULL) + return NULL; + + api = init(); + + /* + * Refuse a mismatch rather than calling through it. A stale module and a new + * main library agree on the symbol name and disagree on the struct, and the + * failure that produces is a wrong function through a valid-looking pointer. + */ + if (api == NULL || api->abi_version != PGCOLUMNAR_OBJSTORE_ABI) + { + ereport(WARNING, + (errmsg("columnar: ignoring object-store module with ABI version %d", + api ? api->abi_version : -1), + errdetail("This build expects ABI version %d.", + PGCOLUMNAR_OBJSTORE_ABI), + errhint("Reinstall pgcolumnar_objstore from the same build as " + "pgcolumnar."))); + return NULL; + } + + objstore_api = api; + return objstore_api; +} diff --git a/src/columnar_objstore.h b/src/columnar_objstore.h new file mode 100644 index 0000000..bc76cf9 --- /dev/null +++ b/src/columnar_objstore.h @@ -0,0 +1,81 @@ +/*------------------------------------------------------------------------- + * columnar_objstore.h + * The ABI between pgColumnar and its object-store module (#393). + * + * Object-store support lives in a SEPARATE, non-preloaded shared library. The + * reason is measured rather than stylistic: pgColumnar loads through + * shared_preload_libraries, so anything it links is mapped into the postmaster + * and inherited by every backend through fork, whether or not any query ever + * reads a remote file. libcurl alone resolves to 30 shared objects against this + * extension's 4, including two TLS implementations, and an OpenSSL-linked client + * brings a TLS stack into a postmaster that may have none. + * + * PostgreSQL made the same call for its own libcurl dependency: configure + * default off, a separate shared library, runtime dlopen behind a frozen ABI, + * and a separate distribution package. This follows that shape. + * + * The module is loaded with load_external_function on the first read of a remote + * path and never before. A build or an install without it is fully functional + * for local files, and reports a remote path as unsupported rather than failing + * to load. + * + * VERSIONING. Bump PGCOLUMNAR_OBJSTORE_ABI whenever the meaning or the order of + * anything below changes. The loader refuses a mismatch, because the failure it + * prevents is a wrong function called through a stale pointer. + *------------------------------------------------------------------------- + */ +#ifndef COLUMNAR_OBJSTORE_H +#define COLUMNAR_OBJSTORE_H + +#include "postgres.h" + +#define PGCOLUMNAR_OBJSTORE_ABI 1 + +/* An open remote object. The module owns everything behind this. */ +typedef struct PgColumnarObjHandle PgColumnarObjHandle; + +typedef struct PgColumnarObjStoreApi +{ + int abi_version; /* must equal PGCOLUMNAR_OBJSTORE_ABI */ + + /* + * Does this module handle `url`? Called before open so the reader can report + * an unsupported scheme without a connection attempt. + */ + bool (*handles_url) (const char *url); + + /* + * Open `url` and report its size. Raises on failure, like every other read + * path in this extension. `len` is required: the Parquet footer is located + * from the end of the object, so a source that cannot report a length cannot + * be read. + */ + PgColumnarObjHandle *(*open) (const char *url, int64 *len); + + /* + * Read exactly `n` bytes at `off`. The caller has already bounded the range + * against the length reported by open. A short read is an error, not a + * partial success, because the reader has no way to make progress from one. + */ + void (*read) (PgColumnarObjHandle *h, int64 off, void *buf, size_t n); + + void (*close) (PgColumnarObjHandle *h); +} PgColumnarObjStoreApi; + +/* + * The module's single exported entry point. Returns a pointer to a static API + * struct owned by the module. + */ +typedef const PgColumnarObjStoreApi *(*PgColumnarObjStoreInitFn) (void); + +/* + * Resolve the module, or return NULL when it is not installed. Cached after the + * first call, including the negative result: a missing module is a property of + * the installation and will not appear mid-session. + */ +extern const PgColumnarObjStoreApi *PgColumnarObjStoreGet(void); + +/* Does `path` look like a remote URL at all? Cheap, no module load. */ +extern bool PgColumnarPathIsRemote(const char *path); + +#endif /* COLUMNAR_OBJSTORE_H */ diff --git a/src/columnar_parquet_reader.c b/src/columnar_parquet_reader.c index f99bc28..f9a2d61 100644 --- a/src/columnar_parquet_reader.c +++ b/src/columnar_parquet_reader.c @@ -18,6 +18,7 @@ *------------------------------------------------------------------------- */ #include "columnar.h" +#include "columnar_objstore.h" #include "columnar_parquet_format.h" #include "columnar_thrift.h" #include "columnar_parquet_codec.h" @@ -1504,6 +1505,34 @@ pq_source_open(const char *path, PqSource *src, PqFile *pf) memset(src, 0, sizeof(*src)); src->path = path; + + /* + * A remote path is not a filename (#393). Without this, AllocateFile reports + * "No such file or directory" for s3://bucket/key, which is true of the + * filesystem and useless to the reader. + * + * The module is loaded here and only here, on the first remote read. An + * installation without it reaches the same error as an unsupported scheme, + * which is the intended behaviour rather than a degradation: local files are + * unaffected either way. + */ + if (PgColumnarPathIsRemote(path)) + { + const PgColumnarObjStoreApi *api = PgColumnarObjStoreGet(); + + if (api == NULL || !api->handles_url(path)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("columnar: reading \"%s\" is not supported", path), + errdetail("Object storage support is not available in this " + "build."), + errhint("Use a local filesystem path."))); + /* the remote source is wired in the next commit */ + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("columnar: object storage is not implemented yet"))); + } + src->f = AllocateFile(path, PG_BINARY_R); if (src->f == NULL) ereport(ERROR, diff --git a/test/objstore_module.sh b/test/objstore_module.sh new file mode 100755 index 0000000..cbee61c --- /dev/null +++ b/test/objstore_module.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# +# The object-store module's packaging and loader (#393). +# +# The module is a SEPARATE, non-preloaded shared library, because pgColumnar loads +# through shared_preload_libraries and anything the main library links is mapped into +# the postmaster and inherited by every backend. This suite asserts that separation +# holds, because it is the property the whole design rests on and it is easy to lose +# by accident: adding one object to the main OBJS list would undo it silently. +# +# It also asserts a remote path reports a remote error. Before this, s3://bucket/key +# reached AllocateFile and reported "No such file or directory", which is true of the +# filesystem and useless to the reader. +# +# Usage: test/objstore_module.sh [PG_CONFIG] +# Written fresh for pgColumnar. +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +LIBDIR="$("$PGC_PG_CONFIG" --pkglibdir 2>/dev/null || echo "")" +[ -n "$LIBDIR" ] || LIBDIR="$(pgc_pg "pg_config --pkglibdir" | tr -d '\r')" + +check "the main library is installed" \ + "$(pgc_pg "test -f '$LIBDIR/pgcolumnar.so' && echo yes || echo no")" "yes" +check "the object-store module is installed BESIDE it, not inside it" \ + "$(pgc_pg "test -f '$LIBDIR/pgcolumnar_objstore.so' && echo yes || echo no")" "yes" + +# The property the design rests on. If someone adds the module's objects to the main +# OBJS list, this is what notices. +check "the module is a separate file, so nothing it links reaches the postmaster" \ + "$(pgc_pg "readelf -d '$LIBDIR/pgcolumnar.so' | grep -c pgcolumnar_objstore" | tail -1)" "0" +check "and the module exports its single entry point" \ + "$(pgc_pg "nm -D --defined-only '$LIBDIR/pgcolumnar_objstore.so' | grep -c ' T pgcolumnar_objstore_init'")" "1" + +# A remote path must report a remote error, from the reader, without a connection. +for url in "s3://bucket/key.parquet" "gs://bucket/key.parquet" "https://host/key.parquet"; do + out=$(psql_run "SELECT * FROM pgcolumnar.read_parquet('$url') AS (a int)" 2>&1) + check "a $(cut -d: -f1 <<<"$url") URL reports an object-storage error, not a missing file" \ + "$([ "$(grep -c 'object storage is not implemented\|is not supported' <<<"$out")" -ge 1 ] && echo yes || echo no)" "yes" + check "and does NOT report it as a missing file" \ + "$(grep -c 'No such file or directory' <<<"$out")" "0" +done + +# A local path must be entirely unaffected, which is the regression this could cause. +psql_run "CREATE TABLE lp (a int) USING pgcolumnar; INSERT INTO lp VALUES (1),(2),(3);" >/dev/null 2>&1 +check "a local path still works" "$(q 'SELECT count(*) FROM lp')" "3" +check "a relative path is not mistaken for a URL" \ + "$(psql_run "SELECT * FROM pgcolumnar.read_parquet('/nonexistent/x.parquet') AS (a int)" 2>&1 | + grep -c 'No such file or directory\|could not open')" "1" +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index ca40ec7..9d60e3d 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -208,7 +208,7 @@ SRCDIR="${PGC_RUN_SRCDIR:-$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)}" SUITES=(harness_selftest docs_style smoke phase2 phase3 phase4 phase5 phase6 audit concurrency unique_conc \ differential recovery replication native_backend_crash fuzz fuzz_parquet fuzz_arrow hardening concurrent_diff parallel sorted_projection \ arrow_export parquet_export read_stream corruption \ - generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_fastdecode native_zonemap write_minmax_fastpath write_fsst_compressed fsst_margin encode_invariants encode_effort native_skip pushdown_report native_agg native_agg_deletes native_agg_addcolumn native_groupagg ungrouped_vector_agg parallel_vector_agg native_bloom bloom_setting bloom_lazy native_vecskip native_index native_index_projection native_fetch_position native_dml alter_column_type native_ios native_projection native_cluster pg19_vacuum_options native_repack native_compact native_recluster recluster_extent native_vacuum_race native_sort_by sort_status native_reclaim native_ownership drop_cleanup pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_format native_truncate native_rewrite native_rewrite_conc rewrite_group_scan native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening server_file_privilege native_parquet_stack native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel cancel_decode wal_envelope decode_interrupts import_exclusion import_deferred parallel_copy parallel_export_parquet fk_referencing row_triggers native_lazy_slot native_ctas native_fetch_cache native_fetch_interrupt analyze_stats analyze_reltuples native_fetch_projection column_projection advisory_lock_class logical_subscriber isolation) + generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_fastdecode native_zonemap write_minmax_fastpath write_fsst_compressed fsst_margin encode_invariants encode_effort native_skip pushdown_report native_agg native_agg_deletes native_agg_addcolumn native_groupagg ungrouped_vector_agg parallel_vector_agg native_bloom bloom_setting bloom_lazy native_vecskip native_index native_index_projection native_fetch_position native_dml alter_column_type native_ios native_projection native_cluster pg19_vacuum_options native_repack native_compact native_recluster recluster_extent native_vacuum_race native_sort_by sort_status native_reclaim native_ownership drop_cleanup pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_format native_truncate native_rewrite native_rewrite_conc rewrite_group_scan native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening server_file_privilege native_parquet_stack native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel cancel_decode wal_envelope decode_interrupts import_exclusion import_deferred parallel_copy parallel_export_parquet fk_referencing row_triggers native_lazy_slot native_ctas native_fetch_cache native_fetch_interrupt analyze_stats analyze_reltuples native_fetch_projection column_projection advisory_lock_class logical_subscriber objstore_module isolation) # Default matrix: one assert-enabled pg_config per major, 15 through 19. DEFAULT_CONFIGS=( From c9fac9bbbaef20ee78ee869c90db7746eaa7130b Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 5 Aug 2026 20:26:24 -0600 Subject: [PATCH 03/14] fix: a missing module must not leak the loader's error, once or twice (#393) @ChronicallyJD falsified the graceful-degradation claim in the header, and then found something worse underneath it. signalNotFound = false suppresses a missing SYMBOL. A missing LIBRARY is raised by internal_load_library before the symbol lookup happens, so that argument never gets a say and the caller sees 'could not access file "pgcolumnar_objstore"'. The api == NULL branch was unreachable on a first read. The load is now inside a PG_TRY, because an installation without the module is a supported configuration rather than an error. The worse half: objstore_tried was set BEFORE the attempt. The ereport unwinds past the assignment while the static keeps its new value, so the first remote read of a session reported the raw load failure and every later one reported the documented message. Two identical queries in one session, two different errors, and the second was the plausible-looking one. The flag is now set after the attempt. Proved by removal, which reproduces exactly what they measured: with the loader fix 16 checks, PASSED with it reverted FAIL neither read leaks the loader's own error: got [1] want [0] FAIL both reads report the SAME unsupported error: got [1] want [2] The coverage runs BOTH reads in one psql session on purpose. Separate sessions cannot see this, because each gets a fresh static. The header claim is corrected rather than deleted: it now says what makes the degradation graceful, which is the PG_TRY and not the signalNotFound argument. Builds clean on 15 to 19. --- src/columnar_objstore.c | 37 +++++++++++++++++++++++++++++-------- src/columnar_objstore.h | 10 +++++++--- test/objstore_module.sh | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/src/columnar_objstore.c b/src/columnar_objstore.c index 7f45a66..25435ed 100644 --- a/src/columnar_objstore.c +++ b/src/columnar_objstore.c @@ -47,17 +47,38 @@ PgColumnarObjStoreGet(void) if (objstore_tried) return objstore_api; - objstore_tried = true; /* - * load_external_function with error_on_fail = false, so an installation - * without the module reports an unsupported scheme rather than failing to - * load. $libdir is resolved by the server, so the module is found wherever - * the main library was installed. + * signalNotFound = false suppresses a missing SYMBOL. It does NOT suppress a + * missing LIBRARY: internal_load_library raises before the symbol lookup + * happens, so that argument never gets a say and the caller sees + * 'could not access file "pgcolumnar_objstore"'. An installation without the + * module is a supported configuration, not an error, so catch it. + * + * Only the load is inside the PG_TRY, so nothing else is swallowed. + * + * objstore_tried is set AFTER the attempt, not before. Setting it first looks + * equivalent and is not: the ereport unwinds past the assignment while the + * static keeps its new value, so the FIRST remote read of a session reported + * the raw load failure and every later one reported the documented message. + * Two identical queries in one session gave two different errors, and the + * second was the plausible-looking one. */ - init = (PgColumnarObjStoreInitFn) - load_external_function("$libdir/pgcolumnar_objstore", - "pgcolumnar_objstore_init", false, NULL); + init = NULL; + PG_TRY(); + { + init = (PgColumnarObjStoreInitFn) + load_external_function("$libdir/pgcolumnar_objstore", + "pgcolumnar_objstore_init", false, NULL); + } + PG_CATCH(); + { + FlushErrorState(); + init = NULL; + } + PG_END_TRY(); + + objstore_tried = true; if (init == NULL) return NULL; diff --git a/src/columnar_objstore.h b/src/columnar_objstore.h index bc76cf9..4360e6b 100644 --- a/src/columnar_objstore.h +++ b/src/columnar_objstore.h @@ -15,9 +15,13 @@ * and a separate distribution package. This follows that shape. * * The module is loaded with load_external_function on the first read of a remote - * path and never before. A build or an install without it is fully functional - * for local files, and reports a remote path as unsupported rather than failing - * to load. + * path and never before. A build or an install without it is fully functional for + * local files. + * + * A missing module reports the remote path as unsupported. That needs a PG_TRY in + * the loader and not just signalNotFound = false, because signalNotFound + * suppresses a missing SYMBOL while a missing LIBRARY is raised earlier, by + * internal_load_library, before the symbol lookup happens. * * VERSIONING. Bump PGCOLUMNAR_OBJSTORE_ABI whenever the meaning or the order of * anything below changes. The loader refuses a mismatch, because the failure it diff --git a/test/objstore_module.sh b/test/objstore_module.sh index cbee61c..a7ee770 100755 --- a/test/objstore_module.sh +++ b/test/objstore_module.sh @@ -48,4 +48,41 @@ check "a local path still works" "$(q 'SELECT count(*) FROM lp')" "3" check "a relative path is not mistaken for a URL" \ "$(psql_run "SELECT * FROM pgcolumnar.read_parquet('/nonexistent/x.parquet') AS (a int)" 2>&1 | grep -c 'No such file or directory\|could not open')" "1" +# ---- the module ABSENT, which is a supported configuration -------------------- +# +# Two things this asserts, both of which were wrong before: +# +# 1 a missing module must report the remote path as unsupported, not +# 'could not access file "pgcolumnar_objstore"'. signalNotFound = false +# suppresses a missing SYMBOL; a missing LIBRARY is raised earlier by +# internal_load_library and needs a PG_TRY. +# +# 2 the SAME query must give the SAME error twice in one session. The cache flag +# used to be set before the load, so the ereport unwound past the assignment +# while the static kept its new value: the first read reported the raw load +# failure and every later one reported the documented message. Two identical +# queries, one session, two different errors, the second one plausible. +# +# Both reads run in ONE psql session on purpose. Separate sessions cannot see it. +Q="SELECT * FROM pgcolumnar.read_parquet('s3://bucket/key.parquet') AS (a int)" +two_reads() { + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" \ + -At -q -c "$Q" -c "$Q" 2>&1 +} +MOD="$LIBDIR/pgcolumnar_objstore.so" + +both=$(two_reads) +check "with the module present, both reads report the same thing" \ + "$(grep -c 'is not implemented yet\|is not supported' <<<"$both")" "2" + +pgc_pg "mv '$MOD' '$MOD.away'" >/dev/null 2>&1 +absent=$(two_reads) +pgc_pg "mv '$MOD.away' '$MOD'" >/dev/null 2>&1 +check "premise: the module really was absent for that run" \ + "$(pgc_pg "test -f '$MOD' && echo restored || echo MISSING")" "restored" +check "with the module absent, neither read leaks the loader's own error" \ + "$(grep -c 'could not access file' <<<"$absent")" "0" +check "and both reads report the SAME unsupported error" \ + "$(grep -c 'is not supported' <<<"$absent")" "2" + pgc_summary From 95e28b67b1b168404ceeb96161998fff08047634 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 5 Aug 2026 20:54:59 -0600 Subject: [PATCH 04/14] fix: a broken module must not masquerade as a missing one (#393) @ChronicallyJD's third finding, now fixed. My first attempt was wrong and this records why, because the reason is the interesting part. I discriminated on SQLSTATE: swallow ERRCODE_UNDEFINED_FILE as "not installed", re-raise anything else. Measured, that is exactly wrong. internal_load_library uses errcode_for_file_access() for BOTH the stat failure and the dlopen failure, so a missing library and "file too short" arrive with the same code: PROBE446 catch: sqlerrcode=16908805 (58P01) msg=could not load library ".../pgcolumnar_objstore.so": file too short 58P01 is ERRCODE_UNDEFINED_FILE. Swallowing on it swallowed both. So ask the filesystem. objstore_module_present() resolves $libdir with get_pkglib_path, which is exported on every major we support, and stats the file. Absent means not installed. Present means the load failed for a reason the operator has to see, and the original error is re-raised intact. The test I wrote to catch this ALSO did nothing at first, and for a reason worth keeping in the file: it corrupted the module with `printf > $MOD`, which fails silently because the module is root-owned 0755 and the suite runs as postgres, which can write the DIRECTORY but not that file. The diagnostic that found it printed the module's size, which was still 20752 rather than 19. It now moves the real module aside, CREATES a corrupt one, and ASSERTS the size is 19 before reading anything. Both premises are in the suite now: that the module is genuinely corrupt during the run, and that the real one is restored afterwards. Proved by removal. With the presence check taken out: FAIL a broken module does NOT masquerade as an unsupported scheme: got [2] want [0] FAIL and the loader's own reason survives: got [no] want [yes] 20 checks. Builds clean on 15 to 19. --- src/columnar_objstore.c | 45 +++++++++++++++++++++++++++++++++++++++++ test/objstore_module.sh | 26 ++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/src/columnar_objstore.c b/src/columnar_objstore.c index 25435ed..afd3182 100644 --- a/src/columnar_objstore.c +++ b/src/columnar_objstore.c @@ -11,6 +11,8 @@ #include "fmgr.h" #include "utils/elog.h" +#include "miscadmin.h" +#include #include "columnar.h" #include "columnar_objstore.h" @@ -24,6 +26,22 @@ static const PgColumnarObjStoreApi *objstore_api = NULL; static bool objstore_tried = false; +/* + * Is the module file actually there? Resolves $libdir the way the server does, + * because the SQLSTATE cannot distinguish a missing library from a broken one. + */ +static bool +objstore_module_present(void) +{ + char libdir[MAXPGPATH]; + char path[MAXPGPATH]; + struct stat st; + + get_pkglib_path(my_exec_path, libdir); + snprintf(path, sizeof(path), "%s/pgcolumnar_objstore%s", libdir, DLSUFFIX); + return stat(path, &st) == 0; +} + bool PgColumnarPathIsRemote(const char *path) { @@ -44,6 +62,7 @@ PgColumnarObjStoreGet(void) { PgColumnarObjStoreInitFn init; const PgColumnarObjStoreApi *api; + MemoryContext loadcxt; if (objstore_tried) return objstore_api; @@ -64,6 +83,7 @@ PgColumnarObjStoreGet(void) * Two identical queries in one session gave two different errors, and the * second was the plausible-looking one. */ + loadcxt = CurrentMemoryContext; init = NULL; PG_TRY(); { @@ -73,6 +93,31 @@ PgColumnarObjStoreGet(void) } PG_CATCH(); { + MemoryContext ecxt = MemoryContextSwitchTo(loadcxt); + + /* + * "Not installed" and "installed but broken" are different situations and + * only the first is supported. Swallowing both reports a truncated + * library or a permission problem as an unsupported scheme, which sends + * the operator looking in the wrong place for a fault that is theirs. + * + * The SQLSTATE cannot tell them apart, which is the trap here and the + * reason the first attempt at this was wrong. internal_load_library uses + * errcode_for_file_access() for BOTH the stat failure and the dlopen + * failure, so a missing file and "file too short" both arrive as + * ERRCODE_UNDEFINED_FILE (58P01). Measured, not assumed. + * + * So ask the filesystem instead. Absent means not installed; present + * means the load failed for a reason the operator needs to see, and the + * original error is re-raised with its own message intact. + */ + if (objstore_module_present()) + { + MemoryContextSwitchTo(ecxt); + PG_RE_THROW(); + } + + MemoryContextSwitchTo(ecxt); FlushErrorState(); init = NULL; } diff --git a/test/objstore_module.sh b/test/objstore_module.sh index a7ee770..e4fd5a3 100755 --- a/test/objstore_module.sh +++ b/test/objstore_module.sh @@ -85,4 +85,30 @@ check "with the module absent, neither read leaks the loader's own error" \ check "and both reads report the SAME unsupported error" \ "$(grep -c 'is not supported' <<<"$absent")" "2" +# ---- installed but BROKEN is not the same as not installed -------------------- +# +# Only "not installed" is supported. A truncated library or a permission problem is the +# operator's to fix, and reporting it as an unsupported scheme sends them elsewhere. +# +# TWO premises, because the first version of this test had neither and passed while doing +# nothing: +# 1 the module must actually BE corrupt during the run. Overwriting it in place fails +# silently: it is root-owned 0755 and this runs as postgres, which can write the +# DIRECTORY but not that file. So move it aside and CREATE a new one. +# 2 the SQLSTATE cannot be used to tell the two cases apart. internal_load_library uses +# errcode_for_file_access() for both the stat failure and the dlopen failure, so +# missing and "file too short" both arrive as 58P01. Measured. The check is on file +# presence for that reason. +pgc_pg "mv '$MOD' '$MOD.away' && printf 'not a shared object' > '$MOD'" >/dev/null 2>&1 +check "premise: the module really is corrupt for this run, not merely intended to be" \ + "$(pgc_pg "stat -c %s '$MOD'" | tail -1)" "19" +broken=$(two_reads) +pgc_pg "rm -f '$MOD' && mv '$MOD.away' '$MOD'" >/dev/null 2>&1 +check "premise: the real module was restored afterwards" \ + "$([ "$(pgc_pg "stat -c %s '$MOD'" | tail -1)" -gt 1000 ] && echo yes || echo no)" "yes" +check "a broken module does NOT masquerade as an unsupported scheme" \ + "$(grep -c 'is not supported' <<<"$broken")" "0" +check "and the loader's own reason survives" \ + "$([ "$(grep -ci 'could not load library' <<<"$broken")" -ge 1 ] && echo yes || echo no)" "yes" + pgc_summary From 428612b64e6fb41c7b212a778a54c6d21da58902 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 05:49:02 -0600 Subject: [PATCH 05/14] test: the objstore suite must run alone, and not drop privileges to move a file CI went red on my own suite, and the premise assertion is why it was legible: FAIL premise: the module really is corrupt for this run, not merely intended to be: got [21592] want [19] Two defects, and the second is the serious one. 1. The file operations went through pgc_pg, which drops to the postgres user. In this container /usr/local/pgsql/lib happens to be writable by postgres, so it worked. In CI the module lives in a root-owned pkglibdir and postgres cannot move it, so the manipulation silently did nothing and the suite measured the installed module three times. They now run as whoever the suite runs as. 2. The suite MOVES the installed module, which every other suite in the run loads. The matrix runs PGC_JOBS suites at once, so this was mutating shared state underneath them. Nothing broke in that run, which is luck rather than safety: the failure it would cause appears in whichever suite happened to load the extension at the wrong moment, and would read as a defect there. It is now in runs_alone beside replication. Also added: if the module cannot be moved at all, the absent and broken arms SKIP visibly instead of failing. A suite that cannot perform its manipulation has not found a defect, and a red gate there is about the environment. 20 checks locally. The premise that caught this stays exactly as it was. --- test/objstore_module.sh | 31 +++++++++++++++++++++++-------- test/run_all_versions.sh | 6 ++++++ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/test/objstore_module.sh b/test/objstore_module.sh index e4fd5a3..679818d 100755 --- a/test/objstore_module.sh +++ b/test/objstore_module.sh @@ -48,6 +48,22 @@ check "a local path still works" "$(q 'SELECT count(*) FROM lp')" "3" check "a relative path is not mistaken for a URL" \ "$(psql_run "SELECT * FROM pgcolumnar.read_parquet('/nonexistent/x.parquet') AS (a int)" 2>&1 | grep -c 'No such file or directory\|could not open')" "1" +MOD="$LIBDIR/pgcolumnar_objstore.so" +# The arms below MOVE the installed module. That needs write permission on its +# directory, which the suite has when it runs as the installing user and may not +# otherwise. Skip visibly rather than fail: a suite that cannot perform its +# manipulation has not found a defect, and a red gate here would be about the +# environment. +# +# This suite also runs ALONE in the matrix, because the file it moves is shared with +# every other suite in the run. +if ! mv "$MOD" "$MOD.probe" 2>/dev/null; then + echo "SKIP cannot move $MOD, so the absent and broken paths are untested here" + pgc_summary + exit 0 +fi +mv "$MOD.probe" "$MOD" 2>/dev/null + # ---- the module ABSENT, which is a supported configuration -------------------- # # Two things this asserts, both of which were wrong before: @@ -69,17 +85,16 @@ two_reads() { env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" \ -At -q -c "$Q" -c "$Q" 2>&1 } -MOD="$LIBDIR/pgcolumnar_objstore.so" both=$(two_reads) check "with the module present, both reads report the same thing" \ "$(grep -c 'is not implemented yet\|is not supported' <<<"$both")" "2" -pgc_pg "mv '$MOD' '$MOD.away'" >/dev/null 2>&1 +mv "$MOD" "$MOD.away" 2>/dev/null absent=$(two_reads) -pgc_pg "mv '$MOD.away' '$MOD'" >/dev/null 2>&1 +mv "$MOD.away" "$MOD" 2>/dev/null check "premise: the module really was absent for that run" \ - "$(pgc_pg "test -f '$MOD' && echo restored || echo MISSING")" "restored" + "$([ -f "$MOD" ] && echo restored || echo MISSING)" "restored" check "with the module absent, neither read leaks the loader's own error" \ "$(grep -c 'could not access file' <<<"$absent")" "0" check "and both reads report the SAME unsupported error" \ @@ -99,13 +114,13 @@ check "and both reads report the SAME unsupported error" \ # errcode_for_file_access() for both the stat failure and the dlopen failure, so # missing and "file too short" both arrive as 58P01. Measured. The check is on file # presence for that reason. -pgc_pg "mv '$MOD' '$MOD.away' && printf 'not a shared object' > '$MOD'" >/dev/null 2>&1 +mv "$MOD" "$MOD.away" 2>/dev/null && printf 'not a shared object' > "$MOD" 2>/dev/null check "premise: the module really is corrupt for this run, not merely intended to be" \ - "$(pgc_pg "stat -c %s '$MOD'" | tail -1)" "19" + "$(stat -c %s "$MOD" 2>/dev/null)" "19" broken=$(two_reads) -pgc_pg "rm -f '$MOD' && mv '$MOD.away' '$MOD'" >/dev/null 2>&1 +rm -f "$MOD" 2>/dev/null; mv "$MOD.away" "$MOD" 2>/dev/null check "premise: the real module was restored afterwards" \ - "$([ "$(pgc_pg "stat -c %s '$MOD'" | tail -1)" -gt 1000 ] && echo yes || echo no)" "yes" + "$([ "$(stat -c %s "$MOD" 2>/dev/null || echo 0)" -gt 1000 ] && echo yes || echo no)" "yes" check "a broken module does NOT masquerade as an unsupported scheme" \ "$(grep -c 'is not supported' <<<"$broken")" "0" check "and the loader's own reason survives" \ diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 9d60e3d..f2602b9 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -334,6 +334,12 @@ is_timing_suite() { runs_alone() { case "$1" in replication) return 0 ;; + # objstore_module moves the INSTALLED module aside to test the + # not-installed and broken paths. That file is shared by every suite in + # the run, so doing it while others execute would break them, and the + # breakage would look like a defect in whichever suite happened to load + # the extension at the wrong moment. + objstore_module) return 0 ;; *) is_timing_suite "$1" ;; esac } From c3d730f3518006061fac5d830511e8423134ce85 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 08:22:13 -0600 Subject: [PATCH 06/14] fix: cache the loader's verdict only once init() can no longer raise (#393) Same trap as the one this branch already fixed, one step later. The flag was set before api = init() and before the ABI check. init() is a call into a separately built library: it can raise, and with the flag already set the unwind leaves the verdict cached as "tried, nothing found". The first remote read of a session then reports init()'s error and every later one reports "requires the object-store module" -- two identical queries, one session, two different errors, the plausible-looking one second. It is latent while init() only returns a pointer to a static struct. It stops being latent in the commit that gives the module something to set up, which is the next one. The flag is now set on each return path with nothing that can raise between the assignment and its return. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf --- src/columnar_objstore.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/columnar_objstore.c b/src/columnar_objstore.c index afd3182..8dd4df8 100644 --- a/src/columnar_objstore.c +++ b/src/columnar_objstore.c @@ -123,9 +123,25 @@ PgColumnarObjStoreGet(void) } PG_END_TRY(); - objstore_tried = true; + /* + * The flag is set on each return path below, never once up here, and nothing + * that can raise sits between an assignment and its return. + * + * This is the same trap as the one above, one step later. init() is a call + * into a separately built library: it can raise, and if the flag were already + * set the unwind would leave the verdict cached as "tried, nothing found", so + * the first remote read of a session would report init()'s error and every + * later one would report "requires the object-store module" — the same two + * identical queries, two different errors, the plausible one second. It is + * latent while init() only returns a pointer to a static struct. It stops + * being latent the moment the module has anything to set up, which is the + * commit after this one. + */ if (init == NULL) + { + objstore_tried = true; return NULL; + } api = init(); @@ -143,9 +159,11 @@ PgColumnarObjStoreGet(void) PGCOLUMNAR_OBJSTORE_ABI), errhint("Reinstall pgcolumnar_objstore from the same build as " "pgcolumnar."))); + objstore_tried = true; return NULL; } objstore_api = api; + objstore_tried = true; return objstore_api; } From 85ddcc34fcc467f6938968d25f7a2705e2c8849b Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 08:22:41 -0600 Subject: [PATCH 07/14] fix: exercise the byte source, and stop it lying about remote paths (#393) Three changes to the seam this branch introduced, all found by reviewing it before building the protocol on top of it. The local file path is now an implementation of the vtable rather than the fallback the dispatcher tested for. `ops` is never NULL. A sentinel left the dispatch branch executed by zero of the suite's checks, so the seam would have shipped unproven and the first thing to exercise it would have been network code. With pq_local_ops installed, every Parquet read in the suite runs through the vtable, which is what the whole commit was for. A missing module and an installed module that handles no such scheme now report separately. They ask the operator for different things -- "install a package" against "this build will never read that URL" -- and collapsing them also made the absent-module test inert: a module that handles nothing yet produced the same message as no module at all, so the arm that moved the library aside passed whether or not the move succeeded. Removing both mv lines left every check green. Measured, on this branch, before the fix. A remote path is no longer expanded against the local filesystem. pq_resolve_paths runs ahead of the byte source at every entry point, so s3://bucket/a*.parquet went to glob() locally and came back "no files match pattern" -- the filesystem-miss-for-a-remote-path report the byte source exists to remove, arriving one layer above the byte source. `*`, `?` and `[` are all legal in an S3 key, so this is an ordinary key and not a crafted one. v1 reads exact keys, so a pattern is refused rather than silently taken as a literal: expanding one needs a LIST call whose paged response is a third hand-rolled parser over input an outside party controls, which is the shape that produced #210 and #228. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf --- src/columnar_parquet_reader.c | 130 +++++++++++++++++++++++----------- 1 file changed, 90 insertions(+), 40 deletions(-) diff --git a/src/columnar_parquet_reader.c b/src/columnar_parquet_reader.c index f9a2d61..4bb684f 100644 --- a/src/columnar_parquet_reader.c +++ b/src/columnar_parquet_reader.c @@ -1408,26 +1408,27 @@ decode_plain_bools(const uint8 *buf, size_t buflen, int n, Datum *out) } /* - * An open Parquet file, read on demand. + * An open Parquet file, read on demand, behind a byte source (#393). * * Only the footer is held: `meta` is the serialized file metadata, and the * parsed PqFile's chunk statistics (PqChunk.stat_min and stat_max) point into * it, so it must outlive every consumer of those pointers, which means the whole * scan of this file. Page bytes are read as they are needed, so peak memory does * not scale with the file. - */ -/* - * A Parquet byte source (#393). * - * Every read of a Parquet file in this reader goes through pq_source_read, which - * is a positional read whose callers have already bounded the offset. That makes - * it the one seam a remote source has to replace, and the reason object storage - * touches three functions rather than the reader. + * Every one of those reads goes through pq_source_read, a positional read whose + * callers have already bounded the offset. That makes it the one seam a remote + * source has to replace, and the reason object storage touches three functions + * rather than the reader. * - * `ops` dispatches. The local implementation is the code that was here before and - * behaves identically; a remote one lives in a separate, non-preloaded module so - * that nothing an object-store client links is mapped into the postmaster. See - * PqObjStoreApi. + * `ops` dispatches, and is never NULL: the local file path is an implementation + * like any other, not a fallback the dispatcher tests for. That is deliberate. + * A sentinel would leave the dispatch branch unexecuted by every existing test + * until a remote source appeared, so the seam would ship unproven and the first + * thing to exercise it would be network code. With pq_local_ops installed, every + * Parquet read in the suite runs through the vtable. A remote implementation + * lives in a separate, non-preloaded module so that nothing an object-store + * client links is mapped into the postmaster. See PgColumnarObjStoreApi. */ typedef struct PqSource PqSource; @@ -1440,7 +1441,7 @@ typedef struct PqSourceOps struct PqSource { - const PqSourceOps *ops; /* NULL means the local file implementation */ + const PqSourceOps *ops; /* the implementation; never NULL once opened */ FILE *f; /* AllocateFile handle (buffered), local only */ void *priv; /* remote implementation's own state */ const char *path; /* for error messages; palloc'd by the caller */ @@ -1478,17 +1479,31 @@ pq_source_read_local(PqSource *src, int64 off, void *buf, size_t n) } } +static void +pq_source_close_local(PqSource *src) +{ + if (src->f != NULL) + { + FreeFile(src->f); + src->f = NULL; + } +} + /* - * The dispatcher every caller uses. Local sources keep the exact path they had - * before this existed: ops NULL means the FILE * implementation above. + * The local file implementation. Named "file" because that is what appears in an + * error message beside "s3". */ +static const PqSourceOps pq_local_ops = { + .name = "file", + .read = pq_source_read_local, + .close = pq_source_close_local, +}; + +/* The dispatchers every caller uses. */ static void pq_source_read(PqSource *src, int64 off, void *buf, size_t n) { - if (src->ops != NULL) - src->ops->read(src, off, buf, n); - else - pq_source_read_local(src, off, buf, n); + src->ops->read(src, off, buf, n); } @@ -1506,26 +1521,45 @@ pq_source_open(const char *path, PqSource *src, PqFile *pf) memset(src, 0, sizeof(*src)); src->path = path; + /* + * Set before anything below can raise, so an error path never leaves a + * source whose dispatch table is NULL. + */ + src->ops = &pq_local_ops; + /* * A remote path is not a filename (#393). Without this, AllocateFile reports * "No such file or directory" for s3://bucket/key, which is true of the * filesystem and useless to the reader. * - * The module is loaded here and only here, on the first remote read. An - * installation without it reaches the same error as an unsupported scheme, - * which is the intended behaviour rather than a degradation: local files are - * unaffected either way. + * The module is loaded here and only here, on the first remote read. + * + * The two failures below are REPORTED SEPARATELY, and that is not cosmetic. + * They ask the operator for different things: one is "install a package", + * the other is "this build will never read that URL". Collapsing them also + * made the absent-module test inert, because a module that handles nothing + * yet produces the same message as no module at all, so the arm that moves + * the library aside passed whether or not the move succeeded. */ if (PgColumnarPathIsRemote(path)) { const PgColumnarObjStoreApi *api = PgColumnarObjStoreGet(); - if (api == NULL || !api->handles_url(path)) + if (api == NULL) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("columnar: reading \"%s\" requires the object-store module", + path), + errdetail("Object storage support is a separate library, " + "pgcolumnar_objstore, which is not installed."), + errhint("Install the pgcolumnar object-store package, or " + "use a local filesystem path."))); + if (!api->handles_url(path)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("columnar: reading \"%s\" is not supported", path), - errdetail("Object storage support is not available in this " - "build."), + errdetail("The installed object-store module handles no " + "such URL scheme."), errhint("Use a local filesystem path."))); /* the remote source is wired in the next commit */ ereport(ERROR, @@ -1586,23 +1620,10 @@ pq_source_open(const char *path, PqSource *src, PqFile *pf) } } -static void -pq_source_close_local(PqSource *src) -{ - if (src->f != NULL) - { - FreeFile(src->f); - src->f = NULL; - } -} - static void pq_source_close(PqSource *src) { - if (src->ops != NULL) - src->ops->close(src); - else - pq_source_close_local(src); + src->ops->close(src); } @@ -2563,6 +2584,14 @@ pq_walk_dir(const char *path, int depth, List **files, int *skipped) * the normal "could not open file" error for a genuine typo. * An empty directory or a non-matching glob is an error: the user named a set and * meant to read something. Returns a List of palloc'd cstrings. + * + * A REMOTE path is none of those things and must not reach any of them (#393). + * This runs ahead of pq_source_open at every entry point, so without the check + * below an s3:// key containing a glob metacharacter went to glob() against the + * LOCAL filesystem and came back "no files match pattern" — the filesystem-miss + * report for a remote path that the byte source exists to eliminate, arriving one + * layer above the byte source. `*`, `?` and `[` are all legal in an S3 key, so + * this is an ordinary key, not a crafted one. */ static List * pq_resolve_paths(const char *path) @@ -2570,6 +2599,27 @@ pq_resolve_paths(const char *path) struct stat st; List *files = NIL; + if (PgColumnarPathIsRemote(path)) + { + /* + * v1 reads exact object keys. Expanding a pattern needs a LIST call, + * whose paged XML or JSON response is a third hand-rolled parser over + * input an outside party controls, which is the shape that produced #210 + * and #228. Refusing here is a decision, so it says so; treating the + * metacharacter as a literal key byte would be the silent alternative + * and would surprise anyone who typed a pattern on purpose. + */ + if (pq_has_glob_meta(path)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("columnar: cannot expand a pattern in the object-storage path \"%s\"", + path), + errdetail("Patterns are expanded on the local filesystem " + "only. Object storage is read by exact key."), + errhint("Name each object explicitly."))); + return list_make1(pstrdup(path)); + } + if (stat(path, &st) == 0 && S_ISDIR(st.st_mode)) { int skipped = 0; From f251a2986c9bae5886f6c92bbc7a7547df103287 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 08:22:41 -0600 Subject: [PATCH 08/14] test: three states, three messages, and an instrument that can fail (#393) The absent-module arm asserted nothing. Present and absent produced the same string, so all three of its checks passed with both mv lines deleted, and its premise ran after the restore and asserted the module was back -- it could only fail if the RESTORE failed. Now present, absent and broken are three distinct messages, each arm asserts its own and the absence of the other two, and the premise records the state DURING the reads. Proved by removal, in the container, on PG17: - both mv lines deleted -> 3 checks fail (premise, and both message counts) - pre-fix code + pre-fix suite + no move at all -> PASSED, 20 checks The separation check could not fail either. `readelf -d | grep -c` counts DT_NEEDED entries, and the failure mode it names -- someone adds the module's objects to the main OBJS list -- links them statically and emits no DT_NEEDED entry, so it read 0 either way. It also read 0 with readelf absent. It is now a symbol pair: the entry point must be absent from the preloaded library and present in the module, so a broken instrument fails the positive half. Proved by removal: with the entry point linked into pgcolumnar.so, the new check fails (got 1, want 0) and the old readelf check still reports 0. Also: pgc_require_tools for nm and stat; a trap and a refuse-to-start guard on the leftover .away, because an interrupt mid-move left this run's 19-byte stand-in installed as the module and the next run would move THAT to .away, overwriting the only real copy; the check named "a relative path" now passes a relative path; and check_num where the measurement is a number. 30 checks, PG17 and PG18. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf --- test/objstore_module.sh | 151 +++++++++++++++++++++++++++++++--------- 1 file changed, 119 insertions(+), 32 deletions(-) diff --git a/test/objstore_module.sh b/test/objstore_module.sh index 679818d..847070c 100755 --- a/test/objstore_module.sh +++ b/test/objstore_module.sh @@ -18,45 +18,98 @@ set -uo pipefail . "$(dirname "${BASH_SOURCE[0]}")/lib.sh" pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" +# nm reads the symbol tables the separation checks rest on; stat sizes the +# stand-in module. Without this, a missing nm makes "the symbol is absent from +# the main library" true for the wrong reason, which is the whole failure this +# suite exists to catch, turned on itself. +pgc_require_tools nm stat || { pgc_summary; exit 1; } + LIBDIR="$("$PGC_PG_CONFIG" --pkglibdir 2>/dev/null || echo "")" -[ -n "$LIBDIR" ] || LIBDIR="$(pgc_pg "pg_config --pkglibdir" | tr -d '\r')" +[ -n "$LIBDIR" ] || LIBDIR="$(pgc_pg "pg_config --pkglibdir" | tail -1 | tr -d '\r')" +MOD="$LIBDIR/pgcolumnar_objstore.so" check "the main library is installed" \ - "$(pgc_pg "test -f '$LIBDIR/pgcolumnar.so' && echo yes || echo no")" "yes" + "$(pgc_pg "test -f '$LIBDIR/pgcolumnar.so' && echo yes || echo no" | tail -1)" "yes" check "the object-store module is installed BESIDE it, not inside it" \ - "$(pgc_pg "test -f '$LIBDIR/pgcolumnar_objstore.so' && echo yes || echo no")" "yes" + "$(pgc_pg "test -f '$MOD' && echo yes || echo no" | tail -1)" "yes" -# The property the design rests on. If someone adds the module's objects to the main -# OBJS list, this is what notices. -check "the module is a separate file, so nothing it links reaches the postmaster" \ - "$(pgc_pg "readelf -d '$LIBDIR/pgcolumnar.so' | grep -c pgcolumnar_objstore" | tail -1)" "0" -check "and the module exports its single entry point" \ - "$(pgc_pg "nm -D --defined-only '$LIBDIR/pgcolumnar_objstore.so' | grep -c ' T pgcolumnar_objstore_init'")" "1" +# ---- the separation the design rests on --------------------------------------- +# +# The question is whether the module's code is INSIDE the preloaded library. The +# obvious check does not answer it: `readelf -d pgcolumnar.so | grep -c objstore` +# counts DT_NEEDED entries, and the failure mode we care about -- someone adds +# the module's objects to the main OBJS list -- links them STATICALLY and emits no +# DT_NEEDED entry at all. That check reads 0 whether or not the mistake was made. +# It also reads 0 when readelf is absent, or when the .so is not there. +# +# Ask about the symbol instead, as a PAIR. The entry point must be absent from +# the main library and present in the module. A broken or missing instrument +# fails the positive half, so the pair cannot go quiet the way a lone count of +# zero can. +check_num "the module's entry point is NOT inside the preloaded library" \ + "$(pgc_pg "nm -D --defined-only '$LIBDIR/pgcolumnar.so' | grep -c pgcolumnar_objstore_init" | tail -1)" "0" +check_num "positive control: it IS defined in the module, so nm really looked" \ + "$(pgc_pg "nm -D --defined-only '$MOD' | grep -c ' T pgcolumnar_objstore_init'" | tail -1)" "1" # A remote path must report a remote error, from the reader, without a connection. for url in "s3://bucket/key.parquet" "gs://bucket/key.parquet" "https://host/key.parquet"; do out=$(psql_run "SELECT * FROM pgcolumnar.read_parquet('$url') AS (a int)" 2>&1) check "a $(cut -d: -f1 <<<"$url") URL reports an object-storage error, not a missing file" \ - "$([ "$(grep -c 'object storage is not implemented\|is not supported' <<<"$out")" -ge 1 ] && echo yes || echo no)" "yes" - check "and does NOT report it as a missing file" \ + "$([ "$(grep -c 'object storage is not implemented\|is not supported\|requires the object-store module' <<<"$out")" -ge 1 ] && echo yes || echo no)" "yes" + check_num "and does NOT report it as a missing file" \ "$(grep -c 'No such file or directory' <<<"$out")" "0" done +# ---- a remote path must not be expanded against the local filesystem ---------- +# +# pq_resolve_paths runs AHEAD of the byte source at every entry point, so before +# #393's fix an s3:// key containing a glob metacharacter went to glob() against +# the LOCAL filesystem and came back "no files match pattern". That is exactly the +# filesystem-miss-for-a-remote-path report the byte source exists to remove, +# arriving one layer above it. `*`, `?` and `[` are all legal in an S3 key, so +# this is an ordinary key and not a crafted one. +for pat in "s3://bucket/a*.parquet" "s3://bucket/a?.parquet" "s3://bucket/a[0-9].parquet"; do + out=$(psql_run "SELECT * FROM pgcolumnar.read_parquet('$pat') AS (a int)" 2>&1) + check "a glob character in an object key is refused as a pattern, not expanded" \ + "$([ "$(grep -c 'cannot expand a pattern in the object-storage path' <<<"$out")" -ge 1 ] && echo yes || echo no)" "yes" + check_num "and it is NOT reported as a local filesystem miss" \ + "$(grep -c 'no files match pattern\|matched no regular files' <<<"$out")" "0" +done + # A local path must be entirely unaffected, which is the regression this could cause. psql_run "CREATE TABLE lp (a int) USING pgcolumnar; INSERT INTO lp VALUES (1),(2),(3);" >/dev/null 2>&1 -check "a local path still works" "$(q 'SELECT count(*) FROM lp')" "3" -check "a relative path is not mistaken for a URL" \ - "$(psql_run "SELECT * FROM pgcolumnar.read_parquet('/nonexistent/x.parquet') AS (a int)" 2>&1 | +check_num "a local path still works" "$(q 'SELECT count(*) FROM lp')" "3" +# A RELATIVE path, which is what the label says. The previous version of this +# passed an absolute one, so the case it named was never exercised. +check_num "a relative path is not mistaken for a URL" \ + "$(psql_run "SELECT * FROM pgcolumnar.read_parquet('nonexistent-dir/x.parquet') AS (a int)" 2>&1 | grep -c 'No such file or directory\|could not open')" "1" -MOD="$LIBDIR/pgcolumnar_objstore.so" -# The arms below MOVE the installed module. That needs write permission on its -# directory, which the suite has when it runs as the installing user and may not -# otherwise. Skip visibly rather than fail: a suite that cannot perform its -# manipulation has not found a defect, and a red gate here would be about the -# environment. + +# ---- everything below MOVES the installed module ------------------------------ +# +# That needs write permission on its directory, which the suite has when it runs as +# the installing user and may not otherwise. Skip visibly rather than fail: a suite +# that cannot perform its manipulation has not found a defect, and a red gate here +# would be about the environment. # # This suite also runs ALONE in the matrix, because the file it moves is shared with # every other suite in the run. +# +# Refuse to start on a leftover .away. An interrupt between the move and the restore +# leaves this run's STAND-IN installed as the module and the real one parked at +# .away; a second run would then move the stand-in to .away, overwriting the only +# real copy with 19 bytes of garbage. That destroys the installation, and running +# alone does not prevent it because the two runs are sequential. +if [ -e "$MOD.away" ]; then + echo "FAIL $MOD.away exists, so an earlier run was interrupted mid-move." + echo " Restore it by hand: mv '$MOD.away' '$MOD'" + echo " Continuing would overwrite the real module with this run's stand-in." + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAIL=1 + pgc_summary + exit 1 +fi + if ! mv "$MOD" "$MOD.probe" 2>/dev/null; then echo "SKIP cannot move $MOD, so the absent and broken paths are untested here" pgc_summary @@ -64,11 +117,31 @@ if ! mv "$MOD" "$MOD.probe" 2>/dev/null; then fi mv "$MOD.probe" "$MOD" 2>/dev/null -# ---- the module ABSENT, which is a supported configuration -------------------- +# The module is moved aside twice below. Put it back on ANY exit, including the +# interrupt that leaves the state the guard above refuses to start on. +restore_module() { + [ -e "$MOD.away" ] || return 0 + rm -f "$MOD" + mv "$MOD.away" "$MOD" +} +trap restore_module EXIT INT TERM + +# ---- present, absent, and broken must be three DIFFERENT reports --------------- +# +# Three states, three messages, and the checks below are written so that each one +# fails if the state it names is not the state that occurred. +# +# The earlier version could not do that. A module that handles no scheme yet and a +# module that is not installed at all both produced "is not supported", so the +# absent arm passed whether or not the mv succeeded: deleting both mv lines left +# every check green. That is why the loader now names the missing module +# separately -- it is a real distinction for the operator ("install a package" +# against "this build will never read that URL"), and it is what makes this arm +# able to fail. # -# Two things this asserts, both of which were wrong before: +# What is still asserted here from before: # -# 1 a missing module must report the remote path as unsupported, not +# 1 a missing module must report the remote path against the missing module, not # 'could not access file "pgcolumnar_objstore"'. signalNotFound = false # suppresses a missing SYMBOL; a missing LIBRARY is raised earlier by # internal_load_library and needs a PG_TRY. @@ -87,18 +160,30 @@ two_reads() { } both=$(two_reads) -check "with the module present, both reads report the same thing" \ - "$(grep -c 'is not implemented yet\|is not supported' <<<"$both")" "2" +check_num "with the module present, both reads report an unhandled scheme" \ + "$(grep -c 'is not supported' <<<"$both")" "2" +check_num "and neither claims the module is missing" \ + "$(grep -c 'requires the object-store module' <<<"$both")" "0" +# ---- the module ABSENT, which is a supported configuration -------------------- mv "$MOD" "$MOD.away" 2>/dev/null +# Recorded DURING the run, not after the restore. The previous premise ran after +# the module was back and asserted it was present, so it could only fail if the +# RESTORE failed -- it said nothing about the state the reads actually saw. +absent_state=$([ -e "$MOD" ] && echo present || echo absent) absent=$(two_reads) mv "$MOD.away" "$MOD" 2>/dev/null -check "premise: the module really was absent for that run" \ - "$([ -f "$MOD" ] && echo restored || echo MISSING)" "restored" -check "with the module absent, neither read leaks the loader's own error" \ + +check "premise: the module really was absent while those two reads ran" \ + "$absent_state" "absent" +check "premise: and the real module is back afterwards" \ + "$([ -e "$MOD" ] && echo yes || echo no)" "yes" +check_num "with the module absent, neither read leaks the loader's own error" \ "$(grep -c 'could not access file' <<<"$absent")" "0" -check "and both reads report the SAME unsupported error" \ - "$(grep -c 'is not supported' <<<"$absent")" "2" +check_num "both reads name the missing module, identically" \ + "$(grep -c 'requires the object-store module' <<<"$absent")" "2" +check_num "and neither downgrades it to an unhandled scheme" \ + "$(grep -c 'is not supported' <<<"$absent")" "0" # ---- installed but BROKEN is not the same as not installed -------------------- # @@ -115,14 +200,16 @@ check "and both reads report the SAME unsupported error" \ # missing and "file too short" both arrive as 58P01. Measured. The check is on file # presence for that reason. mv "$MOD" "$MOD.away" 2>/dev/null && printf 'not a shared object' > "$MOD" 2>/dev/null -check "premise: the module really is corrupt for this run, not merely intended to be" \ +check_num "premise: the module really is corrupt for this run, not merely intended to be" \ "$(stat -c %s "$MOD" 2>/dev/null)" "19" broken=$(two_reads) rm -f "$MOD" 2>/dev/null; mv "$MOD.away" "$MOD" 2>/dev/null check "premise: the real module was restored afterwards" \ "$([ "$(stat -c %s "$MOD" 2>/dev/null || echo 0)" -gt 1000 ] && echo yes || echo no)" "yes" -check "a broken module does NOT masquerade as an unsupported scheme" \ +check_num "a broken module does NOT masquerade as an unhandled scheme" \ "$(grep -c 'is not supported' <<<"$broken")" "0" +check_num "nor as a module that was never installed" \ + "$(grep -c 'requires the object-store module' <<<"$broken")" "0" check "and the loader's own reason survives" \ "$([ "$(grep -ci 'could not load library' <<<"$broken")" -ge 1 ] && echo yes || echo no)" "yes" From df6a73102f6009c2a86de97f1d0bf667c3ff928f Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 08:22:42 -0600 Subject: [PATCH 09/14] docs: record the object-store module and the seam it sits behind (#393) The module map gained no entry for the second shared library. Adds columnar_objstore.c: why object storage is a separate non-preloaded library (30 shared objects against this extension's 4, and a TLS stack in a postmaster that may have none), the frozen ABI, the two load-bearing ordering constraints in the loader, why missing and corrupt cannot be told apart by SQLSTATE, and that v1 reads exact object keys. Notes on the Parquet reader that the byte source dispatches through a vtable that is never null. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf --- docs/ARCHITECTURE.md | 50 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 8804d3a..03ef143 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -320,6 +320,56 @@ outside the buffer, an incorrect value, or work without a bound. Each of those guards has a crafted-file test verified to fail when that specific guard is removed; `test/mutate_guard.py` is the harness that does the removing. +Every read of a Parquet file goes through one positional read, `pq_source_read`, +whose callers have already bounded the offset. That is the seam a non-local byte +source replaces, and it is why reading from object storage touches three +functions rather than the reader. The seam dispatches through a vtable that is +never null: the local file path is an implementation like any other, so the +dispatch is exercised by every Parquet test in the suite rather than only by the +first remote source to arrive. + +### columnar_objstore.c +The loader for the object-store module, and the only file in the main library +that knows the module exists. Object storage lives in a **separate, non-preloaded +shared library**, `pgcolumnar_objstore`, built from `objstore/`. + +The reason is measured rather than stylistic. pgColumnar loads through +`shared_preload_libraries`, so everything it links is mapped into the postmaster +and inherited by every backend through `fork`, whether or not any query ever +reads a remote file. libcurl alone resolves to 30 shared objects against this +extension's 4, including two TLS implementations, and an OpenSSL-linked client +would bring a TLS stack into a postmaster that may have none. PostgreSQL made the +same call for its own libcurl dependency: configure default off, a separate +library, runtime `dlopen` behind a frozen ABI, and a separate package. + +The module is loaded with `load_external_function` on the first read of a remote +path and never before. A build or an install without it is fully functional for +local files; a remote path then reports that the module is not installed, which +is a different report from a scheme the installed module does not handle, because +those ask the operator for different things. + +The ABI is frozen in `columnar_objstore.h` and version-checked on load. A +mismatch is refused rather than called through, since a stale module and a new +main library agree on the symbol name and disagree on the struct. + +Two ordering constraints in the loader are load-bearing and are each covered by a +test. A missing library is raised by `internal_load_library` before the symbol +lookup, so `signalNotFound = false` never gets a say and the miss needs a +`PG_TRY`. And "already tried" is recorded only after the attempt completes, with +nothing that can raise between the assignment and its return: setting it first +lets an `ereport` unwind past the assignment while the static keeps its new value, +so the first remote read of a session reports one error and every later one +reports a different, more plausible-looking one. + +Missing and corrupt cannot be told apart by SQLSTATE: `internal_load_library` +uses `errcode_for_file_access()` for both the `stat` failure and the `dlopen` +failure, so both arrive as 58P01. The discriminator is file presence. + +Object storage reads **exact object keys**. A glob metacharacter in a remote path +is refused rather than expanded, because expanding one needs a LIST call whose +paged response would be a third hand-rolled parser over input an outside party +controls. + ### columnar_visibilitymap.c Index-only-scan support (gap 28). A columnar visibility-map fork records which synthetic blocks (chunk groups) are all-visible. Lazy `VACUUM` From 8690648f9e42e1d401197743ff538be40d52ccf4 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 08:39:36 -0600 Subject: [PATCH 10/14] docs: keep the object-store section inside the measurable STE rules (#393) test/docs_style.sh caught 9 over-long sentences and one banned idiom in the section added by the previous commit. ARCHITECTURE.md was clean on main, so every violation was mine. The limit is 25 words per sentence, and "hand-rolled" is on the idiom list. Split the long sentences rather than compressing them, and said "a third parser written here" for what the idiom was doing. No content removed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf --- docs/ARCHITECTURE.md | 66 ++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 03ef143..364f0b3 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -320,12 +320,14 @@ outside the buffer, an incorrect value, or work without a bound. Each of those guards has a crafted-file test verified to fail when that specific guard is removed; `test/mutate_guard.py` is the harness that does the removing. -Every read of a Parquet file goes through one positional read, `pq_source_read`, -whose callers have already bounded the offset. That is the seam a non-local byte -source replaces, and it is why reading from object storage touches three -functions rather than the reader. The seam dispatches through a vtable that is -never null: the local file path is an implementation like any other, so the -dispatch is exercised by every Parquet test in the suite rather than only by the +Every read of a Parquet file goes through one positional read, `pq_source_read`. +Its callers have already bounded the offset. That is the seam a non-local byte +source replaces, and it is why object storage touches three functions rather +than the reader. + +The seam dispatches through a vtable that is never null. The local file path is +an implementation like any other, not a case the dispatcher tests for. Every +Parquet test in the suite therefore exercises the dispatch, rather than only the first remote source to arrive. ### columnar_objstore.c @@ -334,40 +336,44 @@ that knows the module exists. Object storage lives in a **separate, non-preloade shared library**, `pgcolumnar_objstore`, built from `objstore/`. The reason is measured rather than stylistic. pgColumnar loads through -`shared_preload_libraries`, so everything it links is mapped into the postmaster -and inherited by every backend through `fork`, whether or not any query ever -reads a remote file. libcurl alone resolves to 30 shared objects against this -extension's 4, including two TLS implementations, and an OpenSSL-linked client -would bring a TLS stack into a postmaster that may have none. PostgreSQL made the -same call for its own libcurl dependency: configure default off, a separate -library, runtime `dlopen` behind a frozen ABI, and a separate package. +`shared_preload_libraries`. Everything it links is therefore mapped into the +postmaster and inherited by every backend through `fork`, whether or not any +query reads a remote file. libcurl alone resolves to 30 shared objects against +this extension's 4, including two TLS implementations. An OpenSSL-linked client +would bring a TLS stack into a postmaster that may have none. + +PostgreSQL made the same call for its own libcurl dependency. That shape is +configure default off, a separate library, runtime `dlopen` behind a frozen ABI, +and a separate package. The module is loaded with `load_external_function` on the first read of a remote path and never before. A build or an install without it is fully functional for -local files; a remote path then reports that the module is not installed, which +local files. A remote path then reports that the module is not installed. That is a different report from a scheme the installed module does not handle, because -those ask the operator for different things. +the two ask the operator for different things. The ABI is frozen in `columnar_objstore.h` and version-checked on load. A -mismatch is refused rather than called through, since a stale module and a new -main library agree on the symbol name and disagree on the struct. - -Two ordering constraints in the loader are load-bearing and are each covered by a -test. A missing library is raised by `internal_load_library` before the symbol -lookup, so `signalNotFound = false` never gets a say and the miss needs a -`PG_TRY`. And "already tried" is recorded only after the attempt completes, with -nothing that can raise between the assignment and its return: setting it first -lets an `ereport` unwind past the assignment while the static keeps its new value, -so the first remote read of a session reports one error and every later one -reports a different, more plausible-looking one. - -Missing and corrupt cannot be told apart by SQLSTATE: `internal_load_library` +mismatch is refused rather than called through. A stale module and a new main +library agree on the symbol name and disagree on the struct. + +Two ordering constraints in the loader are load-bearing, and each has a test. + +A missing library is raised by `internal_load_library` before the symbol lookup. +So `signalNotFound = false` never gets a say, and the miss needs a `PG_TRY`. + +"Already tried" is recorded only after the attempt completes, with nothing that +can raise between the assignment and its return. Setting it first lets an +`ereport` unwind past the assignment while the static keeps its new value. The +first remote read of a session then reports one error, and every later one +reports a different and more plausible error. + +Missing and corrupt cannot be told apart by SQLSTATE. `internal_load_library` uses `errcode_for_file_access()` for both the `stat` failure and the `dlopen` failure, so both arrive as 58P01. The discriminator is file presence. Object storage reads **exact object keys**. A glob metacharacter in a remote path -is refused rather than expanded, because expanding one needs a LIST call whose -paged response would be a third hand-rolled parser over input an outside party +is refused rather than expanded. Expanding one needs a LIST call, and its paged +response would be a third parser written here for input an outside party controls. ### columnar_visibilitymap.c From 364ec84c517f5288c027f2dcccc3d1cb8a771912 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 09:09:58 -0600 Subject: [PATCH 11/14] test: arm the restore trap before the probe, which is itself a move (#393) CJD's residual finding, two syscalls wide and correct. The writability probe moves the module, and it sat above the trap: if ! mv "$MOD" "$MOD.probe" ... # move 3, unprotected ... trap restore_module EXIT INT TERM # armed here An interrupt in that window left the real module at $MOD.probe with nothing at $MOD. restore_module knew only .away, and so did the start guard, so the next run sailed past the guard, found no module to move, and reported SKIP -- on an installation that was itself broken and stayed broken until somebody noticed the .probe file. A suite reporting "cannot test the absent path" while being the reason the module is absent. Same shape as the defect this suite exists to catch, one move earlier, and my own comment said "moved aside twice below" while the probe was the third. The trap is armed before the probe and handles both suffixes; the start guard checks both. Verified: normal run 30 checks green, and a planted leftover .probe refuses to start, names the file, and prints the recovery command instead of taking the SKIP branch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf --- test/objstore_module.sh | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/test/objstore_module.sh b/test/objstore_module.sh index 847070c..5131c3f 100755 --- a/test/objstore_module.sh +++ b/test/objstore_module.sh @@ -100,15 +100,33 @@ check_num "a relative path is not mistaken for a URL" \ # .away; a second run would then move the stand-in to .away, overwriting the only # real copy with 19 bytes of garbage. That destroys the installation, and running # alone does not prevent it because the two runs are sequential. -if [ -e "$MOD.away" ]; then - echo "FAIL $MOD.away exists, so an earlier run was interrupted mid-move." - echo " Restore it by hand: mv '$MOD.away' '$MOD'" +for stash in "$MOD.away" "$MOD.probe"; do + [ -e "$stash" ] || continue + echo "FAIL $stash exists, so an earlier run was interrupted mid-move." + echo " Restore it by hand: mv '$stash' '$MOD'" echo " Continuing would overwrite the real module with this run's stand-in." PGC_CHECKS=$((PGC_CHECKS + 1)) PGC_FAIL=1 pgc_summary exit 1 -fi +done + +# Armed BEFORE the writability probe below, which is itself a move. The first +# version armed it after, leaving a two-syscall window in which an interrupt left +# the real module at $MOD.probe with nothing at $MOD: restore_module knew only +# .away, so the next run sailed past the start guard, found no module to move, +# and reported SKIP on an installation that was itself broken and stayed broken +# until somebody noticed the .probe file. Same shape as the defect this suite was +# written to catch, one move earlier. Both suffixes are handled here and above. +restore_module() { + local stash + for stash in "$MOD.away" "$MOD.probe"; do + [ -e "$stash" ] || continue + rm -f "$MOD" + mv "$stash" "$MOD" + done +} +trap restore_module EXIT INT TERM if ! mv "$MOD" "$MOD.probe" 2>/dev/null; then echo "SKIP cannot move $MOD, so the absent and broken paths are untested here" @@ -117,15 +135,6 @@ if ! mv "$MOD" "$MOD.probe" 2>/dev/null; then fi mv "$MOD.probe" "$MOD" 2>/dev/null -# The module is moved aside twice below. Put it back on ANY exit, including the -# interrupt that leaves the state the guard above refuses to start on. -restore_module() { - [ -e "$MOD.away" ] || return 0 - rm -f "$MOD" - mv "$MOD.away" "$MOD" -} -trap restore_module EXIT INT TERM - # ---- present, absent, and broken must be three DIFFERENT reports --------------- # # Three states, three messages, and the checks below are written so that each one From 717f3e823a42ad0db9ec56d978849585a2a45b42 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 18:16:28 -0600 Subject: [PATCH 12/14] test: assert the objstore suite recovers from a stash an interrupt left (#393) objstore_module.sh moves the installed module aside to reach the absent and the broken paths, and an interrupt between a move and its restore leaves a .probe or .away in pkglibdir. The start guard then refuses to run. Refusing forever is the defect. Every matrix leg runs `make install`, so the ordinary leftover is debris beside a module that is already fine, and the suite stays red on that major until somebody moves a file by hand. On 2026-08-06 a .probe from 15:09 turned PG17 red in a five-major matrix while the other 119 suites passed, and the run that dropped it had been interrupted hours earlier. Three states, arranged and then handed to the suite as a black box: stash beside a valid module debris; must be cleared, run must proceed stash with nothing installed the only surviving copy; must be restored stash with a 19-byte stand-in the only surviving copy, under a stand-in The third is why the discriminator has to be validity rather than presence. An interrupt inside the broken-module arm leaves 19 bytes of text installed AS the module with the real one at .away, and a rule written on presence deletes the real module there while looking correct in every other arrangement. Verified by removal: with the discriminator reduced to `return 0`, "the real module survived, rather than being deleted with the stash" fails. The suite asserts only on the other suite's exit status and on what it left in pkglibdir, so the guard can be rewritten without touching this file. Its safety net restores from a pristine copy taken before anything moves, deliberately not from the rule under test, so a failing run cannot leave the box in the state this exists to recover from. Registered to run ALONE, for the same reason objstore_module does: the file it arranges is shared with every other suite in the run. Membership verified by sourcing the SUITES array, not by reading the diff. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky --- test/objstore_stash_recovery.sh | 165 ++++++++++++++++++++++++++++++++ test/run_all_versions.sh | 6 +- 2 files changed, 170 insertions(+), 1 deletion(-) create mode 100755 test/objstore_stash_recovery.sh diff --git a/test/objstore_stash_recovery.sh b/test/objstore_stash_recovery.sh new file mode 100755 index 0000000..51c583f --- /dev/null +++ b/test/objstore_stash_recovery.sh @@ -0,0 +1,165 @@ +#!/usr/bin/env bash +# +# Recovery from a stash left behind by an interrupted objstore_module run (#393). +# +# objstore_module.sh moves the installed module aside to reach the absent and the +# broken paths. An interrupt between a move and its restore leaves a +# pgcolumnar_objstore.so.probe or .away in pkglibdir, and that suite's start guard +# then refuses to run. +# +# Refusing is right when the stash is the only surviving copy of the module: +# continuing would overwrite it with the run's 19-byte stand-in and destroy the +# installation. Refusing FOREVER is not. Every matrix leg runs `make install`, so +# the ordinary leftover is debris sitting beside a perfectly good module, and the +# suite stays red on that major until a person notices and moves a file by hand. +# +# It is not hypothetical. On 2026-08-06 a .probe from 15:09 turned PG17 red in a +# five-major matrix while 119 other suites passed, and the run before it had been +# interrupted hours earlier. The guard's own advice -- `mv "$stash" "$MOD"` -- was +# by then wrong as well: make install had already restored the module, so +# following it would have replaced a freshly built module with a stale one. +# +# What separates the two cases is not whether a stash is PRESENT. It is whether +# the module beside it is VALID. Presence alone cannot tell debris from the only +# real copy, and an interrupt inside objstore_module's broken-module arm leaves a +# 19-byte stand-in installed with the real module at .away -- deleting the stash +# there is the destructive move the guard exists to prevent. +# +# Tested at the only seam that matters: run the suite, then look at its exit +# status and at what it left in pkglibdir. Nothing here reaches inside the guard, +# so the guard can be rewritten without touching this file. +# +# Usage: test/objstore_stash_recovery.sh [PG_CONFIG] +# Written fresh for pgColumnar. +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +PG_CONFIG="${1:-/usr/local/pg17/bin/pg_config}" +SRCDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +SUITE="$SRCDIR/test/objstore_module.sh" + +# nm decides "valid module" for every check below. Without it the discriminator +# silently answers "not valid" for everything, which would make the destructive +# branch look correct. +pgc_require_tools nm || { pgc_summary; exit 1; } + +echo "== pgColumnar test: $(basename "$0") ==" +echo "PG_CONFIG=$PG_CONFIG" + +# No cluster of our own: this suite asserts on files and on another suite's exit +# status, and objstore_module.sh stands up its own. It does need the module +# installed, so build once here and let each invocation skip it. +if [ -z "${PGC_SKIP_BUILD:-}" ]; then + echo "-- building" + make -C "$SRCDIR" PG_CONFIG="$PG_CONFIG" >/dev/null || { + echo "FAIL build failed, so nothing below measures the guard" + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAIL=1; pgc_summary + } + echo "-- installing" + make -C "$SRCDIR" install PG_CONFIG="$PG_CONFIG" >/dev/null || { + echo "FAIL install failed, so nothing below measures the guard" + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAIL=1; pgc_summary + } +fi + +LIBDIR="$("$PG_CONFIG" --pkglibdir)" +MOD="$LIBDIR/pgcolumnar_objstore.so" + +# The discriminator under test, computed here independently of the suite's copy +# of it. A module is valid when it defines the entry point, which is the same +# question objstore_module.sh asks with nm at its positive control. +mod_is_valid() { + [ -e "$1" ] || return 1 + [ "$(nm -D --defined-only "$1" 2>/dev/null | grep -c ' T pgcolumnar_objstore_init')" -ge 1 ] +} + +# Safety net, and deliberately NOT the logic under test: a pristine copy taken +# before anything moves. If the fix is wrong and leaves the installation broken, +# this restores it from that copy rather than from a rule this suite is asserting. +# Without it, a failing run of this suite leaves the box in exactly the state it +# exists to recover from, and poisons every later run on this major. +SAFE="" +cleanup() { + rm -f "$MOD.probe" "$MOD.away" + if [ -n "$SAFE" ] && [ -e "$SAFE" ]; then + mod_is_valid "$MOD" || cp -p "$SAFE" "$MOD" + rm -f "$SAFE" + fi +} +trap cleanup EXIT INT TERM + +if ! mod_is_valid "$MOD"; then + echo "SKIP no valid module installed at $MOD, so there is no state to arrange" + pgc_summary +fi +SAFE="$(mktemp /tmp/pgc-objstore-safe.XXXXXX)" +cp -p "$MOD" "$SAFE" + +# ---- debris beside a good installation must not stop the suite ---------------- +# +# The PG17 case, exactly: a stash left by an interrupted run, and a valid module +# reinstalled beside it by the next `make install`. +check "premise: the installed module is valid before this slice arranges anything" \ + "$(mod_is_valid "$MOD" && echo yes || echo no)" "yes" + +cp -p "$MOD" "$MOD.probe" +check "premise: a debris stash really is in place for the run below" \ + "$([ -e "$MOD.probe" ] && echo yes || echo no)" "yes" + +out="$(PGC_SKIP_BUILD=1 PGC_PORT="$(pgc_pick_port)" bash "$SUITE" "$PG_CONFIG" 2>&1)" +rc=$? + +check_num "the suite runs to completion with debris beside a valid module" "$rc" "0" +check_num "and it did not stop at the start guard" \ + "$(grep -c 'interrupted mid-move' <<<"$out")" "0" +check "the debris is gone afterwards" \ + "$([ -e "$MOD.probe" ] && echo present || echo absent)" "absent" +check "and the module left installed is still valid" \ + "$(mod_is_valid "$MOD" && echo yes || echo no)" "yes" + +# ---- a stash that is the only surviving copy must be restored, not refused ---- +# +# The interrupted state itself: the module moved aside and nothing put back. The +# suite must not proceed past this by treating the stash as debris -- there is no +# module to fall back on -- and it must not leave the box needing a human either. +mv "$MOD" "$MOD.away" +check "premise: nothing is installed at the module's path for this slice" \ + "$([ -e "$MOD" ] && echo present || echo absent)" "absent" +check "premise: and the stash beside it is the real module" \ + "$(mod_is_valid "$MOD.away" && echo yes || echo no)" "yes" + +out="$(PGC_SKIP_BUILD=1 PGC_PORT="$(pgc_pick_port)" bash "$SUITE" "$PG_CONFIG" 2>&1)" +rc=$? + +check_num "the suite recovers the only surviving copy and runs to completion" "$rc" "0" +check "the module is installed again afterwards, and valid" \ + "$(mod_is_valid "$MOD" && echo yes || echo no)" "yes" +check "and the stash is not left behind for the next run to trip on" \ + "$([ -e "$MOD.away" ] && echo present || echo absent)" "absent" + +# ---- a stand-in installed over the real module is the destructive case -------- +# +# An interrupt inside objstore_module's broken-module arm leaves this exact state: +# 19 bytes of text installed AS the module, with the real one at .away. A stash +# rule written on presence alone deletes .away here and destroys the installation, +# and it looks correct in every other arrangement. This is the arm that says the +# discriminator has to be validity. +mv "$MOD" "$MOD.away" +printf 'not a shared object' > "$MOD" +check_num "premise: a 19-byte stand-in really is installed as the module" \ + "$(stat -c %s "$MOD" 2>/dev/null)" "19" +check "premise: which is not a module" \ + "$(mod_is_valid "$MOD" && echo yes || echo no)" "no" +check "premise: and the real module is the thing parked at the stash" \ + "$(mod_is_valid "$MOD.away" && echo yes || echo no)" "yes" + +out="$(PGC_SKIP_BUILD=1 PGC_PORT="$(pgc_pick_port)" bash "$SUITE" "$PG_CONFIG" 2>&1)" +rc=$? + +check_num "the suite replaces the stand-in with the real module and completes" "$rc" "0" +check "the real module survived, rather than being deleted with the stash" \ + "$(mod_is_valid "$MOD" && echo yes || echo no)" "yes" +check "and the stash is cleared" \ + "$([ -e "$MOD.away" ] && echo present || echo absent)" "absent" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index d891129..bd607d6 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -208,7 +208,7 @@ SRCDIR="${PGC_RUN_SRCDIR:-$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)}" SUITES=(harness_selftest docs_style smoke phase2 phase3 phase4 phase5 phase6 audit concurrency unique_conc \ differential recovery replication native_backend_crash fuzz fuzz_parquet fuzz_arrow hardening concurrent_diff parallel sorted_projection \ arrow_export parquet_export read_stream corruption \ - generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_fastdecode native_zonemap write_minmax_fastpath write_fsst_compressed fsst_margin encode_invariants encode_effort native_skip pushdown_report native_agg native_agg_deletes native_agg_addcolumn native_groupagg ungrouped_vector_agg parallel_vector_agg native_bloom bloom_setting bloom_lazy native_vecskip native_index native_index_projection native_fetch_position native_dml alter_column_type native_ios native_projection native_cluster pg19_vacuum_options native_repack native_compact native_recluster recluster_extent native_vacuum_race native_sort_by sort_status native_reclaim native_ownership drop_cleanup pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_format native_truncate native_rewrite native_rewrite_conc rewrite_group_scan native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening server_file_privilege native_parquet_stack native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel cancel_decode wal_envelope decode_interrupts import_exclusion import_deferred parallel_copy parallel_export_parquet fk_referencing row_triggers native_lazy_slot native_ctas native_fetch_cache native_fetch_interrupt analyze_stats analyze_reltuples native_fetch_projection column_projection advisory_lock_class logical_subscriber objstore_module isolation) + generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_fastdecode native_zonemap write_minmax_fastpath write_fsst_compressed fsst_margin encode_invariants encode_effort native_skip pushdown_report native_agg native_agg_deletes native_agg_addcolumn native_groupagg ungrouped_vector_agg parallel_vector_agg native_bloom bloom_setting bloom_lazy native_vecskip native_index native_index_projection native_fetch_position native_dml alter_column_type native_ios native_projection native_cluster pg19_vacuum_options native_repack native_compact native_recluster recluster_extent native_vacuum_race native_sort_by sort_status native_reclaim native_ownership drop_cleanup pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_format native_truncate native_rewrite native_rewrite_conc rewrite_group_scan native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening server_file_privilege native_parquet_stack native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel cancel_decode wal_envelope decode_interrupts import_exclusion import_deferred parallel_copy parallel_export_parquet fk_referencing row_triggers native_lazy_slot native_ctas native_fetch_cache native_fetch_interrupt analyze_stats analyze_reltuples native_fetch_projection column_projection advisory_lock_class logical_subscriber objstore_module objstore_stash_recovery isolation) # Default matrix: one assert-enabled pg_config per major, 15 through 19. DEFAULT_CONFIGS=( @@ -340,6 +340,10 @@ runs_alone() { # breakage would look like a defect in whichever suite happened to load # the extension at the wrong moment. objstore_module) return 0 ;; + # objstore_stash_recovery arranges that same shared file into the states + # an interrupted run leaves behind, and runs objstore_module against each. + # It moves the module for the same reason and must be alone for it. + objstore_stash_recovery) return 0 ;; *) is_timing_suite "$1" ;; esac } From 0ef135b44b80a1f16882c00ed6ab874cdd8290ce Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 18:16:42 -0600 Subject: [PATCH 13/14] fix: recover from a leftover stash instead of refusing forever (#393) The start guard refused to run whenever a .probe or .away existed. That is right when the stash is the only surviving copy of the module, and wrong the rest of the time: `make install` puts a good module back on every matrix leg, so the usual leftover is debris, and the suite stayed red on that major until a person intervened. The advice it printed -- `mv "$stash" "$MOD"` -- had by then become wrong as well, because following it replaces a freshly built module with a stale one. Decide on the module BESIDE the stash, not on the stash. Valid module means the stash is debris: remove it and carry on. Nothing valid installed means the stash is all there is: restore it and carry on. nm is already required by this suite, so the discriminator cannot go quiet the way a bare -e test can, and if the restore produces something that is not a module either, that stops the run with the file named rather than letting every check below report the confusing half of the truth. The recovery moved ahead of the "is the module installed" checks. It ran after them, so a run that recovered correctly still failed on having observed the absent module first. The new suite caught that; the ordering is not incidental. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky --- test/objstore_module.sh | 66 +++++++++++++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 15 deletions(-) diff --git a/test/objstore_module.sh b/test/objstore_module.sh index 5131c3f..a5261a7 100755 --- a/test/objstore_module.sh +++ b/test/objstore_module.sh @@ -28,6 +28,57 @@ LIBDIR="$("$PGC_PG_CONFIG" --pkglibdir 2>/dev/null || echo "")" [ -n "$LIBDIR" ] || LIBDIR="$(pgc_pg "pg_config --pkglibdir" | tail -1 | tr -d '\r')" MOD="$LIBDIR/pgcolumnar_objstore.so" +# Recover from a stash left by an interrupted run, and decide which kind it is. +# +# An interrupt between a move and its restore leaves a .probe or .away behind. The +# dangerous case is real: inside the broken-module arm below, this run's STAND-IN +# is installed as the module and the real one is parked at .away, so a later run +# that moved the stand-in aside would overwrite the only real copy with 19 bytes +# of garbage and destroy the installation. Running alone does not prevent it, +# because the two runs are sequential. +# +# But refusing on PRESENCE alone refuses forever. Every matrix leg runs +# `make install`, so the ordinary leftover is debris sitting beside a module that +# is already fine, and this suite then stays red on that major until somebody +# moves a file by hand. On 2026-08-06 a .probe from 15:09 did exactly that to PG17 +# in a five-major matrix, while the other 119 suites passed. +# +# What tells the two apart is the module BESIDE the stash, not the stash. nm is +# required above, so this discriminator cannot go quiet the way a bare -e test can. +stash_is_debris() { + [ -e "$MOD" ] || return 1 + [ "$(nm -D --defined-only "$MOD" 2>/dev/null | + grep -c ' T pgcolumnar_objstore_init')" -ge 1 ] +} +for stash in "$MOD.away" "$MOD.probe"; do + [ -e "$stash" ] || continue + if stash_is_debris; then + echo "NOTE $stash was left by an interrupted run. The installed module is" + echo " valid, so the stash is debris; removing it and continuing." + rm -f "$stash" + continue + fi + # Nothing valid is installed, so this stash is the only surviving copy. Put it + # back. The old advice was to do this by hand, which is why an interrupt on one + # run reddened every later run on that major until somebody read the message. + echo "NOTE $stash was left by an interrupted run, and the module beside it is" + echo " missing or not a module, so the stash is the only surviving copy." + echo " Restoring it and continuing." + rm -f "$MOD" + mv "$stash" "$MOD" + # Restoring garbage is not recovery. If the stash was not a module either, + # every check below would run against a broken installation and report the + # confusing half of the truth, so stop here and say which file to look at. + if ! stash_is_debris; then + echo "FAIL restored $stash to $MOD, but that is not a module either." + echo " This installation needs 'make install' before the suite can run." + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAIL=1 + pgc_summary + exit 1 + fi +done + check "the main library is installed" \ "$(pgc_pg "test -f '$LIBDIR/pgcolumnar.so' && echo yes || echo no" | tail -1)" "yes" check "the object-store module is installed BESIDE it, not inside it" \ @@ -95,21 +146,6 @@ check_num "a relative path is not mistaken for a URL" \ # This suite also runs ALONE in the matrix, because the file it moves is shared with # every other suite in the run. # -# Refuse to start on a leftover .away. An interrupt between the move and the restore -# leaves this run's STAND-IN installed as the module and the real one parked at -# .away; a second run would then move the stand-in to .away, overwriting the only -# real copy with 19 bytes of garbage. That destroys the installation, and running -# alone does not prevent it because the two runs are sequential. -for stash in "$MOD.away" "$MOD.probe"; do - [ -e "$stash" ] || continue - echo "FAIL $stash exists, so an earlier run was interrupted mid-move." - echo " Restore it by hand: mv '$stash' '$MOD'" - echo " Continuing would overwrite the real module with this run's stand-in." - PGC_CHECKS=$((PGC_CHECKS + 1)) - PGC_FAIL=1 - pgc_summary - exit 1 -done # Armed BEFORE the writability probe below, which is itself a move. The first # version armed it after, leaving a two-syscall window in which an interrupt left From bc4f89d537a697a70edae13e8aa02c776baa51d7 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 18:38:18 -0600 Subject: [PATCH 14/14] fix: stop the cluster as well as restoring the module (#393) pgc_setup installs `trap pgc_teardown EXIT`. This suite then ran trap restore_module EXIT INT TERM which REPLACES it rather than adding to it, so the module was put back and the cluster was never stopped. Every run left a live postmaster holding a port and a workdir behind it. Measured, from a reaped-to-zero box: one run of objstore_module leaves 1 orphan before this change and 0 after, and objstore_stash_recovery -- which runs it three times -- leaves 3 and 0. A full five-major matrix plus one gate had left 37 orphaned postmasters, 32 of them from this family. That is not a tidiness problem. The port band is finite, and a suite that cannot get a port fails after 8 start attempts with "could not create any TCP/IP sockets", which is indistinguishable from a real failure. It cost two majors of a gate in this session before the cause was found. replication.sh already had the right shape (`sb_teardown() { sb_stop; rs_stop; pgc_teardown; }`); this now matches it. Audited the other suites that install their own EXIT trap, and measured rather than inferred, which was worth doing: reading the code said harness_selftest would leak its squatter and it does not (delta 0). logical_subscriber does leak one cluster per run (delta 1). That one is pre-existing on main and is not this PR's to fix; it is filed separately. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky --- test/objstore_module.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/objstore_module.sh b/test/objstore_module.sh index a5261a7..e8c31cc 100755 --- a/test/objstore_module.sh +++ b/test/objstore_module.sh @@ -162,7 +162,15 @@ restore_module() { mv "$stash" "$MOD" done } -trap restore_module EXIT INT TERM +# Chained, not replacing. pgc_setup installs `trap pgc_teardown EXIT`, and a bare +# `trap restore_module EXIT` overwrites it: the module came back but the cluster +# was never stopped and its workdir never removed. Every run of this suite then +# left a live postmaster holding a port. Measured at 32 orphaned postmasters from +# one matrix plus one gate, which is enough to exhaust the port band -- and a +# suite that cannot get a port fails with 8 start attempts, which reads exactly +# like a real red. replication.sh's sb_teardown already chains this way. +objstore_teardown() { restore_module; pgc_teardown; } +trap objstore_teardown EXIT INT TERM if ! mv "$MOD" "$MOD.probe" 2>/dev/null; then echo "SKIP cannot move $MOD, so the absent and broken paths are untested here"