[CBRD-23850] coredump occurs when heap_get_visible_version_from_log() function is called after executing backupdb with -r option.

[hornetmj]

http://jira.cubrid.org/browse/CBRD-23850

When executing backupdb with the -r option, the archive log volume referenced by the transaction(mvcc) is not considered. So, coredump occurs when the transaction reads the old image of mvcc after performing the backupdb. There is a test scenario for this in the comments of the related issue. Looking at that scenario will help you understand. (I defined this problem as PROBLEM-1 in the jira issue.)

[wrlawodms]

Is it okay not to check vacuum_is_safe_to_remove_archives() here? This is used in logpb_remove_archive_logs_exceed_limit() .

[hornetmj]

After vacuum_boot() is called, I think there is no problem with calling vacuum_min_log_pageid_to_keep(). Do you have any other opinions on this?

[wrlawodms]

1. logpb_remove_archive_logs_exceed_limit() seems to be also called after vacuume_boot().
2. In my opinion, if it is missed in logpb_remove_archive_logs(). This function has to be always called when it is guaranteed that vacuum_Data.is_archive_removal_safe is set to true. It seems more stable to check it. Or you can assert() instead of my suggestion if the condition is pre-condition.

[hornetmj]

• The logpb_remove_archive_logs_exceed_limit() function is called from logpb_archive_active_log().
• So it can be called even before vacuum_boot().
• Therefore, I think vacuum_is_safe_to_remove_archives() must be called in order to operate after vacuum_boot().
• However, the logpb_remove_archive_logs() function does not seem to be a problem because it is a function that is called at runtime after server boot. What do you think?

[mhoh3963]

How about replacing all codes (line 6172 ~ 6193) that remove archive log to logpb_remove_archive_logs_exceed_limit() function ?
I think the function is safe, because it have routine such as checking "force remove archive" option, "vacuum min log" and "system crash min log" before removing archive logs.

[hornetmj]

@mhoh3963 Maybe it will work fine I think. However, running 'backupdb -r' will depend on the "log_max_archives" and "force_remove_log_archives" system properties. So, I think this is not a good choice. Nevertheless, if I need to replace it with the logpb_remove_archive_logs_exceed_limit() function, I think we need to reconsider the definition of the -r option. What do other reviewers(@joohok @wrlawodms) think?

[hornetmj]

I think vacuum_is_safe_to_remove_archives() is better than prm_get_bool_value (PRM_ID_DISABLE_VACUUM) for code consistency and readability. So I used it. Please refer to the review. In addition, the vacuum_disable value is being checked by logpb_is_page_in_archive(), so I don't think any issues we discussed will occur. However, I want to call the function vacuum_is_safe_to_remove_archives() for explicit processing.

[wrlawodms]

1. The comment for this function should be modified along.

 * NOTE: Archive that are not needed for system crashes are removed.
 *       That these archives may be needed for media crash recovery.
 *       Therefore, it is important that the user copy these archives
 *       to tape. Check the log information file.
 *
 * TODO: Make sure the removed logs have been processed by vacuum.

2. Could you let me know the reason there's no need to consider the minimum copied page id by copylogdb here?

[hornetmj]

1. The comment for this function should be modified along.

 * NOTE: Archive that are not needed for system crashes are removed.
 *       That these archives may be needed for media crash recovery.
 *       Therefore, it is important that the user copy these archives
 *       to tape. Check the log information file.
 *
 * TODO: Make sure the removed logs have been processed by vacuum.

2. Could you let me know the reason there's no need to consider the minimum copied page id by copylogdb here?

1. I'll fix the comment.
2. Your comments are not included in the purpose of this PR. If necessary, I will process it with another PR.