Skip to content

wip schema query batching - #60

Open
hmcguire-shopify wants to merge 10 commits into
mainfrom
hm-rrtswqsrplnuuxyv
Open

wip schema query batching#60
hmcguire-shopify wants to merge 10 commits into
mainfrom
hm-rrtswqsrplnuuxyv

Conversation

@hmcguire-shopify

Copy link
Copy Markdown

opening for easier diff review

@hmcguire-shopify
hmcguire-shopify force-pushed the hm-rrtswqsrplnuuxyv branch 3 times, most recently from 880fd84 to be1668e Compare July 22, 2026 19:45
@@ -8,9 +8,30 @@ module SchemaStatements # :nodoc:
def indexes(table_name)
indexes = []
current_index = nil

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

TABLE_NAME  INDEX_NAME                          SEQ_IN_INDEX  COLUMN_NAME
books       index_books_on_author_id_and_name   1             author_id
books       index_books_on_author_id_and_name   2             name
adjustments index_adjustments_on_adjustable     1             adjustable_type
adjustments index_adjustments_on_adjustable     2             adjustable_id

The current_index iteration is kind of hard to grok, so documenting my understanding while I have it: each row is not an index but a column within an index. So the looping is to build an IndexDefinition per index, which requires looping all the rows for an index to collect all the columns.

@hmcguire-shopify
hmcguire-shopify force-pushed the hm-rrtswqsrplnuuxyv branch 20 times, most recently from c1a7679 to ca0d6e1 Compare July 23, 2026 18:00
@hmcguire-shopify

Copy link
Copy Markdown
Author

Fable review:

Details

Stack Review: Batched schema-cache dump queries (11 commits, fbd03fc7fa..6f86a89a27)

Branch: hm-rrtswqsrplnuuxyv
Reviewed: 2026-07-23

What was run: targeted suites (abstract_mysql_adapter/schema_test,
case_insensitive_table_names_test, schema_cache_test, migration_test,
schema_dumper_test, active_schema_test, postgresql/schema_test) against
MariaDB 12.3, MySQL 8.4, and PostgreSQL —
all green. Additional hand-written probes for edge cases found two real
regressions (details below).


Per-commit review

1. fe5cd13748 — Use information_schema to query MySQL indexes ✅

Solid. The IF(ignored = 'NO', ...) / is_visible folding into SQL is
equivalent to the old Ruby-side check, and dropping the
rescue StatementInvalid is correct since information_schema returns an empty
set for missing tables. Two notes:

  • Ordering change: SHOW KEYS returns indexes in definition order; the new
    query is ORDER BY index_name. indexes feeds the schema dumper, so
    existing apps may see a one-time index reordering in
    schema.rb/structure.sql diffs. Worth a CHANGELOG mention (PG already
    ordered by i.relname, so this also makes MySQL consistent with PG — a
    defensible change, just an undocumented one).
  • Commit message sells the why well (enables batching). Good.

2. 4c24c62c01 — Extract SchemaCache#add_tables ✅ (with a stack-structure gripe)

Fine as a refactor. Nice hidden win: add_all now populates @data_sources
directly instead of going through data_source_exists? — not mentioned in the
message. But routing add through add_tables here gets reverted in
commit 7
(25ef3d0e72), because add can receive user-supplied,
differently-cased names that don't round-trip through batch results keyed by
the DB's own names. See stack-level notes — restructure so add never goes
through add_tables.

3. 98bae841d3 — Batch index queries for MySQL ✅

The heart of the MySQL side; well done. The [table, Key_name] boundary key
and the parenthesized OR-scope are both correct and both have targeted tests
with excellent explanatory comments (the "alphabetically-last /
alphabetically-first adjacency" test is exactly the kind of regression trap
that deserves documenting). Two nits:

  • indexes_for_tables returns a Hash.new { |h,k| h[k] = [] } with the
    default proc still attached
    . Callers get a hash that mutates on read —
    e.g. multi.key?("x") is false until someone reads multi["x"]. The test
    in commit 8 relies on this quirk (assert_not multi.key?(...) then
    assert_equal [], multi[...]). Set indexes_by_table.default_proc = nil
    before returning, or pre-seed requested names like the PG implementations do.
  • Mid-stack window: from this commit until 25ef3d0e72, SchemaCache#add
    goes through add_tables, which looks up indexes_by_table[table.to_s]
    keyed by the DB's TABLE_NAME. Under lower_case_table_names=2 a mis-cased
    cache.add("mixedcasetbl") silently caches [] indexes. Fixed by the end of
    the stack, but a bisect landing here would see the bug.

4. 0409b9faa0 — Use information_schema to query MySQL columns ⚠️ regression found

The default = nil if default == "NULL" normalization is applied
unconditionally, but the "NULL"-string-means-no-default quirk is
MariaDB-only. On MySQL, information_schema doesn't quote string literals,
so a column with a literal default of the string "NULL" is indistinguishable
— and now misreported. Verified against real servers:

base (fbd03fc7fa), MySQL 8.4:  varchar DEFAULT 'NULL' => default="NULL"   ✓
HEAD,              MySQL 8.4:  varchar DEFAULT 'NULL' => default=nil      ✗ regression
HEAD,              MariaDB 12: varchar DEFAULT 'NULL' => default="NULL"   ✓ (quoted as 'NULL')

Fix: gate it on mariadb? — e.g.
default = nil if mariadb? && default == "NULL" (or move the normalization
into update_fields_for_mariadb, which is where the MariaDB-specific knowledge
already lives). Admittedly an obscure default, but it's a silent data-model
change and trivially avoidable.

Also: the raised error is now a generic
StatementInvalid("Could not find table '...'") instead of the server's
error. The message matches the SQLite adapter's wording, and callers rescue
the class, so this is fine.

5. d6d81c416c — Avoid SHOW CREATE TABLE during MariaDB introspection ✅

Nice simplification — deleting the fragile SHOW CREATE TABLE regex-parse is
a real win, and it removes a whole extra query. The quoted-vs-unquoted
disambiguation is correct (confirmed by probe on MariaDB 12.3). Nits:

  • Commit message typo: "quotes string literals there (e.g. 'uuid()') but
    leaves function defaults unquoted (e.g. 'uuid()')" — both examples are
    identical; the second should be uuid().
  • The new test runs on MySQL too, where it passes for a different reason
    (EXTRA-based detection) — fine, arguably good cross-coverage.

6. e93aeb15dd — Abstract columns_for_tables API ✅

Straightforward. Note the asymmetry it introduces in add_tables:
columns_by_table.fetch(...) (raises KeyError on a miss) vs
indexes_by_table[...] (silently defaults). See stack-level notes on the
missing-table contract.

7. 25ef3d0e72 — Batch column loading for MySQL ✅

The commit message is exemplary — it explains the lower_case_table_names
keying subtlety and why SchemaCache#add reverts to the single-table path.
The new case_insensitive_table_names_test actually exercised on the MySQL
server (lctn=1, no skip) and passes. Nits:

  • Reusing statistics_table_scope_sql for information_schema.columns makes
    the name misleading — rename to something like
    information_schema_scope_sql.
  • The add→add_tables→revert churn across commits 2→7 is the one structural
    wart in the stack (see stack-level notes).

8. 53856b6834 — Batch primary keys for MySQL ✅

Correct. The unwrap logic in add_tables (size > 1 ? array : first) exactly
mirrors AbstractAdapter#primary_key, including nil for keyless tables via
the default-proc []. The parenthesization test for
index_name = 'PRIMARY' AND (scope OR scope) is another well-motivated trap
test. Same default-proc-leak nit as commit 3.

9. 9876eaba6e — Batch primary keys for PostgreSQL ⚠️ broken as committed, fixed in next commit

Two issues:

  • Mid-stack breakage (verified): to_regclass_array_sql here uses
    quote(name.to_s), dropping the quote_table_name the old query had
    (quote(quote_table_name(table_name))::regclass). At this commit,
    primary_keys("MixedCase") returns [] instead of ["id"] — confirmed by
    running a probe against a worktree at this SHA. Commit 10 silently fixes it
    by switching to quote(quote_table_name(name)). Move that one-line hunk
    into this commit
    — as-is, this commit is a bisect hazard and the fix is
    invisible in commit 10's message.
  • Public behavior change: PG primary_keys/primary_key on a missing
    table now returns []/nil instead of raising (verified). The commit
    message discloses this and it aligns PG with MySQL's existing behavior, but
    it's a user-visible API change on the most popular adapter — it should be in
    the CHANGELOG, not just the commit message.

Otherwise good: the to_regclass NULL-instead-of-raise rationale is sound,
array_position ordering is correct even with interleaved tables (per-table
positions are distinct so relative order survives any tie-breaking), and
table_key_lookup's qualified/unqualified split preserves search-path
semantics.

10. 9116d0d2ec — Batch columns for PostgreSQL ⚠️ regression found

  • Zero-column tables: the message claims "empty reliably means missing
    here, since every existing table has at least one column" — false for
    PostgreSQL
    , where CREATE TABLE t () is legal. Verified:
    columns("zero_col") previously returned [], now raises
    StatementInvalid: Could not find table 'zero_col'. Extremely rare in
    practice, but the justification is wrong and it's fixable: distinguish
    "missing" from "empty" by checking to_regclass resolution (e.g. seed
    result_by_table only for names that resolved, or run the key through
    to_regclass when the result is empty) rather than inferring from row count.
  • Contains the quote_table_name fix that belongs in commit 9 (above).
  • Pre-seeding result_by_table with [] for every requested name means a
    table dropped mid-add_all silently caches empty columns on PG, whereas
    MySQL's non-seeded hash + fetch raises KeyError. Neither matches the old
    StatementInvalid. Racy edge, but the two adapters should agree on the
    contract of the same :nodoc: API.

11. 6f86a89a27 — Batch indexes for PostgreSQL ✅

Mostly mechanical extraction of index_from_row (a faithful move) plus the
batch scope. table_scope_sql's ANY (current_schemas(false)) for
unqualified names exactly matches the old quoted_scope behavior, including
the multi-schema-merge semantics, and the comment says so explicitly.
indexes for a missing table still returns [] via pre-seeding. Good tests
including the schema-qualified round-trip. No complaints.


Stack-level assessment

Overall: this is a well-constructed stack. The progression is logical
(make single-table queries batchable → extract the cache hook → batch each of
indexes/columns/PKs, MySQL first, then PG mirroring it), each commit is
genuinely reviewable in isolation, commit messages are unusually good at
explaining why and calling out subtle preserved behaviors, and the tests
document their traps. The stated win (~25% faster schema cache dump; 3N+
queries → ~3) is real and the design — abstract per-table fallback + adapter
overrides — is the right shape and leaves SQLite untouched.

Things to fix before merge, in priority order

  1. MySQL 'NULL' literal default regression (commit 4) — gate the
    normalization on mariadb?. Verified against real servers.
  2. Move the quote_table_name fix from commit 10 into commit 9
    commit 9 is demonstrably broken for mixed-case/quoted PG names as
    committed.
  3. PG zero-column table regression (commit 10) — and correct the
    commit-message claim.
  4. No CHANGELOG entries anywhere in the stack. Per the repo's own
    conventions this stack needs entries for: the perf improvement, the PG
    primary_keys-on-missing-table behavior change, and probably the MySQL
    index-ordering change in dumps.
  5. Restructure the SchemaCache#add churn: commit 2 routes add through
    add_tables, commit 7 reverts it. Since the final state is "only add_all
    uses add_tables", have commit 2 extract add_tables for add_all only
    and never touch add. This also eliminates the mid-stack lctn=2 window in
    commits 3–6 where cache.add with a mis-cased name silently caches []
    indexes / raises KeyError on columns.

Nice-to-haves

  • Strip the default procs (or pre-seed) on the MySQL *_for_tables return
    values so the two adapters expose the same contract (PG: missing key ⇒ [];
    MySQL: missing key ⇒ absent + mutate-on-read).
  • Rename statistics_table_scope_sql once it's used for
    information_schema.columns.
  • Hoist the repeated deep_deduplicate(table) in add_tables into a local.
  • Consider a quick trilogy run before merging (same code path, but it's cheap
    insurance given the query_all result-shape assumptions).

Test evidence

Suite MariaDB 12.3 MySQL 8.4 PostgreSQL
abstract_mysql_adapter/schema_test + case_insensitive_table_names_test 15 runs, 0 F/E (1 skip: lctn=0) ✅ (no skips, lctn=1 exercised) n/a
connection_adapters/schema_cache_test 27 runs, 0 F/E
migration_test + schema_dumper_test + active_schema_test (+ table_options, charset_collation on MariaDB) 177 runs, 0 F/E 158 runs, 0 F/E 169 runs, 0 F/E
postgresql/schema_test n/a n/a 112 runs, 0 F/E (2 skips)

Probe results

# Literal 'NULL' string default (commit 4 regression)
base (fbd03fc7fa), MySQL 8.4:  default="NULL"            ✓ old behavior
HEAD,              MySQL 8.4:  default=nil               ✗ regression
HEAD,              MariaDB 12: default="NULL"            ✓

# PostgreSQL, HEAD
CREATE TABLE zero_col ()  → columns raises StatementInvalid   ✗ (was [])
primary_keys('missing')   → []   (was: raise)                 ⚠ documented change
primary_key('missing')    → nil  (was: raise)                 ⚠ documented change
columns/primary_keys/indexes('MixedCase') → correct           ✓

# PostgreSQL, at commit 9876eaba6e (mid-stack)
primary_keys('MixedCase') → []  (should be ["id"])            ✗ fixed in commit 10

On its own this change may appear worse than the previous solution since
we have to manually construct the information schema query. However,
using information schema enables a future refactor which will query the
indexes for multiple tables at once (which SHOW KEYS FROM) cannot.

This will be particularly useful for dumping the Schema Cache, which
currently queries the indexes (among other things) for every table
sequentially.

One consequence of this change is that indexes will now be returned in
alphabetical order instead of definition order for MySQL. This now
aligns the MySQL behavior with PostgreSQL, where we are already sorting
indexes by their name.
Add internal indexes_for_tables interface which iterates through tables
and calls indexes, but can be optimized by adapter subclasses to be
batched. This new method is used in the Schema Cache dumping code to
replace N queries to retrieve index metadata with a single query (when
supported).

Also implement this new method for MySQL. In my _very_ large
application, it reduces schema dump time by ~25%.
More verbose, but this will enable extending the API to support querying
by multiple table_names at once.

Since information_schema returns an empty set instead of raising for a
missing table, `column_definitions` now raises `StatementInvalid`
explicitly to preserve the old behavior.

information_schema.COLUMNS.COLUMN_DEFAULT differs from the old `SHOW FULL
FIELDS` Default in two ways, both handled here:

- MariaDB reports a nullable no-default column's default as the string
  "NULL" rather than SQL NULL; normalize that to nil so the column is
  treated as having no default. This is gated on `mariadb?` — MySQL
  already returns SQL NULL for a no-default column and leaves a literal
  "NULL" string default unquoted, so normalizing unconditionally would
  wipe MySQL's literal "NULL" defaults.
- MariaDB quotes string-literal defaults (e.g. `'abc'`) and escapes
  embedded single quotes — by doubling them in varchar columns (`''`) but
  with a backslash in text columns (`\'`). MySQL leaves string defaults
  unquoted, so the strip-and-unescape step is gated on `mariadb?`; it
  strips the wrapping quotes and undoes both escape forms so the default
  matches what `SHOW FULL FIELDS` reported.
update_fields_for_mariadb now uses information_schema.COLUMNS.COLUMN_DEFAULT
to distinguish function defaults from literal strings that look like function
calls. MariaDB quotes string literals there (e.g. 'uuid()') but leaves function
defaults unquoted (e.g. uuid()), so the disambiguation no longer requires
parsing SHOW CREATE TABLE output.

Because column_definitions already selects COLUMN_DEFAULT AS 'Default'
from information_schema.columns, the field's Default value already carries the
quoted/unquoted distinction - so no extra query is needed (unlike the SHOW FULL
FIELDS based path), and update_fields_for_mariadb no longer needs the table name.
Introduce a `columns_for_tables` method on the abstract adapter whose
default implementation just loops over each table and calls `columns`,
leaving room for adapters to override it with a single batched query.
`SchemaCache#add_all` now goes through it instead of calling `columns`
once per table, mirroring the `indexes_for_tables` batch added previously.

This sets up the next commit, which overrides `columns_for_tables` on the
MySQL adapter to fetch every table's columns in one query.
Override `columns_for_tables` on the MySQL adapter to pull every requested
table's columns from `information_schema.columns` in one go, ordering by
(`table_name`, `ORDINAL_POSITION`) so the rows come back grouped by table.

`column_definitions_for_tables` builds its result from the query rows (keyed
by the database's own `TABLE_NAME`) rather than pre-seeding by the requested
name, and `column_definitions` recovers its single table via `each_value.first`.
This keeps single-table introspection correct under `lower_case_table_names = 1/2`,
where the database's table name can differ in case from the requested one.

`SchemaCache#add` keeps using the single-table `columns` getter rather than
the batched `columns_for_tables`, because the batch keys results by the
database's `TABLE_NAME` — so a miscased `add("MixedCase")` would be attributed
to the database's lowercased name. `add_all` is the only batched caller, and
it passes the real data-source names from `tables_to_cache`, which already
match the database's `TABLE_NAME`.

`SchemaCache#add_all` (used by `db:schema:cache:dump`) now issues one columns
query instead of one per table.
`SchemaCache#add_all` was still calling `primary_keys` once per table
after the indexes and columns batches. Introduce a `primary_keys_for_tables`
interface on the abstract adapter — defaulting to a per-table loop, like
`indexes_for_tables` and `columns_for_tables` — and route `add_all`
through it.

On MySQL, override `primary_keys_for_tables` to read every requested
table's primary key columns from `information_schema.statistics` in one
query (the same table the indexes batch reads), ordered by
`table_name, seq_in_index` so composite keys come back in order. The
single-table `primary_keys` now delegates to it, so both paths share one
query.

`add_all` stores the key the way `primary_key` does — the column name
for single-column keys, the array for composite ones — so
`SchemaCache#primary_keys` keeps returning what callers expect.
`SchemaCache#add_all` was still calling `primary_keys` once per table
on PostgreSQL, after the indexes and columns batches. Override
`primary_keys_for_tables` to fetch every requested table's primary key
columns from `pg_index` / `pg_attribute` in one query, and route
`add_all` through it (mirroring the MySQL batch).

The query scopes `pg_index.indrelid` with `to_regclass` rather than
casting the names to `regclass`: `to_regclass` resolves each name the same
way (an unqualified name picks the first schema on the search path, a
qualified name pins its schema) but returns NULL for a missing relation
instead of raising, so one bad name can't fail the whole batch. Composite
keys are ordered per table with `array_position(i.indkey, a.attnum)`.

Rows are keyed by the requested table-name string (not the relname), so
schema-qualified requests round-trip through `add_all`' `[table.to_s]`.
`primary_keys` now delegates to `primary_keys_for_tables` for a single
table; a missing relation returns `[]` instead of raising, which the
schema cache already guards against with `data_source_exists?`.
`SchemaCache#add_all` was still calling `columns` once per table on
PostgreSQL. Override `columns_for_tables` to fetch every requested table's
columns in one query, and route `add_all` through it (mirroring the MySQL
batch).

The batch lives in `column_definitions_for_tables`, which reads `pg_attribute`
for all the tables at once, scoping `a.attrelid` with `to_regclass` (as
`primary_keys_for_tables` does) and ordering by `attrelid, attnum` so each
table's columns come back in order. Rows are keyed by the requested table-name
string, so schema-qualified requests round-trip through `add_all`'s
`.fetch(table.to_s)`. `columns_for_tables` maps each table's fields to columns
with the existing `new_column_from_field`.

`column_definitions` now delegates to `column_definitions_for_tables` for a
single table. Because `to_regclass` returns NULL for a missing relation
instead of raising, and the batch pre-seeds an empty array for every requested
name, an empty result no longer distinguishes a missing table from a
zero-column one (PostgreSQL permits `CREATE TABLE t ()`). `column_definitions`
therefore checks `to_regclass` resolution and raises `StatementInvalid` only
when the relation is actually missing, preserving the previous behavior for
both missing and zero-column tables.
Building a schema cache dump issues one query per table for indexes,
columns, and primary keys. The columns and primary keys queries were
already batched for PostgreSQL; indexes were not, so dumping a cache for
a database with many tables paid a round-trip per table just to read
indexes.

Add indexes_for_tables, which fetches indexes for several tables in one
query by scoping pg_class/pg_namespace to the requested names. It reuses
the same WHERE shape as indexes (relname + search-path namespace for
unqualified names, relname + pinned namespace for qualified names), so
the existing multi-schema search-path behavior is preserved exactly.

The single-table indexes method now delegates to indexes_for_tables, and
the row-to-IndexDefinition logic is extracted into a private helper so
both paths share it.

Indexes are keyed by the requested table-name string, matching how
SchemaCache#add_all looks them up, so schema-qualified names
round-trip correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants