-
Notifications
You must be signed in to change notification settings - Fork 421
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
Prepared queries for inbox #3078
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3078 +/- ##
=======================================
Coverage 78.82% 78.83%
=======================================
Files 379 376 -3
Lines 31234 31266 +32
=======================================
+ Hits 24620 24648 +28
- Misses 6614 6618 +4
Continue to review full report at Codecov.
|
Make prepared upserts work with literal expressions e.g. unread_count = unread_count + 1
d99a561
to
1551e71
Compare
8a5cb5f
to
c0a1171
Compare
- Ue two queries in a transaction to guarantee atomicity - Do not use RETURNING/OUTPUT as they are DB-specific and would require preparing different queries for each DB. This is consistent with what we did for previous modules. In the future we might add a function to mongoose_rdbms that would prepare such queries automatically (like upserts).
4165c51
to
d715eb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
-spec prepare_upsert(Host :: mongoose_rdbms:server(), | ||
QueryName :: atom(), | ||
TableName :: atom(), | ||
InsertFields :: [binary()], | ||
UpdateFields :: [binary()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be an extra argument UpdateSQL
, which is empty by default.
And so we would have prepare_upsert/6 and prepare_upsert/7.
But not thaaat critical, it's called once per statement anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit I avoided adding the extra argument. I could do it like you suggest to make it more obvious for the user. It's not just any SQL though, it has to be column = value
separated by commas... or a list. Maybe I will leave it as it is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
mod_inbox_rdbms
Details in commit messages