sql: set zone configs before backfill on ALTER TABLE LOCALITY/PRIMARY KEY

[otan]

Ensure that the zone configurations are correctly set before the
backfill runs, so data is backfilled to the correct location upfront for
REGIONAL BY ROW tables. Note this bug also exists on existing ALTER
PRIMARY KEY statements.

Release justification: bug fixes and low-risk updates to new
functionality

Release note (bug fix): Fix bug where zone configurations on indexes
were not copied before the backfill of an ALTER PRIMARY KEY. They used
to be copied afterwards instead.

[cockroach-teamcity]

This change is 

[otan]

what a false claim ;~;

[ajstorm]

Reviewed 2 of 3 files at r1, 4 of 5 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

---

pkg/ccl/multiregionccl/regional_by_row_test.go, line 144 at r3 (raw file):

	range_max_bytes = 536870912,
	gc.ttlseconds = 90000,
	num_replicas = 3,

I might be missing something, but shouldn't this have a global_reads value set? Or are we dropping the zone config first, and then apply the new one at the end?

---

pkg/ccl/multiregionccl/regional_by_row_test.go, line 187 at r3 (raw file):

	const showCreateTableStringSQL = `SELECT create_statement FROM [SHOW CREATE TABLE t.test]`
	const zoneConfigureSQLStatements = `
		SELECT coalesce(string_agg(raw_config_sql, ';' ORDER BY raw_config_sql), 'NULL')

Why not SELECT zone_config, index_name, partition_name FROM [SHOW PARTITIONS FROM TABLE test];

---

pkg/sql/schema_changer.go, line 2428 at r3 (raw file):

) error {
	if pkSwap := mutation.GetPrimaryKeySwap(); pkSwap != nil {
		// We might have to update some zone configs for indexes that are

Nit: Does this comment belong in with the caller?

---

pkg/sql/schema_changer.go, line 2455 at r3 (raw file):

				case *descpb.TableDescriptor_LocalityConfig_Global_,
					*descpb.TableDescriptor_LocalityConfig_RegionalByTable_:
					// Just the table re-writing the locality config change will suffice.

Nit: wording - "Nothing to do here. The table re-writing the locality config is all that's required"?

---

pkg/sql/schema_changer.go, line 2492 at r3 (raw file):

			)
		}
		// For the normal case, copy the zone configs over.

Nit: Can we explain what we mean by "normal" here?

---

pkg/sql/sqltestutils/alter_primary_key.go, line 31 at r3 (raw file):

// AlterPrimaryKeyCorrectZoneConfigIntermediateZoneConfig is an expected
// intermediate zone configuration in the AlterPrimaryKeyCorrectZoneConfigTestCase.
type AlterPrimaryKeyCorrectZoneConfigIntermediateZoneConfig struct {

Nit: these names are very long. Can we not put this struct in alter_primary_key_test.go, give it a shorter name, and stop using it in regional_by_row_test.go (by moving that test into alter_primary_key_test.go)?

---

pkg/sql/sqltestutils/alter_primary_key.go, line 54 at r3 (raw file):

	defer SetTestJobsAdoptInterval()()

	var chunkSize int64 = 100

Nit: ƒa:= for these two?

---

pkg/sql/sqltestutils/alter_primary_key.go, line 103 at r3 (raw file):

			// Disable strict GC TTL enforcement because we're going to shove a zero-value
			// TTL into the system with AddImmediateGCZoneConfig.
			defer DisableGCTTLStrictEnforcement(t, sqlDB)()

Nit: where is the call to AddImmediateGCZoneConfig?

---

pkg/sql/sqltestutils/alter_primary_key.go, line 107 at r3 (raw file):

			if _, err := sqlDB.Exec(fmt.Sprintf(`
%s;
USE t;

Nit: are we assuming that the database here is named t? If so, is this a reasonable assumption?

---

pkg/sql/sqltestutils/alter_primary_key.go, line 110 at r3 (raw file):

%s
`, createDBStatement, tc.SetupQuery)); err != nil {
				t.Fatal(err)

Nit: Why Fatal vs. require.NoError?

---

pkg/sql/sqltestutils/alter_primary_key.go, line 113 at r3 (raw file):

			}

			if err := BulkInsertIntoTable(sqlDB, maxValue); err != nil {

Nit: Can you add a comment here as to why you're performing this bulk insert?

[otan]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @arulajmani)

---

pkg/ccl/multiregionccl/regional_by_row_test.go, line 144 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

I might be missing something, but shouldn't this have a global_reads value set? Or are we dropping the zone config first, and then apply the new one at the end?

i believe we should only apply the table level zone configs once done() is called.
however, the new indexes (including the new PKs) should be done immediately.

---

pkg/ccl/multiregionccl/regional_by_row_test.go, line 187 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Why not SELECT zone_config, index_name, partition_name FROM [SHOW PARTITIONS FROM TABLE test];

because comparing individual rows is a lot more effort. this should give the exact same set of results, as partition_name and index_name is encoded within the raw_config_sql.

---

pkg/sql/sqltestutils/alter_primary_key.go, line 31 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: these names are very long. Can we not put this struct in alter_primary_key_test.go, give it a shorter name, and stop using it in regional_by_row_test.go (by moving that test into alter_primary_key_test.go)?

this does not really help with the fact that it would need to be in two separate packages.
this can only be tested in ALTER PRIMARY KEY where there are partitions. i don't think the test itself belongs in ccl/multiregionccl

---

pkg/sql/sqltestutils/alter_primary_key.go, line 54 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: ƒa:= for these two?

wa

---

pkg/sql/sqltestutils/alter_primary_key.go, line 103 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: where is the call to AddImmediateGCZoneConfig?

hmm, deleting it. relic of something else.

---

pkg/sql/sqltestutils/alter_primary_key.go, line 107 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: are we assuming that the database here is named t? If so, is this a reasonable assumption?

yes, all the helpers here use that database.

[ajstorm]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

---

pkg/ccl/multiregionccl/regional_by_row_test.go, line 144 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

i believe we should only apply the table level zone configs once done() is called.
however, the new indexes (including the new PKs) should be done immediately.

Yeah, realized that further on in my review. All good

---

pkg/sql/sqltestutils/alter_primary_key.go, line 31 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

this does not really help with the fact that it would need to be in two separate packages.
this can only be tested in ALTER PRIMARY KEY where there are partitions. i don't think the test itself belongs in ccl/multiregionccl

Hmm. Is this what the @arulajmani sadness feels like?

---

pkg/sql/sqltestutils/alter_primary_key.go, line 54 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

wa

Wow, that didn't print out as intended. My question was, should we be using := for these two?

---

pkg/sql/sqltestutils/alter_primary_key.go, line 107 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

yes, all the helpers here use that database.

Seems reasonable for now, but makes this test a bit fragile. Would be good if we passed in the database name so that we could USE the specified database.

[arulajmani]

Nice catch! 💯

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @otan)

---

pkg/ccl/partitionccl/alter_primary_key_test.go, line 34 at r5 (raw file):

					ShowConfigStatement: `SHOW ZONE CONFIGURATION FOR INDEX t.test@v_idx_rewrite_for_primary_key_change`,
					ExpectedTarget:      `INDEX t.public.test@v_idx_rewrite_for_primary_key_change`,
					ExpectedSQL: `ALTER INDEX t.public.test@v_idx_rewrite_for_primary_key_change CONFIGURE ZONE USING

nit: structuring this whole thing in the form of SHOW ZONE CONFIGURATION FROM <old_idx_name> needs to match SHOW ZONE CONFIGURATION FROM <rewrite_idx_name> might make this test more readable.

---

pkg/sql/schema_changer.go, line 1138 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

what a false claim ;~;

Good find!

---

pkg/sql/schema_changer.go, line 2497 at r5 (raw file):

		// For the plain ALTER PRIMARY KEY case, copy the zone configs over
		// for any new indexes.
		return maybeUpdateZoneConfigsForPKChange(

Can we do this only if isDone is false? That way we dont have to re-write the same zone configurations after the change has been finalized for regular PK changes.

---

pkg/sql/sqltestutils/alter_primary_key.go, line 31 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Hmm. Is this what the @arulajmani sadness feels like?

Any reason not to put this file in CCL (say pkg/ccl/testutils/<file_name>.go)? Looks like both its callers are from CCL.

[otan]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, and @otan)

---

pkg/ccl/partitionccl/alter_primary_key_test.go, line 34 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: structuring this whole thing in the form of SHOW ZONE CONFIGURATION FROM <old_idx_name> needs to match SHOW ZONE CONFIGURATION FROM <rewrite_idx_name> might make this test more readable.

hmm, the zone configs have different names so doing that regexp makes it equally annoying to read as a gotcha. i think i'll keep this one :P

---

pkg/sql/schema_changer.go, line 2497 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Can we do this only if isDone is false? That way we dont have to re-write the same zone configurations after the change has been finalized for regular PK changes.

hmm, i think so, but to keep it in line with the original comment of the false claim above, i think it's safer to overwrite. i don't think there's harm in re-writing the same zone config is there?

---

pkg/sql/sqltestutils/alter_primary_key.go, line 31 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Any reason not to put this file in CCL (say pkg/ccl/testutils/<file_name>.go)? Looks like both its callers are from CCL.

yeah i'll do that.

[arulajmani]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, and @otan)

---

pkg/sql/schema_changer.go, line 2497 at r5 (raw file):

i don't think there's harm in re-writing the same zone config is there?

Naa, I don't think it's that big a deal considering we're writing the same thing again. Would you mind capturing this potential overwrite and that we don't care in a comment here?

[otan]

bors r=arulajmani,ajstorm

[craig]

Build succeeded:

• GitHub CI (Cockroach)