-
Notifications
You must be signed in to change notification settings - Fork 25
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
Feature/issue59 #60
base: feature/customUserModel
Are you sure you want to change the base?
Feature/issue59 #60
Conversation
def bulk_create_with_manual_ids(self,instances): | ||
try: | ||
start = self.select_for_update().all().order_by('-pk')[0].pk + 1 | ||
except IndexError: | ||
start = self.select_for_update().latest(field_name='pk').pk + 1 |
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.
Good catch.
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.
oh cool that's much cleaner. I'm guessing it does the same thing under the hood tho right?
(fyi the [0] causes a LIMIT 1 on the query so it doesnt get all pks)
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.
also not sure if it's causing any trouble but I think the select_for_update can be removed. I originally put it in to try and fix the race condition but that never really worked out as you know..
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.
just checked, its the same thing. wont make anything faster but is nicer looking:
In [9]: User.objects.all().values('id').order_by('-pk')[0]
DEBUG util (0.000) SELECT auth_user
.id
FROM auth_user
ORDER BY auth_user
.id
DESC LIMIT 1; args=()
In [10]: User.objects.values('id').latest('pk')
DEBUG util (0.000) SELECT auth_user
.id
FROM auth_user
ORDER BY auth_user
.id
DESC LIMIT 1; args=()
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.
Noted.
SELECT `sendgrid_emailmessage`.`id`, `sendgrid_emailmessage`.`message_id`, `sendgrid_emailmessage`.`from_email`, `sendgrid_emailmessage`.`to_email`, `sendgrid_emailmessage`.`category`, `sendgrid_emailmessage`.`response`, `sendgrid_emailmessage`.`creation_time`, `sendgrid_emailmessage`.`last_modified_time` FROM `sendgrid_emailmessage` ORDER BY `sendgrid_emailmessage`.`id` DESC LIMIT ? FOR UPDATE
I think the solution is something like
self.select_for_update().values('pk').latest(field_name='pk')[0]["pk"] + 1
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.
Huh? latest returns an instance of the model, not a list or queryset so that would raise an exception. Also are we sure that query you posted is coming from this line?
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.
If latest doesn't just slice then maybe that's not quite right. The point it that it only selects values which are indexed.
Should fix #59
I don't think this actually changes the logic, since we never check any of the locked rows bar the latest. @hoddez should verify