From 841534e2afa0b7101a05240e77f761a41cc23b93 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Mon, 8 Jun 2020 08:47:43 -0400 Subject: [PATCH] sqlbase: generalize setting of ModificationTime This commit is the ugly side of copying the leasing fields into each of the descriptors. This leaves the `Descriptor.Table` method in place for now as it forms defense in depth. We'll remove it eventually when this all gets cleaned up. Release note: None --- pkg/sql/catalog/catalogkv/catalogkv.go | 5 +- pkg/sql/sqlbase/structured.go | 143 +++++++++++++++++-------- 2 files changed, 100 insertions(+), 48 deletions(-) diff --git a/pkg/sql/catalog/catalogkv/catalogkv.go b/pkg/sql/catalog/catalogkv/catalogkv.go index 0449d2865777..49105b3592aa 100644 --- a/pkg/sql/catalog/catalogkv/catalogkv.go +++ b/pkg/sql/catalog/catalogkv/catalogkv.go @@ -103,7 +103,8 @@ func unwrapDescriptor( ctx context.Context, txn *kv.Txn, codec keys.SQLCodec, ts hlc.Timestamp, desc *sqlbase.Descriptor, ) (catalog.Descriptor, error) { // TODO(ajwerner): Fill in the ModificationTime field for the descriptor. - table, database, typ, schema := desc.Table(ts), desc.GetDatabase(), desc.GetType(), desc.GetSchema() + desc.MaybeSetModificationTimeFromMVCCTimestamp(ctx, ts) + table, database, typ, schema := desc.Table(hlc.Timestamp{}), desc.GetDatabase(), desc.GetType(), desc.GetSchema() switch { case table != nil: if err := table.MaybeFillInDescriptor(ctx, txn, codec); err != nil { @@ -342,7 +343,7 @@ func GetDatabaseDescriptorsFromIDs( desc.String(), ) } - // TODO(ajwerner): Set ModificationTime. + desc.MaybeSetModificationTimeFromMVCCTimestamp(ctx, result.Rows[0].Value.Timestamp) results = append(results, sqlbase.NewImmutableDatabaseDescriptor(*db)) } return results, nil diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go index 10a8e8b80377..4677147fe1fa 100644 --- a/pkg/sql/sqlbase/structured.go +++ b/pkg/sql/sqlbase/structured.go @@ -351,15 +351,15 @@ func GetDatabaseDescFromID( ) (*ImmutableDatabaseDescriptor, error) { desc := &Descriptor{} descKey := MakeDescMetadataKey(codec, id) - _, err := protoGetter.GetProtoTs(ctx, descKey, desc) + ts, err := protoGetter.GetProtoTs(ctx, descKey, desc) if err != nil { return nil, err } - // TODO(ajwerner): Set the Modification time. db := desc.GetDatabase() if db == nil { return nil, ErrDescriptorNotFound } + desc.MaybeSetModificationTimeFromMVCCTimestamp(ctx, ts) return NewImmutableDatabaseDescriptor(*db), nil } @@ -3699,22 +3699,56 @@ func (desc *Descriptor) GetName() string { } } -// Table is a replacement for GetTable() which seeks to ensure that clients -// which unmarshal Descriptor structs properly set the ModificationTime on -// tables based on the MVCC timestamp at which the descriptor was read. -// -// A linter should ensure that GetTable() is not called. -func (desc *Descriptor) Table(ts hlc.Timestamp) *TableDescriptor { - t := desc.GetTable() - if t != nil { - t.maybeSetTimeFromMVCCTimestamp(ts) +// GetVersion returns the Version of the descriptor. +func (desc *Descriptor) GetVersion() DescriptorVersion { + switch t := desc.Union.(type) { + case *Descriptor_Table: + return t.Table.Version + case *Descriptor_Database: + return t.Database.Version + case *Descriptor_Type: + return t.Type.Version + case *Descriptor_Schema: + return t.Schema.Version + default: + return 0 + } +} + +// GetModificationTime returns the ModificationTime of the descriptor. +func (desc *Descriptor) GetModificationTime() hlc.Timestamp { + switch t := desc.Union.(type) { + case *Descriptor_Table: + return t.Table.ModificationTime + case *Descriptor_Database: + return t.Database.ModificationTime + case *Descriptor_Type: + return t.Type.ModificationTime + case *Descriptor_Schema: + return t.Schema.ModificationTime + default: + return hlc.Timestamp{} } - return t } -// maybeSetTimeFromMVCCTimestamp will update ModificationTime and possible -// CreateAsOfTime with the provided timestamp. If desc.ModificationTime is -// non-zero it must be the case that it is not after the provided timestamp. +// GetModificationTime returns the ModificationTime of the descriptor. +func (desc *Descriptor) setModificationTime(ts hlc.Timestamp) { + switch t := desc.Union.(type) { + case *Descriptor_Table: + t.Table.ModificationTime = ts + case *Descriptor_Database: + t.Database.ModificationTime = ts + case *Descriptor_Type: + t.Type.ModificationTime = ts + case *Descriptor_Schema: + t.Schema.ModificationTime = ts + } +} + +// MaybeSetModificationTimeFromMVCCTimestamp will update ModificationTime and +// possibly CreateAsOfTime on TableDescriptor with the provided timestamp. If +// ModificationTime is non-zero it must be the case that it is not after the +// provided timestamp. // // When table descriptor versions are incremented they are written with a // zero-valued ModificationTime. This is done to avoid the need to observe @@ -3728,30 +3762,34 @@ func (desc *Descriptor) Table(ts hlc.Timestamp) *TableDescriptor { // // It is vital that users which read table descriptor values from the KV store // call this method. -func (desc *TableDescriptor) maybeSetTimeFromMVCCTimestamp(ts hlc.Timestamp) { - // CreateAsOfTime is used for CREATE TABLE ... AS ... and was introduced in - // v19.1. In general it is not critical to set except for tables in the ADD - // ADD state which were created from CTAS so we should not assert on its not - // being set. It's not always sensical to set it from the passed MVCC - // timestamp. However, starting in 19.2 the CreateAsOfTime and - // ModificationTime fields are both unset for the first Version of a - // TableDescriptor and the code relies on the value being set based on the - // MVCC timestamp. - if !ts.IsEmpty() && - desc.ModificationTime.IsEmpty() && - desc.CreateAsOfTime.IsEmpty() && - desc.Version == 1 { - desc.CreateAsOfTime = ts - } - - // Ensure that if the table is in the process of being added and relies on - // CreateAsOfTime that it is now set. - if desc.Adding() && desc.IsAs() && desc.CreateAsOfTime.IsEmpty() { - log.Fatalf(context.TODO(), "table descriptor for %q (%d.%d) is in the "+ - "ADD state and was created with CREATE TABLE ... AS but does not have a "+ - "CreateAsOfTime set", desc.Name, desc.ParentID, desc.ID) +func (desc *Descriptor) MaybeSetModificationTimeFromMVCCTimestamp( + ctx context.Context, ts hlc.Timestamp, +) { + switch t := desc.Union.(type) { + case *Descriptor_Table: + // CreateAsOfTime is used for CREATE TABLE ... AS ... and was introduced in + // v19.1. In general it is not critical to set except for tables in the ADD + // ADD state which were created from CTAS so we should not assert on its not + // being set. It's not always sensical to set it from the passed MVCC + // timestamp. However, starting in 19.2 the CreateAsOfTime and + // ModificationTime fields are both unset for the first Version of a + // TableDescriptor and the code relies on the value being set based on the + // MVCC timestamp. + if !ts.IsEmpty() && + t.Table.ModificationTime.IsEmpty() && + t.Table.CreateAsOfTime.IsEmpty() && + t.Table.Version == 1 { + t.Table.CreateAsOfTime = ts + } + + // Ensure that if the table is in the process of being added and relies on + // CreateAsOfTime that it is now set. + if t.Table.Adding() && t.Table.IsAs() && t.Table.CreateAsOfTime.IsEmpty() { + log.Fatalf(context.TODO(), "table descriptor for %q (%d.%d) is in the "+ + "ADD state and was created with CREATE TABLE ... AS but does not have a "+ + "CreateAsOfTime set", t.Table.Name, t.Table.ParentID, t.Table.ID) + } } - // Set the ModificationTime based on the passed ts if we should. // Table descriptors can be updated in place after their version has been // incremented (e.g. to include a schema change lease). @@ -3759,22 +3797,35 @@ func (desc *TableDescriptor) maybeSetTimeFromMVCCTimestamp(ts hlc.Timestamp) { // with the value that lives on the in-memory copy. That value should contain // a timestamp set by this method. Thus if the ModificationTime is set it // must not be after the MVCC timestamp we just read it at. - if desc.ModificationTime.IsEmpty() && ts.IsEmpty() && desc.Version > 1 { + if modTime := desc.GetModificationTime(); modTime.IsEmpty() && ts.IsEmpty() && desc.GetVersion() > 1 { // TODO(ajwerner): reconsider the third condition here.It seems that there // are some cases where system tables lack this timestamp and then when they // are rendered in some other downstream setting we expect the timestamp to // be read. This is a hack we shouldn't need to do. - log.Fatalf(context.TODO(), "read table descriptor for %q (%d.%d) without ModificationTime "+ - "with zero MVCC timestamp", desc.Name, desc.ParentID, desc.ID) - } else if desc.ModificationTime.IsEmpty() { - desc.ModificationTime = ts - } else if !ts.IsEmpty() && ts.Less(desc.ModificationTime) { - log.Fatalf(context.TODO(), "read table descriptor %q (%d.%d) which has a ModificationTime "+ + log.Fatalf(context.TODO(), "read table descriptor for %q (%d) without ModificationTime "+ + "with zero MVCC timestamp", desc.GetName(), desc.GetID()) + } else if modTime.IsEmpty() { + desc.setModificationTime(ts) + } else if !ts.IsEmpty() && ts.Less(modTime) { + log.Fatalf(context.TODO(), "read table descriptor %q (%d) which has a ModificationTime "+ "after its MVCC timestamp: has %v, expected %v", - desc.Name, desc.ParentID, desc.ID, desc.ModificationTime, ts) + desc.GetName(), desc.GetID(), modTime, ts) } } +// Table is a replacement for GetTable() which seeks to ensure that clients +// which unmarshal Descriptor structs properly set the ModificationTime on +// tables based on the MVCC timestamp at which the descriptor was read. +// +// A linter should ensure that GetTable() is not called. +func (desc *Descriptor) Table(ts hlc.Timestamp) *TableDescriptor { + t := desc.GetTable() + if t != nil { + desc.MaybeSetModificationTimeFromMVCCTimestamp(context.TODO(), ts) + } + return t +} + // IsSet returns whether or not the foreign key actually references a table. func (f ForeignKeyReference) IsSet() bool { return f.Table != 0