Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-21.1: catalog, catalogkv: add and use descriptor builder #61842

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

postamar
Copy link

@postamar postamar commented Mar 11, 2021

Backport 1/1 commits from #61429.

/cc @cockroachdb/release


catalog,catalogkv: add and use descriptor builder

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

This commit is, at heart, a refactor of how catalog.Descriptor objects
are created; either when read from storage via the catalogkv API
or when constructed from an in-memory descpb.*Descriptor struct.

The original motivation for this change was the fact that previously,
calling the descriptor validation methods could sometimes modify the
descriptor itself. Specifically, ValidateSelf would perform "privilege
surgery" and modify the privileges descriptor. This appears to have been
around since version 2 and is, in fact, responsible for a few bugs,
which this commit also fixes.

Taking a broader perspective it appears necessary that all such
descriptor surgeries should be performed together when read from
storage regardless of descriptor subtype (table, db, type, schema).
These surgeries are typically no-ops: table format upgrade, foreign key
representation upgrade, etc. Considering that catalog.Descriptor objects
can be created either from storage or from memory, this involves:
  1. unifying the constructors in the (table|db|type|schema)desc
     packages,
  2. unifying the descriptor unwrapping logic in catalogkv.

To address (1) this commit introduces catalog.DescriptorBuilder, a
builder interface which replaces the NewImmutable, NewExistingMutable,
NewCreatedMutable, etc. constructors. The builder does the same thing as
these except it uses a deep copy of the proto descriptor instead of
a shallow copy. This has, in itself, uncovered a few minor bugs.
Still, the main reason for the existence of this builder object is that
we can now perform all post-deserialization descriptor changes in a
method to be called before building the catalog.Descriptor object
proper.

This commit also changes how we update the descriptor modification time
using the MVCC timestamp. This is done at builder creation time, or, in
cases where we stick to descpb structs, using a new
descp.FromDescriptorWithMVCCTimestamp function which replaces
descpb.TableFromDescriptor and the descpb.Descriptor.GetTable,
GetDatabase, GetType and GetSchema methods. These methods are now all
forbidden from casual use by a new linter check (it used to be just
GetTable).

To address (2) this commit uses the new builder to unify all descriptor
wrapping logic in catalogkv into a new descriptorFromKeyValue function.
This function is what backs the catalogkv API now. This commit also
changes the API somewhat, the old GetDescriptorFromID function with its
many flag arguments is gone in favour of many lightweight public getter
functions (complementing existing ones) which all delegate to a common
private getDescriptorByID function. The most important side benefit to
doing all this is that we now no longer skip cross-reference validation
when getting mutable descriptors through the catalogkv API.
Cross-reference validation is now the default, and the exceptions to
this are few:
  - The catalogkv DescGetter implemementation, aka the
    oneLevelUncachedDescGetter, does not perform these checks, because
    they, in turn, require a DescGetter. In theory we could get around
    this infinite recursion with caching but right now there's no point.
  - GetAllDescriptorsUnvalidated explicitly performs no validation at
    all.

Doing all of this has uncovered a few bugs, the most serious of which is
that the protected_ts_meta system table didn't have the correct
permissions applied.

Furthermore this commit also unifies namespace collision detection logic
in catalogkv. A side-effect of this is that error messages for PG codes
42710 and 42P07 contain qualified names more often than before.

Release note (bug fix): Fixed privileges for system.protected_ts_meta.

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

This commit is, at heart, a refactor of how catalog.Descriptor objects
are created; either when read from storage via the catalogkv API
or when constructed from an in-memory descpb.*Descriptor struct.

The original motivation for this change was the fact that previously,
calling the descriptor validation methods could sometimes modify the
descriptor itself. Specifically, ValidateSelf would perform "privilege
surgery" and modify the privileges descriptor. This appears to have been
around since version 2 and is, in fact, responsible for a few bugs,
which this commit also fixes.

Taking a broader perspective it appears necessary that all such
descriptor surgeries should be performed together when read from
storage regardless of descriptor subtype (table, db, type, schema).
These surgeries are typically no-ops: table format upgrade, foreign key
representation upgrade, etc. Considering that catalog.Descriptor objects
can be created either from storage or from memory, this involves:
  1. unifying the constructors in the (table|db|type|schema)desc
     packages,
  2. unifying the descriptor unwrapping logic in catalogkv.

To address (1) this commit introduces catalog.DescriptorBuilder, a
builder interface which replaces the NewImmutable, NewExistingMutable,
NewCreatedMutable, etc. constructors. The builder does the same thing as
these except it uses a deep copy of the proto descriptor instead of
a shallow copy. This has, in itself, uncovered a few minor bugs.
Still, the main reason for the existence of this builder object is that
we can now perform all post-deserialization descriptor changes in a
method to be called before building the catalog.Descriptor object
proper.

This commit also changes how we update the descriptor modification time
using the MVCC timestamp. This is done at builder creation time, or, in
cases where we stick to descpb structs, using a new
descp.FromDescriptorWithMVCCTimestamp function which replaces
descpb.TableFromDescriptor and the descpb.Descriptor.GetTable,
GetDatabase, GetType and GetSchema methods. These methods are now all
forbidden from casual use by a new linter check (it used to be just
GetTable).

To address (2) this commit uses the new builder to unify all descriptor
wrapping logic in catalogkv into a new descriptorFromKeyValue function.
This function is what backs the catalogkv API now. This commit also
changes the API somewhat, the old GetDescriptorFromID function with its
many flag arguments is gone in favour of many lightweight public getter
functions (complementing existing ones) which all delegate to a common
private getDescriptorByID function. The most important side benefit to
doing all this is that we now no longer skip cross-reference validation
when getting mutable descriptors through the catalogkv API.
Cross-reference validation is now the default, and the exceptions to
this are few:
  - The catalogkv DescGetter implemementation, aka the
    oneLevelUncachedDescGetter, does not perform these checks, because
    they, in turn, require a DescGetter. In theory we could get around
    this infinite recursion with caching but right now there's no point.
  - GetAllDescriptorsUnvalidated explicitly performs no validation at
    all.

Doing all of this has uncovered a few bugs, the most serious of which is
that the protected_ts_meta system table didn't have the correct
permissions applied.

Furthermore this commit also unifies namespace collision detection logic
in catalogkv. A side-effect of this is that error messages for PG codes
42710 and 42P07 contain qualified names more often than before.

Release note (bug fix): Fixed privileges for system.protected_ts_meta.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@postamar postamar marked this pull request as ready for review March 11, 2021 15:42
@postamar postamar requested a review from a team March 11, 2021 15:42
@postamar postamar requested a review from a team as a code owner March 11, 2021 15:42
@postamar postamar requested review from miretskiy and a team and removed request for a team March 11, 2021 15:42
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 87 of 167 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @miretskiy)

@postamar postamar merged commit 7116de8 into cockroachdb:release-21.1 Mar 11, 2021
@postamar postamar deleted the backport21.1-61429 branch March 11, 2021 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants