Log Events for Alter Table, Async Schema Change

[mrtracy]

Commit adds three new event log types:

• "Alter Table", logged for every ALTER TABLE DDL statement.
• "Finish Schema Change", logged when an asynchronous schema change is
  completed.
• "Reverse Schema Change", logged when an asynchronous schema change encounters
  an error (such as a constraint violation during backfill) and is rolled back.

This Commit encountered an interesting, perhaps unintended effect with
SystemConfigTrigger: in a "mixed" transaction which updates both system and
non-system keys, the system changes will only be gossiped if the transaction
record is located on the system range. In other words, "mixed" transactions will
only work correctly if the system keys are modified first. Issue #7570 had been
logged to track this unexpected behavior; this commit has also added a number of
comments and a logged error to address this.

---

This change is

[tamird]

Sorry you had to learn about this the hard way :(

---

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

sql/alter_table.go, line 246 [r1] (raw file):

  }

  // Log "Alter Table" event.

this comment isn't useful =/

---

sql/event_log.go, line 44 [r1] (raw file):

  // EventLogDropTable is recorded when a table is dropped.
  EventLogDropTable EventLogType = "drop_table"
  // EventLogAlterTable is recorded when an ALTER TABLE statement is

none of the others reference a specific syntax. In fact, I think we run through AlterTable on ADD COLUMN, so this comment is inaccurate.

---

sql/lease.go, line 340 [r1] (raw file):

          // More efficient batching can be used if no event log message
          // is required.

Slightly less code duplication:

txn.SetSystemConfigTrigger()
b := txn.NewBatch()
b.Put(descKey, desc)
if logEvent != nil {
  if err := txn.Run(b); err != nil {
    return err
  }
  if err := logEvent(txn); err != nil {
    return err
  }
  return txn.Commit()
}
return txn.CommitInBatch()

---

sql/schema_changer.go, line 520 [r1] (raw file):

              Error      string
              MutationID uint32
          }{causingError.Error(), uint32(sc.mutationID)},

you might want to fmt.Sprintf("%+v", causingError); that will give you the error's stack trace if the error came from a call to errors.Errorf or one of its friends.

---

sql/testdata/event_log, line 263 [r1] (raw file):

----
53 1

remove

---

Comments from Reviewable

[vivekmenezes]

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

sql/event_log.go, line 49 [r1] (raw file):

  // EventLogReverseSchemaChange is recorded when an in-progress schema change
  // encounters a problem and is reversed.
  EventLogReverseSchemaChange EventLogType = "reverse_schema_change"

s/reverse/reversing

---

sql/event_log.go, line 52 [r1] (raw file):

  // EventLogFinishSchemaChange is recorded when a previously initiated schema
  // change has completed.
  EventLogFinishSchemaChange EventLogType = "finish_schema_change"

s/finish/finished

---

Comments from Reviewable

[tamird]

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

sql/event_log.go, line 49 [r1] (raw file):

Previously, vivekmenezes wrote…

s/reverse/reversing

current tense seems consistent with the others in here

---

sql/event_log.go, line 52 [r1] (raw file):

Previously, vivekmenezes wrote…

s/finish/finished

current tense seems consistent with the others in here

---

Comments from Reviewable

[vivekmenezes]

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

sql/event_log.go, line 49 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

current tense seems consistent with the others in here

ok

---

sql/event_log.go, line 52 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

current tense seems consistent with the others in here

ok

---

Comments from Reviewable

[mrtracy]

Review status: 1 of 9 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

sql/alter_table.go, line 246 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this comment isn't useful =/

Done.

---

sql/event_log.go, line 44 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

none of the others reference a specific syntax. In fact, I think we run through AlterTable on ADD COLUMN, so this comment is inaccurate.

Done.

---

sql/schema_changer.go, line 520 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

you might want to fmt.Sprintf("%+v", causingError); that will give you the error's stack trace if the error came from a call to errors.Errorf or one of its friends.

Done.

---

sql/testdata/event_log, line 263 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

remove

Done.

---

Comments from Reviewable

[mrtracy]

Review status: 1 of 9 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

sql/lease.go, line 340 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Slightly less code duplication:

txn.SetSystemConfigTrigger()
b := txn.NewBatch()
b.Put(descKey, desc)
if logEvent != nil {
  if err := txn.Run(b); err != nil {
    return err
  }
  if err := logEvent(txn); err != nil {
    return err
  }
  return txn.Commit()
}
return txn.CommitInBatch()

Done.

---

Comments from Reviewable

[tamird]

Reviewed 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

sql/alter_table.go, line 246 [r1] (raw file):

Previously, mrtracy wrote…

Done.

I meant that you should remove it; it's still not useful - the line it's "documenting" is already clear and succinct.

---

Comments from Reviewable