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: 20.1 telemetry master list #45210

Closed
15 of 48 tasks
awoods187 opened this issue Feb 19, 2020 · 2 comments
Closed
15 of 48 tasks

sql: 20.1 telemetry master list #45210

awoods187 opened this issue Feb 19, 2020 · 2 comments
Labels
A-telemetry C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@awoods187
Copy link
Contributor

awoods187 commented Feb 19, 2020

AppDev

  • Drivers
  • ORMS
  • GUIs
  • Frameworks

SQL Vectorized

  • Vectorized turned on/off cluster setting
  • Query execution time

SQL Features

SQL Planning

SQL Schema

  • SHOW CONSTRAINTS
  • Inverted index
  • SHOW INDEXES
  • Time to complete schema change
  • Age of table at time of change
  • Canceled schema changes job
  • Show jobs
  • Schema changes length (by schema change type)
  • Column type changes (old type and new type)
  • Schema changes in explicit transactions
  • Size of Schema Change
  • Ratio (Size/Time) of Schema Change
  • Number of change in txn
  • JDBC streaming sql: add telemetry for improve streaming behavior on JDBC driver #43763

Questions:
Is ADD CONSTRAINT foo UNIQUE covered in ADD CONSTRAINT? Should it be separated?
Is CREATE UNIQUE INDEX covered in CREATE INDEX? Should it be separated?
Is DROP INDEX foo CASCADE covered in DROP INDEX? Should it be separated?

@awoods187 awoods187 added A-telemetry C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Feb 19, 2020
@yuzefovich
Copy link
Member

I'm looking at adding telemetry for DISTINCT, and it seems to me it's already in place (under sql.plan.opt.node.distinct-on feature name). Are we interested in distinct aggregation here? cc @awoods187

craig bot pushed a commit that referenced this issue Mar 16, 2020
46015: sql: add ability to alter primary key of a new table r=jordanlewis a=rohany

Fixes #46008.
Fixes #46007.

Release justification: low risk change that adds a useful feature

Release note (sql change): This PR makes it possible to create
a table and then add/alter primary key within the same transaction.

46058: sql: add telemetry for ORDER BY and INTERLEAVE r=yuzefovich a=yuzefovich

Release justification: reporting changes only.

This commit adds the reporting of telemetry on the usage of INTERLEAVE
(in `createTableNode.startExec`) and of ORDER BY (by marking
`opt.SortOp` with "Telemetry" flag in the optbuilder and reusing the
same "infrastructure" as relational operators do, right before
constructing the sort). Also it adds a logic test for the telemetry
collection on the window functions' usage.

Addresses: #45210.

Release note: None

46059: sql: add the 'every' aggregate function r=rohany a=rohany

Fixes #45842.

Release justification: low risk update to functionality
Release note (sql change): This PR adds the `every` aggregate function.

46099: pgwire: add telemetry for portal related requests r=rohany a=otan

Resolves #43763 by covering the happy case.

This PR adds telemetry everytime a "limitedCommandResult" is made, which
signifies jdbc streaming.

Release justification: simply telemetry.

Release note: None

46101: kvserver/concurrency: check context cancellation before cancelling r=nvanbenschoten a=nvanbenschoten

Relates to #45700.

This commit fixes a bug in the lockTableWaiter where we manually
cancelled the context (to clean up resources) used by an async push
_before_ checking whether the context was cancelled. This meant that we
alway hit the `pushCtx.Err() == context.Canceled` path and always
cleared the push error. This lead to deadlocks because a transaction
would detect that it was aborted but the error would be swallowed due to
this bug so the transaction would continue to wait instead of exiting
its lock wait-queues.

Release justification: fixes a high-severity bug where user transactions
could deadlock indefinitely.

46105: roachtest: adjust tpchvec r=yuzefovich a=yuzefovich

Release justification: non-production code changes.

This commit adjusts tpchvec test in the following ways:
- it disables the verbose logging of `bytes_usage` (this was enabled to
get something to dig into in case a fatal OOM error that occurred once
would occur again, but it didn't)
- it introduces two "run configurations" into the test:
1. keeps the previous behavior with a minor tweak (disables the
randomization of `workmem` setting). This configuration is meant to be
used to check the correctness of the vectorized engine and compare the
queries' runtimes against row-by-row engine.
2. adds a new configuration in which only the vectorized engine runs all
queries once, but the workmem is randomized in [100KiB, 1000KiB) range.
Also, we enable logging on `vectorized_flow` file (to see that the
spilling does occur). This configuration is meant to stress disk spilling
of the vectorized engine. There is no comparison of the runtimes (since
the row-by-row engine is not used in this config).

Fixes: #46131.

Release note: None

46124: sql,cli: improve the handling of notices r=otan a=knz

Fixes #45833.

tldr:
- notices printed at end in SQL shell (requested by @RaduBerinde),
- new built-in function `crdb_internal.notice()`,
- notices can now be logged with vmodule `notices=2`,
- code simplification.

Details:

The main purpose of this patch was to delay the printing
of notices in the crdb SQL shell to the end, after the results.

The motivation for that is that if there are many (e.g. thousands) of
result rows, a notice printed at the beginning or "in the middle"
would get drowned and missed by the human observer.

This patch achieves that with a straightforward, local change to the
CLI SQL code.

To unit test the change, a way to introduce notices at an arbitrary
point of query execution was also needed. To achieve this, the patch
introduces a new SQL built-in function `crdb_internal.notice()` with
the corresponding plumbing throughout the execution layers. This makes
it possible to send an out-of-band notice in the middle of SQL
execution. The unit test for the first changes already uses this new
built-in; I also foresee that we will discover that our users will
start using that in fancy ways that we are not envisioning yet.

To troubleshoot the implementation of the above, a way to inspect
the notices inside the SQL execution layer was needed; so this
patch also ensures notices are logged when the vmodule config
includes `notices=2`.

Finally, the various changes discussed here revealed that
the notice code in the SQL execution package could be simplified, so
this patch does this too.

Release justification: Category 4: Low risk, high reward changes to existing functionality

Release note (cli change): The `sql` and `demo` client commands now
display out-of-band server notices at the end of execution.

46144: sql: clean up the preliminary notices r=otan a=knz

First commit from #46124.

In the lead up to 20.1, we want the first impression of client notices
by users to be clean and positive. Random notice messages with
inconsistent casing, non-standard terminology and unscoped identifiers
would likely not delight developers.

To achieve this, this commit streamlines the messages for DROP INDEX
and PK changes.


Release justification: Category 4: Low risk, high reward changes to existing functionality

Release note: None

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@jordanlewis
Copy link
Member

@awoods187, please look at this and close/open a new one for 20.2 carryover items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-telemetry C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

5 participants