tree: fix formatting of SHOW BACKUP WITH OPTIONS

[rafiss]

This avoids an ambiguity when formatting the statement.

fixes #110411
Release note: None

[cockroach-teamcity]

This change is 

[annrpom]

lgtm mod a dev gen

[dt]

can you add a testcase that doesn't round-trip in the normalized form before this change but does after?

The example in 110411 of SHOW BACKUP 'placeholder' WITH bucket_count wouldn't round-trip with or without this change, since bucket_count isn't in valid in the show_backup_options rule.

[rafiss]

Ah i see the issue now. The test failed on the 22.2 branch: #110411 (comment). But on that branch, SHOW BACKUP is using opt_with_options in its syntax rules rather than specifying the valid options in the syntax, like how more recent branches do. So I think this might be a more correct fix, but I'm not sure if this breaks a few other things: #110622 Maybe there is a backport from 23.1 that achieves this?

[dt]

I think #110622 shouldn't break too much; none of the SHOW BACKUP options take non-string RHS values, so I don't think there is anything that worked with the 22.2 string/string map that would break if it is switched to a typed rules.

[rafiss]

closing in favor of #110622

[rafiss]

actually, here is a test case that does not round trip (parse, format, parse) on all branches currently:

SHOW BACKUP CONNECTION 'bar' WITH OPTIONS (TIME = '1h')

It is because of this parser lookahead logic:

cockroach/pkg/sql/parser/lexer.go

Lines 245 to 249 in 8316976

	case WITH:
	switch nextToken.id {
	case TIME, ORDINALITY, BUCKET_COUNT:
	lval.id = WITH_LA
	}

For the same reason, the following statement cannot even be parsed in the first place.

SHOW BACKUP CONNECTION 'bar' WITH TIME = '1h'

I think we should go with this PR.

[dt]

That parameter doesn't need to be named TIME. We can use a different token for the duration to run the test.

[dt]

I don't really care all that much about the canonicalization of this SHOW here, since users don't see it, but I'm looking to remove the other WITH OPTIONS () canonicalizations in BACKUP and RESTORE statements, like the one that were added in #107892, since those are user-visible, and caused our job descriptions to no longer match our docs or examples / what the user ran and will be looking for, potentially via regex, in the set of jobs.

So I'm not going to oppose merging this but I also think we could just change TIME to another parameter here to avoid the with-la special case instead of wrapping the whole sub-clause.

[rafiss]

That parameter doesn't need to be named TIME. We can use a different token for the duration to run the test.

I will leave that up to you. It seems like it would be a backwards incompatible syntax change (not just a change in the outputs returned by SHOW). If it's OK to do under our API stability contracts for SHOW, I will let DR drive that effort.

I gave the backport of e6f47a0 a shot, but there were too many conflicts for me to be able to do it myself.

If you don't have any opposition to merging this PR I will go ahead with it.

but I'm looking to remove the other WITH OPTIONS () canonicalizations in BACKUP and RESTORE statements, like the one that were added in #107892, since those are user-visible, and caused our job descriptions to no longer match our docs or examples / what the user ran and will be looking for, potentially via regex, in the set of jobs.

Ack. It doesn't seem like a major priority to me, but you'd know better about this area.

[dt]

If you don't have any opposition to merging this PR I will go ahead with it.

Works for me.

[rafiss]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from e1fbdfe to blathers/backport-release-22.2-110580: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

error creating merge commit from e1fbdfe to blathers/backport-release-23.1-110580: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}