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

feat(dao): support diff transaction #13586

Merged
merged 9 commits into from
Sep 5, 2024
Merged

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Aug 28, 2024

Summary

KAG-3258
Target PR: #13157

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #[issue number]

@chronolaw chronolaw marked this pull request as ready for review August 28, 2024 08:39
@chronolaw chronolaw requested a review from dndx August 29, 2024 02:33
Copy link
Member

@dndx dndx left a comment

Choose a reason for hiding this comment

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

The get_latest_version function need to be made transactional safe as well by returning MAX(version), as in case of transaction rollback, the pg_get_serial_sequence('clustering_sync_version', 'version') function will return a version number that does not exist, and cause issues in the DP.

@chronolaw chronolaw requested a review from dndx September 2, 2024 00:52
This reverts commit c97f868.
@chronolaw
Copy link
Contributor Author

chronolaw commented Sep 2, 2024

The get_latest_version function need to be made transactional safe as well by returning MAX(version), as in case of transaction rollback, the pg_get_serial_sequence('clustering_sync_version', 'version') function will return a version number that does not exist, and cause issues in the DP.

I think we already rollbacked the insert operation of "clustering_sync_version" (INSERT INTO clustering_sync_version DEFAULT VALUES RETURNING version INTO new_version;), is it not enough?

And the function max() can not be applied to the pg_get_serial_sequence('clustering_sync_version', 'version') since it is a serial name (string type).

Copy link
Member

@dndx dndx left a comment

Choose a reason for hiding this comment

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

@chronolaw I do not believe this is safe, as sequences can not be rolled back due to concurrent transaction could be happening while this transaction is active. So in this case, we should apply max on the version field, not the sequence.

See: https://stackoverflow.com/questions/2095917/sequences-not-affected-by-transactions

@chronolaw
Copy link
Contributor Author

@chronolaw I do not believe this is safe, as sequences can not be rolled back due to concurrent transaction could be happening while this transaction is active. So in this case, we should apply max on the version field, not the sequence.

See: https://stackoverflow.com/questions/2095917/sequences-not-affected-by-transactions

Done.
Since version is already a primary key, I think that postgres will create a index for it automaticlly.

@chronolaw chronolaw requested a review from dndx September 5, 2024 00:46
@dndx dndx merged commit 9bc1338 into feat/incremental Sep 5, 2024
22 of 30 checks passed
@dndx dndx deleted the feat/diff_transaction branch September 5, 2024 07:14
dndx pushed a commit that referenced this pull request Sep 9, 2024
* txn in strategy

* register_dao_hooks

* fix txn funcs

* comments

* hook dao:xxx:fail

* dao:delete:fail

* get_latest_version

* Revert "get_latest_version"

This reverts commit c97f868.

* select max
chronolaw added a commit that referenced this pull request Sep 20, 2024
* txn in strategy

* register_dao_hooks

* fix txn funcs

* comments

* hook dao:xxx:fail

* dao:delete:fail

* get_latest_version

* Revert "get_latest_version"

This reverts commit c97f868.

* select max
chronolaw added a commit that referenced this pull request Oct 12, 2024
* txn in strategy

* register_dao_hooks

* fix txn funcs

* comments

* hook dao:xxx:fail

* dao:delete:fail

* get_latest_version

* Revert "get_latest_version"

This reverts commit c97f868.

* select max
dndx pushed a commit that referenced this pull request Oct 16, 2024
* txn in strategy

* register_dao_hooks

* fix txn funcs

* comments

* hook dao:xxx:fail

* dao:delete:fail

* get_latest_version

* Revert "get_latest_version"

This reverts commit c97f868.

* select max
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants