sql,tree: differentiate UNIQUE and UNIQUE INDEX in CREATE TABLE

[ajwerner]

While we treated the cases of ALTER TABLE ... ADD CONSTRAINT UNIQUE from
CREATE UNIQUE INDEX we didn't treat the two clauses differently in the
context of CREATE TABLE. While functionally they are largely identical concepts,
they have different semantic meanings (you can't use drop constraint on the index
or drop index on the constraint). We also didn't differentiate these concepts for display.

The root of this problem is twofold:

1. Unique indexes and unique constraints always displayed indentically in SHOW CREATE TABLE even though some other catalog fields treated them distinctly.
2. UNIQUE and UNIQUE INDEX clauses were treated identically in CREATE TABLE statements.

root@localhost:26257/test> CREATE TABLE t (i INT PRIMARY KEY, j INT, UNIQUE (j), UNIQUE INDEX (j));
CREATE TABLE

Before:

root@localhost:26257/test> SHOW CREATE TABLE t;
  table_name |               create_statement
-------------+------------------------------------------------
  t          | CREATE TABLE public.t (
             |     i INT8 NOT NULL,
             |     j INT8 NULL,
             |     CONSTRAINT "primary" PRIMARY KEY (i ASC),
             |     UNIQUE INDEX t_j_key (j ASC),
             |     UNIQUE INDEX t_j_key1 (j ASC),
             |     FAMILY "primary" (i, j)
             | )

After:

[email protected]:55769/movr> SHOW CREATE TABLE t;
  table_name |               create_statement
-------------+------------------------------------------------
  t          | CREATE TABLE public.t (
             |     i INT8 NOT NULL,
             |     j INT8 NULL,
             |     CONSTRAINT "primary" PRIMARY KEY (i ASC),
             |     UNIQUE INDEX t_j_key1 (j ASC),
             |     FAMILY "primary" (i, j),
             |     CONSTRAINT t_j_key UNIQUE (j)
             | )

Release note (bug fix): UNIQUE INDEX clauses in a CREATE TABLE statement
are no longer treated as CONSTRAINT UNIQUE clauses.

Release note (bug fix): UNIQUE constraints were displayed as UNIQUE INDEX
entries in SHOW CREATE TABLE.

[cockroach-teamcity]

This change is 

[RaduBerinde]

Nice cleanup!

Reviewed 1 of 2 files at r1, 21 of 21 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @mgartner)

---

pkg/sql/show_create_clauses.go, line 604 at r1 (raw file):

		}
	}
	for _, idx := range desc.AllIndexes() {

[nit] Maybe we want to show them before UNIQUE WITHOUT INDEX (since they are more "vanilla")? Also, I'd add a comment along the lines of // Show unique constraints that are implemented by implicitly created unique indexes.

---

pkg/sql/logictest/testdata/logic_test/show_create_all_tables, line 59 at r2 (raw file):

);
CREATE TABLE public.full_test (
                                                                                                                                                                     x INT8 NULL,

What's with the extra blank lines? Maybe a bug in -rewrite?

---

pkg/sql/sem/tree/create.go, line 885 at r2 (raw file):

	PrimaryKey    bool
	WithoutIndex  bool
	ExplicitIndex bool

[nit] Maybe add a comment for the field (mentioning CONSTRAINT vs UNIQUE INDEX syntax). And that at most one of PrimaryKey, WithoutIndex, ExplicitIndex can be true.

[rytaft]

Nice change -- this will get us closer to PG compatibility, since Postgres distinguishes between unique constraints and unique indexes.

This issue seems relevant, since it's related to the fact that we weren't distinguishing between unique constraints and unique indexes before: #42840. (not that you need to fix it with this PR, just wanted to surface it)

Reviewed 2 of 2 files at r1, 21 of 21 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @RaduBerinde)

---

pkg/ccl/logictestccl/testdata/logic_test/partitioning_index, line 50 at r2 (raw file):

     CONSTRAINT "primary" PRIMARY KEY (a ASC),
     FAMILY "primary" (a, b),
     CONSTRAINT ok2_b_key UNIQUE (b)

the partition by list isn't shown now

---

pkg/sql/show_create_clauses.go, line 604 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] Maybe we want to show them before UNIQUE WITHOUT INDEX (since they are more "vanilla")? Also, I'd add a comment along the lines of // Show unique constraints that are implemented by implicitly created unique indexes.

I'd also show them before the families

---

pkg/sql/logictest/testdata/logic_test/create_table, line 279 at r2 (raw file):

            INVERTED INDEX like_table_j_idx (j),
            FAMILY "primary" (a, b, c, h, j, k),
            CONSTRAINT foo UNIQUE (b, c)

now the DESC marker is gone from b

[ajwerner]

Thanks for the early looks! There's a good bit more I want to do here. I'll mark it as Ready for review when it's in good shape.

1. Make the behavior consistent for partially added things.
   • See internal thread
2. Change the parser to disallow a bunch of the index parameters on unique constraints
   • We'll need to allow WHERE clauses on UNIQUE WITHOUT INDEX because we can't take
     that feature away. It's fine because it's a cockroach extension.
   • Other modifiers will be removed from UNIQUE constraints for the most part
3. (follows from 2) Implement a migration and repair logic to turn all of the unique constraints which use index features into unique indexes explicitly. This includes:
   • non-standard column directions,
   • partition clauses
   • interleave clauses
   • storing (technically in postgres you can have an INCLUDE CLAUSE but we don't support that syntax). When we add it, I'm fine supporting it in both cases.
4. Fix some other bugs revealed along the way.
   • For example, we disallow NOT VALID on UNIQUE in a CREATE TABLE but we ignore it in when adding a constraint via ALTER TABLE.

     cockroach/pkg/sql/parser/sql.y

     Line 6206 in c1f7534

     return purposelyUnimplemented(sqllex, "table constraint", typ + " constraints cannot be marked NOT VALID")

   • There's some other issues with adding multiple constraints in a single ALTER TABLE I'm chewing through.

[mgartner]

@ajwerner Do you think we should try to get this into 21.2? I ask not to pressure you, but because there's some related issues that have come up which would be solved by this. There's other temporary solutions that I can work on if we think this PR is too big of a lift for 21.2. I could also take over where you left off.

• sql: predicate ignored when adding unique constraint #67234
• This doesn't really need to be solved, but would be affected: https://github.com/cockroachdb/cockroach/pull/67197/files#diff-931c3f5a978f27bc78dffa628d45152d0c8c6f8be4328c5c5b2e760c28f2b32aR132-R136

[ajwerner]

@ajwerner Do you think we should try to get this into 21.2? I ask not to pressure you, but because there's some related issues that have come up which would be solved by this. There's other temporary solutions that I can work on if we think this PR is too big of a lift for 21.2.

Thanks for the ping and I'm very sorry I let this sit for so long. Increasingly I'm afraid of breaking compatibility with ourselves, even in cases where we handled it poorly in the past. I'd like @vy-ton to get involved here as we do some decision making. Maybe the right approach here is to treat all of the uses of index features in a constraint clause as valid but as syntactically implying an INDEX as opposed to a constraint. More concretely, revert the parser changes but retain the other changes to convert those constraints to indexes. Then during CREATE TABLE and ALTER TABLE ... ADD CONSTRAINT, actually construct an ExplicitlyCreated index, perhaps with a notice. That would give us some latitude to disallow it one day but would be less disruptive if anybody is depending on it.

I could also take over where you left off.

As far as getting this done, is this something you feel like you have resources to tackle?

[mgartner]

As far as getting this done, is this something you feel like you have resources to tackle?

Probably not before the stability period. I'll look for quick workarounds for the minor issues I linked to for now, and hopefully we can make some progress on this in 22.1.