release-23.1: sql: fix formatting of export and changefeed

[annrpom]

Informs: #99185

This commit cherry-picked changes from #107892.

Release note: None
Fixes: #108254 and #108233 (partially)
Release justification: fixes bug with random syntax generation

[blathers-crl]

Thanks for opening a backport.

Please check the backport criteria before merging:

• Patches should only be created for serious issues or test-only changes.
• Patches should not break backwards-compatibility.
• Patches should change as little code as possible.
• Patches should not change on-disk formats or node communication protocols.
• Patches should not add new functionality.
• Patches must not add, edit, or otherwise modify cluster versions; or add version gates.

If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.

• There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
• The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
• New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
• The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

• What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
• Will this work in a cluster of mixed patch versions? Did we test that?
• If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

[cockroach-teamcity]

This change is 

[jayshrivastava]

LGTM! The change itself looks good to me, but can you explain how it fixes the randomized test? Just for my own understanding.

[miretskiy]

I'm also curious... We always supported WITH OPTIONS, but the options was ... well, optional. Is this now recommended to use? I'm not too sure if this raises to the backport level though. Do we need it?

[annrpom]

@jayshrivastava from my understanding, TestRandomSyntaxGeneration has failed before with the same pattern - this is a very specific case due to the fact that a BUCKET_COUNT option that is not wrapped with OPTIONS (...) gets converted to the other WITH BUCKET_COUNT syntax (mentioned #107185 and demonstrated in the lexer:

cockroach/pkg/sql/parser/lexer.go

Lines 245 to 249 in da6a7c4

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

). thus, CREATE CHANGEFEED ... WITH_LA bucket_count; is actually the statement that is being parsed - causing the syntax error that we see:

sql="CREATE CHANGEFEED FOR SURVIVE . ident . FAMILY , ISERROR INTO PLACEHOLDER WITH OPTIONS ( BUCKET_COUNT )", formattedSQL="CREATE CHANGEFEED FOR TABLE survive.ident.family, TABLE \"iserror\" INTO 'placeholder' WITH bucket_count": at or near "with": syntax error

@miretskiy that's a good question - WITH and WITH OPTIONS both should be supported (except in the special case of bucket_count), but (edit: some of) the tests don't really reflect that... I will be adding the WITH tests back in with a new PR

[rafiss]

I'm not too sure if this raises to the backport level though. Do we need it?

Yes, otherwise #108254 will continue failing. In general, it's a good idea to make sure any statement can roundtrip format->parse->format. In some cases, that is actually critical.

[jayshrivastava]

#108254 fails because this SQL

CREATE CHANGEFEED FOR SURVIVE . ident . FAMILY , ISERROR INTO PLACEHOLDER WITH OPTIONS ( BUCKET_COUNT )

Formats to

CREATE CHANGEFEED FOR TABLE survive.ident.family, TABLE \"iserror\" INTO 'placeholder' WITH bucket_count

The latter cannot be parsed because it uses a reserved keyword bucket_count. This change fixes it by making sure we format KV options like this, so it can be parsed.

CREATE CHANGEFEED FOR TABLE survive.ident.family, TABLE \"iserror\" INTO 'placeholder' WITH (bucket_count)

I think the real problem is that both statements (with and without brackets) should be equivalent, but they are not. One can be parsed and the other cannot.

[jayshrivastava]

Actually on second thought, maybe they aren't equivalent. Using brackets allows you to pass in reserved keywords and missing brackets does not let you pass them in.

Nevertheless, I think it's better to format the statement using brackets because we would want formatting to produce the most a parseable output.