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

optimize: When the number of primary keys exceeds 1000, use union to concatenate the SQL #6957 #6987

Closed
wants to merge 3 commits into from

Conversation

remind
Copy link

@remind remind commented Nov 7, 2024

…oncatenate the SQL (#6957)

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

When the number of primary keys exceeds 1000, use union to concatenate the SQL

Ⅱ. Does this pull request fix one issue?

fixes #6957

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

sql生成有测试用例:
io.seata.rm.datasource.SqlGenerateUtilsTest
此测试类之前已经存在,此次在此基础上进行了修改

其它受影响的地方依赖其它已有测试用例

Ⅴ. Special notes for reviews

1、io.seata.rm.datasource.undo.AbstractUndoExecutor#queryCurrentRecords 该方法是使用了For Update,之前是否有可能有重复的PK,之前是一条sql,所有PK都在IN里面,现在拆分成多条sql循环执行,如果有重复PK可能出现死锁。

2、由于sql采用了union拼接,影响到了SQLSelectQueryBlock的获取,因此在*SelectForUpdateRecognizer此种类中,如io.seata.sqlparser.druid.mysql.MySQLSelectForUpdateRecognizer#getSelect,修改了获取方式,是否有可能造成影响。

Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an optimization, not a feature.

@remind
Copy link
Author

remind commented Nov 7, 2024

I think this is an optimization, not a feature.

是我死板了,虽然我也认为是optimization,但是CONTRIBUTING说明中没有这个类型,就写的feature了

@funky-eyes funky-eyes changed the title feature: When the number of primary keys exceeds 1000, use union to concatenate the SQL #6957 optimize: When the number of primary keys exceeds 1000, use union to concatenate the SQL #6957 Nov 7, 2024
@funky-eyes funky-eyes added this to the 2.3.0 milestone Nov 7, 2024
@funky-eyes funky-eyes added the first-time contributor first-time contributor label Nov 7, 2024
}
}
totalRowIndex += sqlCondition.getRowSize();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这段代码的意义是什么?
What is the significance of this code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

当主键超过1000时,sql会拆分成多条,并且按拆分后的sql循环执行,但是所有pk参数都在pkRowValues中,totalRowIndex是为了记录每条sql执行完成之后,在pkRowValues已经取到的参数行位置,做为下一条sql执行时取参数的起始值,其中sqlCondition.getRowSize()为当前sql参数行数,该行代码就为移动参数行。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

当主键超过1000时,sql会拆分成多条,并且按拆分后的sql循环执行,但是所有pk参数都在pkRowValues中,totalRowIndex是为了记录每条sql执行完成之后,在pkRowValues已经取到的参数行位置,做为下一条sql执行时取参数的起始值,其中sqlCondition.getRowSize()为当前sql参数行数,该行代码就为移动参数行。

ok,再github review页没看出来,整体pr改动我看了下应该问题不大, 我拉到本地测试一下,测试通过的话就可以合并进去了

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果没问题了,能否也放到1.8.1

Copy link
Contributor

@funky-eyes funky-eyes Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果没问题了,能否也放到1.8.1

1.8.1应该不会进行发布了

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

那要用这个只能升级到2.x了吗?后续1.x都不维护了吗?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

那要用这个只能升级到2.x了吗?后续1.x都不维护了吗?

没有维护的必要性,因为1.x的问题已经在2.x上修复了,已经是一种迭代了

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

那要用这个只能升级到2.x了吗?后续1.x都不维护了吗?

这个pr能否根据2.x的代码进行提交一个新pr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@funky-eyes
Copy link
Contributor

Could you please send me your DingTalk ID to my email? I would like to invite you to join the Seata developer group.
jianbin@apache.org

@funky-eyes funky-eyes added the mode: AT AT transaction mode label Nov 15, 2024
@remind remind closed this Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time contributor first-time contributor mode: AT AT transaction mode module/rm-datasource rm-datasource module type: optimize
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants