Skip to content

Conversation

@zachasme
Copy link
Contributor

@zachasme zachasme commented Jul 18, 2024

Motivation / Background

As discussed on rubyonrails-core with @flavorjones.

SQLite is a great default database, and even provides a full-text-search module, fts5. However, fts5 is a virtual table module which messes up schema.rb. For example, if you create virtual table searchables in a migration:

class CreateSearchablesTable < ActiveRecord::Migration[8.0]
  def up
    execute 'CREATE VIRTUAL TABLE searchables USING fts5(content)'
  end
end

and run rails db:migrate, then 6 tables are created, 1 virtual and 5 "shadow" tables. This is what the result of SELECT * FROM pragma_table_list ORDER BY name (relevant part) looks like:

schema name type ncol wr strict
main searchables virtual 3 0 0
main searchables_config shadow 2 1 0
main searchables_content shadow 2 0 0
main searchables_data shadow 2 0 0
main searchables_docsize shadow 2 0 0
main searchables_idx shadow 3 1 0

While these tables look like regular tables, they should not be dumped in the same way. In schema.rb there are error messages for some of them (notably the virtual table) while a few shadow tables are dumped like regular tables:

# Could not dump table "searchables" because of following StandardError
#   Unknown type '' for column 'content'

# Could not dump table "searchables_config" because of following StandardError
#   Unknown type '' for column 'k'

# Could not dump table "searchables_content" because of following StandardError
#   Unknown type '' for column 'c0'

  create_table "searchables_data", force: :cascade do |t|
    t.binary "block"
  end

  create_table "searchables_docsize", force: :cascade do |t|
    t.binary "sz"
  end

# Could not dump table "searchables_idx" because of following StandardError
#   Unknown type '' for column 'segid'

This will put the app into a bad state if you run rails db:reset, with some shadow tables as regular tables and the virtual table missing entirely:

schema name type ncol wr strict
main searchables_data table 2 0 0
main searchables_docsize table 2 0 0

The proper way to dump a virtual table to schema.rb would be to ignore the "shadow" tables and only create the virtual table (which in turn will create the shadow tables).

Detail

This PR tries to provide general SQLite3 virtual table support using create_virtual_table and drop_virtual_table. While drop_virtual_table is basically drop_table (since dropping a virtual table also drops connected shadow tables), it allows the CommandRecorder to reverse drop_virtual_table.

It will dump virtual tables to schema.rb and ignore shadow tables. For example:

CREATE VIRTUAL TABLE searchables USING fts5(content, meta UNINDEXED, tokenize = 'porter ascii');

will append to schema.rb:

create_virtual_table "searchables", "fts5", ["content", "meta UNINDEXED", "tokenize='porter ascii'"]

Notable changes:

  • In order to filter out virtual/shadow tables from the regular table dump, I had to use pragma_table_list instead of sqlite_master in ActiveRecord::ConnectionAdapters::SQLite3::SchemaStatements because the schema table sqlite_master lacks virtual/shadow type information.
  • I have added 2 new methods to the abstract adapter, but they are only used by the sqlite3 adapter. This is based on the postgresql enum implementation.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@zachasme zachasme force-pushed the sqlite-virtual-tables branch from 3aca6e0 to 7cee5e9 Compare July 18, 2024 08:55
@fatkodima
Copy link
Member

This change is so specific for sqlite.
Can the problem be solved without this PR, but using structure.sql file format and execute inside the migrations?

@zachasme zachasme force-pushed the sqlite-virtual-tables branch from 7cee5e9 to e7774e4 Compare July 19, 2024 07:06
@zachasme
Copy link
Contributor Author

zachasme commented Jul 19, 2024

Yes this is SQLite only. I took inspiration from the native PostgreSQL enum support that landed in Rails 7.0.

Indeed, the problem does not occur if using structure.sql. However, my hope with this PR is to allow more (of my) projects to stay on the "happy path" using schema.rb and SQLite, even when full-text-search is required.

@dhh
Copy link
Member

dhh commented Jul 31, 2024

I very much like the idea of being able to support sqlite search without having to give up on schema.rb. We've faced this problem with ONCE. @kevinmcconnel, could you have a look whether this would work for that case? Also, @matthewd do you have thoughts on the implementation?

@flavorjones
Copy link
Member

FWIW as someone knowledgeable about the sqlite virtual table implementation, I'm comfortable with this approach from the POV of how rails is interacting with the schema tables.

@zachasme zachasme force-pushed the sqlite-virtual-tables branch from e7774e4 to 0635b72 Compare August 14, 2024 07:33
@flavorjones flavorjones requested a review from matthewd August 14, 2024 18:09
@flavorjones
Copy link
Member

Pinging @kevinmcconnell since your username was misspelled above.

@kevinmcconnell
Copy link

Thanks for the ping @flavorjones! I had missed this earlier.

+1 on adding this. As @dhh mentioned, this came up when building ONCE products. We switched to structure.sql to work around it, but I kept meaning to take a stab at adding this functionality so we could switch back. So it's awesome to see someone already has :)

I just tried this branch in Campfire to get the app back onto schema.rb, and it worked great.

@flavorjones
Copy link
Member

@zachasme Can you please squash the commits and force-push? That will also re-kick CI which had a few failures that I don't think are related.

@zachasme zachasme force-pushed the sqlite-virtual-tables branch from 886f605 to 622277b Compare August 16, 2024 16:55
@zachasme
Copy link
Contributor Author

@flavorjones sure thing! I've squashed and rebased.

@flavorjones
Copy link
Member

@zachasme Hmm, ok, it seems like there are related tests failing, for example https://buildkite.com/rails/rails/builds/110258#01915c1d-f2a6-4f30-95b5-2ce74ef0a7fc

Can you take a look?

@zachasme
Copy link
Contributor Author

Indeed there are! I'll look into it :-)

@zachasme zachasme force-pushed the sqlite-virtual-tables branch from 622277b to c4f1203 Compare August 19, 2024 10:03
@rails-bot rails-bot bot added the railties label Aug 19, 2024
@zachasme
Copy link
Contributor Author

zachasme commented Aug 19, 2024

It looks like some of the railties tests assume that connection.tables is in order of creation. I think it would be cleanest to change the tests to only assert on array content. I've pushed a commit with this change.

Alternatively, when listing tables, I could join on pragma_table_list instead of selecting it directly, and rely on sqlite_master.rowid for ordering.

SELECT m.name FROM sqlite_master m
JOIN pragma_table_list p ON m.name = p.name
WHERE m.name <> 'sqlite_sequence'"
AND p.type IN (...)
ORDER BY m.rowid

Please let me know if you think that would be preferable.

@zachasme zachasme force-pushed the sqlite-virtual-tables branch 2 times, most recently from 8b4da05 to d8a7158 Compare August 19, 2024 10:39
@flavorjones
Copy link
Member

@zachasme Made one small comment, but those test changes mostly look OK to me.

There are still some failures in the rake sqlite3_mem:test test suite that might be related?

@zachasme zachasme force-pushed the sqlite-virtual-tables branch from d8a7158 to bc63d28 Compare August 21, 2024 09:52
@zachasme
Copy link
Contributor Author

@flavorjones Many thanks for your help.

I believe the test are now passing.

@zachasme zachasme force-pushed the sqlite-virtual-tables branch 2 times, most recently from 02577ac to ad52362 Compare August 22, 2024 07:38
@zachasme zachasme force-pushed the sqlite-virtual-tables branch 2 times, most recently from c2e8700 to 51d4899 Compare August 22, 2024 07:54
Previously, adding sqlite3 virtual tables messed up `schema.rb`.

Now, virtual tables can safely be added using `create_virtual_table`.
@rafaelfranca rafaelfranca force-pushed the sqlite-virtual-tables branch from 51d4899 to 1ecb91b Compare August 23, 2024 22:13
@rafaelfranca rafaelfranca merged commit 534b4ab into rails:main Aug 26, 2024
@ankane ankane mentioned this pull request Aug 27, 2024
1 task
@zachasme zachasme deleted the sqlite-virtual-tables branch August 27, 2024 09:36
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.

7 participants