Fix range deletion tombstone ingestion with global seqno

[jeffrey-xiao]

If we are writing a global seqno for an ingested file, the range tombstone metablock gets accessed and put into the cache during ingestion preparation. At the time, the global seqno of the ingested
file has not yet been determined, so the cached block will not have a global seqno. When the file is ingested and we read its range tombstone metablock, it will be returned from the cache with no global seqno. In that case, we use the actual seqnos stored in the range tombstones, which are all zero, so the tombstones cover nothing.

This commit removes global_seqno_ variable from Block. When iterating over a block, the global seqno for the block is determined by the iterator instead of storing this mutable attribute in Block. Additionally, this commit adds a regression test to check that keys are deleted when ingesting a file with a global seqno and range deletion tombstones.

[ajkr]

@siying what do you think about moving global_seqno_ out of Block and instead obtaining it from BlockBasedTableReader? That prevents us from seeing wrong seqnum for blocks that are accessed and cached during file ingestion preparation. There are a couple benefits.

(1) we can remove this workaround for data blocks:

rocksdb/db/external_sst_file_ingestion_job.cc

Lines 409 to 414 in 1729779

	// During reading the external file we can cache blocks that we read into
	// the block cache, if we later change the global seqno of this file, we will
	// have block in cache that will include keys with wrong seqno.
	// We need to disable fill_cache so that we read from the file without
	// updating the block cache.
	ro.fill_cache = false;

(2) seqnums for meta-blocks like range tombstone meta-block will be correct regardless of whether they're placed in cache during file ingestion preparation

[siying]

@ajkr I think it is a good idea.

[ajkr]

I think this is an important fix. If Jeffrey's not available to rebase I can do it once review looks promising.

[ajkr]

Ping @ltamasi. Rebased anyways as I still think this is an important fix. Please let me know any questions.

[ltamasi]

Thanks for the patch @jeffrey-xiao and sorry about the delay! The PR looks very good in general; I just have a few comments.

Thanks for the rebase @ajkr !

[ltamasi]

You can just say ASSERT_OK(s); here, or even combine it with the previous line (ASSERT_OK(sst_file_writer.Finish(&file_info));).

[ltamasi]

ASSERT_TRUE(Get(Key(i)).IsNotFound()) would be a bit cleaner here.

[ltamasi]

The problem here (not with the PR per se but with the pre-existing default parameter values) is that removing _global_seqno changes the meaning of code like this:

Block reader1(std::move(contents), kDisableGlobalSequenceNumber); // kDisableGlobalSequenceNumber is now read_amp_bytes_per_bit !!!

We should review all places where Blocks are constructed and fix any problems like this.

[ltamasi]

Also, could you mention the fix in HISTORY.md?

[ajkr]

I need to open a new PR with a source branch to which I have permission to push changes. Doing that and replying to comments there: #6429.