backupccl: incremental backups can miss non-transactional writes

[pbardea]

Non-transactional writes, such as AddSSTable requests may write at a timestamp arbitrarily far in the past. As such, the incremental iterator used by backup will miss these writes if they write to a timestamp prior to that of the previous backup.

Indexes that are being added to tables should only be backed up when they appear online. Additionally, these spans should be considered newly introduced so that backup performs a full scan of the index, rather than an incremental read.

Offline tables should be included in the backup to account for the case where a table that was previously online was taken offline. The writes to the table while it was OFFLINE will need to be captured in a subsequent incremental backup once the table returns online.

[mwang1026]

As discussed offline (har, har) we also need to exclude non-public tables.

[pbardea]

Chatted with @dt about the approaches on handing OFFLINE tables. I'm summarizing our conversation here and outlining plans on how to move forward here.

Today, database backups do not include OFFLINE tables. This means that they don't run into this particular issue, but an incremental backup that would have missed data would have failed will run into #61571 instead. So, this ticket will address solving the problem for OFFLINE tables include in cluster backups. The solution is similar for database backups with OFFLINE tables once those are included.

In general, the fix that we need (until ExportRequest supports non-MVCC operations) is for the first incremental backup after the offline table returns to public to capture all changes since the table was brought offline (ie all of the AddSSTable traffic).

What We Do Today

Backup/restore manages the covering of span/time space that it needs to include in incremental backups. This can be visualized by plotting the span/time space that each incremental backup covers. Today, each incremental backup of an offline table will just cover the difference from the last backup:

However inc2 and inc3 will be missing AddSSTable requests that have written at time t(import_start) after inc1 was taken. This means that restores at t > t(import_end) will be missing data.

Fix 1

The first step to fixing this is for the first incremental backup after the table comes back online (inc3 in this case), to take another full backup of this table (rather than incrementally capture the difference). This is the easiest option to implement since we already have a mechanism to do this (introduced spans). This would yield in:

The drawback is that inc3 will need to re-backup all of the data that was in the table pre-import as well as all of the imported data.

Fix 2

To resolve the drawback, further changes can be made so that instead of covering all the way back to t=0, inc3 just covers back until the import started:

This optimization will only be implemented after fix 1 since it requires a more disruptive code change, which may not be backportable.

This solution would perform an incremental read from t(import_start) until t(inc3). Today, this won't work because we check that the start time of ExportRequests are above the GC threshold and the GC threshold may be at any point below t(inc2) - including times above t(import_start). However, today IMPORT, IMPORT INTO, and RESTORE do not overwrite keys that previously existed in the table. And since we know that t(import_end) is above the GC threshold, we can conclude that it is safe to read the KVs from this table all the way back to t(import_start). ExportRequest is not aware of this though, so we would need to convince it it is safe (possibly by passing in a flag overriding it's check of the start time against the GC threshold).

[dt]

Closing as done by #63121