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

Support DDL in manual transaction #3128

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support DDL in manual transaction #3128

wants to merge 3 commits into from

Conversation

hououou
Copy link
Collaborator

@hououou hououou commented Mar 24, 2024

see #3062

TODO:

  • do not do much on rollback; for example, rollback hash index in CopyTable.
  • fail with inserting nodes after creating an empty table. Related to local storage issue.
  • should add more test cases. We currently have test cases:
    1. create, copy
    2. create drop tables (fail with create-drop table in one tx with commit_skip_checkpoint)
    3. create alter table

Copy link

codecov bot commented Mar 24, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.87%. Comparing base (c1f68cd) to head (ee892d3).
Report is 4 commits behind head on master.

❗ Current head ee892d3 differs from pull request most recent head 402d009. Consider uploading reports for the commit 402d009 to get more accurate results

Files Patch % Lines
src/storage/wal_replayer.cpp 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3128      +/-   ##
==========================================
- Coverage   91.95%   91.87%   -0.08%     
==========================================
  Files        1168     1167       -1     
  Lines       44085    44068      -17     
==========================================
- Hits        40538    40488      -50     
- Misses       3547     3580      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hououou hououou requested a review from ray6080 March 24, 2024 21:19
Copy link
Contributor

@ray6080 ray6080 left a comment

Choose a reason for hiding this comment

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

I saw incomplete logic and duplicated code, make sure you go through changes before requesting review.

Make sure our test coverage for DDL transactions is full under four cases: COMMIT, COMMIT RECOVERY, ROLLBACK, ROLLBACK RECOVERY.
Also, add tests on more cases, at least following ones (the more the better):

  • create multiple tables in a single transaction (node and rel tables)
  • create node/rel tables, drop node/rel tables in a single transaction
  • create tables, and alter tables in a single transaction

src/processor/operator/persistent/node_batch_insert.cpp Outdated Show resolved Hide resolved
default:
return true;
}
static bool allowActiveTransaction(StatementType /*statementType*/) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a todo for Chang?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Related to KUZU_API, affecting python, java, and rust APIs, which I can not handle

src/main/database.cpp Outdated Show resolved Hide resolved
src/include/storage/storage_manager.h Outdated Show resolved Hide resolved
src/include/storage/wal_replayer.h Outdated Show resolved Hide resolved
src/storage/storage_manager.cpp Outdated Show resolved Hide resolved
src/storage/store/node_table.cpp Outdated Show resolved Hide resolved
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 this pull request may close these issues.

None yet

2 participants