catalog: add pg_type.typsend and type typreceive as regproc - #37896
catalog: add pg_type.typsend and type typreceive as regproc#37896claude[bot] wants to merge 10 commits into
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
`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
2 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.One step is still red.
test/sqllogictest/catalog_server_explain.sltneeds regenerating, and its auto-generated region cannot be hand-edited:CI also uploads the corrected patch as the
slt4.diffartifact on that job.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.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.