workload/tpcc: duplicate FK detection during initialization broken

[nvanbenschoten]

The following TPCC roachtests have been failing for the past 3 days:

• roachtest: tpccbench/nodes=6/cpu=16/multi-az failed [#37866] #37250
• roachtest: tpccbench/nodes=3/cpu=16 failed #37488
• roachtest: tpccbench/nodes=9/cpu=4/chaos/partition failed #37519
• roachtest: tpccbench/nodes=3/cpu=4 failed [skipped] #36697
• roachtest: tpcc/headroom/n4cpu16 failed #37163
• roachtest: tpcc-nowait/nodes=3/w=1 failed #37308
• roachtest: sqlsmith failed #37549

The tests are now failing with the following error while loading their fixtures:

Error: restoring fixture: PostLoad hook: pq: column "ol_d_id" cannot be used by multiple foreign key constraints

This is almost certainly related to #37433, which was merged 4 days ago and touched some of the related logic in create_table.go. That change seems to be interacting poorly with duplicate foreign key detection here:

cockroach/pkg/workload/tpcc/tpcc.go

Lines 277 to 280 in 58c567a

	// If the statement failed because the fk already exists,
	// ignore it. Return the error for any other reason.
	const duplFKErr = "columns cannot be used by multiple foreign key constraints"
	if !strings.Contains(err.Error(), duplFKErr) {

The detection in workload/tpcc is regrettably using string matching instead of checking the pg error code. We should change it to search for CodeInvalidForeignKeyError instead.

@lucy-zhang The fix for this in tpcc.go is very straightforward and I'm happy to make that change, but before doing so, I want to make sure this isn't detecting an actual issue or unexpected change in behavior. A quick look over 3c49a5f doesn't show any deliberate reason why the error string returned here would have changed. Was this change in behavior expected?

NOTE: Most of these issues had previous failures, so once this problem is fixed, we should go back and minimize the comments about this (those in the past 3 days) instead of closing the issues entirely.

[thoszhang]

This is happening because #37433 (somewhat unintentionally) fixed a bug in foreign key validation. The bug was that we would previously allow things like

CREATE TABLE t (a INT, b INT, c INT, PRIMARY KEY (a, b), INDEX (c, b))
ALTER TABLE t ADD FOREIGN KEY (a, b) REFERENCES ...
ALTER TABLE t ADD FOREIGN KEY (c, b) REFERENCES ...

where one FK is on the primary key and the other is on some other index. We weren't including the PK when validating uniqueness of columns in FK constraints across a table, so b gets used twice.

The column that's being used twice here is ol_d_id (in the code path where we load the old incorrect tpcc fixtures with the wrong index). Prior to #37433, we would create the second FK that uses that column, which could have led to undefined behavior, since we assume elsewhere that each column has at most one FK constraint on it.

It's not clear to me how to fix these tests. My understanding is that the tpcc roachtests rely on the old incorrect test fixtures with both FK constraints on ol_d_id, but now it's forbidden to create the second FK constraint at all. So we'd either have to remove this FK from the tests, or re-generate the fixtures. @nvanbenschoten thoughts?

[thoszhang]

Also, to clarify, we weren't actually hitting the hardcoded columns cannot be used by multiple foreign key constraints error before. I'm not sure if we ever were, or what the expected behavior actually was.

[nvanbenschoten]

Got it, thanks for looking into this @lucy-zhang. As we discussed offline, ol_d_id should never have been allowed as the source of two different foreign keys, which would have avoided this complication. Now that we're correctly detecting the issue, we're running into problems with the invalid fixture.

So it sounds like the solution is to remove compatibility with fixtures that contain this incorrect index and foreign key. Conveniently, I already needed to regenerate fixtures for #36854, so I think it's time to just go ahead and do that. Doing so and merging #36854 will resolve this issue.

[nvanbenschoten]

Closing now that #36854 merged. There's still some fallout from all of this, but it should be addressed by #37701 + a re-regeneration of the fixtures.