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

ui: TimeScaleDropdownWithSearchParams can cause infinite re-renders #106395

Open
zachlite opened this issue Jul 7, 2023 · 0 comments
Open

ui: TimeScaleDropdownWithSearchParams can cause infinite re-renders #106395

zachlite opened this issue Jul 7, 2023 · 0 comments
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability

Comments

@zachlite
Copy link
Contributor

zachlite commented Jul 7, 2023 •
edited
Loading

Since #101258, TimeScaleDropdownWithSearchParams is not compatible with certain tree structures, and can cause infinite re-renders. For now, this is un-diagnosed, but can be observed on the key visualizer page. It appears to be related to the custom duration options.

Jira issue: CRDB-29553
@zachlite zachlite added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-cluster-observability labels Jul 7, 2023
zachlite added a commit to zachlite/cockroach that referenced this issue Jul 7, 2023
@zachlite
ui: fix infinite re-render on the key visualizer page
db1d589 
Since cockroachdb#101258, the TimeScaleDropdownWithSearchParams
can cause infinite re-renders. The exact cause of the bug
is not yet diagnosed, but it occurs on the key visualizer page,
and seems to be related to the custom duration options. This
is tracked in cockroachdb#106395.

This commit removes the dependency on TimeScaleDropdownWithSearchParams
from the key visualizer, and replaces it with the vanilla TimeScaleDropdown.
The custom duration options are still present.

Informs: cockroachdb#106395
Epic: none
Release note: None
@zachlite zachlite mentioned this issue Jul 7, 2023
Merged
craig bot pushed a commit that referenced this issue Jul 7, 2023
@mgartner @yuzefovich
@zachlite @rafiss
Merge #103920 #106354 #106398 #106409
afe228e 
103920: opt: loosen restriction on UDF mutations to the same table r=mgartner a=mgartner

#### opt: loosen restriction on UDF mutations to the same table

To prevent index corruption described in #70731, optbuilder raises an
error when a statement performs multiple mutations to the same table.
This commit loosens this restriction for UDFs that perform mutations
because it is overly strict.

---

The index corruption described in #70731 occurs when a statement
performs multiple writes to the same table. Any reads performed by
successive writes see the snapshot of data as of the beginning of the
statement. They do not read values as of the most recent write within
the same statement. Because these successive writes are based on stale
data, they can write incorrect KVs and cause inconsistencies between
primary and secondary indexes.

Each statement in a UDF body is essentially a child of the statement
that is invoking the UDF. Mutations within UDFs are not as susceptible
to the inconsistencies described above because a UDF with a mutation
must be VOLATILE, and each statement in a VOLATILE UDFs reads at the
latest sequence number. In other words, statements within UDFs can see
previous writes made by any outer statement. This prevents
inconsistencies due to writes based on stale reads. Therefore, the
restriction that prevents multiple writes to the same table can be
lifted in some cases when the writes are performed in UDFs.

However, we cannot forgo restrictions for all writes in UDFs. A parent
statement that calls a UDF cannot be allowed to mutate the same table
that the UDF did. Unlike subsequent statements in the UDF after the
write, the parent statement will not see the UDF's writes, and
inconsistencies could occur.

To define acceptable mutations to the same table within UDFs, we define
a statement tree that represents the hierarchy of statements and
sub-statements in a query. A sub-statement `sub` is any statement within
a UDF. `sub`'s parent is the statement invoking the UDF. Other
statements in the same UDF as `sub` are the `sub`'s siblings. Any
statements in a UDF invoked by `sub` are `sub`'s children. For example,
consider:

    CREATE FUNCTION f1() RETURNS INT LANGUAGE SQL AS 'SELECT 1';
    CREATE FUNCTION f2() RETURNS INT LANGUAGE SQL AS 'SELECT 2 + f3()';
    CREATE FUNCTION f3() RETURNS INT LANGUAGE SQL AS 'SELECT 3';

    SELECT f1(), f2(), f3();

The statement tree for this SELECT would be:

    root: SELECT f1(), f2(), f3()
      ├── f1: SELECT 1
      ├── f2: SELECT 2 + f3()
      │    └── f3: SELECT 3
      └── f3: SELECT 3

We define multiple mutations to the same table as safe if, for every
possible path from the root statement to a leaf statement, either of the
following is true:

  1. There is no more than one mutation to any table.
  2. Or, any table with multiple mutations is modified only by simple
     INSERTs without ON CONFLICT clauses.

As a consequence of this definition, a UDF is now allowed to mutate the
same table as long as it does so in different statements in its body.
Such statements are siblings in the statement tree, and therefore do not
share any path from root to leaf. For example, this is now allowed:

    CREATE FUNCTION ups(a1 INT, a2 INT) RETURNS VOID LANGUAGE SQL AS $$
      UPSERT INTO a VALUES (a1);
      UPSERT INTO a VALUES (a2);
    $$

Similarly, successive invocations of the same UDF that mutates a table
are now allowed:

    CREATE FUNCTION upd(k0 INT, v0 INT) RETURNS VOID LANGUAGE SQL AS $$
      UPDATE kv SET v = v0 WHERE k = k0;
    $$;
    SELECT upd(1, 2), upd(1, 3);

The `statementTree` data structure has been added to enforce this
definition. See its documentation for more details.

Note: These restrictions will likely need to be revisited once we support
recursive UDFs.

Epic: CRDB-25388

Informs #70731

Release note: None


106354: sql: report contention on writes in EXPLAIN ANALYZE r=yuzefovich a=yuzefovich

This commit adds the contention events listener to `planNodeToRowSource` which allows us to add contention time information for mutation planNodes to be shown in EXPLAIN ANALYZE.

Fixes: #106266.

Release note (sql change): CockroachDB now reports contention time encountered while executing mutation statements (INSERT, UPSERT, UPDATE, DELETE) when run via EXPLAIN ANALYZE.

106398: ui: fix infinite re-render on the key visualizer page r=zachlite a=zachlite

Since #101258, the TimeScaleDropdownWithSearchParams can cause infinite re-renders. The exact cause of the bug is not yet diagnosed, but it occurs on the key visualizer page, and seems to be related to the custom duration options. This is tracked in #106395.

This commit removes the dependency on TimeScaleDropdownWithSearchParams from the key visualizer, and replaces it with the vanilla TimeScaleDropdown. The custom duration options are still present.

Informs: #106395
Epic: none
Release note: None

106409: workload: ignore QueryCanceled in random schema change test r=rafiss a=rafiss

fixes #105299

Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: zachlite <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
@blathers-crl blathers-crl bot mentioned this issue Jul 7, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue Jul 7, 2023
@zachlite
ui: fix infinite re-render on the key visualizer page
a41bef9 
Since #101258, the TimeScaleDropdownWithSearchParams
can cause infinite re-renders. The exact cause of the bug
is not yet diagnosed, but it occurs on the key visualizer page,
and seems to be related to the custom duration options. This
is tracked in #106395.

This commit removes the dependency on TimeScaleDropdownWithSearchParams
from the key visualizer, and replaces it with the vanilla TimeScaleDropdown.
The custom duration options are still present.

Informs: #106395
Epic: none
Release note: None
maryliag pushed a commit that referenced this issue Jul 24, 2023
@zachlite @maryliag
ui: fix infinite re-render on the key visualizer page
5c293db 
Since #101258, the TimeScaleDropdownWithSearchParams
can cause infinite re-renders. The exact cause of the bug
is not yet diagnosed, but it occurs on the key visualizer page,
and seems to be related to the custom duration options. This
is tracked in #106395.

This commit removes the dependency on TimeScaleDropdownWithSearchParams
from the key visualizer, and replaces it with the vanilla TimeScaleDropdown.
The custom duration options are still present.

Informs: #106395
Epic: none
Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@zachlite