Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make schema cache methods behave consistently #43105

Merged

Conversation

eileencodes
Copy link
Member

@eileencodes eileencodes commented Aug 25, 2021

In #43075 I added the ability to skip adding a configured set of tables
to the schema cache. After pushing that PR I found the behavior across
methods and database adapters to be inconsistent when a table didn't exist.
Since ignored tables should behave the same as missing tables, I wanted
to make the behavior consistent.

Previously the behavior was:

mysql
columns & columns_hash: Raised db error for non-existent table
primary_keys: returned nil for non-existent table
indexes: Raised db error for non-existent table

sqlite3
columns & columns_hash: Raised db error for non-existent table
primary_keys: returned nil for non-existent table
indexes: returned [] for non-existent table

postgresql
columns & columns_hash: Raised db error for non-existent table
primary_keys: returned nil for non-existent table
indexes: returned [] for non-existent table

Current behavior:

mysql, sqlite3, and postgresql
columns & columns_hash: Raise db error for non-existent table
primary_keys: returns nil for non-existent table
indexes: returns [] for non-existent table

The only change is to mysql, which used to raise for indexes, now it
returns and empty array like all the other adapters.

cc/ @tenderlove we chatted about this
cc/ @rafaelfranca @casperisfine how do you all feel about this?

@casperisfine
Copy link
Contributor

Makes sense to me 👍 .

In rails#43075 I added the ability to skip adding a configured set of tables
to the schema cache. After pushing that PR I found the behavior across
methods and database adapters to be inconsistent when a table didn't exist.
Since ignored tables should behave the same as missing tables, I wanted
to make the behavior consistent.

Previously the behavior was:

*mysql*
columns & columns_hash: Raised db error for non-existent table
primary_keys: returned `nil` for non-existent table
indexes: Raised db error for non-existent table

*sqlite3*
columns & columns_hash: Raised db error for non-existent table
primary_keys: returned `nil` for non-existent table
indexes: returned `[]` for non-existent table

*postgresql*
columns & columns_hash: Raised db error for non-existent table
primary_keys: returned `nil` for non-existent table
indexes: returned `[]` for non-existent table

Current behavior:

*mysql, sqlite3, and postgresql*
columns & columns_hash: Raise db error for non-existent table
primary_keys: returns `nil` for non-existent table
indexes: returns `[]` for non-existent table

The only change is to mysql, which used to raise for indexes, now it
returns and empty array like all the other adapters.
@eileencodes eileencodes force-pushed the make-schema-cache-methods-consistent branch from 15afbfd to e323e68 Compare August 26, 2021 17:29
@eileencodes
Copy link
Member Author

After pushing this I found load_schema! that depends on raising the table doesn't exist error for columns hash. This happens if maintain_test_schema is false and migrations haven't been run to create the table. I think overall this isn't a great user experience (you might easily miss the migrations weren't run due to the stack trace). However, I don't want to get distracted working on that so I opted to make each individual method behave consistently for now.

Now the behavior is:

columns and columns_hash: all adapters raise for missing table
primary_keys: all adapters did and still return nil when a table is missing
indexes: all adapters now return an empty array for a missing table. only mysql has had a behavior change there.

In a future time I think there is a lot of room for improvement in the schema cache, but for today I'm aiming to unblock #43075.

I will be offline until Sept 27, please revert this and #43075 if any issues occur.

@eileencodes eileencodes merged commit 6c17335 into rails:main Aug 26, 2021
@eileencodes eileencodes deleted the make-schema-cache-methods-consistent branch August 26, 2021 18:46
eileencodes added a commit that referenced this pull request Aug 26, 2021
eileencodes added a commit to eileencodes/rails that referenced this pull request Aug 26, 2021
In cases where an application uses pt-osc or lhm they may have
temporary tables being used for migrations. Those tables shouldn't be
included by the schema cache because it makes the cache bigger and those
tables shouldn't ever be queried by the application. This feature allows
applications to configure a list of or regex of tables to ignore. The
behavior for ignored tables for each method in the schema cache
(`columns`, `columns_hash`, `primary_keys`, and `indexes`) matches the
behavior of a non-existent table. `columns` and `columns_hash` will
raise a database not found error, `primary_keys` will return `nil`. and
`indexes` will return an empty array. See rails#43105 which make behavior
across adapters consistent.

To use in an application configure `config.active_record.schema_cache_ignored_tables`
to an array of tables or regex's. Those tables will not be included in
the schema cache dump.
@bf4 bf4 mentioned this pull request Oct 7, 2022
9 tasks
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.

None yet

2 participants