sqlString improvements#2781
Closed
jycor wants to merge 5 commits into
Closed
Conversation
Contributor
|
Ito Test Report ✅7 test cases ran. 1 additional finding, 6 passed. The unified run passed 6 of 7 TIME-related tests, confirming stable behavior for canonical formatting (including trailing-zero trimming), consistency between pgwire and ::text output paths, exact preservation of 24:00:00, default six-digit fractional precision when typmod is unspecified, and parse-stable high-fanout validation across 14,336 rows with no malformed values. ✅ Passed (6)
ℹ️ Additional Findings (1)
🟠 TIME typmod precision casts do not enforce rounding
Relevant code:
typmod := val3.(int32)
if typmod == -1 {
typmod = 6
}
precision := tree.TimeFamilyPrecisionToRoundDuration(typmod)
if strings.EqualFold(input, "now") {
// timetypmodin represents the PostgreSQL function of time type IO typmod input.
var timetypmodin = framework.Function1{
Name: "timetypmodin",
Return: pgtypes.Int32,
Parameters: [1]*pgtypes.DoltgresType{pgtypes.CstringArray},
Strict: true,
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
// TODO: typmod=(precision<<16)∣scale
return nil, nil
},
}
// timetypmodout represents the PostgreSQL function of time type IO typmod output.
var timetypmodout = framework.Function1{
Name: "timetypmodout",
Return: pgtypes.Cstring,
Parameters: [1]*pgtypes.DoltgresType{pgtypes.Int32},
Strict: true,
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
// TODO
// Precision = typmod & 0xFFFF
// Scale = (typmod >> 16) & 0xFFFF
return nil, nil
},
}Commit: Tell us how we did: Give Ito Feedback |
Contributor
|
added 2 commits
June 2, 2026 16:21
Ito Diff Report ❌Tested: Overall, this unified run was mixed: 17 of 28 test cases passed, with broad success on canonical TIME formatting (fractional-zero trimming, pgwire/AST consistency, 24:00:00 preservation, column isolation, and loop stability), numeric/integer/mixed serializer stability, invalid TIME literal safety, and most VARCHAR multibyte/rune-boundary behaviors. The most important defects were 11 failures centered on three real issues: a critical PR-introduced startup crash in Function2 overload registration from nil parameter dereferences (blocking SQL service and related safety/buffer scenarios), a medium-severity TIME typmod precision bug where casts/round-trips and mixed-precision columns incorrectly fall back to six fractional digits, and a medium-severity VARCHAR cast bug where explicit varchar(n) casting can return untruncated over-limit values despite typmod constraints. ❌ Failures (7)
🚨 Regtype output migration startup panic
Relevant code:
func keyForParamTypes(types []*pgtypes.DoltgresType) string {
sb := strings.Builder{}
for i, typ := range types {
if i > 0 {
sb.WriteByte(',')
}
sb.WriteString(typ.String())
}
return sb.String()
}
var int2out = framework.Function2{
Name: "int2out",
Return: pgtypes.Cstring,
Parameters: [2]*pgtypes.DoltgresType{nil, pgtypes.Int16},
Strict: true,
Callable: func(ctx *sql.Context, _ [3]*pgtypes.DoltgresType, val, dest any) (any, error) {
return strconv.AppendInt(dest.([]byte), int64(val.(int16)), 10), nil
},
}
var regtypeout = framework.Function2{
Name: "regtypeout",
Return: pgtypes.Cstring,
Parameters: [2]*pgtypes.DoltgresType{pgtypes.Regtype},
Strict: true,
Callable: func(ctx *sql.Context, _ [3]*pgtypes.DoltgresType, val, dest any) (any, error) {
internalID := val.(id.Id)
if internalID.Section() == id.Section_OID {
return internalID.Segment(0), nil
}
toid := id.Cache().ToOID(internalID)🚨 Parallel encoding blocked by startup panic
Relevant code:
func keyForParamTypes(types []*pgtypes.DoltgresType) string {
sb := strings.Builder{}
for i, typ := range types {
if i > 0 {
sb.WriteByte(',')
}
sb.WriteString(typ.String())
}
return sb.String()
}
var time_out = framework.Function2{
Name: "time_out",
Return: pgtypes.Cstring,
Parameters: [2]*pgtypes.DoltgresType{nil, pgtypes.Time},
Strict: true,
Callable: func(ctx *sql.Context, _ [3]*pgtypes.DoltgresType, val, dest any) (any, error) {
return val.(timeofday.TimeOfDay).AppendBytes(dest.([]byte)), nil
},
}
var int2out = framework.Function2{
Name: "int2out",
Return: pgtypes.Cstring,
Parameters: [2]*pgtypes.DoltgresType{nil, pgtypes.Int16},
Strict: true,
Callable: func(ctx *sql.Context, _ [3]*pgtypes.DoltgresType, val, dest any) (any, error) {
return strconv.AppendInt(dest.([]byte), int64(val.(int16)), 10), nil
},
}🚨 Mixed type serialization contract regression
Relevant code:
// IoOutputBytes appends the given type value to dest
func (t *DoltgresType) IoOutputBytes(ctx *sql.Context, val any, dest []byte) ([]byte, error) {
var o any
var err error
if t.ModInFunc != 0 || t.IsArrayType() || t.IsCompositeType() {
send := globalFunctionRegistry.GetFunction(ctx, t.OutputFunc)
resolvedTypes := send.ResolvedTypes()
resolvedTypes[0] = t
o, err = send.WithResolvedTypes(resolvedTypes).(QuickFunction).CallVariadic(ctx, val, dest)
if err != nil {
return nil, err
}
return o.([]byte), err
}
Callable: func(ctx *sql.Context, _ [3]*pgtypes.DoltgresType, val, dest any) (any, error) {
internalID := val.(id.Id)
if internalID.Section() == id.Section_OID {
return internalID.Segment(0), nil
}
toid := id.Cache().ToOID(internalID)
if t, ok := types.OidToType[oid.Oid(toid)]; ok {
return t.SQLStandardName(), nil
} else {
return internalID.Segment(1), nil
}🚨 Core SQL startup panic before queries
Relevant code:
func keyForParamTypes(types []*pgtypes.DoltgresType) string {
sb := strings.Builder{}
for i, typ := range types {
if i > 0 {
sb.WriteByte(',')
}
sb.WriteString(typ.String())
}
var int4out = framework.Function2{
Name: "int4out",
Return: pgtypes.Cstring,
Parameters: [2]*pgtypes.DoltgresType{nil, pgtypes.Int32},
Strict: true,
Callable: func(ctx *sql.Context, _ [3]*pgtypes.DoltgresType, val, dest any) (any, error) {
return strconv.AppendInt(dest.([]byte), int64(val.(int32)), 10), nil
},🚨 Sustained query scenario blocked by crash
Relevant code:
func compileNonOperatorFunction(funcName string, overloads []FunctionInterface) {
overloadTree := NewOverloads()
for _, functionOverload := range overloads {
if err := overloadTree.Add(functionOverload); err != nil {
panic(err)
}
}
func keyForParamTypes(types []*pgtypes.DoltgresType) string {
sb := strings.Builder{}
for i, typ := range types {
if i > 0 {
sb.WriteByte(',')
}
sb.WriteString(typ.String())
}🚨 Startup crash in overload registration
Relevant code:
func keyForParamTypes(types []*pgtypes.DoltgresType) string {
sb := strings.Builder{}
for i, typ := range types {
if i > 0 {
sb.WriteByte(',')
}
sb.WriteString(typ.String())
}
var time_out = framework.Function2{
Name: "time_out",
Return: pgtypes.Cstring,
Parameters: [2]*pgtypes.DoltgresType{nil, pgtypes.Time},
Strict: true,
Callable: func(ctx *sql.Context, _ [3]*pgtypes.DoltgresType, val, dest any) (any, error) {
return val.(timeofday.TimeOfDay).AppendBytes(dest.([]byte)), nil
},🟠 Typmod precision not applied on time round trip
Relevant code:
case "oid", "regclass", "regproc", "regtype":
if _, ok := v.(id.Id); ok {
return v, sql.InRange, nil
}
case "time":
if _, ok := v.(timeofday.TimeOfDay); ok {
return v, sql.InRange, nil
}
typmod := val3.(int32)
if typmod == -1 {
typmod = 6
}
precision := tree.TimeFamilyPrecisionToRoundDuration(typmod)
if strings.EqualFold(input, "now") {
t := ctx.QueryTime()
t = t.Round(precision)
return timeofday.New(t.Hour(), t.Minute(), t.Second(), t.Nanosecond()/1000), nil
}
t, _, err := tree.ParseDTime(nil, input, precision)
if err != nil {
return nil, err
}
return timeofday.TimeOfDay(*t), nil
t, dependsOnContext, err := pgdate.ParseTimeWithoutTimezone(now, pgdate.ParseModeYMD, s)
if err != nil {
// Build our own error message to avoid exposing the dummy date.
return nil, false, makeParseError(s, types.Time, nil)
}
return MakeDTime(timeofday.FromTime(t).Round(precision)), dependsOnContext, nil✅ Verified Passing (17)
|
| Origin | Category | Severity | Summary | Screenshot |
|---|---|---|---|---|
| 🆕 New | Precision | 🟠 Medium | CAST('now' AS time(2)) returns six fractional digits instead of precision-2 output. |
 |
| 🆕 New | Precision | 🟠 Medium | TIME(0), TIME(3), and TIME(6) all serialize with six fractional digits instead of typmod-specific precision. |
 |
| 🆕 New | Varchar | 🟠 Medium | Boundary checks were inconsistent: output cast path returned the one-rune-over value untruncated (6 runes), while input-enforcement path for VARCHAR(5) rejected the over-limit insert with value-too-long. Expected consistent rune-limit behavior across paths was not met. |  |
🟠 TIME typmod precision ignored
- What failed: The query returns six fractional digits instead of rounding/formatting to precision 2 as required by
time(2). - Impact: TIME values declared with explicit precision are serialized incorrectly, which can break SQL compatibility and downstream consumers that rely on typmod-accurate formatting.
- Steps to reproduce:
- Connect to the local database.
- Run SELECT CAST('now' AS time(2))::text;.
- Verify the returned text includes six fractional digits instead of precision 2.
- Stub / mock context: Local authentication bootstrap behavior was adjusted so postgres/public roles are reliably present during startup; this TIME precision defect reproduces in core SQL type handling, not in mocked or bypassed API paths.
- Code analysis: I reviewed TIME type resolution and IO input code; TIME types are resolved to a base type without precision metadata, and
time_indefaults to precision 6 when typmod is unset. - Why this is likely a bug: Production code clearly falls back to default precision when typmod is missing, and the TIME resolution path shown above does not propagate precision-specific type information.
Relevant code:
server/ast/resolvable_type_reference.go (lines 156-163)
case oid.T_time:
doltgresType = pgtypes.Time
case oid.T_timestamp:
doltgresType = pgtypes.Timestamp
case oid.T_timestamptz:
doltgresType = pgtypes.TimestampTZ
case oid.T_timetz:
doltgresType = pgtypes.TimeTZserver/functions/time.go (lines 49-53)
typmod := val3.(int32)
if typmod == -1 {
typmod = 6
}
precision := tree.TimeFamilyPrecisionToRoundDuration(typmod)server/functions/time.go (lines 117-119)
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
// TODO: typmod=(precision<<16)∣scale
return nil, nil
},🟠 Mixed TIME columns lose typmod precision
- What failed: All columns serialize with six fractional digits, so typmod-specific rounding/truncation is not applied per column definition.
- Impact: Column-level TIME precision contracts are violated, causing incorrect query output and possible application-level parsing or comparison issues where exact textual precision matters.
- Steps to reproduce:
- Create a table with TIME(0), TIME(3), and TIME(6) columns.
- Insert the same microsecond TIME literal into all columns.
- Select each column as text and compare the precision in each output.
- Stub / mock context: Local authentication bootstrap behavior was adjusted so postgres/public roles are reliably present during startup; this mixed-precision TIME failure comes from production typmod handling rather than a stubbed SQL response.
- Code analysis: I inspected the same TIME typmod path and found no precision-specific type propagation before
time_inexecutes; with typmod unset, each branch resolves to default precision 6 regardless of declared column typmod. - Why this is likely a bug: A single production-code path forces default precision when typmod is absent, which directly explains why all declared TIME precisions collapse to six fractional digits.
Relevant code:
server/ast/resolvable_type_reference.go (lines 156-163)
case oid.T_time:
doltgresType = pgtypes.Time
case oid.T_timestamp:
doltgresType = pgtypes.Timestamp
case oid.T_timestamptz:
doltgresType = pgtypes.TimestampTZ
case oid.T_timetz:
doltgresType = pgtypes.TimeTZserver/functions/time.go (lines 49-53)
typmod := val3.(int32)
if typmod == -1 {
typmod = 6
}
precision := tree.TimeFamilyPrecisionToRoundDuration(typmod)🟠 Explicit varchar cast skips typmod truncation
- What failed: The explicit cast path returned the over-limit value unchanged instead of enforcing typmod truncation semantics expected by the cast flow.
- Impact: Queries that rely on explicit length-restricted VARCHAR casts can return values that violate the requested typmod, causing incorrect SQL behavior and compatibility drift. Users can hit inconsistent results between cast-based transformations and column-enforced input paths.
- Steps to reproduce:
- Create an unbounded VARCHAR value with six runes, such as A😀BC😀D.
- Execute SELECT (v::varchar(5))::text for that value.
- Compare with inserting the same value into a VARCHAR(5) column, which enforces the limit with a value-too-long error.
- Stub / mock context: A local deterministic auth bootstrap and SCRAM bypass were applied so SQL statements could run consistently; no mock or bypass changed VARCHAR cast logic in the code paths analyzed for this failure.
- Code analysis: I inspected cast execution and string-cast helpers in
server/expression/explicit_cast.go,core/casts/collection.go, andserver/cast/utils.go. The sizing-cast path intentionally uses in/out conversion for same-type typmod changes, and explicit string casts intentionally ignore out-of-range errors, buthandleStringCastreturns the original untruncated value when it raisesErrCastOutOfRange, which directly causes the observed untruncated cast result. - Why this is likely a bug: The explicit cast logic depends on receiving a truncated string even when overflow is reported, but the VARCHAR cast helper returns the original untruncated input on overflow, producing the exact inconsistent behavior seen at runtime.
Relevant code:
server/expression/explicit_cast.go (lines 116-129)
castResult, err := cast.Eval(ctx, val, sourceType, c.castToType)
if err != nil {
// For string types and string array types, we intentionally ignore the error as using a length-restricted cast
// is a way to intentionally truncate the data. All string types will always return the truncated result, even
// during an error, so it's safe to use.
castToType := c.castToType
if c.castToType.IsArrayType() {
castToType = c.castToType.ArrayBaseTypeCtx(ctx)
}
if castToType.TypCategory != pgtypes.TypeCategory_StringTypes || castResult == nil {
return nil, err
}
}core/casts/collection.go (lines 287-303)
if sourceType.GetAttTypMod() != targetType.GetAttTypMod() {
return Cast{
ID: id.NewCast(sourceType.ID, targetType.ID),
CastType: castType,
Function: id.NullFunction,
UseInOut: true,
}
}server/cast/utils.go (lines 60-69)
case pgtypes.VarChar.ID:
if tm == -1 {
return input, nil
}
length := uint32(pgtypes.GetCharLengthFromTypmod(tm))
str, runeLength := truncateString(input, length)
if runeLength > length {
return input, cerrors.Wrap(pgtypes.ErrCastOutOfRange, fmt.Sprintf("value too long for type %s", targetType.String()))
} else {
return str, nil
}🗑️ No Longer Applicable (1)
Prior tests whose target code has been removed or renamed. Retired, not failing.
| Category | Summary | Screenshot |
|---|---|---|
| Safety | 🚨 Repeated fractional TIME output verification is blocked by startup nil dereference. |  |
🚨 Fractional TIME loop blocked at startup
- What failed: The repeated fractional TIME check cannot execute because startup panics first on nil parameter dereference in overload key generation.
- Impact: Users cannot run even basic TIME text queries because the server fails before accepting stable query traffic. This blocks both regression validation and normal database use.
- Steps to reproduce:
- Connect to Doltgres on localhost:5432.
- Execute repeated query loops selecting a fractional TIME value such as SELECT TIME '16:17:18.123400'::text.
- Observe startup panic in overload initialization before repeated result comparisons can proceed.
- Stub / mock context: The retry flow kept local authentication bypassed and included a temporary nil-parameter guard during environment troubleshooting. Even with that setup, the underlying startup and output-path defects remained reproducible and blocked the intended TIME verification loop.
- Code analysis: I inspected the migrated
time_outsignature and overload key generation path. The first parameter is intentionallynilin Function2 declarations, but overload key generation still assumes every pointer is non-nil. - Why this is likely a bug: The failure is a deterministic startup panic in production code, so it is a true application defect rather than a flaky test condition.
Relevant code:
server/functions/time.go (lines 68-75)
var time_out = framework.Function2{
Name: "time_out",
Return: pgtypes.Cstring,
Parameters: [2]*pgtypes.DoltgresType{nil, pgtypes.Time},
Strict: true,
Callable: func(ctx *sql.Context, _ [3]*pgtypes.DoltgresType, val, dest any) (any, error) {
return val.(timeofday.TimeOfDay).AppendBytes(dest.([]byte)), nil
},server/functions/framework/overloads.go (lines 63-70)
func keyForParamTypes(types []*pgtypes.DoltgresType) string {
sb := strings.Builder{}
for i, typ := range types {
if i > 0 {
sb.WriteByte(',')
}
sb.WriteString(typ.String())
}Tell us how we did: Give Ito Feedback
Contributor
Author
|
Apparently these functions HAVE to be defined the way they are. A possible solution would be duplicating the output functions specifically for outputting to wire format. However, that is difficult to maintain so these will be as is until we can come up with a better solution. |
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
1 participant
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.
TODO: consider switching these to
Function1N?