-
Notifications
You must be signed in to change notification settings - Fork 514
catalog: add pg_type.typsend and type typreceive as regproc #37896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
003b6d4
a5af322
908d4b3
208d36b
ec48414
033eca3
4f47bff
8075a5e
8596cf0
2d7aff0
836a109
778fbfa
f50cb79
922263c
de670d5
6ffff65
8734aeb
7ddce8f
a6e0392
ff48413
8822b54
6e7030d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -586,6 +586,14 @@ pub static MZ_TYPE_PG_METADATA: LazyLock<BuiltinTable> = LazyLock::new(|| Builti | |
| .with_column("id", SqlScalarType::String.nullable(false)) | ||
| .with_column("typinput", SqlScalarType::Oid.nullable(false)) | ||
| .with_column("typreceive", SqlScalarType::Oid.nullable(false)) | ||
| // `pack_type_update` always writes an OID here, so this declaration is | ||
| // wider than the data requires. Downstream `COALESCE`s on this column | ||
| // are there for the `LEFT JOIN` against this table, not for this. | ||
| // | ||
| // TODO: Tighten to `nullable(false)` to match `typinput` and | ||
| // `typreceive`. That also changes the descs of the views projecting the | ||
| // column. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we tighten it here and just handle the downstream views? We are already touching them.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, Only other change was Generated by Claude Code |
||
| .with_column("typsend", SqlScalarType::Oid.nullable(true)) | ||
| .finish(), | ||
| column_comments: BTreeMap::new(), | ||
| is_retained_metrics_object: false, | ||
|
|
@@ -4372,6 +4380,9 @@ pub static PG_TYPE_ALL_DATABASES: LazyLock<BuiltinView> = LazyLock::new(|| { | |
| .with_column("typcollation", SqlScalarType::Oid.nullable(false)) | ||
| .with_column("typdefault", SqlScalarType::String.nullable(true)) | ||
| .with_column("database_name", SqlScalarType::String.nullable(true)) | ||
| // Appended rather than placed at PostgreSQL's ordinal position, to | ||
| // keep the existing columns' positions stable. | ||
| .with_column("typsend", SqlScalarType::RegProc.nullable(false)) | ||
| .finish(), | ||
| column_comments: BTreeMap::new(), | ||
| sql: " | ||
|
|
@@ -4440,7 +4451,8 @@ SELECT | |
| -- MZ doesn't support COLLATE so typcollation is filled with 0 | ||
| 0::pg_catalog.oid AS typcollation, | ||
| NULL::pg_catalog.text AS typdefault, | ||
| d.name as database_name | ||
| d.name as database_name, | ||
| COALESCE(mz_internal.mz_type_pg_metadata.typsend, 0)::pg_catalog.regproc AS typsend | ||
| FROM | ||
| mz_catalog.mz_types | ||
| LEFT JOIN mz_internal.mz_type_pg_metadata ON mz_catalog.mz_types.id = mz_internal.mz_type_pg_metadata.id | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you were right to push. "every system table" was false:
bootstrap_tablesfilters throughis_retained_across_restarts, which holds backmz_storage_usage_by_shardandmz_object_arrangement_size_history(coord.rs:3066-3076), andvalidate_migration_stepsasserts against migrating those same two. The conclusion formz_type_pg_metadatastill 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_itemsis derived fromMigrationRunResult::new_shards, which onlymigrate_replacepopulates, and it reaches the coordinator asmigrated_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 driveEvolutionover anything); it now says the list's onlyEvolutionstep targets a source, which is verifiable fromMIGRATIONS.Generated by Claude Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cut it. You're right that
replacementneeds 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 thatevolutionfailed0dt-upgrade-smoke-testwas 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:
regprocoutrendering rules invalue.rs.typreceivenames.value.rs).pg_typeandpg_type_all_databases. Appending is what you would do anyway.typsend_oidfield doc, now a one-liner matchingtypinput_oidandtypreceive_oidinstead of adding a caveat neither of them makes.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
regprocvsoidasymmetry betweenpg_typeandpg_type_all_databases, the reserved-word and search-path divergences fromregprocout, the OID provenance you asked for, and theCOALESCEnote, since aLEFT JOINmaking a non-nullable column nullable is genuinely surprising.Comment-only, no golden moved.
8596cf05c4.Generated by Claude Code