sql: name PRIMARY KEY constraints (table_name)_pkey by default #70604

otan · 2021-09-22T23:56:01Z

resolves #71087

See individual commits for details.
Split up to make reviewing "easier"; more complex changes are separate commits.

cockroach-teamcity · 2021-09-22T23:56:13Z

This change is

dt · 2021-09-23T16:47:33Z

Nice! Just wanted to check when you were thinking of merging this? Would it be onerous to wait until we tag a 21.2 RC? We (bulk at least) are trying to avoid unnecessary refactor churn in our packages until the RC is ready, just in case we discover a last-minute severe and release-blocking issue that requires an urgent fix, so that that fix doesn't risk being delayed by unclean backports.

otan · 2021-09-23T20:15:16Z

this can wait (but oh god the pain of rebasing)

postamar

This all seems fine to me, the only comment I have pertains to maintaining the legacy naming of the primary index of the system tables.

Caveat: I haven't looked at the slew of changes in the expected test output definitions but then I doubt anyone ever will.

As far as I'm concerned this is a welcome change, I'm just curious as to what motivates it, as I see no linked issue anywhere. Do I guess correctly that naming them all "primary" was causing problems when importing mysql dumps?

postamar · 2021-10-04T17:15:15Z

pkg/sql/catalog/tabledesc/structured.go

-	// PrimaryKeyIndexName is the name of the index for the primary key.
-	PrimaryKeyIndexName = "primary"
+	// LegacyPrimaryKeyIndexName is the pre 22.1 default PRIMARY KEY index name.
+	LegacyPrimaryKeyIndexName = "primary"


Why is it necessary to keep the old LegacyPrimaryKeyIndexName name in the system schema? I'd be surprised if the primary index of any system table is ever referred to by name anywhere. In any case, it seems like you could quite easily add a tenant migration which runs a bunch of ALTER INDEX IF EXISTS system.???@primary RENAME TO ???_pkey statements. Perhaps I'm missing something.

I'd hate to run that migration! Two issues top of my head:

Easy to miss a table

In particular, the hardcoded descriptors in code and sql schema have to be kept in sync as well (from what I can tell). That will be challenging in your example.

It's not that bad, I don't think. There's no need to version-gate the system table schemas in systemschema as these are used for bootstrapping new clusters, changes in there are supposed to be backward compatible. In this case changing the name isn't backward compatible but if we never refer to the primary index of a system table by name anywhere then that doesn't matter and it's essentially a no-op. I suspect that we don't, ever. The migration would be in case if ever we chose to in the future.

Just to be clear I'm OK with just leaving their names be. I wanted to understand if there was a deeper motivation that I'd missed.

As for making sure not to miss tables, querying SHOW TABLES FROM system and iterating over the results should take care of that.

rafiss · 2021-10-04T17:21:42Z

This was motivated by trying to get the https://www.prisma.io/ test suite passing against CRDB

Soon, we will no longer have `@primary` as the PRIMARY KEY lookup path. To prepare for the eventuality TPC-C may be used on an older or newer version of cockroach, remove the `@primary` index hint. Release note: None

Note that legacy system tables retain their legacy PRIMARY name. Release note (sql change): Newly created tables now have `<table_name>_pkey` by default as their index/constraint name.

Broken up for ease of reviewing; this does a little bit more than the mechanical work. Release note: None

To match PostgreSQL, we should rename the PRIMARY KEY index name when importing MySQL dumps. Release note: None

… name Separated out from logic changes (previous commit), can squash before merging. Release note: None

otan · 2021-10-14T03:40:18Z

bors r=postamar

craig · 2021-10-14T04:42:47Z

Build failed:

GitHub CI (Cockroach)

otan · 2021-10-14T05:00:46Z

opt flake

bors r=postamar

craig · 2021-10-14T05:13:53Z

Build failed:

GitHub CI (Cockroach)

otan · 2021-10-14T06:15:01Z

inf flake :\

bors r=postamar

craig · 2021-10-14T07:03:06Z

Build succeeded:

GitHub CI (Cockroach)

mgartner · 2021-10-21T00:30:44Z

We should update the optimizer test catalog to match. I'll put up a PR.

This commit updates the test catalog to name primary keys in the form `<table_name>_pkey`. This makes primary index names consistent with the real catalog and Postgres. See cockroachdb#70604. Release note: None

71818: opt: make index names in test catalog consistent with real catalog r=mgartner a=mgartner #### opt: name primary indexes <table_name>_pkey in test catalog This commit updates the test catalog to name primary keys in the form `<table_name>_pkey`. This makes primary index names consistent with the real catalog and Postgres. See #70604. Release note: None #### opt: make secondary index names in test catalog consistent with real catalog This commit updates the test catalog to name secondary keys consistently with the real catalog and Postgres. Release note: None Co-authored-by: Marcus Gartner <[email protected]>

RaduBerinde · 2021-11-08T19:30:23Z

The rename can have important consequences: a user who is using a @primary index hint will see their queries fail once they recreate the schema against the new version. Users will consider this a "breaking change".

I propose that we add some code to continue to support @primary to refer to the primary key (as a special case). I don't see the downside to accepting it (rather than erroring out).

otan · 2021-11-08T19:32:25Z

if they re-use the schema that was dumped, it will have CONSTRAINT "primary" PRIMARY KEY .... no?
i feel like we'd have to differentiate @primary and @"primary" otherwise as someone may name their non-PK primary

RaduBerinde · 2021-11-08T19:43:20Z

if they re-use the schema that was dumped, it will have CONSTRAINT "primary" PRIMARY KEY .... no?

We have seen customers recreate their schema anew using their own tools/scripts. Deploying a fresh instance of their app on a test cluster is their first step to validate that their app still works with a new version.

i feel like we'd have to differentiate @primary and @"primary" otherwise as someone may name their non-PK primary

My proposal is to match primary to the primary key if it matches nothing else. Only cases that would error previously would be affected.

otan · 2021-11-08T19:51:33Z

that will probs work for PKs, do we care about what happens to FKs?
cc @rafiss in case you have an opinion too

RaduBerinde · 2021-11-08T19:58:26Z

FKs don't matter as much, they can't be part of queries as hints.

rafiss · 2021-11-12T17:15:45Z

I'd be fine to add that fallback name resolution logic. So we aren't going to be changing anything about how it shows up in pg_catalog introspection tables, right?

Let's get it tracked in a new github issue

RaduBerinde · 2021-11-12T21:32:04Z

@otan is all action, it's already done (#72534).

This change also adjusts how we respond to the case in which a table has no primary key. The previous SQL query was looking for a hardcoded "primary" index, but that behavior has changed in v22.1 (cockroachdb/cockroach#70604).

otan force-pushed the primary_key_rename branch 7 times, most recently from fe43fff to a99e52c Compare September 23, 2021 12:15

otan requested review from rafiss and a team September 23, 2021 12:16

otan marked this pull request as ready for review September 23, 2021 12:16

otan requested a review from a team September 23, 2021 12:16

otan requested review from a team as code owners September 23, 2021 12:16

otan requested a review from a team September 23, 2021 12:16

otan requested a review from a team as a code owner September 23, 2021 12:16

otan requested review from shermanCRL and removed request for a team September 23, 2021 12:16

shermanCRL requested review from dt and removed request for shermanCRL September 23, 2021 13:54

otan force-pushed the primary_key_rename branch from a99e52c to 4d11190 Compare September 24, 2021 07:52

postamar approved these changes Oct 4, 2021

View reviewed changes

otan added 4 commits October 14, 2021 12:00

tpcc: remove @primary index hint for check3325

9dd5046

Soon, we will no longer have `@primary` as the PRIMARY KEY lookup path. To prepare for the eventuality TPC-C may be used on an older or newer version of cockroach, remove the `@primary` index hint. Release note: None

sql: name PRIMARY KEY constraints (table_name)_pkey by default

833a76f

Note that legacy system tables retain their legacy PRIMARY name. Release note (sql change): Newly created tables now have `<table_name>_pkey` by default as their index/constraint name.

cdctest: fix broken tests after PRIMARY KEY constraint rename

e22e954

Broken up for ease of reviewing; this does a little bit more than the mechanical work. Release note: None

importccl: change MySQL PRIMARY KEY index name on import

4f3ed51

To match PostgreSQL, we should rename the PRIMARY KEY index name when importing MySQL dumps. Release note: None

otan force-pushed the primary_key_rename branch from 4d11190 to 7bd3f75 Compare October 14, 2021 01:19

*: fix mechanical tests for changing PRIMARY INDEX default constraint…

514dfd6

… name Separated out from logic changes (previous commit), can squash before merging. Release note: None

otan force-pushed the primary_key_rename branch from 7bd3f75 to 514dfd6 Compare October 14, 2021 02:30

craig bot merged commit 96bb0bc into cockroachdb:master Oct 14, 2021

cockroach-teamcity mentioned this pull request Oct 14, 2021

sql: name PRIMARY KEY constraints (table_name)_pkey by default cockroachdb/docs#11963

Open

postamar mentioned this pull request Oct 14, 2021

sql: Add IF NOT EXISTS modifier to ALTER TABLE ... ADD CONSTRAINT #71257

Merged

mgartner mentioned this pull request Oct 21, 2021

opt: make index names in test catalog consistent with real catalog #71818

Merged

oninugarte mentioned this pull request Jun 5, 2023

Cockroach database error: constraint "primary" does not exist ory/hydra#3535

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: name PRIMARY KEY constraints (table_name)_pkey by default #70604

sql: name PRIMARY KEY constraints (table_name)_pkey by default #70604

otan commented Sep 22, 2021 •

edited by rafiss

Loading

cockroach-teamcity commented Sep 22, 2021

dt commented Sep 23, 2021

otan commented Sep 23, 2021

postamar left a comment

postamar Oct 4, 2021

otan Oct 4, 2021 •

edited

Loading

postamar Oct 5, 2021

rafiss commented Oct 4, 2021

otan commented Oct 14, 2021

craig bot commented Oct 14, 2021

otan commented Oct 14, 2021

craig bot commented Oct 14, 2021

otan commented Oct 14, 2021

craig bot commented Oct 14, 2021

mgartner commented Oct 21, 2021

RaduBerinde commented Nov 8, 2021

otan commented Nov 8, 2021

RaduBerinde commented Nov 8, 2021

otan commented Nov 8, 2021

RaduBerinde commented Nov 8, 2021

rafiss commented Nov 12, 2021

RaduBerinde commented Nov 12, 2021

sql: name PRIMARY KEY constraints (table_name)_pkey by default #70604

sql: name PRIMARY KEY constraints (table_name)_pkey by default #70604

Conversation

otan commented Sep 22, 2021 • edited by rafiss Loading

cockroach-teamcity commented Sep 22, 2021

dt commented Sep 23, 2021

otan commented Sep 23, 2021

postamar left a comment

Choose a reason for hiding this comment

postamar Oct 4, 2021

Choose a reason for hiding this comment

otan Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

postamar Oct 5, 2021

Choose a reason for hiding this comment

rafiss commented Oct 4, 2021

otan commented Oct 14, 2021

craig bot commented Oct 14, 2021

otan commented Oct 14, 2021

craig bot commented Oct 14, 2021

otan commented Oct 14, 2021

craig bot commented Oct 14, 2021

mgartner commented Oct 21, 2021

RaduBerinde commented Nov 8, 2021

otan commented Nov 8, 2021

RaduBerinde commented Nov 8, 2021

otan commented Nov 8, 2021

RaduBerinde commented Nov 8, 2021

rafiss commented Nov 12, 2021

RaduBerinde commented Nov 12, 2021

otan commented Sep 22, 2021 •

edited by rafiss

Loading

otan Oct 4, 2021 •

edited

Loading