From 302e714af3c73f5269b84ec4c0230a07ad74d6c1 Mon Sep 17 00:00:00 2001 From: Jimmy Angelakos Date: Fri, 28 Aug 2026 15:49:32 +0100 Subject: [PATCH 1/9] feat: accept date_bin, the JSON builders and bound params on tiered reads --- docs/architecture.md | 16 +- docs/usage.md | 27 +- extension/coldfront/Makefile | 4 +- extension/coldfront/src/coldfront.c | 452 ++++++++++++++++-- extension/coldfront/test/README.md | 8 + .../coldfront/test/expected/read_date_bin.out | 76 +++ .../test/expected/read_json_builders.out | 153 ++++++ .../test/expected/read_param_fold.out | 170 +++++++ .../coldfront/test/sql/read_date_bin.sql | 56 +++ .../coldfront/test/sql/read_json_builders.sql | 80 ++++ .../coldfront/test/sql/read_param_fold.sql | 94 ++++ 11 files changed, 1094 insertions(+), 42 deletions(-) create mode 100644 extension/coldfront/test/expected/read_date_bin.out create mode 100644 extension/coldfront/test/expected/read_json_builders.out create mode 100644 extension/coldfront/test/expected/read_param_fold.out create mode 100644 extension/coldfront/test/sql/read_date_bin.sql create mode 100644 extension/coldfront/test/sql/read_json_builders.sql create mode 100644 extension/coldfront/test/sql/read_param_fold.sql diff --git a/docs/architecture.md b/docs/architecture.md index 0b0bd6a..173ece6 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -110,7 +110,7 @@ The following table describes each component, its role, and its license: |-----------|------|---------| | PostgreSQL 16+ | Heap storage; range partitioning for the tiered hot tier. Works uniformly on PG 16, 17, and 18 - the cold-tier secret is a DuckDB persistent secret loaded at instance init, with no version-gated mechanism. | PostgreSQL | | pg_duckdb | DuckDB in-process. Iceberg read + write. Analytics. pg_duckdb 1.5.4 (PR #1025). The `duckdb-iceberg` carries the bakery-aware commit-refresh patch (async parquet overlap, no 409); see [Cold-write strategy](#cold-write-strategy-stock-vs-patched-duckdb-iceberg). | MIT | -| coldfront | PGXS C extension. `post_parse_analyze_hook` rewrites INSERT/UPDATE/DELETE on registered views to the correct tier; `ProcessUtility_hook` handles DDL; the hook lazily ATTACHes the Iceberg catalog on the first query touching a tiered view. | PostgreSQL | +| coldfront | PGXS C extension. `post_parse_analyze_hook` rewrites INSERT/UPDATE/DELETE on registered views to the correct tier and, on a SELECT DuckDB will run, the spellings DuckDB lacks (`date_bin`, `::jsonb`, the JSON builders); `planner_hook` folds bound parameters into such a read; `ProcessUtility_hook` handles DDL; the hook lazily ATTACHes the Iceberg catalog on the first query touching a tiered view. | PostgreSQL | | Lakekeeper | Iceberg REST catalog. Single Rust binary. | Apache 2.0 | | S3-compatible store | Any: SeaweedFS, MinIO, GCS, Azure Blob, etc. | Varies | | Archiver (tiered mode) | Go binary, invoked by cron. Thin SQL orchestrator that moves rows hot→cold. | PostgreSQL | @@ -258,6 +258,20 @@ writes funnel through the `_exec_iceberg_with_claim` chokepoint (see [Concurrency](#concurrency-and-pgedge-spock-deployments)). A `ProcessUtility_hook` handles DDL on the same relations. +The same parse-analyze hook prepares a SELECT that DuckDB will run, +wherever in the statement the view is named (a CTE, a sub-select, a +set-operation branch): `date_bin`, the `::jsonb` cast, +`jsonb_array_length` and the JSON builders (`jsonb_build_object`, +`jsonb_agg` and their `json_` twins) are rewritten into spellings both +engines accept (see +[usage.md → Supported column types](usage.md#supported-column-types)). A +`planner_hook` folds bound parameters into such a read before pg_duckdb +plans it when a parameter sits where DuckDB cannot type a placeholder (a +direct argument of a pg_duckdb function, any argument of a table +function); the plan cache's generic-plan build, which carries no values, +gets a PostgreSQL plan priced above any custom plan, so the read is +planned from its values on every execution. + The following table maps each operation to its interface and routing path: | Operation | Interface | Routed via | diff --git a/docs/usage.md b/docs/usage.md index d08edba..09aa1bf 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -653,8 +653,14 @@ native primitive). On read, `interval` is view-cast back to the rich PG type; `json` and `jsonb` surface as DuckDB's `json` (the equivalent of PG's `jsonb`), not the rich PG `jsonb` type, because Iceberg-backed reads run entirely in DuckDB. Queries like `data->>'key'` and `data->'key'` -work, and ColdFront translates the `::jsonb` cast and `jsonb_array_length` -on read. The jsonb-only operators (`?`, `@>`, `<@`, `#>`, `#>>`) and most +work, and ColdFront translates the `::jsonb` cast, `jsonb_array_length`, +`jsonb_build_object` / `jsonb_agg` (and their `json_` twins, which DuckDB +also lacks) and `date_bin` on read: the builders become the `concat` / +`to_json` / `array_agg` form both engines evaluate identically (the result +is JSON-equal to jsonb's rendering, in compact form and in argument +order), and `date_bin` becomes DuckDB's `time_bucket`, which takes the +same arguments and agrees on every fixed-width bucket. The jsonb-only +operators (`?`, `@>`, `<@`, `#>`, `#>>`) and most jsonb functions (`jsonb_typeof`, `jsonb_extract_path`, `jsonb_extract_path_text`, `jsonb_set`, `jsonb_path_*`, `jsonb_each`, `jsonb_object_keys`) are not supported on tiered or decoupled data: @@ -666,6 +672,17 @@ iceberg-only view, which pg_duckdb plans entirely in DuckDB. Ordinary (non-tiered) PostgreSQL tables are untouched by ColdFront and keep full jsonb support, as does the hot partition table itself. +Bound parameters (`$1` from a prepared statement, a driver's extended +protocol, or a plpgsql variable) work in such reads. DuckDB types most +placeholders from their context (`ts > $1`); one that is a direct +argument of a DuckDB function with several overloads (`time_bucket`'s +origin, so `date_bin`'s) or any argument of a table function +(`generate_series`) it cannot, so ColdFront plans such a read from the +bound values on every execution instead of caching a generic plan. Under +`plan_cache_mode = force_generic_plan` those reads cannot run: the forced +generic plan has no values and fails with `only works with DuckDB +execution`. + A hot-only read is the exception. When a `SELECT` reads a tiered view directly (no join, CTE, or sub-query) and its `WHERE` provably restricts to the hot tier, ColdFront rewrites it to read the hot partition table in @@ -686,9 +703,9 @@ index/compare them; cast on the hot side only if needed). Keep the following caveats in mind when running either mode: - **`jsonb` reads**: surface as `json`. The `->`/`->>` operators work, - and ColdFront translates the `::jsonb` cast and `jsonb_array_length`; - other jsonb operators and functions are unsupported on cold or - cross-tier reads. A hot-only read of the view runs in PostgreSQL with + and ColdFront translates the `::jsonb` cast, `jsonb_array_length`, + `jsonb_build_object` / `jsonb_agg` and `date_bin`; other jsonb + operators and functions are unsupported on cold or cross-tier reads. A hot-only read of the view runs in PostgreSQL with full jsonb (see Supported column types). - **Cross-tier isolation**: a long-running `SELECT` that touches the Iceberg side multiple times within one transaction may see writes from diff --git a/extension/coldfront/Makefile b/extension/coldfront/Makefile index 3f7b12d..a794cff 100644 --- a/extension/coldfront/Makefile +++ b/extension/coldfront/Makefile @@ -24,7 +24,9 @@ REGRESS = load_order update_unregistered_view update_heap_table \ param_cold_via_plpgsql async_requires_patch \ storage_secret_azure storage_secret_vended privilege_model \ partition_config_interval self_join_rejected returning_cold_rejected \ - schema_collision drop_iceberg_table vector_centroids vector_type_map vector_cold_render vector_param_render vector_ops vector_probe vector_status vector_assign vector_multicolumn + schema_collision drop_iceberg_table \ + read_date_bin read_json_builders read_param_fold \ + vector_centroids vector_type_map vector_cold_render vector_param_render vector_ops vector_probe vector_status vector_assign vector_multicolumn REGRESS_OPTS = --inputdir=test --outputdir=test PG_CONFIG ?= pg_config diff --git a/extension/coldfront/src/coldfront.c b/extension/coldfront/src/coldfront.c index a6662b0..fae637b 100644 --- a/extension/coldfront/src/coldfront.c +++ b/extension/coldfront/src/coldfront.c @@ -42,22 +42,31 @@ #include "access/attnum.h" #include "access/xact.h" +#include "catalog/dependency.h" #include "catalog/namespace.h" #include "catalog/pg_class.h" +#include "catalog/pg_collation.h" +#include "catalog/pg_namespace.h" +#include "catalog/pg_proc.h" #include "catalog/pg_type_d.h" +#include "commands/extension.h" #include "executor/executor.h" #include "executor/spi.h" #include "lib/stringinfo.h" #include "nodes/makefuncs.h" #include "nodes/parsenodes.h" #include "nodes/nodeFuncs.h" +#include "nodes/params.h" #include "nodes/pg_list.h" #include "optimizer/optimizer.h" +#include "optimizer/planner.h" #include "parser/analyze.h" +#include "parser/parse_func.h" #include "parser/parsetree.h" #include "tcop/tcopprot.h" #include "tcop/utility.h" #include "utils/builtins.h" +#include "utils/datum.h" #include "utils/guc.h" #include "utils/lsyscache.h" #include "utils/memutils.h" @@ -221,6 +230,7 @@ static char *coldfront_lakekeeper_endpoint = NULL; static char *coldfront_local_pg_dsn = NULL; static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL; +static planner_hook_type prev_planner_hook = NULL; /* Previous ProcessUtility_hook (pg_duckdb's, since coldfront loads after it). */ static ProcessUtility_hook_type prev_process_utility_hook = NULL; @@ -338,31 +348,43 @@ lookup_tiered_view(Oid relid, const char *vname, TieredViewInfo *info) } /* - * True if the query reads from a registered tiered/iceberg-only view — any - * RangeTblEntry that is a VIEW resolving in coldfront.tiered_views. Used to - * lazily attach 'ice' before a plain SELECT against a tiered view executes: - * the view body's iceberg_scan('ice...') only resolves once the catalog is - * attached. The cheap relkind syscache check gates the SPI lookup so plain + * True if the query reads from a registered tiered/iceberg-only view: a VIEW + * RangeTblEntry resolving in coldfront.tiered_views, at any depth (a CTE, a + * sub-select, a set-operation branch): pg_duckdb runs the whole statement in DuckDB + * whichever branch names the view. Once the rewriter has expanded the view its RTE + * is a subquery that keeps the view's relid, so the planner hook sees it too. Used + * to lazily attach 'ice' before the read executes (the view body's + * iceberg_scan('ice...') only resolves once the catalog is attached) and to gate + * the read rewrites. The cheap relkind syscache check gates the SPI lookup so plain * table queries (the OLTP hot path) never pay for it. */ static bool -query_reads_tiered_view(Query *query) +reads_tiered_view_walker(Node *node, void *ctx) { - ListCell *lc; - - foreach(lc, query->rtable) + if (node == NULL) + return false; + if (IsA(node, RangeTblEntry)) { - RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc); + RangeTblEntry *rte = (RangeTblEntry *) node; TieredViewInfo info; - if (rte->rtekind != RTE_RELATION) - continue; - if (get_rel_relkind(rte->relid) != RELKIND_VIEW) - continue; - if (lookup_tiered_view(rte->relid, get_rel_name(rte->relid), &info)) - return true; + return (rte->rtekind == RTE_RELATION || rte->rtekind == RTE_SUBQUERY) && + OidIsValid(rte->relid) && + get_rel_relkind(rte->relid) == RELKIND_VIEW && + get_rel_namespace(rte->relid) != PG_CATALOG_NAMESPACE && + lookup_tiered_view(rte->relid, get_rel_name(rte->relid), &info); } - return false; + if (IsA(node, Query)) + return query_tree_walker((Query *) node, reads_tiered_view_walker, ctx, + QTW_EXAMINE_RTES_BEFORE); + return expression_tree_walker(node, reads_tiered_view_walker, ctx); +} + +static bool +query_reads_tiered_view(Query *query) +{ + return query_tree_walker(query, reads_tiered_view_walker, NULL, + QTW_EXAMINE_RTES_BEFORE); } /* @@ -716,11 +738,17 @@ static const CfSubst cf_write_subst[] = { * json_path_query fails PG reparse (PG has no json_path_query), and * json_extract_path_text / json_type are signature- or vocabulary-incompatible in * DuckDB (array result; UBIGINT/VARCHAR vs number/string). json_array_length is - * verified identical ([10,20,30]→3, []→0) and exists in both. + * verified identical ([10,20,30]→3, []→0) and exists in both. DuckDB has no + * date_bin; its time_bucket takes the same (interval, timestamp[tz], timestamp[tz]) + * arguments, pg_duckdb declares it PG-side, and the two agree on every fixed-width + * bucket (a month or year width, which date_bin rejects, time_bucket accepts). + * The JSON builders (jsonb_build_object, jsonb_agg) need more than a spelling and + * are rewritten on the node tree instead: see cf_json_builder_mutator. */ static const CfSubst cf_read_subst[] = { { "::jsonb", "::json", false }, { "jsonb_array_length(", "json_array_length(", false }, + { "date_bin(", "time_bucket(", false }, }; /* @@ -784,6 +812,11 @@ cf_apply_subst(const char *sql, const CfSubst *map, int map_len, bool jsonb_catc for (i = 0; i < map_len; i++) { size_t plen = strlen(map[i].pg); /* nosemgrep */ + /* A function spelling must start the identifier: undate_bin( is not + * date_bin(. Cast spellings start at '::' and need no such check. */ + if (isalpha((unsigned char) map[i].pg[0]) && p > sql && + (isalnum((unsigned char) p[-1]) || p[-1] == '_')) + continue; if (strncmp(p, map[i].pg, plen) == 0) { appendStringInfoString(&buf, map[i].duck); @@ -836,11 +869,204 @@ normalize_casts_for_duckdb(const char *sql) /* Tiered-read path: whitelist only (output is reparsed by PG, then run by DuckDB). */ static char * -normalize_jsonb_for_read(const char *sql) +normalize_for_read(const char *sql) { return cf_apply_subst(sql, cf_read_subst, lengthof(cf_read_subst), false); } +/* ---------- read-path JSON builders ------------------------------------ */ + +/* + * jsonb_build_object / json_build_object and jsonb_agg / json_agg have no DuckDB + * counterpart a spelling can reach: PostgreSQL's grammar reserves json_object, so + * that name never resolves to a function, and DuckDB's json_group_array is a macro + * that refuses the ORDER BY an aggregate carries. Both engines share concat, + * to_json, array_agg and the json cast, so a builder becomes those: + * jsonb_build_object(k1, v1, …) → concat('{', to_json(k1::text)::text, ':', + * COALESCE(to_json(v1)::text, 'null'), + * ',', …, '}')::json + * jsonb_agg(e ORDER BY …) → to_json(array_agg(e ORDER BY …)) + * to_json(NULL) is SQL NULL in both engines, so a value is COALESCEd to the JSON + * null; a NULL key gives concat text the json cast rejects, as jsonb_build_object + * rejects a NULL key. The result is JSON-equal to jsonb's rendering (jsonb also + * sorts keys and pads punctuation). Pairing keys with values is exact on the + * argument List; the emitted function OIDs come from the catalog, once per backend. + * A VARIADIC array argument cannot be paired and is left alone. + */ +typedef struct { bool changed; } JsonBuilderCtx; + +static Oid cf_to_json_oid = InvalidOid; +static Oid cf_concat_oid = InvalidOid; +static Oid cf_array_agg_oid = InvalidOid; /* array_agg(anynonarray) */ +static Oid cf_array_agg_arr_oid = InvalidOid; /* array_agg(anyarray) */ + +static void +cf_resolve_json_builder_oids(void) +{ + List *array_agg = list_make2(makeString("pg_catalog"), makeString("array_agg")); + Oid argtype; + + if (OidIsValid(cf_to_json_oid)) + return; + cf_to_json_oid = LookupFuncName(list_make2(makeString("pg_catalog"), makeString("to_json")), + -1, NULL, false); + cf_concat_oid = LookupFuncName(list_make2(makeString("pg_catalog"), makeString("concat")), + -1, NULL, false); + argtype = ANYNONARRAYOID; + cf_array_agg_oid = LookupFuncName(array_agg, 1, &argtype, false); + argtype = ANYARRAYOID; + cf_array_agg_arr_oid = LookupFuncName(array_agg, 1, &argtype, false); +} + +static bool +cf_is_pg_catalog_func(Oid funcid, const char *name, const char *alt) +{ + const char *fname; + + if (get_func_namespace(funcid) != PG_CATALOG_NAMESPACE) + return false; + fname = get_func_name(funcid); + return strcmp(fname, name) == 0 || strcmp(fname, alt) == 0; /* nosemgrep */ +} + +static Node * +cf_text_const(const char *s) +{ + return (Node *) makeConst(TEXTOID, -1, DEFAULT_COLLATION_OID, -1, + CStringGetTextDatum(s), false, false); +} + +static Node * +cf_cast_via_io(Node *arg, Oid type) +{ + CoerceViaIO *c = makeNode(CoerceViaIO); + + c->arg = (Expr *) arg; + c->resulttype = type; + c->resultcollid = (type == TEXTOID) ? DEFAULT_COLLATION_OID : InvalidOid; + c->coerceformat = COERCE_EXPLICIT_CAST; + c->location = -1; + return (Node *) c; +} + +/* to_json(e)::text */ +static Node * +cf_json_text(Node *e) +{ + FuncExpr *f = makeFuncExpr(cf_to_json_oid, JSONOID, list_make1(e), + InvalidOid, InvalidOid, COERCE_EXPLICIT_CALL); + + return cf_cast_via_io((Node *) f, TEXTOID); +} + +/* COALESCE(to_json(v)::text, 'null'); a value that is already json (a nested + * builder, a view's json column) needs only the cast. */ +static Node * +cf_json_value(Node *v) +{ + CoalesceExpr *c = makeNode(CoalesceExpr); + + c->coalescetype = TEXTOID; + c->coalescecollid = DEFAULT_COLLATION_OID; + c->args = list_make2(exprType(v) == JSONOID ? cf_cast_via_io(v, TEXTOID) + : cf_json_text(v), + cf_text_const("null")); + c->location = -1; + return (Node *) c; +} + +/* to_json(k::text)::text: a JSON key is a string whatever the key's type. */ +static Node * +cf_json_key(Node *k) +{ + if (exprType(k) != TEXTOID) + k = cf_cast_via_io(k, TEXTOID); + return cf_json_text(k); +} + +/* concat('{', key, ':', value, ',', …, '}')::json over the paired argument list. */ +static Node * +cf_json_object(List *args) +{ + List *cat = list_make1(cf_text_const("{")); + ListCell *lc; + int i = 0; + + foreach(lc, args) + { + Node *a = (Node *) lfirst(lc); + + if (i % 2 == 0) + { + if (i > 0) + cat = lappend(cat, cf_text_const(",")); + cat = lappend(cat, cf_json_key(a)); + cat = lappend(cat, cf_text_const(":")); + } + else + cat = lappend(cat, cf_json_value(a)); + i++; + } + cat = lappend(cat, cf_text_const("}")); + return cf_cast_via_io((Node *) makeFuncExpr(cf_concat_oid, TEXTOID, cat, InvalidOid, + DEFAULT_COLLATION_OID, COERCE_EXPLICIT_CALL), + JSONOID); +} + +/* to_json(array_agg(e …)): the Aggref keeps its ORDER BY, FILTER and DISTINCT. */ +static Node * +cf_json_agg(Aggref *agg, Oid elemtype, Oid arrtype) +{ + agg->aggfnoid = type_is_array(elemtype) ? cf_array_agg_arr_oid : cf_array_agg_oid; + agg->aggtype = arrtype; + return (Node *) makeFuncExpr(cf_to_json_oid, JSONOID, list_make1(agg), + InvalidOid, InvalidOid, COERCE_EXPLICIT_CALL); +} + +static Node * +cf_json_builder_mutator(Node *node, void *ctx) +{ + JsonBuilderCtx *jc = (JsonBuilderCtx *) ctx; + + if (node == NULL) + return NULL; + if (IsA(node, Query)) + return (Node *) query_tree_mutator((Query *) node, cf_json_builder_mutator, ctx, 0); + if (IsA(node, FuncExpr)) + { + FuncExpr *f = (FuncExpr *) expression_tree_mutator(node, cf_json_builder_mutator, ctx); + + if (!f->funcvariadic && + cf_is_pg_catalog_func(f->funcid, "jsonb_build_object", "json_build_object")) + { + cf_resolve_json_builder_oids(); + jc->changed = true; + return cf_json_object(f->args); + } + return (Node *) f; + } + if (IsA(node, Aggref)) + { + Aggref *a = (Aggref *) expression_tree_mutator(node, cf_json_builder_mutator, ctx); + + if (list_length(a->aggargtypes) == 1 && + cf_is_pg_catalog_func(a->aggfnoid, "jsonb_agg", "json_agg")) + { + Oid elemtype = linitial_oid(a->aggargtypes); + Oid arrtype = get_array_type(elemtype); + + if (OidIsValid(arrtype)) + { + cf_resolve_json_builder_oids(); + jc->changed = true; + return cf_json_agg(a, elemtype, arrtype); + } + } + return (Node *) a; + } + return expression_tree_mutator(node, cf_json_builder_mutator, ctx); +} + /* ---------- SQL builder ----------------------------------------------- */ /* @@ -2433,22 +2659,27 @@ cf_try_reroute_hot_read(Query *query) } /* - * jsonb → json on the read path. A query against a tiered / iceberg-only view runs - * entirely in DuckDB (the view body is an iceberg_scan UNION), and DuckDB has no - * jsonb type. Deparse, apply the read whitelist (normalize_jsonb_for_read: the - * ::jsonb cast and the functions verified equivalent in both engines), reparse in - * place. No whitelisted token ⇒ strcmp matches and the query is left untouched (the - * common case — ->>/-> operators and plain reads never reparse). jsonb spellings - * outside the whitelist pass through and DuckDB rejects them clearly (documented). + * Make a read DuckDB will run acceptable to it. A query against a tiered / + * iceberg-only view runs entirely in DuckDB (the view body is an iceberg_scan + * UNION), which has no jsonb type, no date_bin and no JSON builders. Two passes over + * the analysed tree: the JSON builders are rewritten on the node tree + * (cf_json_builder_mutator, where key/value pairing is exact), then the deparsed + * text gets the read whitelist (normalize_for_read: the ::jsonb cast and the + * functions verified equivalent in both engines), and the result is reparsed in + * place. Nothing to rewrite ⇒ the query is left untouched (the common case: ->>/-> + * operators and plain reads never reparse). Other jsonb spellings pass through and + * DuckDB rejects them clearly (documented). */ static void -cf_normalize_read_jsonb(Query *query) +cf_normalize_read(Query *query) { - char *sql = pg_get_querydef(query, false); - char *norm = normalize_jsonb_for_read(sql); - ColdParamSet ps; + JsonBuilderCtx jc = { false }; + Query *tree = query_tree_mutator(query, cf_json_builder_mutator, &jc, 0); + char *sql = pg_get_querydef(tree, false); + char *norm = normalize_for_read(sql); + ColdParamSet ps; - if (strcmp(sql, norm) == 0) /* nosemgrep */ + if (!jc.changed && strcmp(sql, norm) == 0) /* nosemgrep */ return; collect_cold_params(query, &ps); cf_reparse_and_replace(query, norm, &ps); @@ -2748,9 +2979,9 @@ cf_maybe_inject_probe(Query *query) * spans the cold tier: lazily attach * 'ice' (once per session) so the view body's iceberg_scan('ice...') resolves — * the version-agnostic cold-read attach (PG 16/17/18) — narrow a recognised - * similarity search to its probed clusters, and normalize the whitelisted jsonb - * spellings so DuckDB (which runs the whole view query) accepts them. The relkind - * check inside query_reads_tiered_view keeps plain queries off the SPI path. + * similarity search to its probed clusters, and rewrite the spellings DuckDB (which + * runs the whole view query) lacks into ones it accepts. The relkind check inside + * query_reads_tiered_view keeps plain queries off the SPI path. */ static void cf_maybe_attach_for_read(Query *query) @@ -2762,7 +2993,7 @@ cf_maybe_attach_for_read(Query *query) if (!coldfront_ice_attached) ensure_ice_attached_once(); cf_maybe_inject_probe(query); - cf_normalize_read_jsonb(query); + cf_normalize_read(query); } /* @@ -4038,6 +4269,151 @@ register_gucs(void) NULL, NULL, NULL); } +/* ---------- planner hook: bound parameters on a tiered read ------------- */ + +/* + * pg_duckdb deparses a PARAM_EXTERN as a bare $N placeholder. DuckDB types most + * placeholders from their context (ts > $1), but not one that is a direct argument + * of a DuckDB function with several overloads (time_bucket's origin) or any argument + * of a table function (generate_series), so such a prepared tiered read fails to + * plan. The values are known here: the plan cache hands the planner bound_params for + * every custom plan, and a custom plan serves exactly that execution, so when a + * parameter sits in one of those two places every parameter is folded to a Const + * before pg_duckdb plans (what eval_const_expressions would do later for a + * PostgreSQL plan). The generic-plan build carries no values; it is answered with a + * PostgreSQL plan priced above any custom plan, so the cache keeps choosing the + * value-bearing custom plans and never runs the generic one (plan_cache_mode = + * force_generic_plan does run it, and pg_duckdb's read functions then refuse + * PostgreSQL execution). A read whose parameters DuckDB types on its own is left + * alone and keeps its generic plan. A DuckDB function is recognised as one the + * pg_duckdb extension owns: every function it declares stands for a DuckDB one. + */ +#define CF_DECOY_PLAN_COST 1.0e10 + +typedef struct { ParamListInfo params; } FoldParamsCtx; +typedef struct { Oid duckdb_ext; bool in_table_func; } NeedsValueCtx; + +static bool +is_extern_param(Node *node) +{ + return node != NULL && IsA(node, Param) && + ((Param *) node)->paramkind == PARAM_EXTERN; +} + +static bool +has_extern_param_walker(Node *node, void *ctx) +{ + if (node == NULL) + return false; + if (IsA(node, Param)) + return is_extern_param(node); + if (IsA(node, Query)) + return query_tree_walker((Query *) node, has_extern_param_walker, ctx, 0); + return expression_tree_walker(node, has_extern_param_walker, ctx); +} + +/* True if a PARAM_EXTERN sits where DuckDB cannot type a placeholder. */ +static bool +param_needs_value_walker(Node *node, void *ctx) +{ + NeedsValueCtx *nv = (NeedsValueCtx *) ctx; + + if (node == NULL) + return false; + if (IsA(node, Param)) + return nv->in_table_func && is_extern_param(node); + if (IsA(node, RangeTblEntry)) + { + RangeTblEntry *rte = (RangeTblEntry *) node; + bool found; + + if (rte->rtekind != RTE_FUNCTION) + return false; + nv->in_table_func = true; + found = expression_tree_walker((Node *) rte->functions, param_needs_value_walker, ctx); + nv->in_table_func = false; + return found; + } + if (IsA(node, FuncExpr) && + getExtensionOfObject(ProcedureRelationId, ((FuncExpr *) node)->funcid) == nv->duckdb_ext) + { + ListCell *lc; + + foreach(lc, ((FuncExpr *) node)->args) + if (is_extern_param((Node *) lfirst(lc))) + return true; + } + if (IsA(node, Query)) + return query_tree_walker((Query *) node, param_needs_value_walker, ctx, + QTW_EXAMINE_RTES_BEFORE); + return expression_tree_walker(node, param_needs_value_walker, ctx); +} + +static Node * +fold_params_mutator(Node *node, void *ctx) +{ + ParamListInfo params = ((FoldParamsCtx *) ctx)->params; + + if (node == NULL) + return NULL; + if (IsA(node, Param)) + { + Param *p = (Param *) node; + ParamExternData prmdata; + ParamExternData *prm; + int16 typlen; + bool typbyval; + + if (p->paramkind != PARAM_EXTERN || p->paramid < 1 || + p->paramid > params->numParams) + return expression_tree_mutator(node, fold_params_mutator, ctx); + if (params->paramFetch != NULL) + prm = params->paramFetch(params, p->paramid, true, &prmdata); + else + prm = ¶ms->params[p->paramid - 1]; + if (!OidIsValid(prm->ptype) || prm->ptype != p->paramtype) + return expression_tree_mutator(node, fold_params_mutator, ctx); + get_typlenbyval(p->paramtype, &typlen, &typbyval); + return (Node *) makeConst(p->paramtype, p->paramtypmod, p->paramcollid, typlen, + prm->isnull ? (Datum) 0 + : datumCopy(prm->value, typbyval, typlen), + prm->isnull, typbyval); + } + if (IsA(node, Query)) + return (Node *) query_tree_mutator((Query *) node, fold_params_mutator, ctx, 0); + return expression_tree_mutator(node, fold_params_mutator, ctx); +} + +static PlannedStmt * +coldfront_planner(Query *parse, const char *query_string, int cursor_options, + ParamListInfo bound_params) +{ + if (parse->commandType == CMD_SELECT && coldfront_registry_present() && + query_tree_walker(parse, has_extern_param_walker, NULL, 0) && + query_reads_tiered_view(parse)) + { + NeedsValueCtx nv = { get_extension_oid("pg_duckdb", true), false }; + + if (OidIsValid(nv.duckdb_ext) && + query_tree_walker(parse, param_needs_value_walker, &nv, QTW_EXAMINE_RTES_BEFORE)) + { + FoldParamsCtx fc = { bound_params }; + + if (bound_params == NULL) + { + PlannedStmt *decoy = standard_planner(parse, query_string, cursor_options, NULL); + + decoy->planTree->total_cost = CF_DECOY_PLAN_COST; + return decoy; + } + parse = query_tree_mutator(parse, fold_params_mutator, &fc, 0); + } + } + if (prev_planner_hook) + return prev_planner_hook(parse, query_string, cursor_options, bound_params); + return standard_planner(parse, query_string, cursor_options, bound_params); +} + void _PG_init(void) { @@ -4046,6 +4422,12 @@ _PG_init(void) prev_post_parse_analyze_hook = post_parse_analyze_hook; post_parse_analyze_hook = coldfront_post_parse_analyze; + /* Bound parameters on a tiered read (coldfront_planner). Chains pg_duckdb's + * planner_hook (coldfront loads later, so prev == pg_duckdb's): the fold runs + * before DuckDB plans. */ + prev_planner_hook = planner_hook; + planner_hook = coldfront_planner; + /* DDL synchronization for tiered tables. Chains pg_duckdb's * ProcessUtility_hook (coldfront loads later, so prev == pg_duckdb's). */ prev_process_utility_hook = ProcessUtility_hook; diff --git a/extension/coldfront/test/README.md b/extension/coldfront/test/README.md index d3c1637..613024a 100644 --- a/extension/coldfront/test/README.md +++ b/extension/coldfront/test/README.md @@ -41,6 +41,14 @@ extension's non-hook surface (third table below) and register no view. | `self_join_rejected` | a second reference to the tiered view (self-join / `USING` / sub-select) rejected at parse-analyze | | `bakery_wraps_cold_writes` | every cold write funnels through `_exec_iceberg_with_claim` | | `update_unregistered_view`, `update_heap_table` | unregistered / non-tiered relations pass through untouched | +| `read_date_bin` | a read that DuckDB will run has `date_bin` rewritten to `time_bucket` (DuckDB executes it against the heap and agrees with `date_bin`); a hot-rerouted read and a look-alike function name are left alone | +| `read_json_builders` | `jsonb_build_object` / `jsonb_agg` (and the `json_` twins) on a read that DuckDB will run become the `concat` / `to_json` / `array_agg` form; the result is JSON-equal to jsonb's rendering, keeps `ORDER BY` / `FILTER`, still takes `->>`, is rewritten below the top level too, and DuckDB executes it | + +### `planner_hook`: bound parameters on a tiered read (executed) + +| test | checks | +|---|---| +| `read_param_fold` | `$N` values are folded into the read before pg_duckdb plans it when a parameter sits where DuckDB cannot type a placeholder (`time_bucket`'s origin, every `generate_series` argument), through seven executions of a prepared statement and of a plpgsql query, so the plan cache's generic-plan attempt after the fifth never reaches DuckDB; a literal-only read and a parameter DuckDB types from context (`ts > $1`, which keeps its generic plan) are untouched | ### `ProcessUtility_hook` — DDL gating (executed) diff --git a/extension/coldfront/test/expected/read_date_bin.out b/extension/coldfront/test/expected/read_date_bin.out new file mode 100644 index 0000000..7d3abc5 --- /dev/null +++ b/extension/coldfront/test/expected/read_date_bin.out @@ -0,0 +1,76 @@ +-- date_bin on a tiered read. A read that spans the cold tier runs entirely in +-- DuckDB, which has no date_bin; its time_bucket takes the same (interval, +-- timestamptz, timestamptz) arguments and pg_duckdb declares it PG-side, so the +-- read is rewritten to time_bucket and reparses in both engines. White-box: DuckDB +-- executes the rewritten read against the heap (duckdb.force_execution), and the +-- reads that must stay untouched are shown through PostgreSQL's plan; no Iceberg I/O. +-- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress +-- db an earlier test may have created the extensions, standalone not. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +RESET client_min_messages; +SET TIME ZONE 'UTC'; +-- White-box: checks the generated SQL, not Iceberg I/O. +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; +CREATE TABLE public._events (id int, ts timestamptz, val int); +CREATE VIEW public.events AS SELECT * FROM public._events; +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); +INSERT INTO public._events VALUES (1, '2026-01-01 00:05+00', 10), + (2, '2026-01-01 00:50+00', 20), + (3, '2026-01-01 01:07+00', 30); +-- (A) Parity: DuckDB runs the rewritten read (force_execution scans the heap the +-- way a cold read scans Parquet; DuckDB rejecting the spelling would be a planning +-- warning here) and buckets the rows exactly as PostgreSQL's date_bin does on the +-- heap directly. The bucket is in the target list and the GROUP BY. +SET duckdb.force_execution = true; +SELECT date_bin('1 hour'::interval, ts, '2026-01-01'::timestamptz) AS bucket, sum(val) +FROM public.events GROUP BY 1 ORDER BY 1; + bucket | sum +------------------------------+----- + Thu Jan 01 00:00:00 2026 UTC | 30 + Thu Jan 01 01:00:00 2026 UTC | 30 +(2 rows) + +RESET duckdb.force_execution; +SELECT date_bin('1 hour'::interval, ts, '2026-01-01'::timestamptz) AS bucket, sum(val) +FROM public._events GROUP BY 1 ORDER BY 1; + bucket | sum +------------------------------+----- + Thu Jan 01 00:00:00 2026 UTC | 30 + Thu Jan 01 01:00:00 2026 UTC | 30 +(2 rows) + +-- (B) A read the hot heap can answer keeps date_bin: it never reaches DuckDB. +EXPLAIN (COSTS OFF, VERBOSE) +SELECT date_bin('1 hour'::interval, ts, '2026-01-01'::timestamptz) AS bucket +FROM public.events WHERE ts >= '2026-04-01'::timestamptz; + QUERY PLAN +-------------------------------------------------------------------------------------------------------- + Seq Scan on public._events + Output: date_bin('@ 1 hour'::interval, ts, 'Thu Jan 01 00:00:00 2026 UTC'::timestamp with time zone) + Filter: (_events.ts >= 'Wed Apr 01 00:00:00 2026 UTC'::timestamp with time zone) +(3 rows) + +-- (C) A name that merely ends in date_bin( is a different function; it is left +-- alone. The read is not provably hot, so the rewrite pass does run on it. +CREATE FUNCTION public.undate_bin(interval, timestamptz, timestamptz) RETURNS timestamptz +LANGUAGE plpgsql AS $$ BEGIN RETURN $2; END $$; +EXPLAIN (COSTS OFF, VERBOSE) +SELECT undate_bin('1 hour'::interval, ts, '2026-01-01'::timestamptz) FROM public.events; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------ + Seq Scan on public._events + Output: undate_bin('@ 1 hour'::interval, _events.ts, 'Thu Jan 01 00:00:00 2026 UTC'::timestamp with time zone) +(2 rows) + +-- Cleanup. +DROP FUNCTION public.undate_bin(interval, timestamptz, timestamptz); +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; diff --git a/extension/coldfront/test/expected/read_json_builders.out b/extension/coldfront/test/expected/read_json_builders.out new file mode 100644 index 0000000..12bf052 --- /dev/null +++ b/extension/coldfront/test/expected/read_json_builders.out @@ -0,0 +1,153 @@ +-- jsonb_build_object / jsonb_agg (and their json_ twins) on a tiered read. The +-- read runs entirely in DuckDB, which has neither; a name swap cannot reach a +-- DuckDB counterpart (json_object is grammar-reserved in PostgreSQL, and DuckDB's +-- json_group_array is a macro that refuses ORDER BY), so the builders are rewritten +-- into the concat / to_json / array_agg form both engines evaluate identically. +-- White-box: the assertions are on the rewritten SQL, on PostgreSQL producing the +-- same JSON as jsonb's own rendering, and on DuckDB accepting the rewritten read +-- against the heap (duckdb.force_execution); no Iceberg I/O. +-- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress +-- db an earlier test may have created the extensions, standalone not. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +RESET client_min_messages; +SET TIME ZONE 'UTC'; +-- White-box: checks the generated SQL, not Iceberg I/O. +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; +CREATE TABLE public._events (id int, ts timestamptz, name text, setting text); +CREATE VIEW public.events AS SELECT * FROM public._events; +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); +-- A NULL value and a value with a quote: both must survive as JSON. +INSERT INTO public._events VALUES (1, '2026-01-01 00:05+00', 'work_mem', '4MB'), + (2, '2026-01-01 01:07+00', 'max_conn', NULL), + (3, '2026-01-01 02:09+00', 'app_name', 'o''neil'); +-- (A) The rewritten shapes: an object, an ordered aggregate of objects nested in +-- an object, a FILTERed aggregate, and a json_ (not jsonb_) aggregate of scalars. +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_build_object('name', name, 'value', setting)::text AS details FROM public.events; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Seq Scan on public._events + Output: ((concat('{'::text, (to_json('name'::text))::text, ':'::text, COALESCE((to_json(_events.name))::text, 'null'::text), ','::text, (to_json('value'::text))::text, ':'::text, COALESCE((to_json(_events.setting))::text, 'null'::text), '}'::text))::json)::text +(2 rows) + +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_build_object('count', count(*), + 'changes', jsonb_agg(jsonb_build_object('name', name, 'value', setting) ORDER BY name))::text +FROM public.events; + QUERY PLAN +----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Aggregate + Output: ((concat('{'::text, (to_json('count'::text))::text, ':'::text, COALESCE((to_json(count(*)))::text, 'null'::text), ','::text, (to_json('changes'::text))::text, ':'::text, COALESCE((to_json(array_agg((concat('{'::text, (to_json('name'::text))::text, ':'::text, COALESCE((to_json(_events.name))::text, 'null'::text), ','::text, (to_json('value'::text))::text, ':'::text, COALESCE((to_json(_events.setting))::text, 'null'::text), '}'::text))::json ORDER BY _events.name)))::text, 'null'::text), '}'::text))::json)::text + -> Sort + Output: _events.name, _events.setting + Sort Key: _events.name + -> Seq Scan on public._events + Output: _events.name, _events.setting +(7 rows) + +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_agg(name) FILTER (WHERE setting IS NOT NULL) FROM public.events; + QUERY PLAN +----------------------------------------------------------------------------------------- + Aggregate + Output: to_json(array_agg(_events.name) FILTER (WHERE (_events.setting IS NOT NULL))) + -> Seq Scan on public._events + Output: _events.id, _events.ts, _events.name, _events.setting +(4 rows) + +EXPLAIN (COSTS OFF, VERBOSE) +SELECT json_agg(name ORDER BY name) FROM public.events; + QUERY PLAN +------------------------------------------------------------------ + Aggregate + Output: to_json(array_agg(_events.name ORDER BY _events.name)) + -> Sort + Output: _events.name + Sort Key: _events.name + -> Seq Scan on public._events + Output: _events.name +(7 rows) + +-- (B) JSON parity with jsonb's own rendering. The rewritten read, run by +-- PostgreSQL here, is captured and compared as jsonb against the builders on the +-- heap (no tiered view in that statement, so it is left untouched). +SELECT jsonb_build_object('count', count(*), + 'changes', jsonb_agg(jsonb_build_object('name', name, 'value', setting) ORDER BY name))::text AS details +FROM public.events \gset +SELECT :'details' AS rewritten_read; + rewritten_read +--------------------------------------------------------------------------------------------------------------------------------- + {"count":3,"changes":[{"name":"app_name","value":"o'neil"},{"name":"max_conn","value":null},{"name":"work_mem","value":"4MB"}]} +(1 row) + +SELECT :'details'::jsonb = jsonb_build_object('count', count(*), + 'changes', jsonb_agg(jsonb_build_object('name', name, 'value', setting) ORDER BY name)) AS same_json +FROM public._events; + same_json +----------- + t +(1 row) + +SELECT jsonb_agg(name) FILTER (WHERE setting IS NOT NULL) AS filtered FROM public.events \gset +SELECT :'filtered'::jsonb = (SELECT jsonb_agg(name) FILTER (WHERE setting IS NOT NULL) FROM public._events) AS same_json; + same_json +----------- + t +(1 row) + +SELECT json_agg(name ORDER BY name) AS scalars FROM public.events \gset +SELECT :'scalars'::jsonb = (SELECT json_agg(name ORDER BY name)::jsonb FROM public._events) AS same_json; + same_json +----------- + t +(1 row) + +-- The result is still JSON to the rest of the query: ->> works on it. +SELECT jsonb_build_object('name', name, 'value', setting) ->> 'value' AS value FROM public.events ORDER BY id; + value +-------- + 4MB + + o'neil +(3 rows) + +-- A view reference below the top level is rewritten too: the whole statement runs +-- in DuckDB, whichever branch names the view. +WITH changes AS (SELECT name, setting FROM public.events) +SELECT jsonb_build_object('name', name)::text FROM changes ORDER BY 1; + jsonb_build_object +--------------------- + {"name":"app_name"} + {"name":"max_conn"} + {"name":"work_mem"} +(3 rows) + +-- (C) Parity against the live DuckDB: it executes the rewritten read (force_execution +-- scans the heap the way a cold read scans Parquet) and returns the same JSON. +SET duckdb.force_execution = true; +SELECT jsonb_build_object('count', count(*), + 'changes', jsonb_agg(jsonb_build_object('name', name, 'value', setting) ORDER BY name))::text AS details +FROM public.events; + details +--------------------------------------------------------------------------------------------------------------------------------- + {"count":3,"changes":[{"name":"app_name","value":"o'neil"},{"name":"max_conn","value":null},{"name":"work_mem","value":"4MB"}]} +(1 row) + +SELECT json_agg(name ORDER BY name)::text AS scalars FROM public.events; + scalars +------------------------------------ + ["app_name","max_conn","work_mem"] +(1 row) + +RESET duckdb.force_execution; +-- Cleanup. +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; diff --git a/extension/coldfront/test/expected/read_param_fold.out b/extension/coldfront/test/expected/read_param_fold.out new file mode 100644 index 0000000..d1dba5f --- /dev/null +++ b/extension/coldfront/test/expected/read_param_fold.out @@ -0,0 +1,170 @@ +-- Bound parameters on a tiered read. pg_duckdb deparses $N as a placeholder, and +-- DuckDB cannot type a placeholder it sees only as one overload candidate among +-- several (time_bucket's third argument) or as a table-function argument +-- (generate_series), so the prepared read fails to plan. coldfront's planner hook +-- folds the bound values into the query before pg_duckdb plans it, and hands the +-- plan cache a prohibitively costed PostgreSQL plan when no values are bound (the +-- generic-plan build) so it keeps choosing value-bearing custom plans. White-box: +-- DuckDB runs the reads against the heap (duckdb.force_execution); no Iceberg I/O. +-- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress +-- db an earlier test may have created the extensions, standalone not. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +RESET client_min_messages; +SET TIME ZONE 'UTC'; +-- White-box: checks the generated SQL, not Iceberg I/O. +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; +CREATE TABLE public._events (id int, ts timestamptz); +CREATE VIEW public.events AS SELECT * FROM public._events; +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); +INSERT INTO public._events VALUES (1, '2026-01-01 00:05+00'), + (2, '2026-01-01 00:50+00'), + (3, '2026-01-01 01:07+00'); +SET duckdb.force_execution = true; +-- (A) Every parameter sits where DuckDB cannot type a placeholder: the bucket +-- origin and width, and all three generate_series arguments. Seven executions +-- cross the plan cache's switch from custom to generic planning after the fifth, +-- and every one returns the same rows with no planning warning. +PREPARE buckets(timestamptz, timestamptz, interval) AS +SELECT g.bucket, count(e.id) AS n +FROM generate_series($1, $2, $3) AS g(bucket) +LEFT JOIN public.events e ON time_bucket($3, e.ts, $1) = g.bucket +GROUP BY 1 ORDER BY 1; +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); + bucket | n +------------------------------+--- + Thu Jan 01 00:00:00 2026 UTC | 2 + Thu Jan 01 01:00:00 2026 UTC | 1 + Thu Jan 01 02:00:00 2026 UTC | 0 + Thu Jan 01 03:00:00 2026 UTC | 0 +(4 rows) + +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); + bucket | n +------------------------------+--- + Thu Jan 01 00:00:00 2026 UTC | 2 + Thu Jan 01 01:00:00 2026 UTC | 1 + Thu Jan 01 02:00:00 2026 UTC | 0 + Thu Jan 01 03:00:00 2026 UTC | 0 +(4 rows) + +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); + bucket | n +------------------------------+--- + Thu Jan 01 00:00:00 2026 UTC | 2 + Thu Jan 01 01:00:00 2026 UTC | 1 + Thu Jan 01 02:00:00 2026 UTC | 0 + Thu Jan 01 03:00:00 2026 UTC | 0 +(4 rows) + +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); + bucket | n +------------------------------+--- + Thu Jan 01 00:00:00 2026 UTC | 2 + Thu Jan 01 01:00:00 2026 UTC | 1 + Thu Jan 01 02:00:00 2026 UTC | 0 + Thu Jan 01 03:00:00 2026 UTC | 0 +(4 rows) + +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); + bucket | n +------------------------------+--- + Thu Jan 01 00:00:00 2026 UTC | 2 + Thu Jan 01 01:00:00 2026 UTC | 1 + Thu Jan 01 02:00:00 2026 UTC | 0 + Thu Jan 01 03:00:00 2026 UTC | 0 +(4 rows) + +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); + bucket | n +------------------------------+--- + Thu Jan 01 00:00:00 2026 UTC | 2 + Thu Jan 01 01:00:00 2026 UTC | 1 + Thu Jan 01 02:00:00 2026 UTC | 0 + Thu Jan 01 03:00:00 2026 UTC | 0 +(4 rows) + +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); + bucket | n +------------------------------+--- + Thu Jan 01 00:00:00 2026 UTC | 2 + Thu Jan 01 01:00:00 2026 UTC | 1 + Thu Jan 01 02:00:00 2026 UTC | 0 + Thu Jan 01 03:00:00 2026 UTC | 0 +(4 rows) + +-- The plan is built from the values, so new values change the answer. +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 01:00+00', '30 minutes'); + bucket | n +------------------------------+--- + Thu Jan 01 00:00:00 2026 UTC | 1 + Thu Jan 01 00:30:00 2026 UTC | 1 + Thu Jan 01 01:00:00 2026 UTC | 1 +(3 rows) + +-- (B) plpgsql variables reach the planner through the parameter-fetch hook rather +-- than an array of values; the loop crosses the same custom-to-generic switch. +-- pg_duckdb gates DuckDB execution inside functions on its own setting. +SET duckdb.unsafe_allow_execution_inside_functions = true; +CREATE FUNCTION public.bucket_rows(p_from timestamptz, p_to timestamptz, p_w interval) +RETURNS bigint LANGUAGE plpgsql AS $$ +DECLARE + total bigint := 0; +BEGIN + FOR i IN 1..7 LOOP + total := total + (SELECT count(*) + FROM generate_series(p_from, p_to, p_w) AS g(bucket) + LEFT JOIN public.events e ON time_bucket(p_w, e.ts, p_from) = g.bucket); + END LOOP; + RETURN total; +END $$; +SELECT public.bucket_rows('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour') AS rows_over_7_runs; + rows_over_7_runs +------------------ + 35 +(1 row) + +-- (C) A read without parameters is not touched: the same query with literals plans +-- as before. +SELECT g.bucket, count(e.id) AS n +FROM generate_series('2026-01-01 00:00+00'::timestamptz, '2026-01-01 03:00+00'::timestamptz, '1 hour'::interval) AS g(bucket) +LEFT JOIN public.events e ON time_bucket('1 hour'::interval, e.ts, '2026-01-01 00:00+00'::timestamptz) = g.bucket +GROUP BY 1 ORDER BY 1; + bucket | n +------------------------------+--- + Thu Jan 01 00:00:00 2026 UTC | 2 + Thu Jan 01 01:00:00 2026 UTC | 1 + Thu Jan 01 02:00:00 2026 UTC | 0 + Thu Jan 01 03:00:00 2026 UTC | 0 +(4 rows) + +-- (D) A parameter DuckDB types from its context (a comparison) is left alone, so +-- that read keeps a generic plan: it works even when the plan cache is forced to +-- plan without values. The read whose parameters need values has no generic plan; +-- forced to one, it is the placeholder PostgreSQL plan, and pg_duckdb's function +-- refuses to run outside DuckDB. +SET plan_cache_mode = force_generic_plan; +PREPARE since(timestamptz) AS SELECT count(*) FROM public.events WHERE ts > $1; +EXECUTE since('2026-01-01 00:30+00'); + count +------- + 2 +(1 row) + +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); +ERROR: Function 'public.time_bucket' only works with DuckDB execution +RESET plan_cache_mode; +-- Cleanup. +RESET duckdb.force_execution; +DEALLOCATE since; +DEALLOCATE buckets; +DROP FUNCTION public.bucket_rows(timestamptz, timestamptz, interval); +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; diff --git a/extension/coldfront/test/sql/read_date_bin.sql b/extension/coldfront/test/sql/read_date_bin.sql new file mode 100644 index 0000000..426e0fa --- /dev/null +++ b/extension/coldfront/test/sql/read_date_bin.sql @@ -0,0 +1,56 @@ +-- date_bin on a tiered read. A read that spans the cold tier runs entirely in +-- DuckDB, which has no date_bin; its time_bucket takes the same (interval, +-- timestamptz, timestamptz) arguments and pg_duckdb declares it PG-side, so the +-- read is rewritten to time_bucket and reparses in both engines. White-box: DuckDB +-- executes the rewritten read against the heap (duckdb.force_execution), and the +-- reads that must stay untouched are shown through PostgreSQL's plan; no Iceberg I/O. +-- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress +-- db an earlier test may have created the extensions, standalone not. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +RESET client_min_messages; +SET TIME ZONE 'UTC'; +-- White-box: checks the generated SQL, not Iceberg I/O. +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; + +CREATE TABLE public._events (id int, ts timestamptz, val int); +CREATE VIEW public.events AS SELECT * FROM public._events; +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); +INSERT INTO public._events VALUES (1, '2026-01-01 00:05+00', 10), + (2, '2026-01-01 00:50+00', 20), + (3, '2026-01-01 01:07+00', 30); + +-- (A) Parity: DuckDB runs the rewritten read (force_execution scans the heap the +-- way a cold read scans Parquet; DuckDB rejecting the spelling would be a planning +-- warning here) and buckets the rows exactly as PostgreSQL's date_bin does on the +-- heap directly. The bucket is in the target list and the GROUP BY. +SET duckdb.force_execution = true; +SELECT date_bin('1 hour'::interval, ts, '2026-01-01'::timestamptz) AS bucket, sum(val) +FROM public.events GROUP BY 1 ORDER BY 1; +RESET duckdb.force_execution; +SELECT date_bin('1 hour'::interval, ts, '2026-01-01'::timestamptz) AS bucket, sum(val) +FROM public._events GROUP BY 1 ORDER BY 1; + +-- (B) A read the hot heap can answer keeps date_bin: it never reaches DuckDB. +EXPLAIN (COSTS OFF, VERBOSE) +SELECT date_bin('1 hour'::interval, ts, '2026-01-01'::timestamptz) AS bucket +FROM public.events WHERE ts >= '2026-04-01'::timestamptz; + +-- (C) A name that merely ends in date_bin( is a different function; it is left +-- alone. The read is not provably hot, so the rewrite pass does run on it. +CREATE FUNCTION public.undate_bin(interval, timestamptz, timestamptz) RETURNS timestamptz +LANGUAGE plpgsql AS $$ BEGIN RETURN $2; END $$; +EXPLAIN (COSTS OFF, VERBOSE) +SELECT undate_bin('1 hour'::interval, ts, '2026-01-01'::timestamptz) FROM public.events; + +-- Cleanup. +DROP FUNCTION public.undate_bin(interval, timestamptz, timestamptz); +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; diff --git a/extension/coldfront/test/sql/read_json_builders.sql b/extension/coldfront/test/sql/read_json_builders.sql new file mode 100644 index 0000000..85a1103 --- /dev/null +++ b/extension/coldfront/test/sql/read_json_builders.sql @@ -0,0 +1,80 @@ +-- jsonb_build_object / jsonb_agg (and their json_ twins) on a tiered read. The +-- read runs entirely in DuckDB, which has neither; a name swap cannot reach a +-- DuckDB counterpart (json_object is grammar-reserved in PostgreSQL, and DuckDB's +-- json_group_array is a macro that refuses ORDER BY), so the builders are rewritten +-- into the concat / to_json / array_agg form both engines evaluate identically. +-- White-box: the assertions are on the rewritten SQL, on PostgreSQL producing the +-- same JSON as jsonb's own rendering, and on DuckDB accepting the rewritten read +-- against the heap (duckdb.force_execution); no Iceberg I/O. +-- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress +-- db an earlier test may have created the extensions, standalone not. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +RESET client_min_messages; +SET TIME ZONE 'UTC'; +-- White-box: checks the generated SQL, not Iceberg I/O. +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; + +CREATE TABLE public._events (id int, ts timestamptz, name text, setting text); +CREATE VIEW public.events AS SELECT * FROM public._events; +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); +-- A NULL value and a value with a quote: both must survive as JSON. +INSERT INTO public._events VALUES (1, '2026-01-01 00:05+00', 'work_mem', '4MB'), + (2, '2026-01-01 01:07+00', 'max_conn', NULL), + (3, '2026-01-01 02:09+00', 'app_name', 'o''neil'); + +-- (A) The rewritten shapes: an object, an ordered aggregate of objects nested in +-- an object, a FILTERed aggregate, and a json_ (not jsonb_) aggregate of scalars. +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_build_object('name', name, 'value', setting)::text AS details FROM public.events; +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_build_object('count', count(*), + 'changes', jsonb_agg(jsonb_build_object('name', name, 'value', setting) ORDER BY name))::text +FROM public.events; +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_agg(name) FILTER (WHERE setting IS NOT NULL) FROM public.events; +EXPLAIN (COSTS OFF, VERBOSE) +SELECT json_agg(name ORDER BY name) FROM public.events; + +-- (B) JSON parity with jsonb's own rendering. The rewritten read, run by +-- PostgreSQL here, is captured and compared as jsonb against the builders on the +-- heap (no tiered view in that statement, so it is left untouched). +SELECT jsonb_build_object('count', count(*), + 'changes', jsonb_agg(jsonb_build_object('name', name, 'value', setting) ORDER BY name))::text AS details +FROM public.events \gset +SELECT :'details' AS rewritten_read; +SELECT :'details'::jsonb = jsonb_build_object('count', count(*), + 'changes', jsonb_agg(jsonb_build_object('name', name, 'value', setting) ORDER BY name)) AS same_json +FROM public._events; +SELECT jsonb_agg(name) FILTER (WHERE setting IS NOT NULL) AS filtered FROM public.events \gset +SELECT :'filtered'::jsonb = (SELECT jsonb_agg(name) FILTER (WHERE setting IS NOT NULL) FROM public._events) AS same_json; +SELECT json_agg(name ORDER BY name) AS scalars FROM public.events \gset +SELECT :'scalars'::jsonb = (SELECT json_agg(name ORDER BY name)::jsonb FROM public._events) AS same_json; + +-- The result is still JSON to the rest of the query: ->> works on it. +SELECT jsonb_build_object('name', name, 'value', setting) ->> 'value' AS value FROM public.events ORDER BY id; + +-- A view reference below the top level is rewritten too: the whole statement runs +-- in DuckDB, whichever branch names the view. +WITH changes AS (SELECT name, setting FROM public.events) +SELECT jsonb_build_object('name', name)::text FROM changes ORDER BY 1; + +-- (C) Parity against the live DuckDB: it executes the rewritten read (force_execution +-- scans the heap the way a cold read scans Parquet) and returns the same JSON. +SET duckdb.force_execution = true; +SELECT jsonb_build_object('count', count(*), + 'changes', jsonb_agg(jsonb_build_object('name', name, 'value', setting) ORDER BY name))::text AS details +FROM public.events; +SELECT json_agg(name ORDER BY name)::text AS scalars FROM public.events; +RESET duckdb.force_execution; + +-- Cleanup. +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; diff --git a/extension/coldfront/test/sql/read_param_fold.sql b/extension/coldfront/test/sql/read_param_fold.sql new file mode 100644 index 0000000..0d4583e --- /dev/null +++ b/extension/coldfront/test/sql/read_param_fold.sql @@ -0,0 +1,94 @@ +-- Bound parameters on a tiered read. pg_duckdb deparses $N as a placeholder, and +-- DuckDB cannot type a placeholder it sees only as one overload candidate among +-- several (time_bucket's third argument) or as a table-function argument +-- (generate_series), so the prepared read fails to plan. coldfront's planner hook +-- folds the bound values into the query before pg_duckdb plans it, and hands the +-- plan cache a prohibitively costed PostgreSQL plan when no values are bound (the +-- generic-plan build) so it keeps choosing value-bearing custom plans. White-box: +-- DuckDB runs the reads against the heap (duckdb.force_execution); no Iceberg I/O. +-- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress +-- db an earlier test may have created the extensions, standalone not. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +RESET client_min_messages; +SET TIME ZONE 'UTC'; +-- White-box: checks the generated SQL, not Iceberg I/O. +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; + +CREATE TABLE public._events (id int, ts timestamptz); +CREATE VIEW public.events AS SELECT * FROM public._events; +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); +INSERT INTO public._events VALUES (1, '2026-01-01 00:05+00'), + (2, '2026-01-01 00:50+00'), + (3, '2026-01-01 01:07+00'); +SET duckdb.force_execution = true; + +-- (A) Every parameter sits where DuckDB cannot type a placeholder: the bucket +-- origin and width, and all three generate_series arguments. Seven executions +-- cross the plan cache's switch from custom to generic planning after the fifth, +-- and every one returns the same rows with no planning warning. +PREPARE buckets(timestamptz, timestamptz, interval) AS +SELECT g.bucket, count(e.id) AS n +FROM generate_series($1, $2, $3) AS g(bucket) +LEFT JOIN public.events e ON time_bucket($3, e.ts, $1) = g.bucket +GROUP BY 1 ORDER BY 1; +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); +-- The plan is built from the values, so new values change the answer. +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 01:00+00', '30 minutes'); + +-- (B) plpgsql variables reach the planner through the parameter-fetch hook rather +-- than an array of values; the loop crosses the same custom-to-generic switch. +-- pg_duckdb gates DuckDB execution inside functions on its own setting. +SET duckdb.unsafe_allow_execution_inside_functions = true; +CREATE FUNCTION public.bucket_rows(p_from timestamptz, p_to timestamptz, p_w interval) +RETURNS bigint LANGUAGE plpgsql AS $$ +DECLARE + total bigint := 0; +BEGIN + FOR i IN 1..7 LOOP + total := total + (SELECT count(*) + FROM generate_series(p_from, p_to, p_w) AS g(bucket) + LEFT JOIN public.events e ON time_bucket(p_w, e.ts, p_from) = g.bucket); + END LOOP; + RETURN total; +END $$; +SELECT public.bucket_rows('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour') AS rows_over_7_runs; + +-- (C) A read without parameters is not touched: the same query with literals plans +-- as before. +SELECT g.bucket, count(e.id) AS n +FROM generate_series('2026-01-01 00:00+00'::timestamptz, '2026-01-01 03:00+00'::timestamptz, '1 hour'::interval) AS g(bucket) +LEFT JOIN public.events e ON time_bucket('1 hour'::interval, e.ts, '2026-01-01 00:00+00'::timestamptz) = g.bucket +GROUP BY 1 ORDER BY 1; + +-- (D) A parameter DuckDB types from its context (a comparison) is left alone, so +-- that read keeps a generic plan: it works even when the plan cache is forced to +-- plan without values. The read whose parameters need values has no generic plan; +-- forced to one, it is the placeholder PostgreSQL plan, and pg_duckdb's function +-- refuses to run outside DuckDB. +SET plan_cache_mode = force_generic_plan; +PREPARE since(timestamptz) AS SELECT count(*) FROM public.events WHERE ts > $1; +EXECUTE since('2026-01-01 00:30+00'); +EXECUTE buckets('2026-01-01 00:00+00', '2026-01-01 03:00+00', '1 hour'); +RESET plan_cache_mode; + +-- Cleanup. +RESET duckdb.force_execution; +DEALLOCATE since; +DEALLOCATE buckets; +DROP FUNCTION public.bucket_rows(timestamptz, timestamptz, interval); +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; From 70869a8159dc4c15451dad729a4a06ed6f96c041 Mon Sep 17 00:00:00 2001 From: Jimmy Angelakos Date: Fri, 28 Aug 2026 17:35:52 +0100 Subject: [PATCH 2/9] perf: read the tiered registry once per statement, not once per view reference --- extension/coldfront/Makefile | 2 +- extension/coldfront/src/coldfront.c | 161 ++++++++++++------ extension/coldfront/test/README.md | 1 + .../test/expected/registry_snapshot.out | 91 ++++++++++ .../coldfront/test/sql/registry_snapshot.sql | 65 +++++++ 5 files changed, 270 insertions(+), 50 deletions(-) create mode 100644 extension/coldfront/test/expected/registry_snapshot.out create mode 100644 extension/coldfront/test/sql/registry_snapshot.sql diff --git a/extension/coldfront/Makefile b/extension/coldfront/Makefile index a794cff..fb34c7e 100644 --- a/extension/coldfront/Makefile +++ b/extension/coldfront/Makefile @@ -25,7 +25,7 @@ REGRESS = load_order update_unregistered_view update_heap_table \ storage_secret_azure storage_secret_vended privilege_model \ partition_config_interval self_join_rejected returning_cold_rejected \ schema_collision drop_iceberg_table \ - read_date_bin read_json_builders read_param_fold \ + read_date_bin read_json_builders read_param_fold registry_snapshot \ vector_centroids vector_type_map vector_cold_render vector_param_render vector_ops vector_probe vector_status vector_assign vector_multicolumn REGRESS_OPTS = --inputdir=test --outputdir=test diff --git a/extension/coldfront/src/coldfront.c b/extension/coldfront/src/coldfront.c index fae637b..3b4fa51 100644 --- a/extension/coldfront/src/coldfront.c +++ b/extension/coldfront/src/coldfront.c @@ -283,68 +283,125 @@ typedef enum { TIER_HOT, TIER_COLD, TIER_AMBIGUOUS } TierClass; /* ---------- catalog lookup -------------------------------------------- */ /* - * Look up the tiered_views catalog row for relid via SPI, also fetching the - * current archive watermark (if any). vname must be get_rel_name(relid) — - * the caller already has it, so we avoid a redundant syscache hit. - * Returns true and populates *info (palloc'd into CurTransactionContext) - * if found; false otherwise. + * Per-statement snapshot of the tiered registry. + * + * Both hooks ask "is this relation a registered view?" once per range-table + * entry, and a statement may name several views, most of them not ours. The + * registry holds one row per managed table, so the whole of it is read once per + * statement and matched in memory, which keeps the cost off every view a + * statement happens to reference. + * + * The snapshot is keyed on the command id, so a registration made earlier in + * this transaction (create_iceberg_table(), then a write through the view it + * created) belongs to an earlier command and the next statement reloads and + * sees it. The rows live in TopTransactionContext, which transaction end frees; + * the pointer is cleared in coldfront_xact_callback, so a fresh transaction + * cannot match a stale command id. */ -static bool -lookup_tiered_view(Oid relid, const char *vname, TieredViewInfo *info) +typedef struct { + char *schema_name; + char *relname; + TieredViewInfo info; +} CfRegistryRow; + +static List *cf_registry = NIL; /* of CfRegistryRow * */ +static CommandId cf_registry_cid = InvalidCommandId; /* command it was read for */ + +static void +cf_load_registry(void) { - int ret; - bool found = false; - StringInfoData sql; + MemoryContext oldcxt; + uint64 i; - if (SPI_connect() != SPI_OK_CONNECT) - return false; + cf_registry = NIL; + cf_registry_cid = GetCurrentCommandId(false); - initStringInfo(&sql); - appendStringInfo(&sql, - "SELECT tv.hot_table, tv.iceberg_table, tv.partition_col, " - " tv.is_iceberg_only, aw.cutoff_time, tv.vec_columns IS NOT NULL " - "FROM coldfront.tiered_views tv " - "LEFT JOIN coldfront.archive_watermark aw ON aw.table_name = %s " - "WHERE tv.schema_name = %s AND tv.relname = %s", - quote_literal_cstr(vname), - quote_literal_cstr(get_namespace_name(get_rel_namespace(relid))), - quote_literal_cstr(vname)); - ret = SPI_execute(sql.data, true, 1); + /* Absent before CREATE EXTENSION, and while another extension's install + * script runs a query the hooks see. No registered views, so no rewrite. */ + if (!coldfront_registry_present()) + return; + if (SPI_connect() != SPI_OK_CONNECT) + return; - if (ret == SPI_OK_SELECT && SPI_processed == 1) + /* The watermark joins on table_name alone: it is keyed by table name. */ + if (SPI_execute( + "SELECT tv.schema_name, tv.relname, tv.hot_table, tv.iceberg_table, " + " tv.partition_col, tv.is_iceberg_only, aw.cutoff_time, " + " tv.vec_columns IS NOT NULL " + "FROM coldfront.tiered_views tv " + "LEFT JOIN coldfront.archive_watermark aw ON aw.table_name = tv.relname", + true, 0) == SPI_OK_SELECT) { - bool isnull; - Datum d; - char *s; - MemoryContext oldcxt = MemoryContextSwitchTo(CurTransactionContext); - - /* hot_table and partition_col are NULLable for iceberg-only rows. */ - s = SPI_getvalue(SPI_tuptable->vals[0], SPI_tuptable->tupdesc, 1); - info->hot_table = s ? pstrdup(s) : NULL; + oldcxt = MemoryContextSwitchTo(TopTransactionContext); + for (i = 0; i < SPI_processed; i++) + { + HeapTuple tup = SPI_tuptable->vals[i]; + TupleDesc td = SPI_tuptable->tupdesc; + CfRegistryRow *row = (CfRegistryRow *) palloc0(sizeof(CfRegistryRow)); + bool isnull; + Datum d; + + row->schema_name = SPI_getvalue(tup, td, 1); + row->relname = SPI_getvalue(tup, td, 2); + /* hot_table and partition_col are NULLable for iceberg-only rows. */ + row->info.hot_table = SPI_getvalue(tup, td, 3); + row->info.iceberg_table = SPI_getvalue(tup, td, 4); + row->info.partition_col = SPI_getvalue(tup, td, 5); + + d = SPI_getbinval(tup, td, 6, &isnull); + row->info.is_iceberg_only = !isnull && DatumGetBool(d); + + d = SPI_getbinval(tup, td, 7, &isnull); + row->info.has_cutoff = !isnull; + if (!isnull) + row->info.cutoff = DatumGetTimestampTz(d); + + d = SPI_getbinval(tup, td, 8, &isnull); + row->info.has_vector = !isnull && DatumGetBool(d); + + cf_registry = lappend(cf_registry, row); + } + MemoryContextSwitchTo(oldcxt); + } - info->iceberg_table = pstrdup(SPI_getvalue(SPI_tuptable->vals[0], - SPI_tuptable->tupdesc, 2)); + SPI_finish(); +} - s = SPI_getvalue(SPI_tuptable->vals[0], SPI_tuptable->tupdesc, 3); - info->partition_col = s ? pstrdup(s) : NULL; +/* + * Find the registry row for relid, also carrying the archive watermark (if + * any). vname must be get_rel_name(relid): the caller already has it, so we + * avoid a redundant syscache hit. Returns true and populates *info, whose + * strings belong to the snapshot and must not be modified, if found; false + * otherwise. Matching is by name, as the registry is keyed (it replicates by + * value across a mesh, where OIDs diverge), and the first match wins. + */ +static bool +lookup_tiered_view(Oid relid, const char *vname, TieredViewInfo *info) +{ + const char *nspname; + ListCell *lc; - d = SPI_getbinval(SPI_tuptable->vals[0], SPI_tuptable->tupdesc, 4, &isnull); - info->is_iceberg_only = !isnull && DatumGetBool(d); + if (cf_registry_cid != GetCurrentCommandId(false)) + cf_load_registry(); + if (cf_registry == NIL) + return false; - d = SPI_getbinval(SPI_tuptable->vals[0], SPI_tuptable->tupdesc, 5, &isnull); - info->has_cutoff = !isnull; - if (!isnull) - info->cutoff = DatumGetTimestampTz(d); + nspname = get_namespace_name(get_rel_namespace(relid)); + if (nspname == NULL) + return false; - d = SPI_getbinval(SPI_tuptable->vals[0], SPI_tuptable->tupdesc, 6, &isnull); - info->has_vector = !isnull && DatumGetBool(d); + foreach(lc, cf_registry) + { + CfRegistryRow *row = (CfRegistryRow *) lfirst(lc); - MemoryContextSwitchTo(oldcxt); - found = true; + if (strcmp(row->relname, vname) == 0 && /* nosemgrep */ + strcmp(row->schema_name, nspname) == 0) /* nosemgrep */ + { + *info = row->info; + return true; + } } - - SPI_finish(); - return found; + return false; } /* @@ -3497,6 +3554,12 @@ coldfront_xact_callback(XactEvent event, void *arg) if (event != XACT_EVENT_COMMIT && event != XACT_EVENT_ABORT) return; + /* The registry snapshot lives in TopTransactionContext, which this + * transaction's end frees. Drop the pointer with it, so the next + * transaction reloads rather than matching a repeated command id. */ + cf_registry = NIL; + cf_registry_cid = InvalidCommandId; + /* A lazy 'ice' ATTACH runs inside the user's transaction, so an abort rolls * the DuckDB ATTACH back. Clear the once-per-session guard so the next * tiered-view query re-attaches. Before the pending-release early-return diff --git a/extension/coldfront/test/README.md b/extension/coldfront/test/README.md index 613024a..fd19f95 100644 --- a/extension/coldfront/test/README.md +++ b/extension/coldfront/test/README.md @@ -43,6 +43,7 @@ extension's non-hook surface (third table below) and register no view. | `update_unregistered_view`, `update_heap_table` | unregistered / non-tiered relations pass through untouched | | `read_date_bin` | a read that DuckDB will run has `date_bin` rewritten to `time_bucket` (DuckDB executes it against the heap and agrees with `date_bin`); a hot-rerouted read and a look-alike function name are left alone | | `read_json_builders` | `jsonb_build_object` / `jsonb_agg` (and the `json_` twins) on a read that DuckDB will run become the `concat` / `to_json` / `array_agg` form; the result is JSON-equal to jsonb's rendering, keeps `ORDER BY` / `FILTER`, still takes `->>`, is rewritten below the top level too, and DuckDB executes it | +| `registry_snapshot` | the per-statement registry snapshot stays fresh within a transaction: a registration or a moved watermark from an earlier statement of the same transaction is seen by the next one, and a statement naming several views finds the registered one and leaves the others alone | ### `planner_hook`: bound parameters on a tiered read (executed) diff --git a/extension/coldfront/test/expected/registry_snapshot.out b/extension/coldfront/test/expected/registry_snapshot.out new file mode 100644 index 0000000..397cc34 --- /dev/null +++ b/extension/coldfront/test/expected/registry_snapshot.out @@ -0,0 +1,91 @@ +-- The registry is read once per statement and matched in memory. Two behaviours +-- that the snapshot must preserve: +-- (A) a registration made earlier in the same transaction is visible to the +-- next statement, so create_iceberg_table() followed by a write through +-- the view it created still rewrites. +-- (B) a statement naming several views finds the registered one among them, +-- and leaves the others alone. +-- White-box: the assertions are on the rewritten SQL, not on Iceberg I/O. +-- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress +-- db an earlier test may have created the extensions, standalone not. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +RESET client_min_messages; +SET TIME ZONE 'UTC'; +-- White-box: checks the generated SQL, not Iceberg I/O. +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; +-- (A) Register inside a transaction, then use the view in a later statement of +-- the same transaction. The cold UPDATE must still be rewritten through the +-- bakery, which it can only be if the hook sees the registration. +BEGIN; +CREATE TABLE public._events (id int, ts timestamptz, status text); +CREATE VIEW public.events AS SELECT * FROM public._events; +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); +EXPLAIN (COSTS OFF, VERBOSE) + UPDATE public.events SET status = 'x' WHERE ts < '2019-01-01'::timestamptz; + QUERY PLAN +----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Result + Output: _exec_iceberg_with_claim('ice.default.events'::text, 'UPDATE ice.default.events SET status = ''x''::text WHERE (ts < ''Tue Jan 01 00:00:00 2019 UTC''::timestamptz)'::text) +(2 rows) + +COMMIT; +-- The same holds for a watermark moved earlier in the transaction: the second +-- statement classifies against the new cutoff, not the one it started with. +BEGIN; +UPDATE coldfront.archive_watermark SET cutoff_time = '2018-01-01'::timestamptz + WHERE table_name = 'events'; +EXPLAIN (COSTS OFF) + UPDATE public.events SET status = 'y' WHERE ts < '2019-01-01'::timestamptz; + QUERY PLAN +----------------------------------------------------------------------------------------- + Nested Loop + CTE hot + -> Update on _events + -> Seq Scan on _events + Filter: (ts < 'Tue Jan 01 00:00:00 2019 UTC'::timestamp with time zone) + CTE cold + -> Result + -> CTE Scan on cold c + -> CTE Scan on hot h +(9 rows) + +ROLLBACK; +-- (B) Two plain views the registry does not know, named alongside the tiered +-- one. The tiered view is still found, so the builder is rewritten. The JSON +-- rewrite is the one to assert on here: its output is PostgreSQL's own plan, +-- where date_bin's would be a DuckDB plan carrying row estimates. +CREATE VIEW public.plain_a AS SELECT 1 AS k; +CREATE VIEW public.plain_b AS SELECT 1 AS k; +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_build_object('status', e.status)::text AS details +FROM public.events e, public.plain_a a, public.plain_b b +WHERE a.k = b.k; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------------------------------------------- + Seq Scan on public._events + Output: ((concat('{'::text, (to_json('status'::text))::text, ':'::text, COALESCE((to_json(_events.status))::text, 'null'::text), '}'::text))::json)::text +(2 rows) + +-- A statement naming only unregistered views is left alone entirely. +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_build_object('k', a.k)::text AS details +FROM public.plain_a a, public.plain_b b WHERE a.k = b.k; + QUERY PLAN +---------------------------------------------- + Result + Output: (jsonb_build_object('k', 1))::text +(2 rows) + +-- Cleanup. +DROP VIEW public.plain_a; +DROP VIEW public.plain_b; +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; diff --git a/extension/coldfront/test/sql/registry_snapshot.sql b/extension/coldfront/test/sql/registry_snapshot.sql new file mode 100644 index 0000000..b5ccd54 --- /dev/null +++ b/extension/coldfront/test/sql/registry_snapshot.sql @@ -0,0 +1,65 @@ +-- The registry is read once per statement and matched in memory. Two behaviours +-- that the snapshot must preserve: +-- (A) a registration made earlier in the same transaction is visible to the +-- next statement, so create_iceberg_table() followed by a write through +-- the view it created still rewrites. +-- (B) a statement naming several views finds the registered one among them, +-- and leaves the others alone. +-- White-box: the assertions are on the rewritten SQL, not on Iceberg I/O. +-- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress +-- db an earlier test may have created the extensions, standalone not. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +RESET client_min_messages; +SET TIME ZONE 'UTC'; +-- White-box: checks the generated SQL, not Iceberg I/O. +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; + +-- (A) Register inside a transaction, then use the view in a later statement of +-- the same transaction. The cold UPDATE must still be rewritten through the +-- bakery, which it can only be if the hook sees the registration. +BEGIN; +CREATE TABLE public._events (id int, ts timestamptz, status text); +CREATE VIEW public.events AS SELECT * FROM public._events; +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); +EXPLAIN (COSTS OFF, VERBOSE) + UPDATE public.events SET status = 'x' WHERE ts < '2019-01-01'::timestamptz; +COMMIT; + +-- The same holds for a watermark moved earlier in the transaction: the second +-- statement classifies against the new cutoff, not the one it started with. +BEGIN; +UPDATE coldfront.archive_watermark SET cutoff_time = '2018-01-01'::timestamptz + WHERE table_name = 'events'; +EXPLAIN (COSTS OFF) + UPDATE public.events SET status = 'y' WHERE ts < '2019-01-01'::timestamptz; +ROLLBACK; + +-- (B) Two plain views the registry does not know, named alongside the tiered +-- one. The tiered view is still found, so the builder is rewritten. The JSON +-- rewrite is the one to assert on here: its output is PostgreSQL's own plan, +-- where date_bin's would be a DuckDB plan carrying row estimates. +CREATE VIEW public.plain_a AS SELECT 1 AS k; +CREATE VIEW public.plain_b AS SELECT 1 AS k; +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_build_object('status', e.status)::text AS details +FROM public.events e, public.plain_a a, public.plain_b b +WHERE a.k = b.k; + +-- A statement naming only unregistered views is left alone entirely. +EXPLAIN (COSTS OFF, VERBOSE) +SELECT jsonb_build_object('k', a.k)::text AS details +FROM public.plain_a a, public.plain_b b WHERE a.k = b.k; + +-- Cleanup. +DROP VIEW public.plain_a; +DROP VIEW public.plain_b; +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; From c6a2627d09d19f0823f9569694889675dcd41a41 Mon Sep 17 00:00:00 2001 From: Jimmy Angelakos Date: Tue, 1 Sep 2026 15:27:42 +0100 Subject: [PATCH 3/9] feat: reject unmappable column types when registering a tiered table --- ci/journey.sh | 106 ++++++++++++++++++++++++++++++ docs/usage.md | 13 +++- internal/partcfg/commands.go | 42 ++++++++++++ internal/partcfg/commands_test.go | 93 ++++++++++++++++++++++++++ 4 files changed, 251 insertions(+), 3 deletions(-) diff --git a/ci/journey.sh b/ci/journey.sh index 88965f3..78598b1 100755 --- a/ci/journey.sh +++ b/ci/journey.sh @@ -142,6 +142,10 @@ assert_register_rejected() { fi } +# qdb : q against another database on the same server. Extensions are +# per-database, so a database story 1 never touched is stock PostgreSQL. +qdb() { local d="$1"; shift; docker exec -e PGUSER="$CF_DBUSER" -e PGDATABASE="$d" "$HOST" "$CF_PSQL" -tA -c "$*"; } + # archive_only — run the archiver with ONLY the partition_config rows matching a # keep predicate enabled, then restore the rows it disabled. The archiver takes # its table set from coldfront.partition_config, never from the YAML @@ -3328,6 +3332,62 @@ story_partitioner_remove() { q "$HOST" "DROP SCHEMA rmtest CASCADE;" >/dev/null 2>&1 } +# ─────────────────────────────────────────────────────────────────────────── +# Story: the standalone partition manager, on stock PostgreSQL. Every other story +# shares the database story 1 installed pg_duckdb and coldfront into, so nothing +# covers the configuration the partitioner is documented to run in. A database +# story 1 never touched is that configuration, on this same server: no extension, +# so partition_config is the Go-side DDL's to create, and a hot period has no +# cold tier behind it to type-check a column against. +# ─────────────────────────────────────────────────────────────────────────── +story_partitioner_stock_pg() { + step "TC-151: standalone partitioner on stock PostgreSQL (no extension)" + local db=cf_stock_pg + local dsn="host=${DB_IP} port=5432 dbname=$db user=coldfront password=coldfront sslmode=disable" + q "$HOST" "DROP DATABASE IF EXISTS $db;" >/dev/null 2>&1 + q "$HOST" "CREATE DATABASE $db;" >/dev/null + assert_eq "TC-151: the database carries neither extension" "0" \ + "$(qdb $db "SELECT count(*) FROM pg_extension WHERE extname IN ('coldfront','pg_duckdb');")" + qdb $db "CREATE TABLE public.stockev (id bigint GENERATED ALWAYS AS IDENTITY, + ts timestamptz NOT NULL, PRIMARY KEY (id, ts)) PARTITION BY RANGE (ts);" >/dev/null + + # With no extension to have created it, partition_config is materialized by + # the partitioner's own DDL, the copy that exists for exactly this case. + if "$PARTITIONER" register --dsn "$dsn" --table stockev --period monthly \ + --retention "12 months" >$TMPD/stock-reg.log 2>&1; then + assert_eq "TC-151: register materialized its own partition_config" "1" \ + "$(qdb $db "SELECT count(*) FROM coldfront.partition_config WHERE table_name='stockev';")" + else + fail "TC-151: register failed on stock PG"; tail -3 $TMPD/stock-reg.log + fi + + # The tsvector column TC-150 refuses on a tiered node registers here: with no + # extension there is no cold tier for a column type to be wrong about. + qdb $db "CREATE TABLE public.stockfts (id bigint NOT NULL, ts timestamptz NOT NULL, body text, + tsv tsvector GENERATED ALWAYS AS (to_tsvector('english', coalesce(body,''))) STORED, + PRIMARY KEY (id, ts)) PARTITION BY RANGE (ts);" >/dev/null + if "$PARTITIONER" register --dsn "$dsn" --table stockfts --period monthly \ + --hot-period "30 days" >$TMPD/stock-fts.log 2>&1; then + pass "TC-151: no extension, no cold tier, no column type check" + else + fail "TC-151: stock PG must not type-check a column"; tail -3 $TMPD/stock-fts.log + fi + + # And the job itself: a reconcile run creates the forward window. + printf 'postgres: { dsn: "%s" }\n' "$dsn" > $TMPD/stock.yaml + if "$PARTITIONER" --config $TMPD/stock.yaml >$TMPD/stock-run.log 2>&1; then + assert_ne "TC-151: reconcile created partitions" "0" \ + "$(qdb $db "SELECT count(*) FROM pg_inherits WHERE inhparent='public.stockev'::regclass;")" + else + fail "TC-151: reconcile failed on stock PG"; tail -5 $TMPD/stock-run.log + fi + + "$PARTITIONER" remove --dsn "$dsn" --table stockev >/dev/null 2>&1 + assert_eq "TC-151: remove unregistered it" "0" \ + "$(qdb $db "SELECT count(*) FROM coldfront.partition_config WHERE table_name='stockev';")" + q "$HOST" "DROP DATABASE $db;" >/dev/null +} + # ─────────────────────────────────────────────────────────────────────────── # Story — TC-071: a PARTITION BY RANGE (col1, col2) table is rejected at # archive time with a clear error. The PK check at register time passes (the @@ -4158,6 +4218,50 @@ story_bad_source_names_rejected() { q "$HOST" "DROP TABLE IF EXISTS public._mytest, public.${long53}, public.${long54} CASCADE;" >/dev/null 2>&1 } +# ─────────────────────────────────────────────────────────────────────────── +# Story: a column type the cold tier cannot store is refused at register, not +# hours later on the first archive cycle. tsvector is the canonical case: +# PostgreSQL's full-text pattern is a generated tsvector column, and Iceberg has +# no type for it. Registration goes through the extension's own type map, the one +# every cold write already uses, so what registers is what an archive cycle +# accepts. Partition-only management is untouched, since nothing about such a +# table ever reaches Iceberg. +# ─────────────────────────────────────────────────────────────────────────── +story_unmappable_column_rejected() { + step "TC-150: column type with no Iceberg mapping rejected at register" + local dsn="host=${DB_IP} port=5432 dbname=coldfront user=coldfront password=coldfront sslmode=disable" + q "$HOST" "CREATE TABLE IF NOT EXISTS public.tc150_fts ( + id bigint GENERATED ALWAYS AS IDENTITY, ts timestamptz NOT NULL, body text, + tsv tsvector GENERATED ALWAYS AS (to_tsvector('english', coalesce(body,''))) STORED, + PRIMARY KEY (id, ts) + ) PARTITION BY RANGE (ts);" >/dev/null + assert_eq "TC-150: fixture carries a generated tsvector column" "tsvector" \ + "$(q "$HOST" "SELECT format_type(atttypid, atttypmod) FROM pg_attribute + WHERE attrelid='public.tc150_fts'::regclass AND attname='tsv';")" + assert_register_rejected "TC-150: register named the unstorable type" \ + tc150_fts "PG type tsvector has no Iceberg-compatible mapping" + # The partitioner writes tiered rows through the same gate, and the archiver + # is what later reads them, so the refusal must not be dodgeable by + # registering from the binary that owns no cold tier. + if "$PARTITIONER" register --dsn "$dsn" --table tc150_fts \ + --period monthly --hot-period "30 days" >$TMPD/unmappable-part.log 2>&1; then + fail "TC-150: partitioner accepted a tiered row the archiver cannot process" + else + assert_contains "TC-150: partitioner refused it too" "tsvector" "$(cat $TMPD/unmappable-part.log)" + fi + # The same table is fine partition-only: --dry-run validates everything and + # writes nothing, so the acceptance is asserted without leaving a config row. + if "$ARCHIVER" register --config $TMPD/archiver.yaml --table tc150_fts \ + --period monthly --retention "5 years" --dry-run >$TMPD/unmappable-po.log 2>&1; then + pass "TC-150: the same table validates partition-only (no cold tier, no type check)" + else + fail "TC-150: partition-only must be unaffected, see $TMPD/unmappable-po.log"; tail -3 $TMPD/unmappable-po.log + fi + assert_eq "TC-150: still nothing registered after both binaries tried" "0" \ + "$(q "$HOST" "SELECT count(*) FROM coldfront.partition_config WHERE table_name='tc150_fts';")" + q "$HOST" "DROP TABLE IF EXISTS public.tc150_fts CASCADE;" >/dev/null 2>&1 +} + # ─────────────────────────────────────────────────────────────────────────── # Story — TC-114: TEMPORARY table invisible to archiver — register fails with # a "does not exist" error. TEMP tables are session-local; the archiver @@ -4783,6 +4887,7 @@ if [ "$MODE" = "tiered" ]; then story_partitioner_set_retention # TC-052: set --retention updates partition_config story_partitioner_disable_enable # TC-053: disable silently excludes; enable restores story_partitioner_remove # TC-054: remove unregisters; table intact + story_partitioner_stock_pg # TC-151: standalone partitioner on stock PG (no extension) story_composite_key_rejected # TC-071: RANGE (col1, col2) rejected at archive time story_iceberg_metadata # TC-043: cold data confirmed via Parquet metadata story_pg_dump_no_secrets # TC-058: storage secret not in pg_dump @@ -4795,6 +4900,7 @@ if [ "$MODE" = "tiered" ]; then story_unlogged_rejected # TC-113: UNLOGGED rejected at register story_case_collision_rejected # TC-141: name differing only by case rejected at register story_bad_source_names_rejected # TC-139/TC-142: leading underscore and over-long names rejected + story_unmappable_column_rejected # TC-150: column type with no Iceberg mapping rejected at register story_quoted_table_names # TC-140/TC-143/TC-144: dot, hyphen, space in table name story_temp_rejected # TC-114: TEMP table invisible to archiver story_list_partition_rejected # TC-115: LIST partition accepted at register; rejected at archive diff --git a/docs/usage.md b/docs/usage.md index 09aa1bf..a9656cf 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -419,6 +419,12 @@ one command rejects cannot be added by another. Registration fails when: - the name leaves no room for the generated partition suffix: 53 characters for monthly and 50 for daily, within PostgreSQL's 63-byte identifier limit. +- a tiered table carries a column whose type has no Iceberg equivalent + (see [Supported column types](#supported-column-types)). Every column + goes through the same type map the cold tier itself uses, so the answer + comes back at the prompt rather than hours later from cron. + Partition-only tables are exempt: nothing about them reaches Iceberg, + so their column types are PostgreSQL's business alone. Registration validates the table as it is at that moment, not continuously. Adding a `DEFAULT` partition to an already-registered table @@ -637,9 +643,10 @@ The following PostgreSQL column types are supported: `interval` Anything else (unbounded `numeric`, `xml`, `tsvector`, range/multirange -types, custom enums, arrays, composite types) is rejected at -table-creation time. We refuse silent fallback to `varchar` - losing -precision/identity is worse than no support. +types, custom enums, arrays, composite types) is rejected when a tiered +table is registered, and when a decoupled table is created. We refuse +silent fallback to `varchar` - losing precision/identity is worse than +no support. `char(N)` is stored and read as `varchar`. The data round-trips losslessly: values, comparisons, and `length()` match a hot PG table, diff --git a/internal/partcfg/commands.go b/internal/partcfg/commands.go index 2b210c9..8a3d8a3 100644 --- a/internal/partcfg/commands.go +++ b/internal/partcfg/commands.go @@ -193,6 +193,7 @@ type validateDB interface { // leading underscore, and short enough for the generated leaf names. // - every relation in the partition tree is WAL-logged (see requireLogged). // - the table has no DEFAULT partition (see requireNoDefaultPartition). +// - tiered only: every column has an Iceberg type (see requireMappableColumns). func validateRow(ctx context.Context, db validateDB, row configRow) error { // After the archiver's first-run swap the source is a VIEW over "_"+name, so // validate the PK / partition key against the real partitioned table. register @@ -216,9 +217,50 @@ func validateRow(ctx context.Context, db validateDB, row configRow) error { if err := validatePKSuperset(ctx, db, row.schema, base, row.column, row.subValues != ""); err != nil { return err } + if err := requireMappableColumns(ctx, db, row.schema, base, row.hot); err != nil { + return err + } return partition.ValidatePeriods(ctx, db, row.hot, row.retention) } +// requireMappableColumns rejects a tiered table carrying a column whose PG type +// the cold tier cannot store, which otherwise registers cleanly and hard-errors +// on the first archive cycle, hours later out of cron. Only a hot period makes a +// table's column types Iceberg's problem. The extension's own type map decides, +// in the database: it is the function every cold write and view rebuild already +// goes through, and asking it keeps Iceberg out of partition-core (a stock-PG +// partitioner node has no extension, and no cold tier to be wrong about). +func requireMappableColumns(ctx context.Context, db partition.RowQuerier, schema, table, hot string) error { + if hot == "" { + return nil + } + var coldTier bool + if err := db.QueryRow(ctx, + `SELECT EXISTS (SELECT 1 FROM pg_extension WHERE extname = 'coldfront')`).Scan(&coldTier); err != nil { + return fmt.Errorf("check for the coldfront extension: %w", err) + } + if !coldTier { + return nil + } + // count() forces the per-column call, which RAISES on the first unstorable + // type. The companion filter is the extension's own predicate, so a vector's + // generated real[] column is not read as a user column. + var checked int + if err := db.QueryRow(ctx, ` + SELECT count(coldfront._iceberg_storage_type(format_type(a.atttypid, a.atttypmod))) + FROM pg_attribute a + JOIN pg_class c ON c.oid = a.attrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = $1::text AND c.relname = $2::text + AND a.attnum > 0 AND NOT a.attisdropped + AND NOT coldfront._is_vec_companion(a.attname, a.attgenerated)`, + schema, table).Scan(&checked); err != nil { + return fmt.Errorf("%s.%s cannot be tiered: %w. Registering it without a "+ + "hot period manages partitions only, with no cold tier", schema, table, err) + } + return nil +} + // requireNoCaseCollision rejects a table whose name differs only by case from one // already registered. DuckDB matches identifiers case-insensitively even when // quoted, so public."Events" and public.events are one Iceberg table. The exact diff --git a/internal/partcfg/commands_test.go b/internal/partcfg/commands_test.go index a625b77..e1b369a 100644 --- a/internal/partcfg/commands_test.go +++ b/internal/partcfg/commands_test.go @@ -297,3 +297,96 @@ func TestRequireNoDefaultPartition_PropagatesQueryError(t *testing.T) { t.Fatal("query failure must not be reported as no default") } } + +// coldTierDB answers the two questions the column guard asks: does this database +// have the extension, then what does its type map say about the table's columns. +// checkErr stands in for the RAISE the extension throws on a type Iceberg cannot +// store. +type coldTierDB struct { + mockDB + installed bool + checkErr error + asked []string + args []any +} + +func (d *coldTierDB) QueryRow(_ context.Context, sql string, args ...any) pgx.Row { + d.asked = append(d.asked, sql) + if len(d.asked) == 1 { + return &mockRow{scan: func(dest ...any) error { + *(dest[0].(*bool)) = d.installed + return nil + }} + } + d.args = args + return &mockRow{scan: func(dest ...any) error { + if d.checkErr != nil { + return d.checkErr + } + *(dest[0].(*int)) = 3 + return nil + }} +} + +// The message the extension raises for the canonical case: PostgreSQL's +// full-text pattern is a generated tsvector column, and Iceberg has no type +// for it. +var errUnmappable = errors.New("ERROR: coldfront: PG type tsvector has no Iceberg-compatible mapping") + +func TestRequireMappableColumns_RejectsUnmappableType(t *testing.T) { + // The rejection carries the extension's own wording (which type), the table + // it came from, and the partition-only alternative, which is open at + // registration and gone by archive time. + db := &coldTierDB{installed: true, checkErr: errUnmappable} + err := requireMappableColumns(context.Background(), db, "public", "events", "1 month") + if err == nil { + t.Fatal("expected rejection") + } + for _, want := range []string{"public.events", "tsvector", "partitions only"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err.Error(), want) + } + } +} + +func TestRequireMappableColumns_SkipsPartitionOnlyRow(t *testing.T) { + // No hot period, no cold tier: the column types are PostgreSQL's business + // alone, and a table that registers today must keep registering. The same + // unmappable table passes, without the database being asked at all. + db := &coldTierDB{installed: true, checkErr: errUnmappable} + if err := requireMappableColumns(context.Background(), db, "public", "events", ""); err != nil { + t.Fatalf("a partition-only row must not be type-checked: %v", err) + } + if len(db.asked) != 0 { + t.Errorf("a partition-only row queried the database: %v", db.asked) + } +} + +func TestRequireMappableColumns_SkipsDatabaseWithoutTheExtension(t *testing.T) { + // A stock-PG partitioner node has no cold tier, so there is nothing for the + // row to be wrong about, and nothing to ask. + db := &coldTierDB{installed: false, checkErr: errUnmappable} + if err := requireMappableColumns(context.Background(), db, "public", "events", "1 month"); err != nil { + t.Fatalf("a database with no cold tier must not be type-checked: %v", err) + } + if len(db.asked) != 1 { + t.Errorf("expected only the extension probe, got %d queries", len(db.asked)) + } +} + +func TestRequireMappableColumns_AcceptsMappableTable(t *testing.T) { + db := &coldTierDB{installed: true} + if err := requireMappableColumns(context.Background(), db, "public", "events", "1 month"); err != nil { + t.Fatalf("a fully mappable table must pass: %v", err) + } + if len(db.args) != 2 || db.args[0] != "public" || db.args[1] != "events" { + t.Errorf("schema/table not passed as args: %v", db.args) + } + // The extension's map decides, not a second copy in Go, and its companion + // predicate is what keeps a vector's generated real[] column out of the check. + for _, want := range []string{"coldfront._iceberg_storage_type", "coldfront._is_vec_companion"} { + if !strings.Contains(db.asked[1], want) { + t.Errorf("the check does not go through %s", want) + } + } +} From 99f6686bea0e09fa59cf0b9b34bb6ba3d8f69f64 Mon Sep 17 00:00:00 2001 From: Jimmy Angelakos Date: Fri, 28 Aug 2026 17:35:44 +0100 Subject: [PATCH 4/9] fix: rewrite the JSON aggregates DuckDB lacks on the cold write path --- extension/coldfront/Makefile | 1 + extension/coldfront/src/coldfront.c | 78 ++++++-- .../test/expected/cold_write_json_agg.out | 169 ++++++++++++++++++ .../test/sql/cold_write_json_agg.sql | 84 +++++++++ 4 files changed, 319 insertions(+), 13 deletions(-) create mode 100644 extension/coldfront/test/expected/cold_write_json_agg.out create mode 100644 extension/coldfront/test/sql/cold_write_json_agg.sql diff --git a/extension/coldfront/Makefile b/extension/coldfront/Makefile index fb34c7e..60ac940 100644 --- a/extension/coldfront/Makefile +++ b/extension/coldfront/Makefile @@ -26,6 +26,7 @@ REGRESS = load_order update_unregistered_view update_heap_table \ partition_config_interval self_join_rejected returning_cold_rejected \ schema_collision drop_iceberg_table \ read_date_bin read_json_builders read_param_fold registry_snapshot \ + cold_write_json_agg \ vector_centroids vector_type_map vector_cold_render vector_param_render vector_ops vector_probe vector_status vector_assign vector_multicolumn REGRESS_OPTS = --inputdir=test --outputdir=test diff --git a/extension/coldfront/src/coldfront.c b/extension/coldfront/src/coldfront.c index 3b4fa51..8a3958e 100644 --- a/extension/coldfront/src/coldfront.c +++ b/extension/coldfront/src/coldfront.c @@ -757,7 +757,18 @@ classify_tier(Query *query, TieredViewInfo *info) /* drop_typmod: after substituting, skip a following "(...)" so a typmod that is * valid on the PG spelling but not on the DuckDB one does not survive. */ -typedef struct { const char *pg; const char *duck; bool drop_typmod; } CfSubst; +/* wrap: the replacement opens one paren more than the spelling it replaces + * (to_json(array_agg( for jsonb_agg(), so a closing paren is added at the + * matched call's own close. */ +typedef struct { + const char *pg; + const char *duck; + bool drop_typmod; + bool wrap; +} CfSubst; + +/* Deepest nesting of wrapped calls one statement may carry. */ +#define CF_WRAP_MAX 16 /* * Cold-WRITE substitutions. The deparsed cold DML is handed to DuckDB inside a @@ -768,21 +779,30 @@ typedef struct { const char *pg; const char *duck; bool drop_typmod; } CfSubst; * whose DuckDB name is not json_ are listed here, matched with the opening * paren so a column prefix can't false-match. A result DuckDB lacks (e.g. * json_set) errors in DuckDB — its boundary, not a rewrite coldfront withholds. + * + * The JSON aggregates are the one pair whose target is not a single name. + * DuckDB has neither json_agg nor jsonb_agg, and its json_group_array is a macro + * that refuses the ORDER BY an aggregate carries, so both spellings become + * to_json(array_agg(...)): array_agg is a DuckDB aggregate and keeps the + * ORDER BY. That target opens one paren more than the spelling it replaces, + * which is what `wrap` closes. */ static const CfSubst cf_write_subst[] = { - { "::timestamp with time zone", "::timestamptz", false }, - { "::timestamp without time zone", "::timestamp", false }, - { "::character varying", "::varchar", false }, - { "::double precision", "::double", false }, + { "::timestamp with time zone", "::timestamptz", false, false }, + { "::timestamp without time zone", "::timestamp", false, false }, + { "::character varying", "::varchar", false, false }, + { "::double precision", "::double", false, false }, /* pgvector's types are unknown to DuckDB, and the Iceberg column is FLOAT[]. * The dimension typmod goes with the name: FLOAT[](3) is not a type. The * cast's operand is already bracketed here, since a vector Const deparses * through pgvector's own output function. */ - { "::vector", "::FLOAT[]", true }, - { "::halfvec", "::FLOAT[]", true }, - { "jsonb_build_object(", "json_object(", false }, - { "jsonb_build_array(", "json_array(", false }, - { "to_jsonb(", "to_json(", false }, + { "::vector", "::FLOAT[]", true, false }, + { "::halfvec", "::FLOAT[]", true, false }, + { "jsonb_build_object(", "json_object(", false, false }, + { "jsonb_build_array(", "json_array(", false, false }, + { "to_jsonb(", "to_json(", false, false }, + { "jsonb_agg(", "to_json(array_agg(", false, true }, + { "json_agg(", "to_json(array_agg(", false, true }, }; /* @@ -803,9 +823,9 @@ static const CfSubst cf_write_subst[] = { * are rewritten on the node tree instead: see cf_json_builder_mutator. */ static const CfSubst cf_read_subst[] = { - { "::jsonb", "::json", false }, - { "jsonb_array_length(", "json_array_length(", false }, - { "date_bin(", "time_bucket(", false }, + { "::jsonb", "::json", false, false }, + { "jsonb_array_length(", "json_array_length(", false, false }, + { "date_bin(", "time_bucket(", false, false }, }; /* @@ -822,6 +842,9 @@ cf_apply_subst(const char *sql, const CfSubst *map, int map_len, bool jsonb_catc const char *p = sql; bool in_quote = false; bool in_dquote = false; + int depth = 0; /* paren depth, outside quotes */ + int wrap_at[CF_WRAP_MAX]; /* depths owing a closing paren */ + int nwrap = 0; initStringInfo(&buf); while (*p) @@ -885,6 +908,19 @@ cf_apply_subst(const char *sql, const CfSubst *map, int map_len, bool jsonb_catc if (*p == ')') p++; } + /* The spelling ends with its own '(', already consumed, so + * the call's arguments sit one level in. Its close is the + * ')' that brings the depth back to where it started. */ + if (map[i].wrap) + { + if (nwrap == CF_WRAP_MAX) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("coldfront: JSON aggregates nested deeper than %d", + CF_WRAP_MAX))); + wrap_at[nwrap++] = depth; + depth++; + } replaced = true; break; } @@ -910,6 +946,22 @@ cf_apply_subst(const char *sql, const CfSubst *map, int map_len, bool jsonb_catc continue; } } + + /* Track the depth the wrap flag closes against, and emit the added + * paren when a wrapped call's own close is reached. */ + if (*p == '(') + depth++; + else if (*p == ')' && depth > 0) + { + depth--; + if (nwrap > 0 && wrap_at[nwrap - 1] == depth) + { + nwrap--; + appendStringInfoString(&buf, "))"); + p++; + continue; + } + } } appendStringInfoChar(&buf, *p++); diff --git a/extension/coldfront/test/expected/cold_write_json_agg.out b/extension/coldfront/test/expected/cold_write_json_agg.out new file mode 100644 index 0000000..44286f3 --- /dev/null +++ b/extension/coldfront/test/expected/cold_write_json_agg.out @@ -0,0 +1,169 @@ +-- A JSON aggregate on the cold write path. DuckDB has neither jsonb_agg nor +-- json_agg, and its json_group_array is a macro that refuses the ORDER BY an +-- aggregate carries, so the rewrite target is to_json(array_agg(...)): both +-- engines have those, and array_agg keeps the ORDER BY. The substitution adds a +-- closing paren at the aggregate's own, which is what the map's `wrap` flag +-- does. Two halves: +-- (A) white-box: the deparsed cold SQL carries to_json(array_agg(...)) for an +-- UPDATE (with and without ORDER BY) and for an INSERT ... SELECT. +-- (B) parity: DuckDB accepts the target and rejects what it replaces. +-- White-box: we do NOT exercise Iceberg I/O. +-- Suppress NOTICEs: raw_query echoes each DuckDB result as a (version-dependent) +-- NOTICE, and CREATE EXTENSION emits "already exists" depending on suite order. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +SET TIME ZONE 'UTC'; +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; +CREATE TABLE public._events (id int, ts timestamptz, data jsonb); +CREATE VIEW public.events AS SELECT * FROM public._events; +CREATE TABLE public.src (k text); +INSERT INTO public.src VALUES ('b'), ('a'); +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); +-- (A1) Cold UPDATE, aggregate carrying ORDER BY: the ORDER BY rides along inside +-- array_agg, which is an aggregate in DuckDB (json_group_array is not). +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = (SELECT jsonb_agg(k ORDER BY k) FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + QUERY PLAN +-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Result + Output: _exec_iceberg_with_claim('ice.default.events'::text, 'UPDATE ice.default.events SET data = ( SELECT to_json(array_agg(src.k ORDER BY src.k)) AS json_agg FROM src) WHERE (ts < ''Tue Jan 01 00:00:00 2019 UTC''::timestamptz)'::text) +(2 rows) + +-- (A2) Cold UPDATE, no ORDER BY. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = (SELECT jsonb_agg(k) FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + QUERY PLAN +----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Result + Output: _exec_iceberg_with_claim('ice.default.events'::text, 'UPDATE ice.default.events SET data = ( SELECT to_json(array_agg(src.k)) AS json_agg FROM src) WHERE (ts < ''Tue Jan 01 00:00:00 2019 UTC''::timestamptz)'::text) +(2 rows) + +-- (A3) The json_ spelling reaches DuckDB the same way: the catch-all leaves it +-- alone (no jsonb token), so it needs its own entry. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = (SELECT json_agg(k)::jsonb FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Result + Output: _exec_iceberg_with_claim('ice.default.events'::text, 'UPDATE ice.default.events SET data = ( SELECT (to_json(array_agg(src.k)))::json AS json_agg FROM src) WHERE (ts < ''Tue Jan 01 00:00:00 2019 UTC''::timestamptz)'::text) +(2 rows) + +-- (A4) Cold INSERT ... SELECT: the aggregate sits in the row source that the +-- cold leg streams out of PostgreSQL. +EXPLAIN (COSTS OFF, VERBOSE) +INSERT INTO public.events +SELECT 9, '2019-01-01'::timestamptz, jsonb_agg(k ORDER BY k) FROM public.src; + QUERY PLAN +--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Result + Output: (InitPlan 3).col1, (InitPlan 4).col1 + CTE hot_ins + -> Insert on public._events + Output: 1 + -> Aggregate + Output: 9, 'Tue Jan 01 00:00:00 2019 UTC'::timestamp with time zone, jsonb_agg(k ORDER BY k) + Filter: false + -> Sort + Output: k + Sort Key: k + -> Result + Output: k + One-Time Filter: false + CTE cold_call + -> Result + Output: _exec_iceberg_with_claim('ice.default.events'::text, 'INSERT INTO ice.default.events SELECT id, ts, data FROM (SELECT 9, ''Tue Jan 01 00:00:00 2019 UTC''::timestamptz AS timestamptz, to_json(array_agg(pglocal.public.src.k ORDER BY pglocal.public.src.k)) AS json_agg FROM pglocal.public.src) AS coldfront_src(id, ts, data) WHERE ts < ''Sun Mar 01 00:00:00 2026 UTC''::timestamptz'::text) + InitPlan 3 + -> Aggregate + Output: count(*) + -> CTE Scan on hot_ins + Output: hot_ins."?column?" + InitPlan 4 + -> Aggregate + Output: count(*) + -> CTE Scan on cold_call + Output: cold_call._exec_iceberg_with_claim +(27 rows) + +-- (A5) An aggregate nested inside an object builder: the added paren must land +-- at the aggregate's own close, leaving the builder's intact. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events +SET data = (SELECT jsonb_build_object('all', jsonb_agg(k ORDER BY k)) FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + QUERY PLAN +--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Result + Output: _exec_iceberg_with_claim('ice.default.events'::text, 'UPDATE ice.default.events SET data = ( SELECT json_object(''all'', to_json(array_agg(src.k ORDER BY src.k))) AS json_build_object FROM src) WHERE (ts < ''Tue Jan 01 00:00:00 2019 UTC''::timestamptz)'::text) +(2 rows) + +-- (A6) A hot-tier UPDATE is plain PostgreSQL DML: jsonb_agg stays, and the +-- column stays jsonb. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = (SELECT jsonb_agg(k) FROM public.src) +WHERE ts >= '2026-06-01'::timestamptz; + QUERY PLAN +------------------------------------------------------------------------------------------ + Update on public._events + InitPlan 1 + -> Aggregate + Output: jsonb_agg(src.k) + -> Seq Scan on public.src + Output: src.k + -> Seq Scan on public._events + Output: (InitPlan 1).col1, _events.ctid + Filter: (_events.ts >= 'Mon Jun 01 00:00:00 2026 UTC'::timestamp with time zone) +(9 rows) + +-- (A7) The aggregate's name inside a string literal is not a call: left intact. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = to_jsonb('jsonb_agg(x)'::text) +WHERE ts < '2019-01-01'::timestamptz; + QUERY PLAN +----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Result + Output: _exec_iceberg_with_claim('ice.default.events'::text, 'UPDATE ice.default.events SET data = to_json(''jsonb_agg(x)''::text) WHERE (ts < ''Tue Jan 01 00:00:00 2019 UTC''::timestamptz)'::text) +(2 rows) + +-- (B) Parity against the live DuckDB: the target is accepted (ordered and not), +-- and both spellings it replaces are rejected. A void row = accepted. +SELECT duckdb.raw_query($$ SELECT to_json(array_agg(x ORDER BY x)) FROM (VALUES ('b'),('a')) v(x) $$); + raw_query +----------- + +(1 row) + +SELECT duckdb.raw_query($$ SELECT to_json(array_agg(x)) FROM (VALUES ('b'),('a')) v(x) $$); + raw_query +----------- + +(1 row) + +SELECT duckdb.raw_query($$ SELECT json_agg(x) FROM (VALUES ('a')) v(x) $$); +ERROR: (PGDuckDB/pgduckdb_raw_query_cpp) Catalog Error: Scalar Function with name json_agg does not exist! +Did you mean "json"? + +LINE 1: SELECT json_agg(x) FROM (VALUES ('a')) v(x) + ^ +SELECT duckdb.raw_query($$ SELECT jsonb_agg(x) FROM (VALUES ('a')) v(x) $$); +ERROR: (PGDuckDB/pgduckdb_raw_query_cpp) Catalog Error: Scalar Function with name jsonb_agg does not exist! +Did you mean "json"? + +LINE 1: SELECT jsonb_agg(x) FROM (VALUES ('a')) v(x) + ^ +-- json_group_array is why the target is not that: a macro cannot take ORDER BY. +SELECT duckdb.raw_query($$ SELECT json_group_array(x ORDER BY x) FROM (VALUES ('a')) v(x) $$); +ERROR: (PGDuckDB/pgduckdb_raw_query_cpp) Invalid Input Error: Function "json_group_array" is a Macro Function. "DISTINCT", "FILTER", and "ORDER BY" are only applicable to aggregate functions. +-- Cleanup. +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; +DROP TABLE public.src; diff --git a/extension/coldfront/test/sql/cold_write_json_agg.sql b/extension/coldfront/test/sql/cold_write_json_agg.sql new file mode 100644 index 0000000..6546f68 --- /dev/null +++ b/extension/coldfront/test/sql/cold_write_json_agg.sql @@ -0,0 +1,84 @@ +-- A JSON aggregate on the cold write path. DuckDB has neither jsonb_agg nor +-- json_agg, and its json_group_array is a macro that refuses the ORDER BY an +-- aggregate carries, so the rewrite target is to_json(array_agg(...)): both +-- engines have those, and array_agg keeps the ORDER BY. The substitution adds a +-- closing paren at the aggregate's own, which is what the map's `wrap` flag +-- does. Two halves: +-- (A) white-box: the deparsed cold SQL carries to_json(array_agg(...)) for an +-- UPDATE (with and without ORDER BY) and for an INSERT ... SELECT. +-- (B) parity: DuckDB accepts the target and rejects what it replaces. +-- White-box: we do NOT exercise Iceberg I/O. +-- Suppress NOTICEs: raw_query echoes each DuckDB result as a (version-dependent) +-- NOTICE, and CREATE EXTENSION emits "already exists" depending on suite order. +SET client_min_messages = warning; +CREATE EXTENSION IF NOT EXISTS pg_duckdb; +CREATE EXTENSION IF NOT EXISTS coldfront; +SET TIME ZONE 'UTC'; +SET coldfront.warehouse = ''; +SET coldfront.lakekeeper_endpoint = ''; + +CREATE TABLE public._events (id int, ts timestamptz, data jsonb); +CREATE VIEW public.events AS SELECT * FROM public._events; +CREATE TABLE public.src (k text); +INSERT INTO public.src VALUES ('b'), ('a'); +INSERT INTO coldfront.tiered_views(schema_name, relname, hot_table, iceberg_table, partition_col) +VALUES ('public', 'events', 'public._events', 'ice.default.events', 'ts'); +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('public', 'events', '2026-03-01'::timestamptz); + +-- (A1) Cold UPDATE, aggregate carrying ORDER BY: the ORDER BY rides along inside +-- array_agg, which is an aggregate in DuckDB (json_group_array is not). +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = (SELECT jsonb_agg(k ORDER BY k) FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + +-- (A2) Cold UPDATE, no ORDER BY. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = (SELECT jsonb_agg(k) FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + +-- (A3) The json_ spelling reaches DuckDB the same way: the catch-all leaves it +-- alone (no jsonb token), so it needs its own entry. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = (SELECT json_agg(k)::jsonb FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + +-- (A4) Cold INSERT ... SELECT: the aggregate sits in the row source that the +-- cold leg streams out of PostgreSQL. +EXPLAIN (COSTS OFF, VERBOSE) +INSERT INTO public.events +SELECT 9, '2019-01-01'::timestamptz, jsonb_agg(k ORDER BY k) FROM public.src; + +-- (A5) An aggregate nested inside an object builder: the added paren must land +-- at the aggregate's own close, leaving the builder's intact. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events +SET data = (SELECT jsonb_build_object('all', jsonb_agg(k ORDER BY k)) FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + +-- (A6) A hot-tier UPDATE is plain PostgreSQL DML: jsonb_agg stays, and the +-- column stays jsonb. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = (SELECT jsonb_agg(k) FROM public.src) +WHERE ts >= '2026-06-01'::timestamptz; + +-- (A7) The aggregate's name inside a string literal is not a call: left intact. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events SET data = to_jsonb('jsonb_agg(x)'::text) +WHERE ts < '2019-01-01'::timestamptz; + +-- (B) Parity against the live DuckDB: the target is accepted (ordered and not), +-- and both spellings it replaces are rejected. A void row = accepted. +SELECT duckdb.raw_query($$ SELECT to_json(array_agg(x ORDER BY x)) FROM (VALUES ('b'),('a')) v(x) $$); +SELECT duckdb.raw_query($$ SELECT to_json(array_agg(x)) FROM (VALUES ('b'),('a')) v(x) $$); +SELECT duckdb.raw_query($$ SELECT json_agg(x) FROM (VALUES ('a')) v(x) $$); +SELECT duckdb.raw_query($$ SELECT jsonb_agg(x) FROM (VALUES ('a')) v(x) $$); +-- json_group_array is why the target is not that: a macro cannot take ORDER BY. +SELECT duckdb.raw_query($$ SELECT json_group_array(x ORDER BY x) FROM (VALUES ('a')) v(x) $$); + +-- Cleanup. +DELETE FROM coldfront.tiered_views; +DELETE FROM coldfront.archive_watermark; +DROP VIEW public.events; +DROP TABLE public._events; +DROP TABLE public.src; From 7e190e58ed3862ed57764e39ce49c0ef39f1bec4 Mon Sep 17 00:00:00 2001 From: Jimmy Angelakos Date: Tue, 1 Sep 2026 16:14:45 +0100 Subject: [PATCH 5/9] fix: join the registry snapshot's watermark by schema and table --- extension/coldfront/src/coldfront.c | 4 +-- .../test/expected/registry_snapshot.out | 30 ++++++++++++++++--- .../coldfront/test/sql/registry_snapshot.sql | 24 ++++++++++++--- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/extension/coldfront/src/coldfront.c b/extension/coldfront/src/coldfront.c index 8a3958e..df3d8cb 100644 --- a/extension/coldfront/src/coldfront.c +++ b/extension/coldfront/src/coldfront.c @@ -323,13 +323,13 @@ cf_load_registry(void) if (SPI_connect() != SPI_OK_CONNECT) return; - /* The watermark joins on table_name alone: it is keyed by table name. */ if (SPI_execute( "SELECT tv.schema_name, tv.relname, tv.hot_table, tv.iceberg_table, " " tv.partition_col, tv.is_iceberg_only, aw.cutoff_time, " " tv.vec_columns IS NOT NULL " "FROM coldfront.tiered_views tv " - "LEFT JOIN coldfront.archive_watermark aw ON aw.table_name = tv.relname", + "LEFT JOIN coldfront.archive_watermark aw " + " ON aw.schema_name = tv.schema_name AND aw.table_name = tv.relname", true, 0) == SPI_OK_SELECT) { oldcxt = MemoryContextSwitchTo(TopTransactionContext); diff --git a/extension/coldfront/test/expected/registry_snapshot.out b/extension/coldfront/test/expected/registry_snapshot.out index 397cc34..4a81d3f 100644 --- a/extension/coldfront/test/expected/registry_snapshot.out +++ b/extension/coldfront/test/expected/registry_snapshot.out @@ -1,9 +1,11 @@ --- The registry is read once per statement and matched in memory. Two behaviours --- that the snapshot must preserve: +-- The registry is read once per statement and matched in memory. Three +-- behaviours that the snapshot must preserve: -- (A) a registration made earlier in the same transaction is visible to the -- next statement, so create_iceberg_table() followed by a write through -- the view it created still rewrites. --- (B) a statement naming several views finds the registered one among them, +-- (B) the watermark attaches by (schema_name, table_name), never by table +-- name alone. +-- (C) a statement naming several views finds the registered one among them, -- and leaves the others alone. -- White-box: the assertions are on the rewritten SQL, not on Iceberg I/O. -- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress @@ -56,7 +58,27 @@ EXPLAIN (COSTS OFF) (9 rows) ROLLBACK; --- (B) Two plain views the registry does not know, named alongside the tiered +-- (B) The watermark joins by (schema_name, table_name), the key it is stored +-- under: a same-named table's watermark in another schema must not attach to +-- this view. With public.events's own row gone the view has no cutoff, so the +-- write stays plain hot-tier DML; the decoy row would otherwise classify it +-- cold. +BEGIN; +DELETE FROM coldfront.archive_watermark + WHERE schema_name = 'public' AND table_name = 'events'; +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('other', 'events', '2026-03-01'::timestamptz); +EXPLAIN (COSTS OFF) + UPDATE public.events SET status = 'z' WHERE ts < '2019-01-01'::timestamptz; + QUERY PLAN +--------------------------------------------------------------------------------- + Update on _events + -> Seq Scan on _events + Filter: (ts < 'Tue Jan 01 00:00:00 2019 UTC'::timestamp with time zone) +(3 rows) + +ROLLBACK; +-- (C) Two plain views the registry does not know, named alongside the tiered -- one. The tiered view is still found, so the builder is rewritten. The JSON -- rewrite is the one to assert on here: its output is PostgreSQL's own plan, -- where date_bin's would be a DuckDB plan carrying row estimates. diff --git a/extension/coldfront/test/sql/registry_snapshot.sql b/extension/coldfront/test/sql/registry_snapshot.sql index b5ccd54..73697f8 100644 --- a/extension/coldfront/test/sql/registry_snapshot.sql +++ b/extension/coldfront/test/sql/registry_snapshot.sql @@ -1,9 +1,11 @@ --- The registry is read once per statement and matched in memory. Two behaviours --- that the snapshot must preserve: +-- The registry is read once per statement and matched in memory. Three +-- behaviours that the snapshot must preserve: -- (A) a registration made earlier in the same transaction is visible to the -- next statement, so create_iceberg_table() followed by a write through -- the view it created still rewrites. --- (B) a statement naming several views finds the registered one among them, +-- (B) the watermark attaches by (schema_name, table_name), never by table +-- name alone. +-- (C) a statement naming several views finds the registered one among them, -- and leaves the others alone. -- White-box: the assertions are on the rewritten SQL, not on Iceberg I/O. -- Suppress the run-order-dependent "already exists" NOTICE: in the shared regress @@ -40,7 +42,21 @@ EXPLAIN (COSTS OFF) UPDATE public.events SET status = 'y' WHERE ts < '2019-01-01'::timestamptz; ROLLBACK; --- (B) Two plain views the registry does not know, named alongside the tiered +-- (B) The watermark joins by (schema_name, table_name), the key it is stored +-- under: a same-named table's watermark in another schema must not attach to +-- this view. With public.events's own row gone the view has no cutoff, so the +-- write stays plain hot-tier DML; the decoy row would otherwise classify it +-- cold. +BEGIN; +DELETE FROM coldfront.archive_watermark + WHERE schema_name = 'public' AND table_name = 'events'; +INSERT INTO coldfront.archive_watermark(schema_name, table_name, cutoff_time) +VALUES ('other', 'events', '2026-03-01'::timestamptz); +EXPLAIN (COSTS OFF) + UPDATE public.events SET status = 'z' WHERE ts < '2019-01-01'::timestamptz; +ROLLBACK; + +-- (C) Two plain views the registry does not know, named alongside the tiered -- one. The tiered view is still found, so the builder is rewritten. The JSON -- rewrite is the one to assert on here: its output is PostgreSQL's own plan, -- where date_bin's would be a DuckDB plan carrying row estimates. From 203d3331844c13a838acb2daca6443477bf0f7bc Mon Sep 17 00:00:00 2001 From: Jimmy Angelakos Date: Tue, 1 Sep 2026 16:15:04 +0100 Subject: [PATCH 6/9] fix: count nested builder parens when closing the JSON aggregate wrap --- extension/coldfront/src/coldfront.c | 25 +++++++++++-------- .../test/expected/cold_write_json_agg.out | 20 +++++++++++++++ .../test/sql/cold_write_json_agg.sql | 10 ++++++++ 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/extension/coldfront/src/coldfront.c b/extension/coldfront/src/coldfront.c index df3d8cb..9652a07 100644 --- a/extension/coldfront/src/coldfront.c +++ b/extension/coldfront/src/coldfront.c @@ -908,17 +908,22 @@ cf_apply_subst(const char *sql, const CfSubst *map, int map_len, bool jsonb_catc if (*p == ')') p++; } - /* The spelling ends with its own '(', already consumed, so - * the call's arguments sit one level in. Its close is the - * ')' that brings the depth back to where it started. */ - if (map[i].wrap) + /* A function spelling ends with its own '(', already + * consumed, so the call's arguments sit one level in and + * the depth must count it. The call's close is the ')' + * that brings the depth back to where it started, and a + * wrapped call owes an added close there. */ + if (map[i].pg[plen - 1] == '(') { - if (nwrap == CF_WRAP_MAX) - ereport(ERROR, - (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), - errmsg("coldfront: JSON aggregates nested deeper than %d", - CF_WRAP_MAX))); - wrap_at[nwrap++] = depth; + if (map[i].wrap) + { + if (nwrap == CF_WRAP_MAX) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("coldfront: JSON aggregates nested deeper than %d", + CF_WRAP_MAX))); + wrap_at[nwrap++] = depth; + } depth++; } replaced = true; diff --git a/extension/coldfront/test/expected/cold_write_json_agg.out b/extension/coldfront/test/expected/cold_write_json_agg.out index 44286f3..7ed61f8 100644 --- a/extension/coldfront/test/expected/cold_write_json_agg.out +++ b/extension/coldfront/test/expected/cold_write_json_agg.out @@ -132,6 +132,20 @@ WHERE ts < '2019-01-01'::timestamptz; Output: _exec_iceberg_with_claim('ice.default.events'::text, 'UPDATE ice.default.events SET data = to_json(''jsonb_agg(x)''::text) WHERE (ts < ''Tue Jan 01 00:00:00 2019 UTC''::timestamptz)'::text) (2 rows) +-- (A8) The reverse nesting of (A5): a builder inside the aggregate, with an +-- operator applied to the builder's result. The builder's own paren also counts +-- toward the depth, so the added paren still lands at the aggregate's close, +-- keeping the operator and the ORDER BY inside array_agg. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events +SET data = (SELECT jsonb_agg(jsonb_build_object('k', k) ->> 'k' ORDER BY k) FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + QUERY PLAN +---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Result + Output: _exec_iceberg_with_claim('ice.default.events'::text, 'UPDATE ice.default.events SET data = ( SELECT to_json(array_agg((json_object(''k'', src.k) ->> ''k''::text) ORDER BY src.k)) AS json_agg FROM src) WHERE (ts < ''Tue Jan 01 00:00:00 2019 UTC''::timestamptz)'::text) +(2 rows) + -- (B) Parity against the live DuckDB: the target is accepted (ordered and not), -- and both spellings it replaces are rejected. A void row = accepted. SELECT duckdb.raw_query($$ SELECT to_json(array_agg(x ORDER BY x)) FROM (VALUES ('b'),('a')) v(x) $$); @@ -146,6 +160,12 @@ SELECT duckdb.raw_query($$ SELECT to_json(array_agg(x)) FROM (VALUES ('b'),('a') (1 row) +SELECT duckdb.raw_query($$ SELECT to_json(array_agg(json_object('k', x) ->> 'k' ORDER BY x)) FROM (VALUES ('b'),('a')) v(x) $$); + raw_query +----------- + +(1 row) + SELECT duckdb.raw_query($$ SELECT json_agg(x) FROM (VALUES ('a')) v(x) $$); ERROR: (PGDuckDB/pgduckdb_raw_query_cpp) Catalog Error: Scalar Function with name json_agg does not exist! Did you mean "json"? diff --git a/extension/coldfront/test/sql/cold_write_json_agg.sql b/extension/coldfront/test/sql/cold_write_json_agg.sql index 6546f68..c0581b2 100644 --- a/extension/coldfront/test/sql/cold_write_json_agg.sql +++ b/extension/coldfront/test/sql/cold_write_json_agg.sql @@ -67,10 +67,20 @@ EXPLAIN (COSTS OFF, VERBOSE) UPDATE public.events SET data = to_jsonb('jsonb_agg(x)'::text) WHERE ts < '2019-01-01'::timestamptz; +-- (A8) The reverse nesting of (A5): a builder inside the aggregate, with an +-- operator applied to the builder's result. The builder's own paren also counts +-- toward the depth, so the added paren still lands at the aggregate's close, +-- keeping the operator and the ORDER BY inside array_agg. +EXPLAIN (COSTS OFF, VERBOSE) +UPDATE public.events +SET data = (SELECT jsonb_agg(jsonb_build_object('k', k) ->> 'k' ORDER BY k) FROM public.src) +WHERE ts < '2019-01-01'::timestamptz; + -- (B) Parity against the live DuckDB: the target is accepted (ordered and not), -- and both spellings it replaces are rejected. A void row = accepted. SELECT duckdb.raw_query($$ SELECT to_json(array_agg(x ORDER BY x)) FROM (VALUES ('b'),('a')) v(x) $$); SELECT duckdb.raw_query($$ SELECT to_json(array_agg(x)) FROM (VALUES ('b'),('a')) v(x) $$); +SELECT duckdb.raw_query($$ SELECT to_json(array_agg(json_object('k', x) ->> 'k' ORDER BY x)) FROM (VALUES ('b'),('a')) v(x) $$); SELECT duckdb.raw_query($$ SELECT json_agg(x) FROM (VALUES ('a')) v(x) $$); SELECT duckdb.raw_query($$ SELECT jsonb_agg(x) FROM (VALUES ('a')) v(x) $$); -- json_group_array is why the target is not that: a macro cannot take ORDER BY. From 42b2cf1c52a801132fe61ec40354e3025c79d2b4 Mon Sep 17 00:00:00 2001 From: Jimmy Angelakos Date: Tue, 1 Sep 2026 16:15:04 +0100 Subject: [PATCH 7/9] docs: note the force_generic_plan exception to the custom-plan pricing --- docs/architecture.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 173ece6..0338e69 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -269,8 +269,12 @@ engines accept (see plans it when a parameter sits where DuckDB cannot type a placeholder (a direct argument of a pg_duckdb function, any argument of a table function); the plan cache's generic-plan build, which carries no values, -gets a PostgreSQL plan priced above any custom plan, so the read is -planned from its values on every execution. +gets a PostgreSQL plan priced above any custom plan, so under the plan +cache's cost-based selection the read is planned from its values on every +execution. `plan_cache_mode = force_generic_plan` bypasses that selection +and picks the value-less generic plan, which fails with `only works with +DuckDB execution` (see +[usage.md → Supported column types](usage.md#supported-column-types)). The following table maps each operation to its interface and routing path: From 1e5aaa5d80227a3b78202ef2781068269858238a Mon Sep 17 00:00:00 2001 From: Jimmy Angelakos Date: Tue, 1 Sep 2026 16:46:29 +0100 Subject: [PATCH 8/9] refactor: rename CF_DECOY_PLAN_COST to CF_GENERIC_PLAN_COST --- extension/coldfront/src/coldfront.c | 8 ++++---- extension/coldfront/test/expected/registry_snapshot.out | 4 ++-- extension/coldfront/test/sql/registry_snapshot.sql | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/extension/coldfront/src/coldfront.c b/extension/coldfront/src/coldfront.c index 9652a07..5046a41 100644 --- a/extension/coldfront/src/coldfront.c +++ b/extension/coldfront/src/coldfront.c @@ -4408,7 +4408,7 @@ register_gucs(void) * alone and keeps its generic plan. A DuckDB function is recognised as one the * pg_duckdb extension owns: every function it declares stands for a DuckDB one. */ -#define CF_DECOY_PLAN_COST 1.0e10 +#define CF_GENERIC_PLAN_COST 1.0e10 typedef struct { ParamListInfo params; } FoldParamsCtx; typedef struct { Oid duckdb_ext; bool in_table_func; } NeedsValueCtx; @@ -4521,10 +4521,10 @@ coldfront_planner(Query *parse, const char *query_string, int cursor_options, if (bound_params == NULL) { - PlannedStmt *decoy = standard_planner(parse, query_string, cursor_options, NULL); + PlannedStmt *generic = standard_planner(parse, query_string, cursor_options, NULL); - decoy->planTree->total_cost = CF_DECOY_PLAN_COST; - return decoy; + generic->planTree->total_cost = CF_GENERIC_PLAN_COST; + return generic; } parse = query_tree_mutator(parse, fold_params_mutator, &fc, 0); } diff --git a/extension/coldfront/test/expected/registry_snapshot.out b/extension/coldfront/test/expected/registry_snapshot.out index 4a81d3f..9ffc1b9 100644 --- a/extension/coldfront/test/expected/registry_snapshot.out +++ b/extension/coldfront/test/expected/registry_snapshot.out @@ -61,8 +61,8 @@ ROLLBACK; -- (B) The watermark joins by (schema_name, table_name), the key it is stored -- under: a same-named table's watermark in another schema must not attach to -- this view. With public.events's own row gone the view has no cutoff, so the --- write stays plain hot-tier DML; the decoy row would otherwise classify it --- cold. +-- write stays plain hot-tier DML; the other schema's row would otherwise +-- classify it cold. BEGIN; DELETE FROM coldfront.archive_watermark WHERE schema_name = 'public' AND table_name = 'events'; diff --git a/extension/coldfront/test/sql/registry_snapshot.sql b/extension/coldfront/test/sql/registry_snapshot.sql index 73697f8..1995d32 100644 --- a/extension/coldfront/test/sql/registry_snapshot.sql +++ b/extension/coldfront/test/sql/registry_snapshot.sql @@ -45,8 +45,8 @@ ROLLBACK; -- (B) The watermark joins by (schema_name, table_name), the key it is stored -- under: a same-named table's watermark in another schema must not attach to -- this view. With public.events's own row gone the view has no cutoff, so the --- write stays plain hot-tier DML; the decoy row would otherwise classify it --- cold. +-- write stays plain hot-tier DML; the other schema's row would otherwise +-- classify it cold. BEGIN; DELETE FROM coldfront.archive_watermark WHERE schema_name = 'public' AND table_name = 'events'; From 138bcea68486019a2dc2f5e11789204a8594ca54 Mon Sep 17 00:00:00 2001 From: Jimmy Angelakos Date: Tue, 1 Sep 2026 18:28:48 +0100 Subject: [PATCH 9/9] fix: free superseded registry snapshots at reload, not transaction end --- extension/coldfront/src/coldfront.c | 32 +++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/extension/coldfront/src/coldfront.c b/extension/coldfront/src/coldfront.c index 5046a41..a55816f 100644 --- a/extension/coldfront/src/coldfront.c +++ b/extension/coldfront/src/coldfront.c @@ -294,9 +294,11 @@ typedef enum { TIER_HOT, TIER_COLD, TIER_AMBIGUOUS } TierClass; * The snapshot is keyed on the command id, so a registration made earlier in * this transaction (create_iceberg_table(), then a write through the view it * created) belongs to an earlier command and the next statement reloads and - * sees it. The rows live in TopTransactionContext, which transaction end frees; - * the pointer is cleared in coldfront_xact_callback, so a fresh transaction - * cannot match a stale command id. + * sees it. The rows live in a child context of TopTransactionContext that each + * reload resets, so a superseded snapshot is freed at the next load rather + * than accumulating until transaction end; the pointers are cleared in + * coldfront_xact_callback, so a fresh transaction cannot match a stale + * command id. */ typedef struct { char *schema_name; @@ -304,8 +306,9 @@ typedef struct { TieredViewInfo info; } CfRegistryRow; -static List *cf_registry = NIL; /* of CfRegistryRow * */ -static CommandId cf_registry_cid = InvalidCommandId; /* command it was read for */ +static List *cf_registry = NIL; /* of CfRegistryRow * */ +static CommandId cf_registry_cid = InvalidCommandId; /* command it was read for */ +static MemoryContext cf_registry_cxt = NULL; /* holds the snapshot's rows */ static void cf_load_registry(void) @@ -316,6 +319,16 @@ cf_load_registry(void) cf_registry = NIL; cf_registry_cid = GetCurrentCommandId(false); + /* The snapshot's own context: created on first use, reset (freeing the + * superseded snapshot) on every reload, freed with its parent at + * transaction end. */ + if (cf_registry_cxt == NULL) + cf_registry_cxt = AllocSetContextCreate(TopTransactionContext, + "coldfront registry snapshot", + ALLOCSET_SMALL_SIZES); + else + MemoryContextReset(cf_registry_cxt); + /* Absent before CREATE EXTENSION, and while another extension's install * script runs a query the hooks see. No registered views, so no rewrite. */ if (!coldfront_registry_present()) @@ -332,7 +345,7 @@ cf_load_registry(void) " ON aw.schema_name = tv.schema_name AND aw.table_name = tv.relname", true, 0) == SPI_OK_SELECT) { - oldcxt = MemoryContextSwitchTo(TopTransactionContext); + oldcxt = MemoryContextSwitchTo(cf_registry_cxt); for (i = 0; i < SPI_processed; i++) { HeapTuple tup = SPI_tuptable->vals[i]; @@ -3611,11 +3624,12 @@ coldfront_xact_callback(XactEvent event, void *arg) if (event != XACT_EVENT_COMMIT && event != XACT_EVENT_ABORT) return; - /* The registry snapshot lives in TopTransactionContext, which this - * transaction's end frees. Drop the pointer with it, so the next - * transaction reloads rather than matching a repeated command id. */ + /* The registry snapshot's context is a child of TopTransactionContext, + * which this transaction's end frees. Drop the pointers with it, so the + * next transaction reloads rather than matching a repeated command id. */ cf_registry = NIL; cf_registry_cid = InvalidCommandId; + cf_registry_cxt = NULL; /* A lazy 'ice' ATTACH runs inside the user's transaction, so an abort rolls * the DuckDB ATTACH back. Clear the once-per-session guard so the next