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

Add: note re: gin index for jsonb #535

Merged
merged 3 commits into from
May 4, 2021

Conversation

benkoshy
Copy link
Contributor

Why this PR?

  • I forgot to add a gin index. Hopefully this will help people remember.

Please feel free to close if this is not useful/beneficial.

Ben

@jrochkind
Copy link
Contributor

Thanks for the suggestion!

Thinking about it, the point of a gin index would be if you are trying to querying on specific properties of the json. Like say "WHERE file_data ->> metadata -> content_type = 'application/pdf'" -- and I'm not totaly sure a GIN index will catch that particular example, but something like that.

But nothing in shrine will do that itself, right? You'd have to be doing things yourself to need it.

It can also be a bit confusing to be sure exactly what queries will be covered by a GIN index or not, I'm not sure it will reliably handle any possible query, or do so ideally.

If we are going to include mention of postgres-specific indexing for jsonb, I think we should probably link to a postgres documentation page on jsonb indexing options, or perhaps just note "you may want to look into indexing", rather than suggesting that a GIN index will be universally appropriate or taking responsibility for that recommendation.

@benkoshy
Copy link
Contributor Author

@jrochkind agreed:

If we are going to include mention of postgres-specific indexing for jsonb, I think we should probably link to a postgres documentation page on jsonb indexing options, or perhaps just note "you may want to look into indexing", rather than suggesting that a GIN index will be universally appropriate or taking responsibility for that recommendation.

I shall add in a link when I get the chance.

My assumption was that folks who use jsonb will want to search within the image_data field conveniently and powerfully - otherwise plain text would do just as well. the only real benefit of jsonb, as I understand it, is indexing and searchability.

@jrochkind
Copy link
Contributor

I see your point. Postgres docs say "in general, most applications should prefer to store JSON data as jsonb, unless there are quite specialized needs, such as legacy assumptions about ordering of object keys," so I just use jsonb for json values without thinking about it, whether I am going to query a key or not.

jsonb has some other potential uses even if not searching, like using json operators for atomic changes on update too. And even if searching, whether an index is important can depend on how many rows you have etc.

Anyway, I'm not a shrine maintainer, this is just my thoughts!

README.md Outdated Show resolved Hide resolved
@benkoshy benkoshy force-pushed the add-note-re-gin-index branch 2 times, most recently from 0c9eeca to 8a03fb5 Compare April 30, 2021 08:30
@benkoshy
Copy link
Contributor Author

@jrochkind @hmistry I have updated as per comments / discussion (with a force push).

Feel free to close without compunction if not useful, or does not align with the library's goals etc.

@hmistry
Copy link
Member

hmistry commented May 4, 2021

@benkoshy Thank you for the PR and the following note:

Feel free to close without compunction

It's nice to know your stance as being maintainers we want to encourage contributions, give feedback for changes, and allow them to make so they get credit and feel good about contributing.

@hmistry hmistry merged commit e442b68 into shrinerb:master May 4, 2021
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