catalog: add pg_type.typsend and type typreceive as regproc - #37896
Conversation
The Apache Arrow ADBC PostgreSQL driver runs a type-resolution query at connect time that references pg_catalog.pg_type.typsend. Materialize did not expose that column, so every adbc_driver_postgresql connection failed with `column "typsend" does not exist` before any user query ran. Carry the real PostgreSQL typsend OID per builtin type, verified against a live PostgreSQL 16 server for all 76 types that have a PG counterpart. Nine types get 0, matching PostgreSQL, which gives them no binary send function. _mz_aclitem is Materialize-invented and takes array_send by analogy with every other array type. typsend is appended to mz_internal.mz_type_pg_metadata, then projected through mz_internal.pg_type_all_databases and pg_catalog.pg_type. The new columns are appended rather than placed at PostgreSQL's ordinal positions, so existing column positions stay stable. pg_catalog.pg_type.typreceive also becomes regproc, which is what PostgreSQL declares and what lets a client's typreceive::TEXT comparison resolve a function name instead of digits. The underlying all-databases view keeps oid, because it backs a builtin index and resolving a regproc to a name reads current_database(), which is not materializable. Adds a test/adbc mzcompose suite covering the driver end to end. Connecting and streaming rows are hard assertions. Full Arrow type fidelity is not yet reachable, because Materialize encodes regproc over pgwire as a bare OID rather than as the function name, so the driver's name-keyed type map misses and every column resolves to Arrow binary. Those tests are marked expectedFailure so they report an unexpected success once that encoding is fixed.
`ci/test/lint-main/checks/check-python-files.sh` runs pyright with `--warnings`, and `pyproject.toml` maps `reportMissingImports` to a warning, so any import pyright cannot resolve fails the lint-and-rustfmt job. `adbc-driver-postgresql` and `duckdb` are pinned in `test/adbc/requirements.txt` and installed only inside the composition's test container, never in the repo-wide virtualenv pyright resolves against, so both imports reported as unresolved. Suppress the two imports with targeted `# pyright: ignore` comments rather than pulling the drivers into `misc/python/requirements.txt`, which would install heavy native wheels into every developer's virtualenv purely to satisfy a lint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K8HcpvmmntWK9QuJWvKVbg
PostgreSQL renders a regproc through regprocout, which resolves the OID to the function's name. Materialize emitted the bare OID, so a client that keys on the text of pg_catalog.pg_type.typreceive, as the Apache Arrow ADBC PostgreSQL driver does, missed every lookup and degraded every column to opaque binary. Value::from_datum collapsed Oid, RegClass, RegProc and RegType into one Value::Oid, erasing the reg flavor before encode_text ran. RegProc gets its own Value variant, so the text encoding can resolve the name while the binary encoding stays a bare 4-byte OID, matching regprocsend. Resolution needs no catalog access. Materialize has no CREATE FUNCTION, so all 626 functions are builtins and the OID to name mapping is fixed when the binary is built. That matters because mz-pgrepr sits below the catalog crates and two of its encode paths, the Kafka FORMAT TEXT sink and COPY TO with an S3 target, run in clusterd where no catalog exists. Resolving from a static table therefore fixes every text rendering uniformly, at no per-statement cost and with no signature changes, so mz-pgwire, mz-pgcopy, mz-storage-operators, mz-interchange and mz-adapter are untouched. The table lives in mz-pgrepr-consts, which already holds the OID constants and has no dependencies. mz-pgrepr cannot read mz_sql::func directly, because mz-sql depends on mz-pgrepr, so the entries are checked in as data and mz_catalog::builtin's test_regproc_names_match_builtin_functions recomputes the whole table from the builtin registry, failing with the corrected contents on drift. Renderings follow regprocout, verified against PostgreSQL 16. OID 0 prints `-`, an OID naming no function prints in decimal, a uniquely named function prints bare, and an overloaded name prints schema-qualified, so one of the six abs impls prints pg_catalog.abs. That rule reproduces Materialize's own regproc::text cast for all 626 function OIDs, so the two paths agree except on OID 0, where the cast prints 0 and PostgreSQL prints `-`. decode_text accepts all of those renderings back, which keeps COPY TO output loadable by COPY FROM and matches regprocin, including its refusal of an ambiguous name. regclass and regtype keep rendering as digits. They can name objects a user created, which no static table can know. The JSON encoder keeps emitting a number, because build_row_schema_json in the same module declares these columns as 4-byte unsigned integers and the Avro encoder writes them that way. The HTTP SQL API shares that encoder. typreceive now resolves for all 67 builtin types that have one, which is the column the ADBC driver reads, so the driver's type fidelity tests become hard assertions rather than expected failures. typsend still renders in decimal, because the builtin registry defines receive functions but no send functions, leaving those OIDs out of the table.
A rendering that is schema-qualified because its name is overloaded still names every one of that name's impls, so decode_text cannot pick one and returns the same error PostgreSQL's regprocin does. The encoding test asserted pg_catalog.abs round tripped, contradicting the decoding test right below it, and parse_regproc's doc comment claimed round tripping without that qualification. Also skip the reverse-lookup coverage test under miri. Covering every entry costs a table scan per entry, which is milliseconds natively and far too slow interpreted.
The `typsend` addition to `mz_internal.mz_type_pg_metadata` was registered as a `MigrationStep::evolution`. `Evolution` is the wrong mechanism for a builtin table. Under `Evolution` the read-only side of a 0dt upgrade never registers the new schema. `migrate_evolve_one` only checks `backward_compatible` and returns, leaving the shard advertising the old three-column schema while the new binary's catalog describes four. Every dataflow the read-only environment must hydrate before it reports ready, here the `pg_type_all_databases_ind` builtin index, then has to read the old parts through persist's part-migration path. No builtin table has ever taken a column addition, so that combination is unexercised. `Replacement` is what the rest of the list uses. It is also what populates `MigrationResult::replaced_items`, which flows into `migrated_storage_collections_0dt` and switches on the read-only back-fill in `Coordinator::bootstrap`. The new shard is written directly and the dependent dataflow hydrates from it. Replacement discards the old shard contents, which costs nothing here. `Coordinator::bootstrap_tables` retracts every system table's full persist snapshot on each boot and re-derives the rows from the catalog. The only two builtin tables whose contents must survive, `mz_storage_usage_by_shard` and `mz_object_arrangement_size_history`, are excluded there and rejected outright by `validate_migration_steps`. `mz_type_pg_metadata` is in neither set. Also corrects the justification for `typsend` being `nullable(true)`. It claimed persist schema evolution requires an appended field to be nullable. That rule exists in `mz_persist_types::schema::backward_compatible`, but it is unreachable for row columns because `RowColumnarEncoder::finish` hardcodes Arrow nullability to `true` for every column. The real situation is that the declaration is simply wider than `pack_type_update` needs.
`regprocout` quotes a rendering that is not a bare identifier, printing `"left"` and `pg_catalog."substring"`. The static table leaves those unquoted to match Materialize's `regproc` to `text` cast, so the wire format and the in-product cast agree. Record that divergence and why the unquoted form is still loadable. Measured against PostgreSQL 16.14. Qualification itself is not a divergence: `regprocout` qualifies whenever the bare name would not be found uniquely by `regprocin`, so a merely overloaded name such as `abs` renders `pg_catalog.abs` there too, independent of `search_path`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K8HcpvmmntWK9QuJWvKVbg
Adding typsend to pg_type_all_databases widens the view from 19 to 20 columns, which shifts column references throughout the recorded EXPLAIN plans and adds an oidtoregproc(coalesce(#12{typsend}, 0)) projection alongside the existing oidtoregproc(#10{typinput}). Regenerated from the rewrite-results diff emitted by the slt-4 job of Buildkite build #129572, which reported the two expected output failures in this file (lines 4203 and 19091) and no others. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K8HcpvmmntWK9QuJWvKVbg
| // `Evolution` a read-only replica would instead have to read the old | ||
| // parts through the schema-migration path, which no builtin table has | ||
| // ever exercised. |
There was a problem hiding this comment.
No, you were right to push. "every system table" was false: bootstrap_tables filters through is_retained_across_restarts, which holds back mz_storage_usage_by_shard and mz_object_arrangement_size_history (coord.rs:3066-3076), and validate_migration_steps asserts against migrating those same two. The conclusion for mz_type_pg_metadata still holds, it just isn't one of the retained ones. Reworded to say exactly that and to name them.
While in there I also stopped asserting the rest and traced it instead: replaced_items is derived from MigrationRunResult::new_shards, which only migrate_replace populates, and it reaches the coordinator as migrated_storage_collections_0dt, which gates the read-only back-fill at coord.rs:2813. And I dropped "no builtin table has ever exercised" since it isn't checkable (the forced-migration knob can drive Evolution over anything); it now says the list's only Evolution step targets a source, which is verifiable from MIGRATIONS.
Generated by Claude Code
There was a problem hiding this comment.
Cut it. You're right that replacement needs no defending: 32 of the 33 steps in that list use it, so it is the convention and the module docs already carry the rule. The entry is down to one line saying what changed about the object, matching its neighbours. The fact that evolution failed 0dt-upgrade-smoke-test was the only durable bit in there, and the PR description already records it.
I took this as licence to audit the whole diff rather than just that entry, and cut 86 lines of comment net across 11 files. The test I applied: a comment stays only if it records a non-obvious constraint, a measured PostgreSQL behaviour, or a reason a tempting simplification is wrong. Things that went:
- Enumerations of the match arms directly below them, for example the
regprocoutrendering rules invalue.rs. - Motivation that belongs in the PR description, for example why clients care about
typreceivenames. - The same fact stated three times in one file (the overloaded-name ambiguity in
value.rs). - "Appended rather than placed at PostgreSQL's ordinal position", duplicated in
pg_typeandpg_type_all_databases. Appending is what you would do anyway. - The
typsend_oidfield doc, now a one-liner matchingtypinput_oidandtypreceive_oidinstead of adding a caveat neither of them makes. - The
json.rsnote is 7 lines down to 4. It stays because it stops someone "fixing" that arm to emit names and contradicting the integer schemabuild_row_schema_jsonpublishes below it.
What I kept, trimmed: the regproc vs oid asymmetry between pg_type and pg_type_all_databases, the reserved-word and search-path divergences from regprocout, the OID provenance you asked for, and the COALESCE note, since a LEFT JOIN making a non-nullable column nullable is genuinely surprising.
Comment-only, no golden moved. 8596cf05c4.
Generated by Claude Code
| // TODO: Tighten to `nullable(false)` to match `typinput` and | ||
| // `typreceive`. That also changes the descs of the views projecting the | ||
| // column. |
There was a problem hiding this comment.
Why can't we tighten it here and just handle the downstream views? We are already touching them.
There was a problem hiding this comment.
Done, typsend is now SqlScalarType::Oid.nullable(false) and the TODO is gone. Nothing downstream actually needed to move: both views already declared typsend as RegProc.nullable(false), and the COALESCE(..., 0) in pg_type_all_databases has to stay because it guards the LEFT JOIN against this table, not the column's declaration. typreceive is the precedent (already non-nullable in the table, still coalesced), and typinput shows the other half of it, non-nullable in the table but nullable(true) in the view because nothing coalesces it.
Only other change was console/types/materialize.d.ts, where typsend becomes Generated<number> to match typinput/typreceive. The existing MigrationStep::replacement covers the fingerprint change on its own, since BuiltinTable::fingerprint is just the desc and replacement rebuilds the shard.
Generated by Claude Code
| /// This table matches the SQL cast from `regproc` to `text` instead, so the two | ||
| /// Materialize paths agree with each other. The unquoted form still resolves, | ||
| /// because `regprocin` accepts a reserved word with or without quotes. | ||
| pub const NAMES: &[(u32, &str)] = &[ |
There was a problem hiding this comment.
where does this mapping come from? can we link to some canonical source from the pg docs or something?
There was a problem hiding this comment.
Not from the pg docs, and I did not want to link a page that doesn't actually document it. Expanded the module comment to state the real provenance: the table is generated from Materialize's builtin function registry via BUILTINS::funcs() (the same registry behind mz_catalog.mz_functions, which pg_catalog.pg_proc is a view over), and test_regproc_names_match_builtin_functions recomputes the whole thing and prints the corrected table on drift.
For the OIDs themselves it now points at src/include/catalog/pg_proc.dat in the postgres tree, which is where mz_sql::func's PG_CATALOG_BUILTINS says its literals came from, and notes that everything else comes from crate::oid's FIRST_UNPINNED_OID / FIRST_MATERIALIZE_OID ranges. It says outright that no PostgreSQL document lists this mapping.
Generated by Claude Code
There was a problem hiding this comment.
Can it add the Materialize-specific OIDs on the fly? Otherwise we always have to keep two places in sync.
There was a problem hiding this comment.
No, the OIDs aren't derivable, so there's nothing to compute on the fly. Every OID in the table, PostgreSQL's 435 and Materialize's 196, is a literal in a mz_sql::func builtins! arm (e.g. src/sql/src/func.rs:1868), the Materialize ones hand-assigned from FIRST_MATERIALIZE_OID. The table is a projection of that registry, and the registry sits above the encoder: mz-catalog → mz-sql → mz-pgrepr → mz-pgrepr-consts, whose [dependencies] is empty.
That isn't only a layering nicety. Value::encode_text also runs in clusterd, which links no catalog at all (src/interchange/src/text_binary.rs:37 for Kafka sink text, src/storage-operators/src/s3_oneshot_sink/pgcopy.rs:103 for COPY TO S3), so a runtime-populated table would render pg_catalog.lower over pgwire and 870 in a sink, silently and per-process.
You're right that the remedy shouldn't be pasting a panic message, though. The drift test now writes the file itself: REWRITE=1 cargo test -p mz-catalog test_regproc_names_match_builtin_functions regenerates the table in place, matching the REWRITE=1 idiom in src/compute-types/tests/lir_schema.rs. Two places still exist, but keeping them in sync is one command.
Generated by Claude Code
`pack_type_update` always writes an OID for `typsend`, so declare the column `nullable(false)` to match `typinput` and `typreceive`. The `COALESCE`s in `pg_type_all_databases` stay, because they guard the `LEFT JOIN` against this table rather than the column's declaration. `typreceive` is the precedent: it is already `nullable(false)` and still needs its `COALESCE`, and `typinput` shows the other side, non-nullable in the base table but `nullable(true)` in the view because nothing coalesces it. Update the console's generated `MzTypePgMetadata.typsend` to match. The two views' `typsend` descs already read `RegProc.nullable(false)` and are unchanged, since `COALESCE(x, 0)` is non-nullable either way. Document where `regproc::NAMES` comes from: the builtin function registry reached through `BUILTINS::funcs()`, kept honest by `test_regproc_names_match_builtin_functions`, with PostgreSQL-assigned OIDs for the compatible subset and `crate::oid` ranges for the rest. Correct the `MigrationStep::replacement` justification. `bootstrap_tables` does not re-derive *every* system table, it holds back `mz_storage_usage_by_shard` and `mz_object_arrangement_size_history` through `is_retained_across_restarts`. Name them, and state that `mz_type_pg_metadata` is not among them. Also trace `replaced_items` to `new_shards` and `migrated_storage_collections_0dt` instead of asserting the back-fill, and replace the unfalsifiable "no builtin table has ever exercised" with the checkable fact that the list's only `Evolution` step targets a source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K8HcpvmmntWK9QuJWvKVbg
Cut restatement, motivation, and duplicated reasoning from the comments this branch adds. What stays records a non-obvious constraint, a measured PostgreSQL behaviour, or a reason a tempting simplification is wrong. The `mz_type_pg_metadata` migration step no longer defends its use of `MigrationStep::replacement`, which is what every other step in the list uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K8HcpvmmntWK9QuJWvKVbg
| // Mirrors PostgreSQL's `regprocout`. | ||
| // | ||
| // NOTE: The SQL cast from `regproc` to `text` renders OID 0 as `0` | ||
| // rather than `-`, so the two Materialize paths disagree on that one | ||
| // value. PostgreSQL prints `-` for both. |
There was a problem hiding this comment.
this is probably a deviation we shouldn't accept before shipping
There was a problem hiding this comment.
Agreed, this one shouldn't ship. Fixed in 2d7aff0.
I measured PostgreSQL 16.13 first rather than assuming, and it prints - for all three reg types at OID 0, through both the type's own output function and an explicit cast to text:
SELECT 0::regproc::text, 0::regclass::text, 0::regtype::text;
- | - | -
SELECT 0::regproc, 0::regclass, 0::regtype;
- | - | -
SELECT 99999::regproc::text, 99999::regclass::text, 99999::regtype::text;
99999 | 99999 | 99999
So regclassout and regtypeout agree with regprocout here, and a nonzero OID that names nothing keeps its digits. NULL::reg*::text stays NULL in all three.
Since it's the same bug in all three, I fixed the shared REG_STRING_CAST_TEMPLATE rather than special-casing regproc, so regproc, regclass and regtype now all render OID 0 as -. I fixed the cast rather than the wire encoding on purpose: making pgwire print 0 would have made our two paths agree while moving both away from PostgreSQL.
I checked the blast radius before touching the shared template, since a builtin view plan change would move catalog_server_explain.slt. It doesn't move, and the reason is structural: all three reg-to-text casts are registered Explicit, so no implicit or assignment coercion can reach the template, and no builtin view or index SQL casts a reg type to text. No builtin plan changes, and catalog_server_explain.slt is not in the diff.
No existing golden asserted the old 0. The reg-to-text casts in cockroach/pgoidtype.slt are all onlyif cockroach, the one in cockroach/orms.slt returns no rows because pg_constraint is empty, and pg-cdc/types-reg.td ingests regproc as a text column, so PostgreSQL does that rendering itself. Added coverage for OID 0 and for a nonzero unmatched OID to regproc.slt, regclass.slt and regtype.slt, and shrank the NOTE: here to just the regprocout line, since the divergence it documented is gone.
Generated by Claude Code
There was a problem hiding this comment.
One nuance worth stating explicitly, since path agreement was the point here.
For regproc the two paths now agree at OID 0 and both match PostgreSQL. For regclass and regtype the pgwire TEXT path still emits 0, because those two collapse into Value::Oid and render as digits by design. That is the pre-existing, documented behaviour in the NOTE at value.rs:150-153: they can name objects a user created, which no static table can know, so name resolution for them lives in the SQL cast. That wire-vs-cast difference already applies at every OID, not just 0. 't'::regclass is 20196 on the wire and t through the cast today.
So this change doesn't introduce a new class of divergence for those two. It makes the one path that can resolve names agree with PostgreSQL for all three. Aligning the wire encoder for regclass/regtype would be a separate decision, and the only golden it would touch is test/pgtest-mz/datums.pt:26, where _regproc is already - while _regtype and _regclass are 0. I left that alone as out of scope here.
Generated by Claude Code
The SQL casts from `regproc`, `regclass` and `regtype` to `text` rendered OID
0 as `0`. OID 0 misses the `mz_objects` join, so the cast fell through to
stringifying the OID. PostgreSQL's `regprocout`, `regclassout` and `regtypeout`
all render OID 0 as `-`, and so does our own pgwire TEXT output for `regproc`.
The SQL cast therefore disagreed with both PostgreSQL and our wire path.
Add an OID 0 case to the shared cast template so all three reg types render
`-`. A nonzero OID that names nothing still renders as its digits, matching
PostgreSQL. NULL still casts to NULL.
Measured against PostgreSQL 16.13:
SELECT 0::regproc::text, 0::regclass::text, 0::regtype::text;
- | - | -
SELECT 99999::regproc::text, 99999::regclass::text, 99999::regtype::text;
99999 | 99999 | 99999
Fixing all three is safe for builtin plans. Every reg-to-text cast is
registered `Explicit`, so no implicit or assignment coercion can reach the
template, and no builtin view or index SQL casts a reg type to text. No
builtin plan moves, so `catalog_server_explain.slt` is unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K8HcpvmmntWK9QuJWvKVbg
Resolves a conflict in src/pgrepr/src/value.rs, where main's extra_float_digits work (#37829) and this branch's regproc text encoding both appended items at the same two anchors: the top-level region after `impl Value` and the end of the tests module. Both sides are kept. main threaded a new `TextEncodeSettings` argument through `Value::encode_text`, so this branch's regproc encoding test now passes `TextEncodeSettings::STABLE`. `Value::RegProc` needs no settings of its own, as it does not recurse into nested values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K8HcpvmmntWK9QuJWvKVbg
Conflict in src/adapter/src/catalog/open/builtin_schema_migration.rs: main added four `mz_*_source_tables` replacement steps at the same anchor as our `mz_type_pg_metadata` step. Kept both. Repinned our step from 26.36.0-dev.0 to 26.37.0-dev.0. Main bumped the workspace dev version, and the NOTE in that file requires the step to track the current dev version until the change ships. Added the `parse_source_export_details` entry to mz_pgrepr::regproc::NAMES. Main registered that builtin function, and the table is checked in as data, so it has to be updated in lockstep.
Conflict in ci/test/pipeline.template.yml. Main pruned the PR test pipeline (#38028), relocating chbench-demo, metabase-demo and fdb to the nightly pipeline. Our adbc step sits immediately before chbench-demo, so the removal overlapped our addition. Resolved by keeping only the adbc step and taking main's removal of the other three, rather than restoring them as a naive union of both sides would have done.
Conflict in src/adapter/src/catalog/open/builtin_schema_migration.rs: both sides appended MigrationStep entries at the end of MIGRATIONS and shared the trailing `),`. Kept both sets and restored the delimiter. Our mz_type_pg_metadata step stays at CatalogItemType::Table in mz_internal (main converted the connection-detail tables, not this one) and stays pinned to 26.37.0-dev.0, which is still the workspace dev version and the version main's own new steps use. Also refresh mz_pgrepr::regproc::NAMES, which main made stale: #37725 added four builtin functions (parse_connection_details in mz_internal, plus mz_aws_account_id / mz_aws_external_id_prefix / mz_aws_connection_role_arn in mz_catalog). Without these four entries test_regproc_names_match_builtin_functions fails. The three mz_catalog functions render bare (single impl each, implicitly searched schema); parse_connection_details is schema-qualified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K8HcpvmmntWK9QuJWvKVbg
| Service( | ||
| name="adbc", | ||
| config={ | ||
| "image": "python:3.14.1-trixie", |
There was a problem hiding this comment.
| "image": "python:3.14.1-trixie", | |
| "image": "python:3.14.7-trixie", |
There was a problem hiding this comment.
Taken, and I bumped test/lang/python/mzcompose.py with it since it pinned the identical 3.14.1-trixie and the two are meant to stay in sync.
Worth noting that /test/adbc is absent from .github/dependabot.yml's pip entries (unlike /test/lang/python), so these pins will keep needing manual bumps unless a block is added — do you want that in this PR?
Generated by Claude Code
| adbc-driver-manager==1.11.0 | ||
| adbc-driver-postgresql==1.11.0 | ||
| duckdb==1.5.5 | ||
| pyarrow==25.0.0 |
There was a problem hiding this comment.
| pyarrow==25.0.0 | |
| pyarrow==25.0.1 |
There was a problem hiding this comment.
Taken. 25.0.1 is a patch release — segfault fixes in the SVE128 unpack path and in the bundled mimalloc — nothing near the Arrow/ADBC path this test exercises.
Generated by Claude Code
| adbc-driver-manager==1.11.0 | ||
| adbc-driver-postgresql==1.11.0 |
There was a problem hiding this comment.
| adbc-driver-manager==1.11.0 | |
| adbc-driver-postgresql==1.11.0 | |
| adbc-driver-manager==1.12.0 | |
| adbc-driver-postgresql==1.12.0 |
There was a problem hiding this comment.
Taken, but it is not a no-op, so it came with a test change: apache/arrow-adbc#4415 makes the postgres driver return JSON/JSONB as the arrow.json extension type, so the c_jsonb expectation in smoketest.py moved from pa.string() to pa.json_(pa.string()).
The reassuring part is that 1.12.0 leaves the pg_type resolver query byte-identical for a non-Redshift server, so typsend presence, typreceive resolution by rendered name, and the array_recv filter — everything this PR's fix depends on — are unchanged.
Generated by Claude Code
QA LLM Review1. MEDIUM -- builtin schema migration step pinned below the workspace dev version
The new Details
The dev-source escape hatch does not cover this: Fix: repin to the workspace dev version at merge time. The newest entries already on 2. LOW --
|
…14.7 The ADBC 1.12.0 bump is not a no-op for this test. apache/arrow-adbc#4415 makes the postgres driver return JSON and JSONB as the `arrow.json` Arrow extension type instead of plain utf8, so the `c_jsonb` expectation in `smoketest.py` has to move from `pa.string()` to `pa.json_(pa.string())` or the type assertion fails. pyarrow 25.0.1 is a patch release with no changes near this path. `test/lang/python/mzcompose.py` moves along with `test/adbc/mzcompose.py` because it pinned the identical `python:3.14.1-trixie`; the two pins are deliberately kept in sync, and bumping only one would leave the repo with two different python image tags for no reason.
Rendering OID 0 as `-` in the reg* text output was only half of the round trip: `STRING_REG_CAST_TEMPLATE` had no branch for `-`, so Materialize could not parse back its own rendering. `parse_ident` rejected it with `string is not a valid identifier: "-"`, which broke `0::regproc::text::regproc` and `0::regtype::text::regtype`. PostgreSQL's `regprocin`, `regclassin` and `regtypein` all go through `parseDashOrOid`, so `-` means OID 0 for every reg* type. Accept it in all cases, including text to regclass: `-` is not number-shaped, so the restriction that disables the OID-like branch there does not apply. Adds round-trip coverage to regproc.slt, regtype.slt and regclass.slt.
|
Finding 1 (migration step pinned below the workspace dev version): already fixed. Merging Finding 2: real, and fixed in this push. Two corrections to the report:
Follow-up question for human reviewers. The PR now renders OID 0 as Generated by Claude Code |
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
Seems fine. Might need to define a new docker image, not sure who has permission to do it.
The drift test recomputed `regproc::NAMES` from the builtin function registry and printed the corrected table in its panic message, leaving the reader to paste hundreds of lines back into the source. It now splices the recomputed table into `src/pgrepr-consts/src/regproc.rs` itself when `REWRITE` is set, matching the idiom in `src/compute-types/tests/lir_schema.rs`, and names that command when it fails without it. `// BEGIN GENERATED` / `// END GENERATED` mark the region the splice replaces. Both markers must appear exactly once before anything is written, since a splice anchored elsewhere in the file would clobber the lookup functions below the table. The table stays static: `mz-pgrepr-consts` sits below the catalog crates and the text encoder also runs in `clusterd`, which links no catalog.
The round-trip cases fed a cast-derived value into the string to reg* cast: `0::reg*::text::reg*::text`. A non-constant input cannot fold away the `If` branches of `STRING_REG_CAST_TEMPLATE`, so MFP fusion eagerly evaluates the name-resolution `ELSE` and its `mz_error_if_null` raises even though the `-` branch matched. That is the pre-existing evaluation order limitation akin to STG-54 already asserted as a `query error` in regtype.slt and behind the neighbouring commented-out cases, not something these tests should be exercising. Assert the same behaviour from a literal input instead, matching the shape of the passing neighbours. regclass additionally covers `'-'::text`, which routes through the explicit text to regclass template that rejects OID-like input and so is the interesting context for the ungated `-` branch.
|
@antiguru on whether this needs a new Docker image: it doesn't. The A correction to scope on my earlier reg-cast round-trip claim. Everything else in #132917 was green, including Generated by Claude Code |
The Arrow ADBC driver suite ran on every PR, which costs CI time for a driver-integration test that rarely regresses. Move it to the nightly pipeline, where the other client-library suites live. The step drops its `inputs: [test/adbc]` gating, which only has meaning in the per-PR pipeline, and takes the 30 minute timeout used by the rest of the Language group. Composition, agent queue, and build dependency are unchanged.
7 participants
Requested by Dov Alperin · Slack thread
Motivation
adbc_driver_postgresqlcannot connect to Materialize. Before running any user query it resolves the type catalog, and that query filters onpg_type.typsend, which we do not have:Reported by a user who wants to pull result sets straight into DuckDB without going through S3.
Description
Before:
adbc.dbapi.connect()fails. After: it connects, and every column arrives as its real Arrow type, so an Arrow table can be handed to DuckDB directly.Two things were broken.
pg_type.typsendwas missing entirely. Added, with each type's real PostgreSQL OID.typreceiverendered as a number. The driver keys its Postgres to Arrow type map on that column's text value (int4recv,boolrecv). We sent2406, so every type silently fell back to opaque binary.regprocnow renders the resolved function name in text output, matching PostgreSQL. Binary output is unchanged, since PostgreSQL sends a bare OID there too.The OID to name table lives in
mz-pgrepr-consts, which has no dependencies, so no encoder signatures change and pgwire,COPY, and the Kafka text sink all pick it up for free.test_regproc_names_match_builtin_functionsrecomputes the table fromBUILTINS::funcs()and fails printing the corrected version, so it cannot drift.This also fixes a latent bug:
typreceive::TEXT != 'array_recv'was a tautology, so that filter returned 68 rows where a correct catalog returns 34.Verification
New
test/adbcsuite, CI steparrow-adbc-driver: connect, scalar type resolution, arrays, nulls, a 5000 row fetch, and an Arrow to DuckDB handoff. Green.test_compare_builtins_postgresindependently checks every newtypsendOID against a live PostgreSQL.CI was fully green on the pre-merge head
778fbfa0e3, 133/133, includingslt-4once the auto-generated region oftest/sqllogictest/catalog_server_explain.sltwas regenerated. The branch has since been merged withmaintwice, so the current head is awaiting a fresh run.Notes for review
mz_type_pg_metadatagains a column, so it needs aMigrationStep. This usesreplacement, notevolution: onlyreplacementpopulatesreplaced_items, which drives the read-only backfill that letspg_type_all_databases_indhydrate.evolutionfailed0dt-upgrade-smoke-test.26.37.0-dev.0, repinned from26.36.0-dev.0whenmainbumped the workspace dev version. A step pinned to a superseded dev version is skipped on upgrade, and the fingerprint check then panics at catalog open.typsenditself still renders in decimal, because we register no*sendfunctions. It does not affect ADBC, which only compares it against0.pg_catalog.abs), matching both PostgreSQL'sregprocoutand our ownregproc::textcast. Those renderings do not parse back, which is also true of PostgreSQL.NAMEStable now has 627 entries, gainingmz_internal.parse_source_export_detailsaftermainadded that builtin function. The drift test is what forced the update, which is the point of having it.