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

sql: make deterministic descriptor ID generation possible #69226

Closed
ajwerner opened this issue Aug 23, 2021 · 1 comment · Fixed by #85366
Closed

sql: make deterministic descriptor ID generation possible #69226

ajwerner opened this issue Aug 23, 2021 · 1 comment · Fixed by #85366
Labels
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

@ajwerner
Copy link
Contributor

ajwerner commented Aug 23, 2021

Is your feature request related to a problem? Please describe.

Descriptor IDs are allocated by incrementing a constant key non-transactionally. This means that descriptor ID generation is non-deterministic in the face of transaction retries. This can be painful for logictests and tests in general which would like to depend on deterministic ID generation. Consider #69225.

The generation occurs here:

// GenerateUniqueDescID returns the next available Descriptor ID and increments
// the counter. The incrementing is non-transactional, and the counter could be
// incremented multiple times because of retries.
func GenerateUniqueDescID(ctx context.Context, db *kv.DB, codec keys.SQLCodec) (descpb.ID, error) {
// Increment unique descriptor counter.
newVal, err := kv.IncrementValRetryable(ctx, db, codec.DescIDSequenceKey(), 1)

Describe the solution you'd like

We should make an interface around descriptor ID generation and then plumb through an implementation of that interface. That may permit deterministic allocation. The wrinkle here is that we'd want such a thing to likely have a handle to a transaction, meaning that it'd likely need to be constructed with a transaction or add a transaction to the API.

Describe alternatives you've considered

We can keep hacking around flakes related to the assumed determinism. Another option is to randomize the allocation under testing to shake out any reliance on the determinism.

Jira issue: CRDB-9500

@ajwerner ajwerner added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Aug 23, 2021
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Aug 23, 2021
@postamar
Copy link
Contributor

postamar commented Mar 8, 2022

Doing this would unblock fixing #37751

craig bot pushed a commit that referenced this issue Aug 1, 2022
84865: kvserver: always return NLHE on lease acquisition timeouts r=nvanbenschoten a=erikgrinaker

In ab74b97 we added internal timeouts for lease acquisitions. These
were wrapped in `RunWithTimeout()`, as mandated for context timeouts.
However, this would mask the returned `NotLeaseHolderError` as a
`TimeoutError`, preventing the DistSender from retrying it and instead
propagating it out to the client. Additionally, context cancellation
errors from the actual RPC call were never wrapped as a
`NotLeaseHolderError` in the first place.

This ended up only happening in a very specific scenario where the outer
timeout added to the client context did not trigger, but the inner
timeout for the coalesced request context did trigger while the lease
request was in flight. Accidentally, the outer `RunWithTimeout()` call
did not return the `roachpb.Error` from the closure but instead passed
it via a captured variable, bypassing the error wrapping.

This patch replaces the `RunWithTimeout()` calls with regular
`context.WithTimeout()` calls to avoid the error wrapping, and returns a
`NotLeaseHolderError` from `requestLease()` if the RPC request fails and
the context was cancelled (presumably causing the error). Another option
would be to extract an NLHE from the error chain, but this would require
correct propagation of the structured error chain across RPC boundaries,
so out of an abundance of caution and with an eye towards backports, we
instead choose to return a bare `NotLeaseHolderError`.

The empty lease in the returned error prevents the DistSender from
updating its caches on context cancellation.

Resolves #84258.
Resolves #85115.

Release note (bug fix): Fixed a bug where clients could sometimes
receive errors due to lease acquisition timeouts of the form
`operation "storage.pendingLeaseRequest: requesting lease" timed out after 6s`.

84946: distsql: make the number of DistSQL runners dynamic r=yuzefovich a=yuzefovich

**distsql: make the number of DistSQL runners dynamic**

This commit improves the infrastructure around a pool of "DistSQL
runners" that are used for issuing SetupFlow RPCs in parallel.
Previously, we had a hard-coded number of 16 goroutines which was
probably insufficient in many cases. This commit makes it so that we use
the default value of `4 x N(cpus)` to make it proportional to how beefy
the node is (under the expectation that the larger the node is, the more
distributed queries it will be handling). The choice of the four as the
multiple was made so that we get the previous default on machines with
4 CPUs.

Additionally, this commit introduces a mechanism to dynamically adjust
the number of runners based on a cluster setting. Whenever the setting
is reduced, some of the workers are stopped, if the setting is
increased, then new workers are spun up accordingly. This coordinator
listens on two channels: one about the server quescing, and another
about the new target pool size. Whenever a new target size is received,
the coordinator will spin up / shut down one worker at a time until that
target size is achieved. The worker, however, doesn't access the server
quescing channel and, instead, relies on the coordinator to tell it to
exit (either by closing the channel when quescing or sending a single
message when the target size is decreased).

Fixes: #84459.

Release note: None

**distsql: change the flow setup code a bit**

Previously, when setting up a distributed plan, we would wait for all
SetupFlow RPCs to come back before setting up the flow on the gateway.
Most likely (in the happy scenario) all those RPCs would be successful,
so we can parallelize the happy path a bit by setting up the local flow
while the RPCs are in-flight which is what this commit does. This seems
especially beneficial given the change in the previous commit to
increase the number of DistSQL runners for beefy machines - we are now
more likely to issue SetupFlow RPCs asynchronously.

Release note: None

85091: flowinfra: disable queueing mechanism of the flow scheduler by default r=yuzefovich a=yuzefovich

This commit disables the queueing mechanism of the flow scheduler as
part of the effort to remove that queueing altogether during 23.1
release cycle. To get there though we choose a conservative approach of
introducing a cluster setting that determines whether the queueing is
enabled or not, and if it is disabled, then we effectively a treating
`sql.distsql.max_running_flows` limit as infinite. By default, the
queueing is now disabled since recent experiments have shown that the
admission control does a good job of protecting the nodes from the
influx of remote flows.

Addresses: #34229.

Release note: None

85134: sql: allow NULL in create view definition r=mgartner a=rafiss

fixes #84000

Release note (sql change): CREATE VIEW statements can now have a
constant NULL column definition. The resulting column is of type TEXT.

85178: kvserver: record batch requests with no gateway r=kvoli a=kvoli

Previously, batch requests with no `GatewayNodeID` would not be
accounted for on the QPS of a replica. By extension, the store QPS would
also not aggregate this missing QPS over replicas it holds. This patch
introduces tracking for all requests, regardless of the `GatewayNodeID`.

This was done to as follow the workload lease transfers consider the
per-locality counts, therefore untagged localities were not useful. This
has since been updated to ignore filter out localities directly, so it
is not necessary to exclude them anymore.

`leaseholderStats`, which previously tracked the QPS, and `writeStats`
tracking the mvcc keys written, have also been removed. They are
duplicated in `batchRequest` and `writeKeys` respectively, within the
`loadStats` of a replica.

resolves #85157

Release note: None

85355: sql: improve physical planning of window functions r=yuzefovich a=yuzefovich

**sql: remove shouldNotDistribute recommendation**

It doesn't seem to be used much.

Release note: None

**sql: improve physical planning of window functions**

This commit improves the physical planning of window functions in
several ways.

First, the optimizer is updated so that all window functions with a
PARTITION BY clause are constructed first followed by the remaining
window functions without PARTITION BY. This is needed by the execution
which can only evaluate functions with PARTITION BY in the distributed
fashion - as a result of this change, we are now more likely to get
partial distributed execution (previously things depended on the order
in which window functions were mentioned in the query).

Second, the physical planner now thinks that we "should distribute" the
plan if it finds at least one window function with PARTITION BY clause.
Previously, we didn't make any recommendation about the distribution
based on the presence of the window functions (i.e. we relied on the
rest of the plan to do so), but they can be quite computation-intensive,
so whenever we can distribute the execution, we should do so.

Additionally, this commit removes some of the code in the physical
planner which tries to find window functions with the same PARTITION BY
and ORDER BY clauses - that code has been redundant for long time given
that the optimizer does that too.

Release note: None

85366: sql,logictest,descidgen: abstract descriptor ID generation, make deterministic in logictests r=ajwerner a=ajwerner

The first commit adds an interface for descriptor ID generation and propagates the interface from the ExecCfg into the EvalContext. There are some minor refactoring to avoid propagating an ExecCfg further up the stack by making the parameters more specific. The second commit adds a testing knob to use a transactional implementation in the EvalContext.

Fixes #37751
Fixes #69226

85406: schemachanger: check explain diagrams during rollback test r=postamar a=postamar

This commit enriches the declarative schema changer integration tests by
making data-driven EXPLAIN output assertions easier to add as
a complement to otherwise unrelated tests. In particular, this commit
improves the rollback test to check the explained rollback plan for each
post-commit revertible stage. This should make it easier to debug bad
rule definitions which otherwise would manifest themselves as causing
the schema change to hang during the rollback.

Release note: None

85414: colflow: fix a recent flake r=yuzefovich a=yuzefovich

In 0866ddc we merged a change that
relied on the assumption that the allocator passed to the parallel
unordered synchronizer was not used by anyone else, but this assumption
was broken in a test and is now fixed.

Fixes: #85360.

Release note: None

Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Austen McClernon <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
@craig craig bot closed this as completed in 85ce24d Aug 1, 2022
@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
craig bot pushed a commit that referenced this issue Apr 19, 2024
121380: ui: add static images to asset build step r=laurenbarker a=dhartunian

During the `genassets` build + embed step, we were taking just the output of the `db-console-ccl` or `db-console-oss` step which is just a build.js file. This commit adds references to the image assets we want bundled as well. This includes favicon.ico and everything in `./ assets` relative to the db-console build directory.

We disable content hashing in webpack in order to keep the filenames static, which bazel requires. The impact should be minimal as we rarely change these images so if they're cached forever, it's fine.

This change restores the favicon to the CRDB build and the nice image that shows up in the background of the email signup bar.

The size of the final zipped bundle only differs by around 1MB and is already 10MB in size.

Fixes: #117876
Epic: None

Release note (ui change): the favicon now renders properly for DB Console along with other image files.

122151: ui: make custom chart tool work at store level r=koorosh a=abarganier

Fixes: #121364

This patch fixes a bug in the DB Console custom chart tool, where
selecting the "Per Node" checkbox on a metric would not properly display
store-level metrics. The previous expected behavior was that the check
box would cause the metric to aggregate across stores at the node level
(e.g. if the node had 3 stores, it'd SUM the store-level timeseries
together and return a single timeseries for the node). Instead, the
feature was only showing the 1st store associated with the node.

This was due to a bug in the code used to determine if a metric was
store-level. A function was used that improperly assumed that the
`cr.node.*` or `cr.store.*` prefix had been stripped from the metric
name, which was not always the case. This led to us improperly
interpret store-level metrics as node-level.

The fix is to fix the logic used to determine if a metric is
store-level.

Additionally, this patch updates the code to no longer aggregate
store-level metrics across each node. Instead, we will now show a single
timeseries per-store to provide finer-grained observability into
store-level metrics within the custom chart tool.

Release note (bug fix): A bug has been fixed in the DB Console's custom
chart tool, where store-level metrics were not being displayed properly.
Previously, if a store-level metric was selected to be displayed at the
store-level on a multi-store node, only data for the 1st store ID
associated with that node would be displayed.

This patch ensures that data is displayed for all stores present on a
node. Additionally, it updates the behavior to show a single timeseries
for each store, as opposed to aggregating (e.g. SUM) all stores across
the node. This allows finer-grained observability into store-level
metrics when using the custom chart tool in DB Console.

122539: spanconfigreconcilerccl: use txn descriptor ID generation for test r=rimadeodhar a=rimadeodhar

This PR updates the spanconfigreconciler data driven test to use transactional descriptor ID generation
(#69226) to generate deterministic descriptor IDs. This will help avoid test flakes around changing descriptor IDs due to transaction retries etc.

Epic: none
Fixes: #122343
Release note: None

122557: catalog: add descriptor repair to remove missing roles r=fqazi a=fqazi

Previously, we had a bug that could lead to descriptors having privileages to roles that no longer exist. This could lead to certain commands like SHOW GRANTS breaking. To address this, this patch will add descirptor repair logic to automatically clean up oprhaned privileges.

Fixes: #122552

Release note (bug fix): Add automated clean up / validation for dropped roles inside descriptors.

Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Alex Barganier <[email protected]>
Co-authored-by: rimadeodhar <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
2 participants