Fully handle type initialization#2778
Merged
Merged
Conversation
Hydrocharged
force-pushed
the
daylon/full-type-resolution
branch
from
be5994a to
da77132
Compare
Hydrocharged
force-pushed
the
daylon/full-type-resolution
branch
2 times, most recently
from
16b778d to
59b6c92
Compare
Contributor
|
Hydrocharged
force-pushed
the
daylon/full-type-resolution
branch
from
e699b6d to
176cc27
Compare
Ito Test Report ❌14 test cases ran. 3 failed, 2 additional findings, 9 passed. Overall, 14 test cases ran with 9 passing and 5 failing: recursive type-graph resolution, most explicit-cast behavior, custom type creation happy paths, array/ANY runtime checks, and cold-start initialization scenarios all behaved correctly in local verification (with SCRAM authentication disabled for deterministic access). The key confirmed defects are four High-severity issues and one Medium issue in production code paths: domain-default explicit casts can panic (unqualified table lookup and nil resolved-type dereference), type serialization and pg_type relationship projection can nil-dereference pointer fields, CREATE TYPE/DOMAIN companion-array creation is non-atomic and can leave orphaned/conflicting metadata on failure, and domain merge logic can drop check constraints by appending to the wrong object. ❌ Failed (3)
|
| Category | Summary | Screenshot |
|---|---|---|
| Cast | Explicit enum cast target resolved correctly in SELECT and UPDATE flows. |  |
| Cast | Shell and unresolved cast targets were rejected early with deterministic errors. |  |
| Cast | Default-expression and direct explicit casts both resolved and returned matching values. |  |
| Creation | Enum/composite/domain creation and companion array linkage worked, with successful insert/query checks and consistent pg_type relationships. | N/A |
| Initialization | Cold-start and second-session type-heavy queries succeeded with no nil-pointer signatures in logs. |  |
| Initialization | Parallel early and settled startup queries remained consistent with no initialization-order crash evidence. |  |
| Record | Verified array casting applies per-element conversion on built-in scalar cast path (int4[] -> int8[]), preserves NULLs ({1,NULL,2}), and surfaces element-conversion errors for invalid input. |  |
| Record | ANY with json operands failed immediately with operator resolution error ('operator does not exist: json = json'), while integer control query executed successfully and returned true. |  |
| Recursive | Deep nested domain/array/composite types resolved and round-tripped successfully with coherent pg_type metadata. |  |
ℹ️ Additional Findings (2)
These findings are unrelated to the current changes but were observed during testing.
| Category | Summary | Screenshot |
|---|---|---|
| Creation |  |
|
| Metadata | 🟠 Domain merge appends checks to ourType instead of mergedType, so returned merged metadata can lose constraints. |
 |
⚠️ Non-atomic companion array type creation
- What failed: Base type insertion is committed before companion array insertion, and the failure path returns without rollback, leaving orphaned/conflicting metadata that blocks retries.
- Impact: A failed type-creation attempt can leave persistent inconsistent metadata that breaks retries and destabilizes follow-up introspection. This affects a core schema-management workflow with no practical user workaround besides manual cleanup.
- Steps to reproduce:
- Start CREATE TYPE or CREATE DOMAIN for a user-defined type that requires a companion array type.
- Force the companion array creation step to fail after base type insertion.
- Retry creation and inspect pg_type metadata and cast behavior for conflicts or orphaned entries.
- Stub / mock context: Authentication checks were bypassed, and the run intentionally pre-created a conflicting companion-array type name to simulate a second-step create failure and verify rollback behavior.
- Code analysis: I reviewed CREATE TYPE, CREATE DOMAIN, and type collection insertion logic. Both nodes insert the base type first, then attempt array creation; on second-step failure they return immediately, while TypeCollection.CreateType has already mutated cache state and does not provide rollback semantics.
- Why this is likely a bug: The production create flow performs multi-step writes without compensating rollback, so an induced second-step error can leave persistent partial state that violates expected atomic type creation.
Relevant code:
server/node/create_type.go (lines 145-158)
err = collection.CreateType(ctx, newType)
if err != nil {
return nil, err
}
// create array type for defined types
if newType.IsDefined {
arrayType := types.CreateArrayTypeFromBaseType(newType)
err = collection.CreateType(ctx, arrayType)
if err != nil {
return nil, err
}
newType.Array = arrayType
}server/node/create_domain.go (lines 103-114)
newType := types.NewDomainType(ctx, c.AsType, defExpr, c.IsNotNull, checkDefs, arrayID, internalID)
err = collection.CreateType(ctx, newType)
if err != nil {
return nil, err
}
// create array type of this type
arrayType := types.CreateArrayTypeFromBaseType(newType)
err = collection.CreateType(ctx, arrayType)
if err != nil {
return nil, err
}core/typecollection/typecollection.go (lines 66-77)
// Ensure that the type does not already exist in the cache or underlying map
if _, ok := pgs.accessedMap[typ.ID]; ok {
return pgtypes.ErrTypeAlreadyExists.New(typ.Name())
}
if ok, err := pgs.underlyingMap.Has(ctx, string(typ.ID)); err != nil {
return err
} else if ok {
return pgtypes.ErrTypeAlreadyExists.New(typ.Name())
}
// Add it to our cache, which will be written when we do anything permanent
pgs.accessedMap[typ.ID] = typ🟠 Domain merge drops checks on returned merged type
- What failed: Merge logic returns
mergedType, but appendstheirType.ChecksintoourType.Checks; this can drop check constraints from the returned merged object and create metadata/runtime drift. - Impact: Merged domains can carry incomplete check metadata, causing inconsistent behavior between expected merged definitions and runtime constraint application. This creates hard-to-debug post-merge validation drift.
- Steps to reproduce:
- Create diverging branches that modify domain default and check definitions in different ways.
- Merge those branches through the normal merge flow for the same domain identifier.
- Execute inserts and casts against the merged domain and compare runtime behavior to merged metadata expectations.
- Stub / mock context: Authentication remained bypassed for deterministic local execution, and a temporary bypass of domain-cast constraint rewrite logic was active during this run to avoid a separate analyzer panic while validating merge behavior.
- Code analysis: I reviewed merge assembly in
core/typecollection/collection_funcs.goand verified that checks are appended to the wrong target object right before returning the merged type. - Why this is likely a bug: The function mutates
ourType.Checksbut returnsmergedType, so merged results can omit newly merged checks in the object that is actually persisted.
Relevant code:
core/typecollection/collection_funcs.go (lines 61-90)
mergedType := *ourType
switch theirType.TypType {
case pgtypes.TypeType_Domain:
if ourType.BaseTypeType.ID != theirType.BaseTypeType.ID {
return nil, nil, errors.Errorf(`base types of domain type "%s" do not match`, theirType.ID.TypeName())
}
...
if len(theirType.Checks) > 0 {
// TODO: check for duplicate check constraints
ourType.Checks = append(ourType.Checks, theirType.Checks...)
}
return TypeWrapper{Type: &mergedType}, &merge.MergeStats{Commit: b33c0af
Tell us how we did: Give Ito Feedback
Contributor
|
zachmu
approved these changes
May 30, 2026
Member
zachmu
left a comment
zachmu
left a comment
There was a problem hiding this comment.
Can't find too much to fault. The AI tester's comments seem relevant in light of the regressions.
Hydrocharged
force-pushed
the
daylon/full-type-resolution
branch
2 times, most recently
from
88afb3d to
cd4e68e
Compare
Ito Test Report ❌
16 test cases ran. 3 failed, 13 passed. Across the unified verification, 16 test cases were exercised with 13 passing and 3 failing (plus one execution that yielded no verifiable functional outcome), indicating broad stability but not release readiness. Most cast/operator/metadata and deserialization-guard paths behaved correctly—including cyclic type-graph handling, trailing-byte rejection, composite attribute hydration after restart, explicit cast/domain resolution, assignment-cast and synthesized array-cast behavior, expected ANY/missing-type errors, and consistent pg_type/OID exposure—but three high-severity regressions introduced by this PR remain: restart-time crashes when persisted enum or migrated user-defined types are deserialized via recursive context-dependent type loading with nil SQL context, and a domain-backed composite literal path that succeeds at DDL creation but fails later with unresolved attribute-type deserialization. ❌ Failed (3)
|
| Category | Summary | Screenshot |
|---|---|---|
| Deserialization | Cyclic type graph checks stayed deterministic without recursion loops or hangs. |  |
| Deserialization | Trailing-byte payloads are rejected by deserialization guards as expected. |  |
| Deserialization | Composite type casts and attribute reads succeeded after restart. |  |
| Metadata | Domain column metadata correctly exposed the base type OID instead of the domain OID. |  |
| Metadata | Re-run confirmed stable pg_type linked IDs without reproducing the earlier deserialization error. |
 |
| Metadata | Code-level safeguards prevent malformed pointer graphs from emitting spoofed catalog or wire OIDs. |  |
| Operator | IN tuple with unknown params compiles via assignment-cast fallback and returns true. |  |
| Operator | Array-to-array cast succeeds by using synthesized base-type cast. |  |
| Operator | Incompatible ANY comparison correctly returns operator resolution error. |  |
| Resolution | Explicit cast to a newly created enum resolved and returned the expected value new. |
 |
| Resolution | Domain creation over an enum base type and value round-trip succeeded. | N/A |
| Resolution | Casting to a missing type returned the expected unresolved-type error at bind/resolution time. | N/A |
| Resolution | Alternate cast execution paths returned consistent new results and did not reproduce instability. |
 |
Commit: cd4e68e
Tell us how we did: Give Ito Feedback
Collaborator
Author
|
Regressions are exposing missing logic that didn't cause errors before, and are causing errors now that we're effectively enforcing our created types to actually be correct. |
Hydrocharged
force-pushed
the
daylon/full-type-resolution
branch
from
cd4e68e to
c08a1d7
Compare
Ito Test Report ❌
17 test cases ran. 2 failed, 2 additional findings, 13 passed. Overall, the unified run failed with 13 of 17 tests passing and 4 high-severity failures, despite broad stability in recursive unresolved-type handling, concurrent lookup determinism, array operator error safety, polymorphic anyarray behavior, and pg_type/pgwire metadata integrity across reconnects. The critical defects were reproducible panics in restart-time and post-restart domain/composite type paths (including a nil SQL-context deserialization crash and cast panic, both marked as introduced by this PR), a recovered insert-time domain-constraint analyzer panic (“table not found”), and non-atomic CREATE DOMAIN behavior that can leave orphaned type state after companion array creation failure. ❌ Failed (2)
|
| Category | Summary | Screenshot |
|---|---|---|
| Array | Repeated adversarial ANY/IN probes produced consistent invalid-input failures, while valid control queries still succeeded and service health remained stable. |  |
| Array | Unsupported ANY comparisons consistently returned operator-binding errors, while supported controls returned true and post-check health queries succeeded. |  |
| Cast | Domain creation, defaults/checks, and catalog entries behaved as expected. |  |
| Cast | Missing and shell base types were rejected cleanly; valid control domain succeeded. |  |
| Deserialization | Unresolved type request failed fast and the next query succeeded. |  |
| Deserialization | Concurrent resolved/unresolved lookups stayed deterministic across 12 workers. |  |
| Deserialization | Repeated unresolved lookups remained deterministic and did not poison nearby types. |  |
| Metadata | Domain column wire metadata matched the domain base type OID. |  |
| Metadata | pg_type linkage fields (typelem, typarray, typbasetype) were consistent and stable across reconnect. |  |
| Metadata | Code-first re-check showed SQL PREPARE text is unsupported by design, while protocol prepared statements and wire/catalog OID mapping were consistent. |  |
| Metadata | Pointer-linked type graph invariants and decode behavior were coherent after reconnect in remediation validation. |  |
| Migration | Verified reciprocal typarray / typelem linkage for a user-defined enum and its array type, including persistence after reconnect. |
 |
| Migration | Verified array_position and array_to_string resolved element semantics correctly for user-defined and built-in arrays. |
 |
ℹ️ Additional Findings (2)
These findings are unrelated to the current changes but were observed during testing.
| Category | Summary | Screenshot |
|---|---|---|
| Cast |  |
|
| Migration | table not found) instead of stable constraint handling. |
 |
⚠️ CREATE DOMAIN leaves orphaned type state after companion array failure
- What failed: The domain type is created and cached first, then array creation fails; retry reports the domain already exists and the type graph remains partially written instead of rolling back atomically.
- Impact: A failed CREATE DOMAIN can leave non-retryable catalog state that breaks subsequent domain creation attempts and related array-domain operations. This disrupts a core DDL workflow with no practical workaround besides manual cleanup.
- Steps to reproduce:
- Trigger CREATE DOMAIN while forcing companion array type creation to fail, such as by pre-creating a conflicting _domain_name type.
- Retry the same CREATE DOMAIN statement.
- Run catalog checks and array-domain casts to verify orphaned state and retry failure behavior.
- Stub / mock context: Authentication bypass remained enabled for local execution, and this test intentionally injected a companion-array naming conflict to force second-step CREATE DOMAIN failure and verify rollback behavior.
- Code analysis: I inspected server/node/create_domain.go and core/typecollection/typecollection.go. CREATE DOMAIN performs two sequential CreateType calls without rollback, and CreateType immediately stores the first type in cache; cache flush paths persist cached types later, allowing partial state to survive when the second step fails.
- Why this is likely a bug: The production code has a clear non-atomic two-step write path with no compensation on second-step failure, which directly explains the observed orphaned/retry-failure behavior.
Relevant code:
server/node/create_domain.go (lines 103-114)
newType := pgtypes.NewDomainType(ctx, c.AsType, defExpr, c.IsNotNull, checkDefs, arrayID, internalID)
err = collection.CreateType(ctx, newType)
if err != nil {
return nil, err
}
// create array type of this type
arrayType := pgtypes.CreateArrayTypeFromBaseType(newType)
err = collection.CreateType(ctx, arrayType)
if err != nil {
return nil, err
}core/typecollection/typecollection.go (lines 61-78)
func (pgs *TypeCollection) CreateType(ctx context.Context, typ *pgtypes.DoltgresType) error {
// First we check the built-in types
if _, ok := pgtypes.IDToBuiltInDoltgresType[typ.ID]; ok {
return pgtypes.ErrTypeAlreadyExists.New(typ.Name())
}
// Ensure that the type does not already exist in the cache or underlying map
if _, ok := pgs.accessedMap[typ.ID]; ok {
return pgtypes.ErrTypeAlreadyExists.New(typ.Name())
}
// Add it to our cache, which will be written when we do anything permanent
pgs.accessedMap[typ.ID] = typ
return nil
}core/typecollection/typecollection.go (lines 505-529)
func (pgs *TypeCollection) writeCache(ctx context.Context) (err error) {
if len(pgs.accessedMap) == 0 {
return nil
}
mapEditor := pgs.underlyingMap.Editor()
for _, t := range pgs.accessedMap {
data := t.Serialize()
h, err := pgs.ns.WriteBytes(ctx, data)
if err != nil {
return err
}
if err = mapEditor.Update(ctx, string(t.ID), h); err != nil {
return err
}
}
pgs.underlyingMap = flushed
clear(pgs.accessedMap)
return nil
}⚠️ Domain constraint analysis panics during insert
- What failed: Domain-check evaluation hit a recovered panic (
table not found: t) instead of consistently enforcing domain checks without panicking. - Impact: Domain-typed write paths can fail unpredictably and emit panic traces in normal SQL usage. This breaks a core data-write workflow with no practical workaround for affected schemas.
- Steps to reproduce:
- Start the server with a clean data directory.
- Create enum/domain/table objects that exercise domain check constraints.
- Run insert/select statements that trigger domain check compilation.
- Observe recovered panic output from domain-constraint analyzer logic.
- Stub / mock context: Local SCRAM authentication was bypassed (
EnableAuthentication = falseinserver/authentication_scram.go) and the server was restarted on a clean data directory to exercise first-start type paths; no API or route-response stubs were used. - Code analysis: I traced domain-check construction in analyzer code and found table-bound scalar building from a synthesized
FROMvalue derived from column source names; this can fail resolution in insert/update flows and matches the observedtable not foundpanic site. - Why this is likely a bug: The analyzer builds domain-check expressions against a synthesized table expression path that can be unresolved at runtime, directly aligning with the observed panic location and violating expected panic-free domain enforcement.
Relevant code:
server/analyzer/domain_constraints.go (lines 105-113)
func getDomainCheckConstraintsForTable(ctx *sql.Context, a *analyzer.Analyzer, colName string, tblName string, checkDefs []*sql.CheckDefinition) (sql.CheckConstraints, error) {
checks := make(sql.CheckConstraints, len(checkDefs))
for i, check := range checkDefs {
q := fmt.Sprintf("select %s from %s", check.CheckExpression, tblName)
checkExpr, err := parseAndReplaceDomainCheckConstraint(ctx, a, check.CheckExpression, q, &tree.ColumnItem{
ColumnName: tree.Name(colName),
TableName: &tree.UnresolvedObjectName{NumParts: 1, Parts: [3]string{tblName}},
})server/analyzer/domain_constraints.go (lines 231-236)
builder := planbuilder.New(ctx, a.Catalog, nil)
var tblExpr vitess.TableExpr
if len(convertedSelectStmt.From) == 1 {
tblExpr = convertedSelectStmt.From[0]
}
return builder.BuildScalarWithTable(ae.Expr, tblExpr), nilCommit: c08a1d7
Tell us how we did: Give Ito Feedback
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We deserialize types such that all related types (the array type, the base type, composite attributes, etc.) are IDs that are stored on the type, requiring a lookup when that related type is needed. This fails in cases where we either do not have a context available, or something in GMS causes incomplete type information to be created after the type resolution analyzer step.
To work around these limitations, we now pass a context to the deserialization step, and allow the collection to cache types that are currently being deserialized to allow for recursive type references (such as the base type and array type referring to one another). All other changes are related to this core idea, being either interface/field changes or bug fixes.