cockroachdb / cockroachdb/cockroach

sql/parser: narrow replication source URI in CREATE/ALTER VIRTUAL CLUSTER to string_or_placeholder

Open
#174,325 1 comment 0 reactions 1 assignee Assigned to @andyyang890 View on GitHub
A-disaster-recovery A-sql-syntax C-cleanup O-agent T-disaster-recovery
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Is your feature request related to a problem? Please describe.**

Every other statement that takes a URI/location argument accepts it as `string_or_placeholder` (or its list form): `BACKUP`, `RESTORE`, `IMPORT`, `EXPORT`, changefeed sinks, `CREATE EXTERNAL CONNECTION`, and `CREATE LOGICAL REPLICATION STREAM ... ON string_or_placeholder`.

The physical replication statements are the lone exception; their source URI slot is a general `d_expr`:

```
CREATE virtual_cluster virtual_cluster_spec FROM REPLICATION OF d_expr ON d_expr opt_with_replication_options
ALTER virtual_cluster virtual_cluster_spec START REPLICATION OF d_expr ON d_expr opt_with_replication_options
```

so the grammar admits function calls, parenthesized expressions, subqueries, etc. in the URI position. The value must evaluate to a string at plan time regardless, and a string literal or placeholder is the only form that's actually useful (or documented). The wide production dates to the by-name tenant reference change (272d5f1434e), which needed `d_expr` for the tenant-name slots; the `ON` slot was widened in the same sweep without a stated reason.

Because of this, any code that handles these ASTs has to account for arbitrary expressions in the URI field, while for every sibling statement the parser guarantees literal-or-placeholder.

**Describe the solution you'd like**

Change `ON d_expr` to `ON string_or_placeholder` in the two `create_virtual_cluster_stmt` productions and the `ALTER ... START REPLICATION OF` production (the `REPLICATION OF d_expr` name slots stay as-is; they're what by-name promotion needs). Verify the regenerated parser has no new grammar conflicts.

This removes accepted syntax (e.g. `ON concat(...)` currently parses), so it needs a release note. Nothing in-repo uses a non-literal expression in this position.

**Describe alternatives you've considered**

Leave the grammar as-is and keep special-casing this statement in code that consumes the AST.

Jira issue: CRDB-67628

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.