sql: differentiate unique constraints from unique indexes

[ajwerner]

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

Relates to https://github.com/cockroachdb/cockroach/issues/65475

Cockroach currently only does a haphazard job distinguishing unique indexes from unique constraints. In postgres these are two distinct objects; a unique constraint is implemented by a unique index but a unique index is not a unique constraint (see https://github.com/cockroachdb/cockroach/issues/65885). Furthermore, we had code to differentiate these two things, but it was not used properly in CREATE TABLE or in IMPORT. We never really noticed because our display logic for SHOW CREATE TABLE etc hid any fact that they might differ.

The rub here is that unique constraints offer fewer features than unique indexes. We syntactically supported some of these features already. To improve postgres compatibility, we should remove some of this syntax support from unique constraints (meaning people may need to insert the keyword INDEX in some places.

This is likely to be a backwards incompatible change.

Open questions

• Should we allow columns storage parameters like ASC/DESC or NULLS FIRST in the elements of the unique constraint. Postgres certainly does not. However, we do in PRIMARY KEY constraints where postgres also does not and we don't have a great hope of eliminating that. Maybe that implies that we should support some, or all of these indexing features.

Epic: CRDB-10239

Jira issue: CRDB-7802

[ajwerner]

Upon further discussion, we probably ought to preserve the old syntax in some form or another for at least one release. An idea is to support it behind a feature flag, warn about it when used, and internally convert such constraints to UNIQUE INDEX.

[ajwerner]

We still don't differentiate the index and the constraint properly but we now treat them all as indexes. I think at this point the fix ought to be to allow dropping the indexes, at least if they aren't weird like partial or have descending orders via the alter table ... drop constraint. That would address #42840.

[rafiss]

This also came up in https://github.com/cockroachdb/cockroach/issues/91902

I tried something quick:

diff --git a/pkg/sql/sem/tree/create.go b/pkg/sql/sem/tree/create.go
index 1ab5dab813..b1ce867d5a 100644
--- a/pkg/sql/sem/tree/create.go
+++ b/pkg/sql/sem/tree/create.go
@@ -1069,7 +1069,7 @@ func (node *UniqueConstraintTableDef) SetIfNotExists() {

 // Format implements the NodeFormatter interface.
 func (node *UniqueConstraintTableDef) Format(ctx *FmtCtx) {
-   if node.Name != "" {
+   if node.Name != "" && node.PrimaryKey {
        ctx.WriteString("CONSTRAINT ")
        if node.IfNotExists {
            ctx.WriteString("IF NOT EXISTS ")
@@ -1081,9 +1081,11 @@ func (node *UniqueConstraintTableDef) Format(ctx *FmtCtx) {
        ctx.WriteString("PRIMARY KEY ")
    } else {
        ctx.WriteString("UNIQUE ")
-   }
-   if node.WithoutIndex {
-       ctx.WriteString("WITHOUT INDEX ")
+       if node.WithoutIndex {
+           ctx.WriteString("WITHOUT INDEX ")
+       } else {
+           ctx.WriteString("INDEX ")
+       }
    }
    ctx.WriteByte('(')
    ctx.FormatNode(&node.Columns)
diff --git a/pkg/sql/sem/tree/pretty.go b/pkg/sql/sem/tree/pretty.go
index b56c73517a..7bb0699d60 100644
--- a/pkg/sql/sem/tree/pretty.go
+++ b/pkg/sql/sem/tree/pretty.go
@@ -1772,10 +1772,12 @@ func (node *UniqueConstraintTableDef) doc(p *PrettyCfg) pretty.Doc {
        title = pretty.Keyword("UNIQUE")
        if node.WithoutIndex {
            title = pretty.ConcatSpace(title, pretty.Keyword("WITHOUT INDEX"))
+       } else {
+           title = pretty.ConcatSpace(title, pretty.Keyword("INDEX"))
        }
    }
    title = pretty.ConcatSpace(title, p.bracket("(", p.Doc(&node.Columns), ")"))
-   if node.Name != "" {
+   if node.Name != "" && node.PrimaryKey {
        clauses = append(clauses, title)
        title = pretty.ConcatSpace(pretty.Keyword("CONSTRAINT"), p.Doc(&node.Name))
    }

but then it led to a different round-tripping failure:

--- FAIL: TestParseDatadriven (1.92s)
    --- FAIL: TestParseDatadriven/alter_table (0.00s)
        parse_test.go:49: at or near "index": syntax error: ALTER TABLE a ADD COLUMN b INT8, ADD UNIQUE INDEX (a)
FAIL

[ajwerner]

I don't think I understand your patch. We should not be able to ADD UNIQUE INDEX.

[rafiss]

Agreed. I'm just pointing out that the patch I attempted to fix round-tripping CREATE TABLE statements with unique indexes (as reported in https://github.com/cockroachdb/cockroach/issues/91902) is not sufficient since it breaks ALTER TABLE round-tripping. I don't see an easy way to fix one without breaking the other.

[ajwerner]

I think that if we were to differentiate them, it'd look like:

create table foo(i int); create unique index on foo(i);

would be

  CREATE TABLE public.foo (
      i INT8 NULL,
      rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
      CONSTRAINT foo_pkey PRIMARY KEY (rowid ASC),
      UNIQUE INDEX foo_i_idx (i ASC)
  )

and

create table foo (i int); alter table foo add unique(i);

would be

  CREATE TABLE public.foo (
      i INT8 NULL,
      rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
      CONSTRAINT foo_pkey PRIMARY KEY (rowid ASC),
      CONSTRAINT foo_i_key UNIQUE (i)
  )