siddhantgoel / tornado-sqlalchemy

SQLAlchemy support for Tornado
https://tornado-sqlalchemy.readthedocs.io/en/latest/
MIT License
125 stars 21 forks source link

avoid commit on select queries #84

Closed WebDevelopNemanja closed 5 years ago

WebDevelopNemanja commented 5 years ago

On services where only fetching from database, commit it not necessary. Can you add some flag on context manager to avoid commit on that kind of session. I think that would be optimization.

siddhantgoel commented 5 years ago

Can you give me an example of how you're using it where you feel avoiding a commit would help?

WebDevelopNemanja commented 5 years ago

here is service: https://user-images.githubusercontent.com/4808609/66203689-426fde80-e6a9-11e9-9295-375ae5fffaac.png

here is raw log from sqlalchemy for that query: https://user-images.githubusercontent.com/4808609/66203737-5f0c1680-e6a9-11e9-89f6-fbb7a41c22b9.png

as you can see in the last line of log there is "commit" line, but database was not changed/updated with what service, just fetching.

siddhantgoel commented 5 years ago

Not sure if a flag is the right thing to do here. A better option would be to check if SQLAlchemy marked the session as dirty.

WebDevelopNemanja commented 5 years ago

Sure, it's better. So, am I connecting properly in that code snippet?

siddhantgoel commented 5 years ago

In your snippet, the session wouldn't be marked dirty. So if we were to add such a check, the commit wouldn't happen.

I still feel such a check (or a flag) is not necessary. Sounds like a micro optimisation. Unless you're noticing problems in your response times because of the final commit?

WebDevelopNemanja commented 5 years ago

Yes, it is micro-optimization, but we get 1 hit less to database :)

siddhantgoel commented 5 years ago

Sorry, but I'm hesitant about implementing optimizations without seeing numbers that prove otherwise :man_shrugging: