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

Insert ignore for vindexes without autocommit appear incorrect. #4974

Closed
mpawliszyn opened this issue Jul 2, 2019 · 7 comments · Fixed by #4978
Closed

Insert ignore for vindexes without autocommit appear incorrect. #4974

mpawliszyn opened this issue Jul 2, 2019 · 7 comments · Fixed by #4978

Comments

@mpawliszyn
Copy link
Collaborator

I created a test that proves this.
mpawliszyn@8202a11

@sougou
Copy link
Contributor

sougou commented Jul 2, 2019

@mpawliszyn
Copy link
Collaborator Author

No that one uses an autocommit vindex so it works. I'm pretty sure the problem is that the insert ignore verify only works when the insert into the vindex has been committed. I suspect we aren't sending the right transaction id during that verify

@sougou
Copy link
Contributor

sougou commented Jul 2, 2019

Ahh. I see. Looks like I tested the wrong table. Let me try this one out.

@sougou
Copy link
Contributor

sougou commented Jul 2, 2019

Ok. I believe the test for t2 is still legit, and it would have failed without the ordering fix in that PR.
However, insert ignore does not work for consistent lookups. This is a different bug in the consistent lookup itself?

@mpawliszyn
Copy link
Collaborator Author

Yes t2 is absolutely legit and it fixes the problem we were having. We only use autocommit vindexes. I just found this bug while I was working on this so I wanted to track it somewhere.

Note that if you try the insert ignore twice it works because in the first call the vindex is inserted so when the second call is made the verify works.

@sougou
Copy link
Contributor

sougou commented Jul 2, 2019

Yeah. I found the problem. The fix may be tricky because Verify is used from two different code paths (one for owned, the other unowned), and may have to use different commit order directives. I have to think of a way to do this without changing the Vindex API.

@mpawliszyn
Copy link
Collaborator Author

mpawliszyn commented Jul 2, 2019 via email

sougou added a commit to planetscale/vitess that referenced this issue Jul 3, 2019
Fixes vitessio#4974

Insert ignore wasn't working correctly because the verify was using
the normal transaction instead of the 'pre' transaction where the
rows were actually created.

This implementation changes the consistent lookup's Verify function
to also use the 'pre' transaction always.

There is another code path that gets used if the vindex is unowned.
That will also cause a 'pre' transaction to be created. Without this
change, the transaction would have been a normal one. This mildly
affects the commit order, but there should be no material difference.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
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 a pull request may close this issue.

2 participants