Handle file boundaries when timestamps should not be persisted

[jowlyzhang]

Handle file boundaries FileMetaData.smallest, FileMetaData.largest for when persist_user_defined_timestamps is false:
1) on the manifest write path, the original user-defined timestamps in file boundaries are stripped. This stripping is done during VersionEdit::Encode to limit the effect of the stripping to only the persisted version of the file boundaries.
2) on the manifest read path during DB open, a a min timestamp is padded to the file boundaries. Ideally, this padding should happen during VersionEdit::Decode so that all in memory file boundaries have a compatible user key format as the running user comparator. However, because the user-defined timestamp size information is not available at that time. This change is added to VersionEditHandler::OnNonCfOperation.

Test Plan:

make all check
./version_edit_test --gtest_filter="*EncodeDecodeNewFile4HandleFileBoundary*". 
./db_with_timestamp_basic_test --gtest_filter="*HandleFileBoundariesTest*"

[pdillinger]

LGTM. Thanks for the good comments and tests! (some recommendations)

[pdillinger]

I know that this is not performance critical code, but it bothers me somewhat that there's an extra key copy to do this. Not really because of this code but I expect something similar to happen in code that is more performance critical. I believe it's not simple because the timestamp is between the user key and the seqno+kind, but there could be a version of StripTimestampFromInternalKey that includes the length prefix. Or something like SliceParts could be used (but SliceParts does not encapsulate the sequence of Slices, just provides a view into an existing array of Slices). Something to think about.

[jowlyzhang]

I appreciate a lot all the performance related suggestions! You are right that even this path is not a hot one, similar ideas are used in more performance critical places. Also this is a great idea! It looks like there is an implementation PutLengthPrefixedSliceParts that is ready to use to help save this one copy. I will work on a follow up to update this and places with similar logic to use an in place stripping version of function StripTimestampFromInternalKey that returns a SliceParts, and encoding with PutLengthPrefixedSliceParts to save this extra copy.

[facebook-github-bot]

@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@jowlyzhang merged this pull request in f745263.