search

ElixirMostWanted / elixirmostwanted.com

58 stars 8 forks source link

Votes counter cache #12

Closed sylvesterroos closed 1 month ago

sylvesterroos commented 2 months ago

Closes #10

sylvesterroos commented 2 months ago

After thinking about it, I think using triggers is not the correct approach here because I don't see the problem with running a simple count(*), while building triggers takes a lot of SQL code and whoever manages the database must be careful to not disable the trigger. Given that the votes table will not be excessively large, I think that we should simply use a join on the votes table whenever we want to know how many votes a wanted has.

oliver-kriska commented 2 months ago

You can do materialized view and make code which will update it in Elixir. Views can be connected with Ecto like regular schema so it's nothing special.

sylvesterroos commented 2 months ago

Materialized views are often used in big reporting databases. Here we can do a simple join, so why not do just that? Follow KISS

oliver-kriska commented 2 months ago

I know, just suggesting better solution than db triggers how you mention.

Materialized views are often used in big reporting databases. Not really big reporting databases, mostly when it makes sense, it depends on data and queries. You can have small data but too complex query with calculation.

bcardarella commented 1 month ago

This looks good other than the lockfile change.

sylvesterroos commented 1 month ago

@bcardarella how do you feel about this though?

After thinking about it, I think using triggers is not the correct approach here because I don't see the problem with running a simple count(*), while building triggers takes a lot of SQL code and whoever manages the database must be careful to not disable the trigger. Given that the votes table will not be excessively large, I think that we should simply use a join on the votes table whenever we want to know how many votes a wanted has.

bcardarella commented 1 month ago

you want to run count on every single read?

sylvesterroos commented 1 month ago

Oops, clicked review request without seeing you already replied.

Yes, that's what I was thinking. To me triggers seem like unneeded complexity but I'd like to hear your thoughts on it :slightly_smiling_face:

bcardarella commented 1 month ago

I don't agree

sylvesterroos commented 1 month ago

That's settled then. Feel free to merge this PR if the latest changes look good to you

bcardarella commented 1 month ago

Sorry was on mobile, let me provide more context.

Most app devs tend to shy away from db layer stuff. We app devs do lean heavily into abstractions and APIs and see the SQL itself as verbose and less maintainable. I'd be willing to wager that the trigger code will sit unchanged and undisturbed for quite some time.

Databases are heavily under utilized when it comes to so many tasks that we end up using the application layer for, and usually app layer is far less performant. Embrace the database :)

Will merge, thank you!