sql: delete db zone config after GC TTL

[eriktrinh]

Previously the database's zone config would be deleted immediately when
a DROP DATABASE is initiated, including if there are tables that the
drop cascades down to. This causes dropped tables waiting for the GC
period without a table zone config to have values from the default
config to be applied (which can cause unnecessary data movement).

This change removes the database's zone config once all the cascaded
table data is completely removed, using the dropped table status in the
job details.

Fixes #24179.

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[benesch]

🎉 🙇 this is huge!

[BramGruneir]

Reviewed 5 of 5 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/jobs/jobspb/jobs.proto, line 139 at r1 (raw file):

  repeated DroppedTableDetails dropped_tables = 3 [(gogoproto.nullable) = false];
  // The descriptor ID of the dropped database which created this job.
  uint32 drop_database_id = 4 [

please call this dropped_database_id instead of drop_database_id to match the dropped_tables.

---

pkg/sql/drop_database.go, line 175 at r1 (raw file):

			return err
		}
		details := job.Details().(jobspb.SchemaChangeDetails)

Why do you need to set the details after creating the job? Why not do it within createDropTableJob()?

---

pkg/sql/drop_database.go, line 180 at r1 (raw file):

			return errors.Wrapf(err, "failed to set drop database ID %d for job", n.dbDesc.ID)
		}
	} else {

Why would the jobID be 0? Could you add a comment about it?

Also, do you test this code path?

---

pkg/sql/drop_test.go, line 214 at r1 (raw file):

		t.Fatal(err)
	}
	// Database zone config is removed once all table data and zone configs are removed.

Can you add a check to make sure that after the job is created, but before it is finished that the zone config still exists?

[eriktrinh]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/jobs/jobspb/jobs.proto, line 139 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

please call this dropped_database_id instead of drop_database_id to match the dropped_tables.

Done.

---

pkg/sql/drop_database.go, line 175 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Why do you need to set the details after creating the job? Why not do it within createDropTableJob()?

Done.

---

pkg/sql/drop_database.go, line 180 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Why would the jobID be 0? Could you add a comment about it?

Also, do you test this code path?

Done. Also added a test that goes down this code path when the database is empty

---

pkg/sql/drop_test.go, line 214 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Can you add a check to make sure that after the job is created, but before it is finished that the zone config still exists?

TestDropDatabaseDeleteData should already be testing this.

[BramGruneir]

Reviewed 8 of 8 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/drop_database.go, line 133 at r2 (raw file):

	}

	jobID, err := p.createDropTablesJob(ctx, tableDescs, droppedTableDetails, tree.AsStringWithFlags(n.n,

nit: for readability, please you put the tree.AsStringWithFlags(..) all on one line

---

pkg/sql/drop_test.go, line 242 at r2 (raw file):

}

func TestDropDatabaseEmpty(t *testing.T) {

Please add a comment as to what this test is doing. It helps others that are not familiar with the code. For the other one as well.

---

pkg/sql/truncate.go, line 89 at r2 (raw file):

	}

	dropJobID, err := p.createDropTablesJob(ctx, stmtTableDescs, droppedTableDetails, tree.AsStringWithFlags(n,

nit: Please keep the asstringwithflags() call all on one line

[BramGruneir]

with some small nits

[BramGruneir]

with some small nits

[vivekmenezes]

I'll just take a quick look at this. Hold on

[vivekmenezes]

I'll just take a quick look at this. Hold on

[BramGruneir]

Reviewed 3 of 3 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[vivekmenezes]

Nice work!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/drop_test.go, line 388 at r3 (raw file):

	if _, err := addDefaultZoneConfig(sqlDB, dbDesc.ID); err != nil {
		t.Fatal(err)
	}

perhaps add the zone config before dropping the database above.

---

pkg/sql/drop_test.go, line 402 at r3 (raw file):

	tests.CheckKeyCount(t, kvDB, table2Span, 6)

	// Database zone config is removed once all table data and zone configs are removed.

this comment is more appropriate for the check done at the end.

[eriktrinh]

Thanks for the review!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/drop_test.go, line 242 at r2 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Please add a comment as to what this test is doing. It helps others that are not familiar with the code. For the other one as well.

Done.

---

pkg/sql/drop_test.go, line 388 at r3 (raw file):

Previously, vivekmenezes wrote…

perhaps add the zone config before dropping the database above.

Done.

---

pkg/sql/drop_test.go, line 402 at r3 (raw file):

Previously, vivekmenezes wrote…

this comment is more appropriate for the check done at the end.

Done.

[eriktrinh]

bors r+

[eriktrinh]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)

[knz]

This needs a backport. @benesch @BramGruneir please assist Erik if this is his first, our window for backporting is only 2 days from today.

[knz]

This needs a backport. @benesch @BramGruneir please assist Erik if this is his first, our window for backporting is only 2 days from today.

[vivekmenezes]

This is for 2.2

…

On Fri, 12 Oct, 2018, 6:42 AM kena, ***@***.***> wrote: This needs a backport. @benesch <https://github.com/benesch> @BramGruneir <https://github.com/BramGruneir> please assist Erik if this is his first, our window for backporting is only 2 days from today. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#30666 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALOpBBCH39uKCpsx1UK8rbqE6raNJp2mks5ukHH7gaJpZM4W65DT> .

[vivekmenezes]

This is for 2.2

…

On Fri, 12 Oct, 2018, 6:42 AM kena, ***@***.***> wrote: This needs a backport. @benesch <https://github.com/benesch> @BramGruneir <https://github.com/BramGruneir> please assist Erik if this is his first, our window for backporting is only 2 days from today. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#30666 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALOpBBCH39uKCpsx1UK8rbqE6raNJp2mks5ukHH7gaJpZM4W65DT> .