Skip to content

[no-release-notes] fix tests dropping null checks on not nullable columns#2839

Open
jycor wants to merge 6 commits into
mainfrom
james/prefix
Open

[no-release-notes] fix tests dropping null checks on not nullable columns#2839
jycor wants to merge 6 commits into
mainfrom
james/prefix

Conversation

@jycor

@jycor jycor commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

@zachmu zachmu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor
Main PR
covering_index_scan_postgres 1848.89/s 1913.15/s +3.4%
groupby_scan_postgres 110.72/s 121.48/s +9.7%
index_join_postgres 635.08/s 636.25/s +0.1%
index_join_scan_postgres 851.94/s 806.71/s -5.4%
index_scan_postgres 24.81/s 25.20/s +1.5%
oltp_delete_insert_postgres 807.89/s 789.07/s -2.4%
oltp_insert 713.75/s 697.69/s -2.3%
oltp_point_select 3051.61/s 3020.70/s -1.1%
oltp_read_only 3040.73/s 2992.61/s -1.6%
oltp_read_write 2209.57/s 2208.74/s -0.1%
oltp_update_index 719.93/s 689.65/s -4.3%
oltp_update_non_index 748.40/s 728.06/s -2.8%
oltp_write_only 1787.95/s 1695.78/s -5.2%
select_random_points 1922.93/s 1857.21/s -3.5%
select_random_ranges 1113.70/s 1112.14/s -0.2%
table_scan_postgres 24.43/s 24.06/s -1.6%
types_delete_insert_postgres 772.78/s 770.64/s -0.3%
types_table_scan_postgres 8.51/s 8.35/s -1.9%

@itoqa

itoqa Bot commented Jun 10, 2026

Copy link
Copy Markdown

Ito QA test results
Commit: cec86df: 8 test cases ran, 0 failed ❌, 6 passed ✅, 2 additional findings ⚠️.

Summary

Overall, 8 test cases ran with 6 passing and 2 failing: boundary/index regression checks and dependency-upgrade parity validations passed, including stable NULL-exclusive range rendering, correct upgraded module resolution, and fallback join-plan stability. The most important issues were a high-severity runtime join failure (bigint: unhandled type: int32) on a non-hinted bigint/int32 path and a medium-severity startup-parameter bug where client timezone is ignored, causing TIMESTAMPTZ output to use host-local UTC instead of requested PST/PDT.

Tests run by Ito

View full run

Result Severity Type Description
Boundary Out-of-range join key coverage stayed on NULL-exclusive fallback filters.
Boundary OR predicate EXPLAIN output kept NULL-exclusive split ranges as expected.
Dependency The upgraded go-mysql-server module version resolved to the expected commit-based tag.
Dependency The targeted TestBasicIndexing run kept EXPLAIN output and runtime query assertions aligned.
Dependency Two clean environments resolved the same dependency graph and produced matching out-of-range join-plan results.
Join Fallback join EXPLAIN and execution checks passed for the constant-conjunct join path.
⚠️ High severity Join The join fails with bigint: unhandled type: int32 instead of returning rows or completing normal fallback-plan execution.
⚠️ Medium severity Dependency The session does not honor the client startup timezone, so TIMESTAMPTZ text results are rendered using host-local timezone instead of the requested PST/PDT behavior expected by the wire test.
Additional Findings Details

These findings are unrelated to the current changes but were observed during testing.

🟠 Non-hinted join fails on bigint and int32 coercion
  • Severity: High High severity
  • Description: The join fails with bigint: unhandled type: int32 instead of returning rows or completing normal fallback-plan execution.
  • Impact: Queries on this join path can fail outright for affected type combinations, so users cannot complete expected SQL join reads in that scenario. This blocks a core query workflow for impacted statements until code-level type handling is corrected.
  • Steps to Reproduce:
    1. Create and seed the local test and jointable fixtures used by the JOIN-2 case.
    2. Run the non-hinted join query on test.v1 = jointable.v3 and test.v2 = 22.
    3. Observe the runtime failure with bigint unhandled type handling instead of successful join execution.
  • Stub / mock content: The run used local fixture seeding and local container recovery steps, and then executed the failing join against the real local database engine without mocked SQL result injection.
  • Code Analysis: I reviewed the join expectations in testing/go/index_test.go and the conversion path in server/types/type.go. The test suite expects the JOIN-2-style join to execute, but production conversion code only accepts int64 for int8, which can surface as an unhandled-type failure when join values are represented as int32.
🟡 Startup timezone is ignored for TIMESTAMPTZ sessions
  • Severity: Medium Medium severity
  • Description: The session does not honor the client startup timezone, so TIMESTAMPTZ text results are rendered using host-local timezone instead of the requested PST/PDT behavior expected by the wire test.
  • Impact: Timestamp text output can be incorrect for clients that set timezone during startup, causing user-visible correctness issues in timezone-sensitive queries and tests. Workflows continue, but returned values are misleading until timezone is applied correctly.
  • Steps to Reproduce:
    1. Run go test ./... with the upgraded dependency graph.
    2. Exercise wire TIMESTAMPTZ coverage where the startup message sets timezone=PST8PDT.
    3. Compare expected PST/PDT text output against actual output on a UTC host.
  • Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code Analysis: I inspected startup parameter ingestion and timezone defaults in server/connection_handler.go, server/config/parameters_list.go, and testing/go/wire_test.go. The wire test sends timezone=PST8PDT, but chooseInitialParameters only applies datestyle; when timezone is skipped, the session uses time.Local.String() as configured default, which explains UTC output on this host.

Tip

Reply with @itoqa to send us feedback on this test run.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor
Main PR
Total 42090 42090
Successful 18268 18270
Failures 23822 23820
Partial Successes1 5333 5333
Main PR
Successful 43.4022% 43.4070%
Failures 56.5978% 56.5930%

${\color{lightgreen}Progressions (2)}$

random

QUERY: (SELECT unique1 AS random
  FROM onek ORDER BY random() LIMIT 1)
INTERSECT
(SELECT unique1 AS random
  FROM onek ORDER BY random() LIMIT 1)
INTERSECT
(SELECT unique1 AS random
  FROM onek ORDER BY random() LIMIT 1);

type_sanity

QUERY: SELECT c1.oid, c1.relname
FROM pg_class AS c1
WHERE c1.relnatts != (SELECT count(*) FROM pg_attribute AS a1
                      WHERE a1.attrelid = c1.oid AND a1.attnum > 0);

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@itoqa

itoqa Bot commented Jun 11, 2026

Copy link
Copy Markdown

Ito QA test results

History reset (rebase or force-push detected). Starting test narrative over.

Commit: dcb6518: 8 test cases ran, 1 failed ❌, 7 passed ✅, 0 additional findings ⚠️.

Summary

Across 8 QA cases, 7 passed: pg_constraint overflow-guard scenarios (max contypid/conrelid and lower-bound variant parity) behaved correctly, null-bound OR-filter behavior matched between EXPLAIN and runtime including nullable-index handling, and the targeted TestBasicIndexing regression command remained stable on upgraded dependencies.
The single failure was a confirmed medium-severity, PR-introduced pg_constraint defect where conname+connamespace range conversion can return zero rows instead of six due to upper-bound key construction conflicting with name/schema comparator ordering, with conclusions supported by SQL and go-test text evidence (while screenshots were mostly non-informative 404 captures) and no stubs/mocks except a local SCRAM-auth bypass for deterministic execution.

Tests run by Ito

View full run

Result Severity Type Description
Medium severity Dependency The query returns no rows when six constraint names are expected, and explain output shows open-lower-bound formatting changes alongside the semantic drift.
Constraint Verified max contypid upper-bound scans stayed constrained without wraparound.
Constraint Verified max conrelid partial-predicate scans stayed bounded and did not reintroduce low rows.
Constraint Verified the cast-based lower-bound variant matched baseline row semantics.
Dependency The upgraded dependency set kept the targeted index regression command stable and passing.
Filter The OR-filter EXPLAIN output used the expected open lower bound format for the indexed string predicate.
Filter Nullable index behavior stayed correct: query rows matched the control query and EXPLAIN retained explicit NULL handling.
Filter The updated explain assertions and direct SQL runtime comparison both matched, with no hidden semantic drift.

Tip

Reply with @itoqa to send us feedback on this test run.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

View replay

Medium severity Range conversion drops valid constraint rows

What failed: The query returns no rows when six constraint names are expected, and explain output shows open-lower-bound formatting changes alongside the semantic drift.

Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
  • Severity: Medium Medium severity
  • Impact: Constraint metadata queries can miss valid rows on this path, which can break expected introspection behavior for affected workloads. The issue is persistent but scoped to a specific index-range conversion flow.
  • Steps to Reproduce:
    1. Build and run the repository with the upgraded dependency set from this PR.
    2. Execute go test ./testing/go -run TestPgConstraintIndexes --count=1.
    3. Inspect the assertion for SELECT conname FROM pg_catalog.pg_constraint WHERE (conname LIKE '%_pkey' OR conname LIKE '%_key') AND connamespace = 2200 ORDER BY conname.
    4. Observe that no rows are returned where six constraint names are expected.
  • Stub / mock content: Real SCRAM sign-in was bypassed by turning off the auth gate in server/authentication_scram.go so the regression failure could be reproduced consistently in the local QA environment.
  • Code Analysis: I inspected server/tables/pgcatalog/pg_constraint.go and verified that the pg_constraint_conname_nsp_index upper-bound logic increments schema upper bounds while leaving conNameUpper empty, then uses that as the less-than key. With the same file's (conname, schema) comparator ordering, this upper key can sort below normal names and exclude valid rows.
  • Why this is likely a bug: The production range-bound construction and comparator order together provide a concrete code path that explains the observed empty-result regression for a valid query.
Relevant code

server/tables/pgcatalog/pg_constraint.go:344-354

if conNameRange.HasUpperBound() || schemaOidRange.HasUpperBound() {
	// our less-than upper bound depends on whether we have a prefix match or both fields were set
	if !schemaOidUpperSet {
		conNameUpper = fmt.Sprintf("%s%o", conNameUpper, rune(0))
	} else {
		schemaOidUpper = schemaOidUpper + 1
	}
	lt = &pgConstraint{
		name:            conNameUpper,
		schemaOidNative: schemaOidUpper,
	}
}

server/tables/pgcatalog/pg_constraint.go:578-583

func lessConstraintNameSchema(a, b []*pgConstraint) bool {
	if a[0].name == b[0].name {
		return a[0].schemaOidNative < b[0].schemaOidNative
	}
	return a[0].name < b[0].name
}
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.

**Medium severity — Range conversion drops valid constraint rows**

**What failed:** The query returns no rows when six constraint names are expected, and explain output shows open-lower-bound formatting changes alongside the semantic drift.

- **Impact:** Constraint metadata queries can miss valid rows on this path, which can break expected introspection behavior for affected workloads. The issue is persistent but scoped to a specific index-range conversion flow.
- **Steps to reproduce:**
  1. Build and run the repository with the upgraded dependency set from this PR.
  2. Execute `go test ./testing/go -run TestPgConstraintIndexes --count=1`.
  3. Inspect the assertion for `SELECT conname FROM pg_catalog.pg_constraint WHERE (conname LIKE '%_pkey' OR conname LIKE '%_key') AND connamespace = 2200 ORDER BY conname`.
  4. Observe that no rows are returned where six constraint names are expected.
- **Stub / mock content:** Real SCRAM sign-in was bypassed by turning off the auth gate in `server/authentication_scram.go` so the regression failure could be reproduced consistently in the local QA environment.
- **Code analysis:** I inspected `server/tables/pgcatalog/pg_constraint.go` and verified that the `pg_constraint_conname_nsp_index` upper-bound logic increments schema upper bounds while leaving `conNameUpper` empty, then uses that as the less-than key. With the same file's `(conname, schema)` comparator ordering, this upper key can sort below normal names and exclude valid rows.
- **Why this is likely a bug:** The production range-bound construction and comparator order together provide a concrete code path that explains the observed empty-result regression for a valid query.

**Relevant code:**

`server/tables/pgcatalog/pg_constraint.go:344-354`

~~~go
if conNameRange.HasUpperBound() || schemaOidRange.HasUpperBound() {
	// our less-than upper bound depends on whether we have a prefix match or both fields were set
	if !schemaOidUpperSet {
		conNameUpper = fmt.Sprintf("%s%o", conNameUpper, rune(0))
	} else {
		schemaOidUpper = schemaOidUpper + 1
	}
	lt = &pgConstraint{
		name:            conNameUpper,
		schemaOidNative: schemaOidUpper,
	}
}
~~~

`server/tables/pgcatalog/pg_constraint.go:578-583`

~~~go
func lessConstraintNameSchema(a, b []*pgConstraint) bool {
	if a[0].name == b[0].name {
		return a[0].schemaOidNative < b[0].schemaOidNative
	}
	return a[0].name < b[0].name
}
~~~

@itoqa

itoqa Bot commented Jun 11, 2026

Copy link
Copy Markdown

Ito QA test results

History reset (rebase or force-push detected). Starting test narrative over.

Commit: 1be39de: 13 test cases ran, 0 failed ❌, 10 passed ✅, 3 additional findings ⚠️.

Summary

Overall, 10 of 13 tests passed, confirming that max-uint32 pg_constraint boundary handling (no wraparound, range-restricted index access, and terminal composite-key row preservation) and all EXPLAIN-notation/semantic regression checks behaved correctly in local verification. The key issue is 3 medium-severity failures showing a real pg_constraint range-conversion and iterator-routing bug—especially with disjoint OR and mixed/edge bounds—where concrete bounds can be built but bound flags remain unsynchronized, causing incorrect EOF routing and empty results instead of expected rows.

Tests run by Ito

View full run

Result Severity Type Description
Constraint Max typOidUpper boundary stayed bounded and avoided wraparound behavior.
Constraint Composite conrelid/contypid/conname lookup stayed inside the requested interval.
Constraint relOidUpper max-bound path remained stable when typOidUpper was unset.
Constraint Adversarial max-OID query remained index-bounded without broad-scan fallback.
Constraint Terminal composite-key boundary rows were preserved at max uint32 upper bounds.
Explain Verified simple-range EXPLAIN output uses the expected (NULL, 2) lower-bound notation.
Explain Confirmed join-plan explain assertions stayed green for the updated parenthesized NULL-to-infinity expectation.
Explain Verified notation updates did not change query semantics after row-level checks on simple and join predicates.
Range Two-sided bounded lookup returned the expected in-window rows in remediation verification.
Range Composite key ordering stayed aligned with comparator behavior under edge bounds.
⚠️ Medium severity Range The query returns no rows, but expected rows such as test_table1_pkey, test_table2_pkey, and related _key rows are missing.
⚠️ Medium severity Range Valid lookup predicates can collapse into the unsupported neither-bound branch and return empty results.
⚠️ Medium severity Range The range-conversion output can be routed as if no bounds exist, producing EOF instead of traversing valid lookup intervals.
Additional Findings Details

These findings are unrelated to the current changes but were observed during testing.

🟡 Disjoint OR range scan drops expected constraint rows
  • Severity: Medium Medium severity
  • Description: The query returns no rows, but expected rows such as test_table1_pkey, test_table2_pkey, and related _key rows are missing.
  • Impact: Catalog queries using disjoint name predicates can return incomplete results, which breaks correctness for metadata-dependent workflows. Users may make incorrect decisions based on missing constraint rows.
  • Steps to Reproduce:
    1. Run the disjoint predicate query (conname LIKE '%_pkey' OR conname LIKE '%_key') AND connamespace = 2200 ORDER BY conname.
    2. Compare returned rows against the expected union across both OR branches.
    3. Observe that the query returns an empty set instead of matched rows.
  • Stub / mock content: Authentication was intentionally bypassed for this run by turning off SCRAM auth checks in server/authentication_scram.go, so local SQL validation could proceed consistently.
  • Code Analysis: I inspected the range converter and iterator dispatcher. In pg_constraint_conname_nsp_index, schema OID bounds are parsed but do not set the bound-presence flags; downstream, the iterator treats both flags false as unsupported and returns EOF.
🟡 Bound pair mismatch can force premature EOF
  • Severity: Medium Medium severity
  • Description: Valid lookup predicates can collapse into the unsupported neither-bound branch and return empty results.
  • Impact: This creates false empty-result responses for legitimate metadata filters and reduces trust in query correctness. The issue persists until code is fixed because it is in deterministic routing logic.
  • Steps to Reproduce:
    1. Execute indexed predicates that combine edge bound forms in the pg_constraint lookup path.
    2. Confirm whether the converted range includes lower or upper key objects for the scan.
    3. Observe that valid predicates can still terminate via EOF with missing rows.
  • Stub / mock content: Authentication was intentionally bypassed for this run by turning off SCRAM auth checks in server/authentication_scram.go, so local SQL validation could proceed consistently.
  • Code Analysis: I verified that schema-bound conversions do not mark lower/upper presence for this index path, while the iterator implementation requires those flags to pick a scan primitive and otherwise exits with EOF.
🟡 Null boundary routing selects wrong iterator path
  • Severity: Medium Medium severity
  • Description: The range-conversion output can be routed as if no bounds exist, producing EOF instead of traversing valid lookup intervals.
  • Impact: Metadata queries can silently miss valid constraints in affected boundary cases, leading to wrong query outcomes. Workloads that depend on those lookups can fail functional checks even though matching data exists.
  • Steps to Reproduce:
    1. Execute pg_constraint lookup predicates that mix null and non-null boundaries.
    2. Inspect the chosen iterator behavior for each converted range.
    3. Observe affected predicates returning EOF-like empty results instead of expected rows.
  • Stub / mock content: Authentication was intentionally bypassed for this run by turning off SCRAM auth checks in server/authentication_scram.go, so local SQL validation could proceed consistently.
  • Code Analysis: I cross-checked remediation evidence and source paths: the same converter/dispatcher mismatch explains misrouting under range contract drift scenarios, with constructed bounds but no corresponding bound flags.

Tip

Reply with @itoqa to send us feedback on this test run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants