backupccl: add deprecation notice for backup cmd with explicit subdir

[msbutler]

backupccl: add deprecation notice for backup cmd with explicit subdir

Release note (sql change): Previously, Backup commands allowed the user to
specify a custom subdirectory name for their backups via BACKUP .. INTO <subdir> IN <collectionURI>. After this change, this will no longer be
supported. Users can only create a full backup via Backup ... INTO <collectionURI> or an incremental backup on the latest full backup in their
collection via BACKUP ... INTO LATEST IN <collectionURI>. This deprecation
also removes the need to address a bug in SHOW BACKUPS IN which cannot display
user defined subdirectories.

[cockroach-teamcity]

This change is 

[msbutler]

One nuance to note: if the latest backup in a collection is one with a user defined subdir, an incremental backup with LATEST will not issue a warning. I think this is fine.

# Enter \? for a brief introduction.
#
demo@127.0.0.1:26257/movr> BACKUP INTO 'sub' IN 'userfile:///foo';
        job_id       |  status   | fraction_completed | rows | index_entries | bytes
---------------------+-----------+--------------------+------+---------------+---------
  750985278723489793 | succeeded |                  1 | 2602 |          1052 | 474395
(1 row)

NOTICE: BACKUP commands with an explicitly specified subdirectory will be removed in a future release. Users can create a full backup via `BACKUP ... INTO <collectionURI>`, or an incremental backup on the latest full backup in their collection via `BACKUP ... INTO LATEST IN <collectionURI>`

Time: 252ms total (execution 251ms / network 0ms)

demo@127.0.0.1:26257/movr> BACKUP INTO LATEST IN 'userfile:///foo';
        job_id       |  status   | fraction_completed | rows | index_entries | bytes
---------------------+-----------+--------------------+------+---------------+---------
  750985434582843393 | succeeded |                  1 |   41 |            47 | 251706
(1 row)


Time: 290ms total (execution 290ms / network 0ms)

[benbardin]

Just to make sure I understand correctly - we want to deprecate this in BACKUP, but not RESTORE, right? So they can restore from arbitrary points in a series of incremental layers?

[msbutler]

that's correct! Users of SHOW BACKUP and RESTORE should still be able to pick the specific the full backup in a collection.

[benbardin]

Nice, thank you!

[msbutler]

bors r=benbardin

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[dt]

Huh, I thought we only wanted to deprecate this only if there is not an existing backup there?

I believe it is perfectly fine to say BACKUP TO x/y/z IN abc if x/y/z is an existing full backup in abc -- that is how you indicate you want to add an incremental backup to x/y/z.

The deprecation here was motivated by our desire to limit how new possible values of x/y/z are added to abc, specifically to have them only be the date-based values we choose during backup into abc. But once there is a value of x/y/z in abc, I don't think we care how you say you want to backup into it, i.e. whether you say so explicitly or via reading latest .

[msbutler]

hm, i guess you're right. At first I didn't see a use case for creating an incremental backup for the non latest full backup in a collection; but if something goes wrong with the latest full backup chain in a collection, you may want to increment on an earlier backup. I'll fix this.

[dt]

use case for creating an incremental backup for the non latest full backup

You could also be backing up AS OF a particular time that is before the end time of the chain in latest, e.g. if latest has already captured some fat-finger mistake so you're going back and backing up AS OF right before you e.g. ran the UPDATE without the WHERE, into the previous chain.

[msbutler]

hmmmmm, now that is an intriguing use case I hadn't thought about, which implies we should clarify some things in the code:

1. Suppose the user runs: BACKUP INTO LATEST IN dest AS OF SYSTEM TIME earlyTime. This will always increments to the latest full backup in the collection. The last branch in this conditional never gets reached if LATEST is specified. Is that our desired behavior? LATEST trumps everything?

			if backupStmt.AppendToLatest {
				initialDetails.Destination.Subdir = latestFileName
			} else if subdir != "" {
				initialDetails.Destination.Subdir = "/" + strings.TrimPrefix(subdir, "/")
				// Deprecation notice for `BACKUP INTO` syntax with an explicit subdir.
				// Remove this once the syntax is deleted in 22.2.
				p.BufferClientNotice(ctx,
					pgnotice.Newf("BACKUP commands with an explicitly specified"+
						" subdirectory will be removed in a future release. Users can create a full backup via `BACKUP ... "+
						"INTO <collectionURI>`, or an incremental backup on the latest full backup in their "+
						"collection via `BACKUP ... INTO LATEST IN <collectionURI>`"))

			} else {
				initialDetails.Destination.Subdir = endTime.GoTime().Format(DateBasedIntoFolderName)
			}

2. Or, say the user runs BACKUP INTO someTimeSubdir IN dest AS OF SYSTEM TIME earlyTime. again, because of the code above, the explicit subdir overrides the subdir AS OF SYSTEM TIME wants to create.

3. In fact, with the collection based syntax, the only way to create a full backup chain with an AS OF SYTEM TIME subdir is to specify the time twice BACKUP INTO myTime IN dest AS OF SYSTEM TIME myTime.

I think we should discuss next week and create another issue with a ga blocker? I think we never caught this bc all of the as of system time tests use the old backup syntax :(

Another complication: this ambiguity is in 21.2.

[benbardin]

use case for creating an incremental backup for the non latest full backup

You could also be backing up AS OF a particular time that is before the end time of the chain in latest, e.g. if latest has already captured some fat-finger mistake so you're going back and backing up AS OF right before you e.g. ran the UPDATE without the WHERE, into the previous chain.

Wouldn't we still want that to be done on top of LATEST?

[dt]

Wouldn't we still want that to be done on top of LATEST?

Eh, not necessarily. Say it is now 22-04-08 09:15, and you have backup /22/4/7 with inc backups at 06, 12, 18 and then /22/4/8 with one inc backup at 06. You look at the logs and go "oh no, we ran this bad UPDATE last night at 21:30". You might then do BACKUP INTO /22/4/7 IN abc AS OF SYSTEM TIME 22-04-07 21:29:29 to extend the 22/4/7 chain from 18 to that last known-good time before the bad thing happened (21:29). You don't want to extend the latest chain, /22/4/8, since it is starts after the time in question (22/4/7-21:29).

[benbardin]

Sorry, I was ambiguous with "we" :) Users for sure might want to do what you describe, but don't we as CRDB developers think that's too complicated to be a good idea? I.e. the simplicity of "all incremental backups go on LATEST" seems really powerful to me - kinda feels like asking for trouble to help users make trees of backups that we can't really help them manage.

[dt]

Eh, I dunno. I think these should be separate concerns. "LATEST" as just a shorthand/helper for picking a value of x/y/z to which you want to (try to) append. If you say "LATEST", we read go read the value that that points to for you and substitute it into your command. But everything from then on should not care how you arrived at "x/y/z", just what is or isn't in the collection at x/y/z. I think the real distinction is whether you said a subdirectory at all, not whether it was implicit latest or explicit.

[msbutler]

What if we add the following guardrail: if the user passes AS OF SYSTEM TIME, fail fast if the LATEST subdir (or the explicitly passed time based subdir) is a timestamp after the provided AOST?

[dt]

I'd step back:

1. If the user passes a subdirectory at all -- either explicitly or as LATEST (so subdirectory!="" || appendToLatest) -- then if that subdirectory does not have a backup in it when we go to resolve it i.e. we elect to make a full backup there, that is deprecated in 22.1; we warn that falling back to a creating a full backup when attempting to append an incremental backup to a missing backup is deprecated. 22.2 makes it a hard failure.

2. if a user attempts to use an AOST when appending -- again, via latest or explicit -- and that time is less than start time of the prior backup they are appending to -- again, no matter how we found it -- hard fail. end cannot be before start (this might already be this way, but we should check/test using collections).

[dt]

More concretely, that'd probably mean:
a) remove the third case in the code you quoted that is run during planning, where we populate initialDetails.Destination.Subdir with the timestamp-based name. Instead, planning will leave Destination.Subdir empty to indicate "did not indicate into existing backup (aka subdir) in the collection".
b) Then, during the real resolution in the execution phase, where we're actually ready to fully resolve the actual incremental against real cloud storage, we take one of two paths:

1. if subdirectory is empty then, we are making a new full backup; we do the endTime.GoTime().Format(DateBasedIntoFolderName) and verify that that path does indeed not already contain a backup (prevent overwrite), or
2. else if subdirectory is not empty, we have been told to backup into an existing backup (append). if it is "latest" we do the lookup and replacement. then we go try to do an inc backup to the backup chain rooted there. If there isn't a backup there, this should fail.

[msbutler]

sounds good. I can file a ga blocked issue.