Disable subcompactions for user-defined timestamps

[ltamasi]

Summary:
The subcompaction boundary picking logic does not currently guarantee
that all user keys that differ only by timestamp get processed by the same
subcompaction. This can cause issues with the CompactionIterator state
machine: for instance, one subcompaction that processes a subset of such KVs
might drop a tombstone based on the KVs it sees, while in reality the
tombstone might not have been eligible to be optimized out.
(See also #6645, which adjusted the way compaction inputs are picked for the
same reason.)

Test Plan:
Ran make check and the crash test script with timestamps enabled.

[facebook-github-bot]

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

[jay-zhuang]

I think it worth documenting that, maybe here:

rocksdb/include/rocksdb/options.h

Lines 615 to 621 in 80a59a0

	// This value represents the maximum number of threads that will
	// concurrently perform a compaction job by breaking it into multiple,
	// smaller ones that are run simultaneously.
	// Default: 1 (i.e. no subcompactions)
	//
	// Dynamically changeable through SetDBOptions() API.
	uint32_t max_subcompactions = 1;

Or maybe we should just explicitly return unsupported if the user is setting subcompaction number > 1 and timestamp is set.

[ltamasi]

I think it worth documenting that, maybe here:

rocksdb/include/rocksdb/options.h

Lines 615 to 621 in 80a59a0

	// This value represents the maximum number of threads that will
	// concurrently perform a compaction job by breaking it into multiple,
	// smaller ones that are run simultaneously.
	// Default: 1 (i.e. no subcompactions)
	//
	// Dynamically changeable through SetDBOptions() API.
	uint32_t max_subcompactions = 1;

Or maybe we should just explicitly return unsupported if the user is setting subcompaction number > 1 and timestamp is set.

I consider this just a temporary measure since it is possible to support subcompactions for user-defined timestamps with some work. So I don't think we should update the API description; we should probably call this out in HISTORY.md though.

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

@ltamasi merged this pull request in 1462638.

[riversand963]

Thanks @ltamasi for the fix. Could you open a task so that we can track it?