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.

Sign up for GitHub

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

Jump to bottom

[CBRD-23866] Fix not to reuse heap when creating a table with REUSE_OID #2584

Merged
merged 2 commits into from
Mar 8, 2021
Merged

[CBRD-23866] Fix not to reuse heap when creating a table with REUSE_OID #2584

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bdbac0
Fix not to reuse when creating with REUSE_OID
wrlawodms Feb 26, 2021
aa767e2
Use more readable condition
wrlawodms Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/storage/heap_file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5187,7 +5187,7 @@ heap_create_internal (THREAD_ENTRY * thread_p, HFID * hfid, const OID * class_oi

memset (&des, 0, sizeof (des));

if (prm_get_bool_value (PRM_ID_DONT_REUSE_HEAP_FILE) == false)
if (prm_get_bool_value (PRM_ID_DONT_REUSE_HEAP_FILE) == false && file_type != FILE_HEAP_REUSE_SLOTS)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. How about using '==' like the first condition of &&? I mean that the 'file_type == FILE_HEAP' expression is more readable I think.
  2. Codes that check FILE_HEAP_REUSE_SLOTS remain in the functions called when the 'if condition' is true. This is no longer unnecessary and needs to be removed. However, I ask you to keep a separate record for this because it is more efficient to deal with this problem in the reuse heap improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

in the code sequence, the heap_cache_class_info is called. the heap_cache_class_info asserts that file type is FILE_HEAP. I think therefore it needs check the file type is FILE_HEAP or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hornetmj

  1. I partially agree with that. I did like you said, but I changed because in pre-version like 10.1, file_type is already filtered and this code is based on it. If file_type == FILE_HEAP is used, the code will be different by version although the purpose is the same. What do you think about it?
  2. Do you mean all the code blocks not covered due to this patch should be removed together? For example, file_tracker_item_reuse_heap() and even heap_reuse()? I'm not sure it's right because that functions are designed to cover both file types and it'd be better to leave it for future use.

Copy link
Contributor Author

@wrlawodms wrlawodms Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@byungwook-kim Could you let me know more clearly? Do you have the same opinion with @hornetmj and give me one more reason?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wrlawodms

  1. I think it's right to move in the right direction. And as the code is changed to cpp, the logic is the same, but there are many cases where the code has changed.
  2. I partially agree with your opinion. The important thing is to write it down so we can consider it if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I agree

{
/*
* Try to reuse an already mark deleted heap file
Expand Down