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

descs: refactor GetAll* methods #93543

Merged
merged 3 commits into from
Dec 14, 2022
Merged

Conversation

postamar
Copy link

@postamar postamar commented Dec 13, 2022

This commit refactors the descs.Collection's GetAll* methods, which
return descriptors and other catalog metadata in bulk, to share a common
access path in which the descriptor's layers are properly aggregated.
This is non-trivial as there are numerous edge cases:

  • virtual schemas and objects exist for all databases and their
    descriptors don't reference the parent database at all,
  • descriptorless public schemas and temporary schemas only exist as
    namespace entries,
  • functions don't have namespace entries,
  • and so forth.

These new methods are delegated to by the existing methods which are now
deprecated.

Informs #64089.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

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 32 of 32 files at r1, 2 of 2 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @postamar)


pkg/sql/catalog/descs/collection.go line 861 at r3 (raw file):

	ctx context.Context, txn *kv.Txn, stored nstree.Catalog, seedIDs catalog.DescriptorIDSet,
) (ret nstree.MutableCatalog, _ error) {
	descIDs := catalog.MakeDescriptorIDSet(seedIDs.Ordered()...)

nit: it'd be nice to not need to make this slice, but it's a nit


pkg/sql/catalog/tabledesc/table_desc.go line 78 at r3 (raw file):

// SkipNamespace implements the descriptor interface.
func (desc *wrapper) SkipNamespace() bool {
	return desc.IsVirtualTable()

This could use some commentary

@postamar postamar force-pushed the collection-get-all branch 2 times, most recently from 5f4c7d7 to 10648d7 Compare December 13, 2022 22:04
@postamar
Copy link
Author

Thanks for the review. I've addressed your comments. Notice also that the rttanalysis benchmark regresses with this PR; I'm well aware & this will be addressed shortly. There's some duplication of work due to some uncalled-for use of the catkv.Direct interface in pg_catalog.go, it's quite straightforward.

@postamar postamar force-pushed the collection-get-all branch 2 times, most recently from 4cd6cd4 to 6e8db4c Compare December 14, 2022 02:29
Marius Posta added 3 commits December 13, 2022 22:20
This commit renames existing methods and adds a few missing pieces for
the sake of consistency.

Release note: None
This commit adds a new Visit method to the VirtualSchemas interface
which allows traversing the entirety of the virtual catalog.

Release note: None
This commit refactors the descs.Collection's GetAll* methods, which
return descriptors and other catalog metadata in bulk, to share a common
access path in which the descriptor's layers are properly aggregated.
This is non-trivial as there are numerous edge cases:
  - virtual schemas and objects exist for all databases and their
    descriptors don't reference the parent database at all,
  - descriptorless public schemas and temporary schemas only exist as
    namespace entries,
  - functions don't have namespace entries,
  - and so forth.

These new methods are delegated to by the existing methods which are now
deprecated.

Informs cockroachdb#64089.

Release note: None
@postamar postamar marked this pull request as ready for review December 14, 2022 03:20
@postamar postamar requested a review from a team December 14, 2022 03:20
@postamar postamar requested review from a team as code owners December 14, 2022 03:20
@postamar postamar requested review from a team and rhu713 and removed request for a team December 14, 2022 03:20
@postamar
Copy link
Author

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 14, 2022

Build succeeded:

@craig craig bot merged commit 3f94d3d into cockroachdb:master Dec 14, 2022
@postamar postamar deleted the collection-get-all branch December 14, 2022 04:28
postamar pushed a commit to postamar/cockroach that referenced this pull request Dec 22, 2022
Recent changes in cockroachdb#93543 had modified the contract of this method (it no
longer returns dropped tables) and made it unsuitable for its main use
case, the SQLTranslator.

This commit fixes this regression by removing this deprecated method
entirely and using correct alternatives instead.

Fixes cockroachdb#93614.

Release note: None
craig bot pushed a commit that referenced this pull request Dec 22, 2022
94110: roachprod: include storage workload metadata on snapshot r=jbowens a=coolcom200

Currently when a snapshot is taken of a volume that has been used for storage workload collection, the snapshot only contains the user provided information--name of the snapshot and description. Which could lead to data not being included about which cluster this ran on, machine type, crdb version, etc.

As a result, we encode this metadata in the labels / tags when we create a snapshot allowing the user to provide both a name and a description while also capturing metadata that can be used for searching and further reference. There are some limitations with the maximum length of the labels (aws key: 128 chars value: 256 chars; gcp: both 63 chars) and which characters are allowed to be used (gcp: lowercase, digits, _, -; aws: letters, digits, spaces, ., :, +, =, `@,` _, /, -)

Alternatively, the metadata could be encoded into the description field which would allow for more data to be saved at the cost of it being harder to search / filter.

Fixes: #94075

Release note: None

94123: sql: implement the `log_timezone` session variable r=rafiss a=otan

Informs #84505

Release note (sql change): Add the `log_timezone` session variable, which is read only and always UTC.

94154: cloud: set orchestration version updated to 22.2.1 r=absterr08 a=absterr08

links Epic: https://cockroachlabs.atlassian.net/browse/REL-228

Release note: none

94178: descs: remove GetAllTableDescriptorsInDatabase r=postamar a=postamar

Recent changes in #93543 had modified the contract of this method (it no longer returns dropped tables) and made it unsuitable for its main use case, the SQLTranslator.

This commit fixes this regression by removing this deprecated method entirely and using correct alternatives instead.

Fixes #93614.

Release note: None

Co-authored-by: Leon Fattakhov <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Abby Hersh <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
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