Skip to content

Avoid fmt.Sprintf #2910

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

Avoid fmt.Sprintf #2910
jycor wants to merge 6 commits into
mainfrom
james/rw

Conversation

@jycor

@jycor jycor commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Also tests IndexSearchable optimization from dolthub/go-mysql-server#3613

@github-actions

github-actions Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
Main PR
covering_index_scan_postgres 1871.63/s 1872.30/s 0.0%
groupby_scan_postgres 120.76/s 118.62/s -1.8%
index_join_postgres 657.12/s 658.83/s +0.2%
index_join_scan_postgres 839.01/s 839.50/s 0.0%
index_scan_postgres 26.92/s 27.21/s +1.0%
oltp_delete_insert_postgres 789.67/s 805.36/s +1.9%
oltp_insert 706.26/s 702.39/s -0.6%
oltp_point_select 3021.03/s ${\color{lightgreen}3443.12/s}$ ${\color{lightgreen}+13.9\%}$
oltp_read_only 2959.72/s ${\color{lightgreen}3258.86/s}$ ${\color{lightgreen}+10.1\%}$
oltp_read_write 2214.57/s 2380.72/s +7.5%
oltp_update_index 714.67/s 760.09/s +6.3%
oltp_update_non_index 771.38/s 813.66/s +5.4%
oltp_write_only 1729.21/s 1806.64/s +4.4%
select_random_points 1849.12/s 1902.91/s +2.9%
select_random_ranges 1546.66/s 1571.20/s +1.5%
table_scan_postgres 25.39/s 25.82/s +1.6%
types_delete_insert_postgres 775.42/s 803.56/s +3.6%
types_table_scan_postgres 10.99/s 11.18/s +1.7%

@itoqa

itoqa Bot commented Jul 9, 2026

Copy link
Copy Markdown

Ito QA test results
Commit: 1677e0b: 5 test cases ran, 4 passed ✅, 1 additional finding ⚠️.

Summary

Coverage in this run centered on startup and configuration behavior, exercising normal boot flows, first-time initialization, and persisted-setting handling, plus edge-case behavior with malformed stored data. The main startup and configuration paths look healthy, with one medium-severity crash condition observed only in a pre-existing malformed-data path.

Safe to merge — this run shows no regressions, no new failures, and no still-failing issues attributable to this PR. The only failure is a medium-severity pre-existing robustness bug in malformed-data handling, which is a flag-for-later concern rather than a merge blocker for this change.

Tests run by Ito

View full run

Result Severity Type Description
✅ — Persisted Persisted auto_increment_increment value (42) loads successfully, and the configured system variable value (7) is the final runtime value after startup.
✅ — Persisted Server startup intentionally fails fast when a persisted system variable cannot be parsed, and later startup stages do not run.
✅ — Startup Doltgres started with statistics enabled, completed the planner-sensitive workload and repeated ANALYZE cycles, and stayed stable without crashes during the run.
✅ — Startup A fresh startup against an empty data directory successfully created the default postgres database and accepted client connections.
⚠️ Medium severity Schema Expected graceful handling of malformed encoded keys, but decodeTableNameForAddressMap panics with index out of range when encodedName is empty.
Additional Findings Details

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

🟡 Empty address-map key triggers panic
  • Severity: Medium Medium severity
  • Description: Expected graceful handling of malformed encoded keys, but decodeTableNameForAddressMap panics with index out of range when encodedName is empty.
  • Impact: Metadata and schema-list operations can crash when they encounter an empty encoded table-name key, interrupting normal database use until the malformed entry is corrected.
  • Steps to Reproduce:
    1. Create or inject an address-map entry whose encoded table key is an empty string.
    2. Run metadata traversal that reaches RootTableMap.Iter.
    3. Observe panic from decodeTableNameForAddressMap when it indexes encodedName[0].
  • Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code Analysis: In /tmp/output-agent-workspace/repo/core/storage/storage.go, decodeTableNameForAddressMap (lines 319-327) reads encodedName[0] on lines 320 and 322 before checking length. The function is called from RootTableMap.Iter at line 345, so iteration-based metadata paths inherit the panic risk. The PR diff for core/storage/storage.go only changes encodeTableNameForAddressMap string construction at lines 311-316 and does not modify the vulnerable decode logic, indicating a pre-existing defect rather than a PR-introduced regression.
Evidence Package

Tip

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

@github-actions

github-actions Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
Main PR
Total 42090 42090
Successful 18276 18275
Failures 23814 23815
Partial Successes1 5335 5335
Main PR
Successful 43.4212% 43.4189%
Failures 56.5788% 56.5811%

${\color{red}Regressions (1)}$

subselect

QUERY:          select count(*) from tenk1 t
where (exists(select 1 from tenk1 k where k.unique1 = t.unique2) or ten < 0);
RECEIVED ERROR: timeout during Receive

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. ↩

@jycor jycor changed the title enable stats Avoid fmt.Sprintf Jul 9, 2026
fulghum
fulghum approved these changes Jul 9, 2026
View reviewed changes

@fulghum fulghum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@itoqa

itoqa Bot commented Jul 10, 2026

Copy link
Copy Markdown

Ito QA test results

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

Commit: 92148d0: 9 test cases ran, 8 passed ✅, 1 additional finding ⚠️.

Summary

This run exercised core database behavior around startup and restart setting persistence, upgrade consistency, schema and table DDL flows, indexed query planning, and concurrent table-operation edge conditions. Overall application behavior looked stable in these areas, with expected outcomes for isolation, persistence, and query correctness.

Safe to merge — no failures in this run were attributed to this PR, and the exercised behaviors showed no regressions tied to the change. A medium-severity schema-qualified rename problem is present but appears pre-existing and unrelated, so it is a follow-up item rather than a merge blocker for this PR.

Tests run by Ito

View full run

Result Severity Type Description
✅ — Index The indexed predicate query returned the expected paid invoice IDs (1, 3, 5, 7, 9), and EXPLAIN confirmed IndexedTableAccess on the status index, so the planner optimization behaved correctly in this run.
✅ — Schema Schema-scoped catalog queries and direct reads stayed isolated for public.events and archive.events before and after restart, with no cross-schema leakage.
✅ — Startup After setting the persisted dolt_stats_enabled value to 1 and restarting the server, current_setting('dolt_stats_enabled') returned 1, so startup respected the persisted setting in this run.
✅ — Startup First-boot initialization and post-restart execution produced identical stats-variable values, indexed query plan, and persisted row count, so no one-boot-only regression was confirmed.
✅ — Startup Upgrading from a pre-change build to the current build changes dolt_stats_enabled from forced-off to default/persisted behavior as intended, while query results, indexed plans, and persisted data remain consistent.
✅ — Table ALTER TABLE old_name RENAME TO new_name succeeded; selecting from new_name returned row 7 and old_name was no longer resolvable.
✅ — Table Creating custom.orders, inserting (10, 99), and reading it back succeeded, and querying public.orders returned table not found as expected.
✅ — Table Across 20 concurrent iterations, DROP TABLE consistently won, ALTER TABLE RENAME failed only after the drop removed the source table, and no phantom or orphan table mappings were observed.
⚠️ Medium severity Schema The schema qualifier from ALTER TABLE . is not preserved into the planner lookup, so execution checks the wrong schema and raises table-not-found.
Additional Findings Details

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

🟡 Schema-qualified rename loses source schema context
  • Severity: Medium Medium severity
  • Description: The schema qualifier from ALTER TABLE . is not preserved into the planner lookup, so execution checks the wrong schema and raises table-not-found.
  • Impact: Renaming tables with schema-qualified names can fail for non-default schemas, blocking expected DDL changes until users adjust session search_path and retry.
  • Steps to Reproduce:
    1. Create a table in a non-default schema, for example archive.repro_test2, and insert a row.
    2. Run SELECT count(*) FROM archive.repro_test2 to confirm the table resolves and is readable.
    3. Run ALTER TABLE archive.repro_test2 RENAME TO repro_renamed2 and observe table not found: repro_test2.
    4. Set search_path to archive and run ALTER TABLE repro_test2 RENAME TO repro_renamed2 to see the rename succeed.
  • Stub / mock content: The run disabled SCRAM authentication in the local QA server to allow direct automated SQL connections, and no mocks or stubs were applied to schema resolution or rename logic.
  • Code Analysis: In server/ast/rename_table.go, nodeRenameTable forwards a vitess.TableName from nodeUnresolvedObjectName. In server/ast/unresolved_object_name.go, schema is placed in SchemaQualifier, but downstream name stringification drops that qualifier before table lookup, matching the observed error that omits schema and fails on non-default schemas. A practical fix is to make rename resolution pass schema-qualified identifiers explicitly (or preserve SchemaQualifier through planning), consistent with other DDL paths that include schema fields.
    • Evidence Package

    [!TIP]
    Reply with @itoqa to send us feedback on this test run.

@itoqa

itoqa Bot commented Jul 10, 2026

Copy link
Copy Markdown

Ito QA test results

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

Commit: 6401627: 10 test cases ran, 10 passed ✅.

Summary

This run exercised core database behavior across normal and edge conditions, including table and schema name resolution, index-backed query correctness and performance stability, startup configuration persistence, and table mutation operations. Coverage includes both standard workflows and adversarial concurrency/startup-validation scenarios, and overall application behavior remained consistent and healthy.

Safe to merge — this run shows no PR-attributable regressions, new failures, or previously flagged failures still failing for this change. The tested behavior stayed stable across correctness, performance, and startup-state handling, indicating low merge risk from the validated areas.

Tests run by Ito

View full run

Result Severity Type Description
✅ — Encoding Unqualified public_table was created, SELECT * succeeded with 0 rows, \dt listed public_table in public schema, and information_schema.tables confirmed exactly one row under table_schema=public. Default schema resolution works without schema prefix.
✅ — Encoding Created schema accounting and table accounting.ledger, inserted rows, and confirmed qualified and search_path-based resolution behaves correctly for that schema only.
✅ — Encoding Unqualified metrics resolution consistently returned the table from the first schema in search_path (b1 then a1 when order was reversed), so no wrong-schema early-stop binding was observed for this scenario.
✅ — Engine EXPLAIN and EXPLAIN ANALYZE both show IndexedTableAccess(perf_idx) using index [perf_idx.k] for k=100, and the SELECT result returns the expected four rows.
✅ — Engine A 50,000-row high-cardinality table with an index on k kept IndexedTableAccess plans for selective predicates, and repeated runs stayed in a stable ~38-40 ms range without scan amplification.
✅ — Engine The indexed SELECT on test_stats returned the same three rows in the same order with dolt_stats_enabled set to 1 and then 0, with the same IndexedTableAccess plan in both runs.
✅ — Engine Across seven startup scenarios, valid persisted variables loaded correctly while invalid, read-only, and unknown keys produced explicit validation errors or warnings without silent behavior drift.
✅ — Mutation Renaming rename_src to rename_dst succeeded, preserved the existing row, and removed access through the old table name as expected.
✅ — Mutation Ten concurrent ALTER TABLE RENAME and DROP TABLE race attempts all ended in a coherent ONLY_DST state with no duplicate or ghost table entries.
✅ — Startup The test confirms dolt_stats_enabled remains true after restart when persisted as 1 in global config, indicating startup no longer forces this variable to false.

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

Reviewers

@fulghum fulghum fulghum approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jycor @fulghum