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

Feature/read transaction for non rmw commands #440

Merged
merged 10 commits into from
Dec 29, 2024

Conversation

danielealbano
Copy link
Owner

This pull request introduces transaction management improvements to several benchmark operations in the hashtable implementation. The changes ensure that transactions are properly acquired and released during various operations, enhancing the consistency and reliability of the hashtable.

The change is partial and to be revised with a different PR.

Transaction management improvements:

Function signature updates:

  • src/data_structures/hashtable/mcmp/hashtable_op_delete.c and hashtable_op_delete.h: Modified function signatures to include transaction_t parameters, ensuring transactions are passed and managed correctly. [1] [2] [3]
  • src/data_structures/hashtable/mcmp/hashtable_op_get.c: Updated the hashtable_mcmp_op_get function to accept a transaction_t parameter, facilitating proper transaction management.

Code cleanup and consistency:

@danielealbano danielealbano added the enhancement New feature or request label Dec 29, 2024
@danielealbano danielealbano self-assigned this Dec 29, 2024

// Drops the increment and retry
bucket_index -= increment;
keys_eviction_candidates_list_count--;

Check notice

Code scanning / CodeQL

For loop variable changed in body Note

Loop counters should not be modified in the body of the
loop
.
storage_db_entry_index_t *entry_index = NULL;
if (!hashtable_mcmp_op_get_by_index(
db->hashtable,
database_number,

Check failure

Code scanning / CodeQL

Potentially uninitialized local variable High

The variable
database_number
may not be initialized at this access.

Copilot Autofix AI 2 months ago

To fix the problem, we need to ensure that database_number is initialized before it is used. The best way to fix this without changing existing functionality is to initialize database_number to a default value at the point of declaration. This ensures that it has a defined value when it is used later in the code.

Suggested changeset 1
src/storage/db/storage_db_snapshot.c

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/storage/db/storage_db_snapshot.c b/src/storage/db/storage_db_snapshot.c
--- a/src/storage/db/storage_db_snapshot.c
+++ b/src/storage/db/storage_db_snapshot.c
@@ -942,3 +942,3 @@
          bucket_index++) {
-        storage_db_database_number_t database_number;
+        storage_db_database_number_t database_number = 0;
         char *key = NULL;
EOF
@@ -942,3 +942,3 @@
bucket_index++) {
storage_db_database_number_t database_number;
storage_db_database_number_t database_number = 0;
char *key = NULL;
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@danielealbano
Copy link
Owner Author

danielealbano commented Dec 29, 2024

A set of commands need to be updated to use the new transaction model, now to modify an entry after a read lock has been issued requires upgrading the lock to a write lock.

This change will be implemented with a separated PR, until then some commands will be broken.

The security issue is coming off a different, older, PR and will be fixed with an ad hoc PR.

@danielealbano danielealbano merged commit b1b51a9 into main Dec 29, 2024
1 of 4 checks passed
@danielealbano danielealbano deleted the feature/read-transaction-for-non-rmw-commands branch December 29, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant