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.

Sign up for GitHub

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

Jump to bottom

descs: extend uncommitted layer beyond descriptors #88571

Closed
postamar opened this issue Sep 23, 2022 · 2 comments
Closed

descs: extend uncommitted layer beyond descriptors #88571

postamar opened this issue Sep 23, 2022 · 2 comments
Assignees
@chengxiong-ruan
Labels
A-schema-catalog Related to the schema descriptors collection and the catalog API in general. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@postamar
Copy link
Contributor

postamar commented Sep 23, 2022 •
edited
Loading

The descs.Collection should mediate and maintain in-memory changes not just to descriptors, but also to anything joining on descriptors:

  • namespace entries,
  • zone configs,
  • comments,
  • is there anything else? idk.

This implies also caching in the stored layer, plumbing the reads of these new system tables into catkv.catalogQuerier, adding and adopting more accessor methods on the Collection, etc.

We can then get rid of the MetadataUpdater, CommentCache etc. interfaces and implementations.

At that point it should also be trivial to extend nstree.Catalog to include zone configs and comments.

Jira issue: CRDB-19972

Epic CRDB-19158
@postamar postamar added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-schema-catalog Related to the schema descriptors collection and the catalog API in general. labels Sep 23, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Sep 23, 2022
@postamar
Copy link
Contributor Author

This is a prereq for #88294

chengxiong-ruan added a commit to chengxiong-ruan/cockroach that referenced this issue Nov 1, 2022
@chengxiong-ruan
upgrades,sql: alter system.comments table primary key
3d76422 
Part of cockroachdb#88571

We need to read and write `system.comments` through `kv.Batch`
if we want extend the `uncommitted` layer of collection to cache
original comments and changes to comments, and be able to write
changes to comments together with other mutations to descriptor
in same transaction. To achieve that it'd be more convenient to
have the descriptor id as the first column of primary key, so that
we can construct a kv key like `/commentsTableID/descID` to fetch
all comments of an object. Otherwise we'd need to loop through
all comment types.

We're lucky that none of the existing code really rely on the old
primary key column order.

Release note: None
@chengxiong-ruan chengxiong-ruan mentioned this issue Nov 1, 2022
Closed
chengxiong-ruan added a commit to chengxiong-ruan/cockroach that referenced this issue Nov 1, 2022
@chengxiong-ruan
upgrades,sql: alter system.comments table primary key
95baec0 
Part of cockroachdb#88571

We need to read and write `system.comments` through `kv.Batch`
if we want extend the `uncommitted` layer of collection to cache
original comments and changes to comments, and be able to write
changes to comments together with other mutations to descriptor
in same transaction. To achieve that it'd be more convenient to
have the descriptor id as the first column of primary key, so that
we can construct a kv key like `/commentsTableID/descID` to fetch
all comments of an object. Otherwise we'd need to loop through
all comment types.

We're lucky that none of the existing code really rely on the old
primary key column order.

Release note: None
@chengxiong-ruan chengxiong-ruan mentioned this issue Nov 3, 2022
Closed
chengxiong-ruan added a commit to chengxiong-ruan/cockroach that referenced this issue Nov 4, 2022
@chengxiong-ruan
sql: uncommitted layer for comments
7cfc3cc 
part of cockroachdb#88571

This commit adds uncommitted layer cache for system.comments.
A few things are done in this commit:
1. scan system.comments within same batches of descriptors.
2. added system.comments cache in `stored` layer cache.
3. added uncommited cache in `Collection`
4. expanded Collection interface, so that caches are utilized
   when comments are fetched and written to.
5. modified declarative schema changer to read comments from
   `Collection`, though still keep the `CommentGetter` interface.
6. modified the write path of comments updates in declarative
   schema changer to write within same kv.Batch of descriptors.
   And make sure the change goes into the new uncommitted layer.
   (of course, this would need to changed when we start the
   generalization work).

Release note: None.
chengxiong-ruan added a commit to chengxiong-ruan/cockroach that referenced this issue Nov 8, 2022
@chengxiong-ruan
sql: uncommitted layer for comments and zone configs
6c5b842 
part of cockroachdb#88571

This commit adds uncommitted layer cache for system.comments
and system.zones.
A few things are done in this commit:
1. scan both tables within same batches of descriptors.
2. added cache(a storage mirror) in `stored` layer cache.
3. added uncommited cache in `Collection`
4. expanded Collection interface, so that caches are utilized
   when comments or zone configs are fetched and written to.
5. modified declarative schema changer to read comments and zone
   configs from `Collection`, though still keep the `CommentGetter`
   interface.
6. modified the write path of comments updates in declarative
   schema changer to write within same kv.Batch of descriptors.
   And make sure the change goes into the new uncommitted layer.
   For zone configs, for now we only do deletes, we may add similar
   updates as comments by utilizing the `KvWriter`.
   (of course, this would need to changed when we start the
   generalization work).

Release note: None.
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 8, 2022
catkv: replace StoredCatalog with cached CatalogReader
21f2fbb 
This commit leverages the previous one and uses nstree.Catalog as the
data structure for caching all catalog data read by catkv.CatalogReader.
The StoredCatalog object is removed in favor of a cached CatalogReader
implementation.

This results in more consistent behavior when reading descriptors,
comments and zone configs.

Informs cockroachdb#88571.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 8, 2022
catkv: replace StoredCatalog with cached CatalogReader
d1fba4c 
This commit leverages the previous one and uses nstree.Catalog as the
data structure for caching all catalog data read by catkv.CatalogReader.
The StoredCatalog object is removed in favor of a cached CatalogReader
implementation.

This results in more consistent behavior when reading descriptors,
comments and zone configs.

Informs cockroachdb#88571.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 8, 2022
catkv: replace StoredCatalog with cached CatalogReader
b3725e1 
This commit leverages the previous one and uses nstree.Catalog as the
data structure for caching all catalog data read by catkv.CatalogReader.
The StoredCatalog object is removed in favor of a cached CatalogReader
implementation.

This results in more consistent behavior when reading descriptors,
comments and zone configs.

Informs cockroachdb#88571.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 8, 2022
catkv: replace StoredCatalog with cached CatalogReader
f19946b 
This commit leverages the previous one and uses nstree.Catalog as the
data structure for caching all catalog data read by catkv.CatalogReader.
The StoredCatalog object is removed in favor of a cached CatalogReader
implementation.

This results in more consistent behavior when reading descriptors,
comments and zone configs.

Informs cockroachdb#88571.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 9, 2022
catkv: replace StoredCatalog with cached CatalogReader
db87999 
This commit leverages the previous one and uses nstree.Catalog as the
data structure for caching all catalog data read by catkv.CatalogReader.
The StoredCatalog object is removed in favor of a cached CatalogReader
implementation.

This results in more consistent behavior when reading descriptors,
comments and zone configs.

Informs cockroachdb#88571.

Release note: None
craig bot pushed a commit that referenced this issue Dec 9, 2022
@mgartner
Merge #93080 #93104
153448e 
93080: sql: alias tree.ParamTypes as []tree.ParamType r=mgartner a=mgartner

Epic: None

Release note: None

93104: catkv: replace StoredCatalog with cached CatalogReader r=postamar a=postamar

catkv: replace StoredCatalog with cached CatalogReader

This commit leverages the previous one and uses nstree.Catalog as the
data structure for caching all catalog data read by catkv.CatalogReader.
The StoredCatalog object is removed in favor of a cached CatalogReader
implementation.

This results in more consistent behavior when reading descriptors,
comments and zone configs.

Informs #88571.

Release note: None

----

nstree: refactor Catalog entries

This commit changes no functionality. Comments and zone configs are
joined by descriptor ID in the Catalog's by-ID btree entries.

Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
@postamar postamar mentioned this issue Dec 9, 2022
Merged
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 9, 2022
sql: add kv_catalog_* virtual tables
17faa4f 
This commit introduces the following virtual tables, which each mirror
more or less exactly a system table containing catalog information:
  - crdb_internal.kv_catalog_descriptor mirrors system.descriptor,
  - crdb_internal.kv_catalog_namespace mirrors system.namespace,
  - crdb_internal.kv_catalog_zones mirrors system.zones,
  - crdb_internal.kv_catalog_comments mirrors system.comments.

Each virtual table is constructed by overlaying the data in the system
table with the in-memory data in the transaction's descs.Collection.
This may include virtual object descriptors and comments as well.

This makes it possible to immediately get rid of the predefined_comments
virtual table and simply replace its usages with kv_catalog_comments.
Longer term, these new virtual tables are expected to become useful to
test declarative schema changer behavior, also they might prove
convenient for troubleshooting schema-related incidents because the
descriptors and zone configs are readily exposed as JSON values.

Informs cockroachdb#88571.

Release note: None
@postamar postamar mentioned this issue Dec 12, 2022
Merged
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 12, 2022
descs: fix incorrect namespace shadowing
5519206 
Recent work towards cockroachdb#88571 has made the descriptors collection aware of
namespace operations. This awareness erroneously did not extend to
operations involving cached scans of the namespace table.

Fixes cockroachdb#93002.

Release note: None
craig bot pushed a commit that referenced this issue Dec 13, 2022
Merge #93461
966463a 
93461: descs: fix incorrect namespace shadowing r=postamar a=postamar

Recent work towards #88571 has made the descriptors collection aware of
namespace operations. This awareness erroneously did not extend to
operations involving cached scans of the namespace table.

Fixes #93002.

Release note: None

Co-authored-by: Marius Posta <[email protected]>
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 14, 2022
sql: add crdb_internal.kv_catalog_* virtual tables
71d5c5a 
This commit introduces the following virtual tables, which each mirror
more or less exactly a system table containing catalog information:

crdb_internal.kv_catalog_descriptor mirrors system.descriptor,
crdb_internal.kv_catalog_namespace mirrors system.namespace,
crdb_internal.kv_catalog_zones mirrors system.zones,
crdb_internal.kv_catalog_comments mirrors system.comments.
Each virtual table is constructed by overlaying the data in the system
table with the in-memory data in the transaction's descs.Collection.
This may include virtual object descriptors and comments as well.

This makes it possible to immediately get rid of the predefined_comments
virtual table and simply replace its usages with kv_catalog_comments.
Longer term, these new virtual tables are expected to become useful to
test declarative schema changer behavior, also they might prove
convenient for troubleshooting schema-related incidents because the
descriptors and zone configs are readily exposed as JSON values.

Informs cockroachdb#88571.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 14, 2022
sql: add crdb_internal.kv_catalog_* virtual tables
aff4cd1 
This commit introduces the following virtual tables, which each mirror
more or less exactly a system table containing catalog information:

crdb_internal.kv_catalog_descriptor mirrors system.descriptor,
crdb_internal.kv_catalog_namespace mirrors system.namespace,
crdb_internal.kv_catalog_zones mirrors system.zones,
crdb_internal.kv_catalog_comments mirrors system.comments.
Each virtual table is constructed by overlaying the data in the system
table with the in-memory data in the transaction's descs.Collection.
This may include virtual object descriptors and comments as well.

This makes it possible to immediately get rid of the predefined_comments
virtual table and simply replace its usages with kv_catalog_comments.
Longer term, these new virtual tables are expected to become useful to
test declarative schema changer behavior, also they might prove
convenient for troubleshooting schema-related incidents because the
descriptors and zone configs are readily exposed as JSON values.

Informs cockroachdb#88571.

Release note: None
craig bot pushed a commit that referenced this issue Dec 14, 2022
@andreimatei @andyyang890
Merge #91038 #93281 #93336
9c8ac7b 
91038: singleflight: spruce up singleflight  r=andreimatei a=andreimatei

This patch improves singleflight with a couple of features that were
done manually by most callers. The singleflight now optionally makes a flight's
context not respond to the cancelation of the caller's ctx, the flights
now run in stopper tasks, and stopper quiescence cancels the flights'
contexts. The callers were doing some of these things, but
inconsistently.

Also, the flights now get a tracing span and callers that join an
existing span get log messages demarcating the wait time. We've wanted
this multiple times when debugging, for example in the context of table
descriptor lease acquisitions and range descriptor resolving. To
accomodate for this, the contract of singleflight.DoChan() gets a bit
more complicated: the singleflight captures the tracing spans of all
non-leaders, and these callers have to call a new ReaderClosed() method
on the DoChan() result if they're not going to wait for the flight
result.

Fixes #90854

Release note: None
Epic: None

93281: upgrades: delete V22_2SystemPrivilegesTable upgrade and version gates r=rafiss a=andyyang890

This patch deletes the `V22_2SystemPrivilegesTable` upgrade since its
associated unit test starts to fail when the bootstrap schema for the
`system.privileges` table is updated. This is safe to do since the
release engineering team has previously stated they will bump
`binaryMinSupportedVersion` before cutting the branch for 23.1.

Epic: None

Release note: None

93336: sql: add kv_catalog_* virtual tables r=postamar a=postamar

This commit introduces the following virtual tables, which each mirror
more or less exactly a system table containing catalog information:
  - crdb_internal.kv_catalog_descriptor mirrors system.descriptor,
  - crdb_internal.kv_catalog_namespace mirrors system.namespace,
  - crdb_internal.kv_catalog_zones mirrors system.zones,
  - crdb_internal.kv_catalog_comments mirrors system.comments.

Each virtual table is constructed by overlaying the data in the system
table with the in-memory data in the transaction's descs.Collection.
This may include virtual object descriptors and comments as well.

This makes it possible to immediately get rid of the predefined_comments
virtual table and simply replace its usages with kv_catalog_comments.
Longer term, these new virtual tables are expected to become useful to
test declarative schema changer behavior, also they might prove
convenient for troubleshooting schema-related incidents because the
descriptors and zone configs are readily exposed as JSON values.

Informs #88571.

Release note: None

Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Andy Yang <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
@postamar
Copy link
Contributor Author

This has been done for a month now :)

@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chengxiong-ruan chengxiong-ruan

Labels
A-schema-catalog Related to the schema descriptors collection and the catalog API in general. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@postamar @chengxiong-ruan