-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
bugfix: fix the failure of rollbacking back of the delete SQL at AT m… #6511
base: 2.x
Are you sure you want to change the base?
bugfix: fix the failure of rollbacking back of the delete SQL at AT m… #6511
Conversation
…ode when using SQLServer (apache#6511)
70b68db
to
1453629
Compare
…ode when using SQLServer (apache#6511)
1453629
to
7104fb0
Compare
At AT mode when using SQLServer, if any column of a table is with "IDENTITY", before inserting a value into this column, we should judge whether there is a "IDENTITY" with it or not. Or the database will report errors, ie "Table 'xxx' does not have the identity property. Cannot perform SET operation" and "Cannot insert explicit value for identity column in table 'xxx' when IDENTITY_INSERT is set to OFF." |
import java.sql.Connection; | ||
import java.sql.PreparedStatement; | ||
import java.sql.ResultSet; | ||
import java.sql.SQLException; | ||
import java.sql.Statement; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.stream.Collectors; |
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.
Please put the Java package at the top level.
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.
Done!
|
||
public class SqlServerUndoDeleteExecutor extends BaseSqlServerUndoExecutor { | ||
|
||
private boolean tableIdentifyExistence = false; |
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 hope this variable will eventually be cached in TableMeta and the caching-related refresh and retrieval will be implemented in SqlServerTableMetaCache, rather than in the executor, where it needs to be executed every time executeOn is called.
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.
Originally, I considered putting it in a cache. But then I thought of that the application has to be started again to refresh the cache when a column that has something to do with ""INDENTITY changed. So I quit to do that.
Now I rethink that it's probable for a application to restart when a column is changed.
I will fix it tonight.
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.
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.
My idea is to have tableIdentifyExistence
handled together with resultSetMetaToSchema
in SqlServerTableMetaCache
, rather than handling it separately in SqlServerUndoDeleteExecutor
.
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 pondered about it last night. And I found that the cahce key should be a combination of resourceId, schema and table name. It may be a feasible way to put it in the subclass of TableMeta. But when I was creating a subclass of TableMeta named SqlServerTableMeta, I found it a little elegant again. So I make a compromise and fix it in this way. What do you think of it? Any better ideas?
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.
There is a typo there. It should be " I found it a little not elegant again". o(╯□╰)o
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 found it a good way to judge whether there is a "IDENTITY" in a table by creating a subclass of TableMeta named SqlServerTableMeta with a new filed in it for justification. But I found there are no other subclass of TableMeta for other SQL dialect. Maybe it sounds a bit inelegant, too. What do you think of it? @funky-eyes
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 found it a good way to judge whether there is a "IDENTITY" in a table by creating a subclass of TableMeta named SqlServerTableMeta with a new filed in it for justification. But I found there are no other subclass of TableMeta for other SQL dialect. Maybe it sounds a bit inelegant, too. What do you think of it? @funky-eyes
I think it's possible to create a subclass to provide specific metadata for different databases. I agree with your approach, let's go ahead and do it.
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.
Done! Please review! Thanks!
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.
@funky-eyes Please help me review! Thanks!
…ode when using SQLServer (apache#6511)
7104fb0
to
7643244
Compare
…identity-error # Conflicts: # changes/en-us/2.x.md # changes/zh-cn/2.x.md
…ode when using SQLServer (apache#6511)
Ⅰ. Describe what this PR did
fix the failure of rollbacking back of the delete SQL at AT mode when using SQLServer
Ⅱ. Does this pull request fix one issue?
fix #6449
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews