-
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:PK constraint name isn't the same as the unique index name which is belong to PK #4005
Conversation
… some whitespace around if else)
Codecov Report
@@ Coverage Diff @@
## develop #4005 +/- ##
=============================================
- Coverage 49.85% 49.81% -0.04%
+ Complexity 3761 3759 -2
=============================================
Files 698 698
Lines 23478 23495 +17
Branches 2902 2908 +6
=============================================
Hits 11705 11705
- Misses 10594 10609 +15
- Partials 1179 1181 +2
|
rm-datasource/src/main/java/io/seata/rm/datasource/sql/struct/cache/OracleTableMetaCache.java
Outdated
Show resolved
Hide resolved
if (index.getIndextype().value() == IndexType.UNIQUE.value()) { | ||
for (ColumnMeta col : index.getValues()) { | ||
if (pkcol.contains(col.getColumnName())) { | ||
times++; |
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.
How about change it to Primary directly if pkcol constains col.getColumnName?
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 thought we need to check the situation that PK is multiple column PK (i was not sure if the seata will surpport composite primary key). another situation is the PK only include one column,but the unique index was composite key, so there need to compare it exactly.
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 have a idea that is when rsPrimary is not empty and put a new IndexMeta instead of change it. We separate the Unique Key and Primary Key. How do you think about this?
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.
We change the Unique Key to Primary key before because when we create the primary key and Oracle will create a unique key automatically but now look like the solution of change the key type is now that good.
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.
for oracle will create a unique key automatically, it's not happend in all the situation. only ,if there is no unique index,it would create one automatically.but i thought my PR logic is right.if the index is unique and the column is exacly same as PK column. it means the unique index is the PK. I couldn't agree with you more. it's a good idea that use a individual IndexMeta to indicate PK,actually,i had thought this solution before. but i didn't check all the code of the seata,i have no idea how many places need to update for. IN some other Oracle like database, example, 达梦,the defalut unique index name that was created by when we created the PK always was not the same as PK name.
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.
You do not need to change all the code in seata, just need to keep the number of primary key and index type is right that s ok. You can check out the main code here, it will no problem if you keep the logic as same as before.
io.seata.rm.datasource.sql.struct.TableMeta#getPrimaryKeyOnlyName
io.seata.rm.datasource.sql.struct.TableMeta#getPrimaryKeyMap
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 got what you meaning is.but it's not easy to determine how can we get the indexMeta property.actually,the PK is constraint,the actual unique index was created by PK is stored in the view of all_indexes.it means we need to determine which unique index was a part of the PK(PK include constraint name and index name).
indeed,at step 2,it's not a indexname,it's a constraint name. maybe the current PR is good. i thought the key logic is accoding the PK constriant name to determine which unique index name was a part of the PK.
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.
OK. You idea was right. You still have to add some comments and let other developer understand . For example:
// when we create a primary key constraint oracle will uses and existing unique index.
// if we create a unique index before create a primary constraint in the same column will cause the problem that primary key constraint name was different from the unique name.
// a table can have only one primary key
List<String> pkcol = new ArrayList<>();
...
//save the columns that constraint primary key name was different from unique index name
pkcol.add(rsPrimary.getString("COLUMN_NAME"));
...
//find the index that belong to the primary key constraint
for (Map.Entry<String, IndexMeta> entry : tm.getAllIndexes().entrySet())
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.
thanks for your advice. I'll add some necessary comments later.
@@ -30,6 +25,14 @@ | |||
import io.seata.rm.datasource.sql.struct.TableMeta; | |||
import io.seata.sqlparser.util.JdbcConstants; | |||
|
|||
import java.sql.Connection; |
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.
don't move imports
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.
@a364176773 @slievrly @l81893521
1, I moved back some imports that i moved down before.
2,I have added some comments.
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.
LGTM
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.
LGTM
Ⅰ. Describe what this PR did
In some case,the table first created the unique index, and then created the PK but didn't give a explicit name the same as unique index name.
Ⅱ. Does this pull request fix one issue?
fixes #3987
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews