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

Store metadata by default in a json(b) column in the lines table #178

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

swrobel
Copy link
Contributor

@swrobel swrobel commented Nov 21, 2019

This is my attempt at improving performance of metadata storage by moving it from a separate key/value table into the lines table. The results of tests seem to show a substantial improvement on my machine:

Running tests with `DB=mysql`
transfer Time: 5.84s
transfer-with-metadata-table Time: 17.3s
transfer-with-metadata-column Time: 6.06s

Running tests with `DB=postgres`
transfer Time: 5.91s
transfer-with-metadata-table Time: 18.5s
transfer-with-metadata-column Time: 6.42s

Running tests with `DB=sqlite`
transfer Time: 6.20s
transfer-with-metadata-table Time: 17.2s
transfer-with-metadata-column Time: 6.02s

This is achieved via a default for new installs, which sets config.json_metadata = true in an initializer, which is also newly generated by the installer. The default value is false, so existing installs should work just fine with the old metadata table. I have tests in place for both scenarios.

It's worth noting that I haven't added any indexes on the metadata column. I know that for Postgres, efficient querying is possible using a gin index. However I'm not sure about MySQL, and I honestly doubt it matters for SQLite since that's presumably just used for development. I'd be interested in input on that subject.

@swrobel swrobel force-pushed the json-metadata branch 7 times, most recently from 722ca5e to c219373 Compare November 21, 2019 17:33
@swrobel
Copy link
Contributor Author

swrobel commented Dec 2, 2019

@orien any thoughts? I'd love to get this merged in for the 2.0 release.

@swrobel
Copy link
Contributor Author

swrobel commented Jan 6, 2020

Happy New Year @orien 🎉 Any chance you could take a look at this soon? Thanks!

@orien
Copy link
Member

orien commented Jan 6, 2020

Happy new year @swrobel! I'm loving this change. Thanks heaps.

@swrobel
Copy link
Contributor Author

swrobel commented Jan 6, 2020

Great! I'd love your thoughts on indexes, if you have any

@swrobel
Copy link
Contributor Author

swrobel commented Jan 10, 2020

To focus the discussion, it seems that the current index on the line_metadata is composite, and really only optimizes querying for a specific key within a particular line's metadata, not querying for a certain value across lines. Since this implementation already stores the metadata in the row, there's no further optimization needed unless we want to enable queries like Line.where(metadata: { key: 'value' }), but based on how I believe metadata is intended to be used, this is an unnecessary optimization for most use cases and would just create index bloat. Users can invividually add this index if they need to support such queries (although as I mentioned, I know what the appropriate index is for Postgres, but not MySQL or SQLite).

If you agree, @orien, I would consider this ready to merge.

Copy link
Member

@orien orien left a comment

Choose a reason for hiding this comment

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

Users can invividually add this index if they need to support such queries.

I agree. There's nothing in this gem that queries the metadata. This is left for individual teams to decide which selects will be performed and thus which indexes will be optimal.

envato/double_entry-reporting provides one example although that is in desperate need of documenting.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@swrobel
Copy link
Contributor Author

swrobel commented Jan 23, 2020

@orien requested changes were made

Copy link
Contributor

@ricobl ricobl left a comment

Choose a reason for hiding this comment

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

I'm happy with the change, just have a question about using the config for conditionally requiring the line_metadata module.

lib/double_entry.rb Outdated Show resolved Hide resolved
@swrobel
Copy link
Contributor Author

swrobel commented Jan 24, 2020

@ricobl I believe I've implemented a fix that should work for both configurations.

@ricobl
Copy link
Contributor

ricobl commented Jan 24, 2020

@ricobl I believe I've implemented a fix that should work for both configurations.

Looks good! Nice fix @swrobel.

@orien orien merged commit 3da7d0b into envato:master Jan 24, 2020
@swrobel swrobel deleted the json-metadata branch January 25, 2020 00:33
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