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: row-level TTL behaves incorrectly with composite type primary key #116845

Closed
andyyang890 opened this issue Dec 20, 2023 · 4 comments · Fixed by #116988
Closed

sql: row-level TTL behaves incorrectly with composite type primary key #116845

andyyang890 opened this issue Dec 20, 2023 · 4 comments · Fixed by #116988
Assignees
Labels
A-row-level-ttl C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Originated from a customer P-0 Issues/test failures with a fix SLA of 2 weeks T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@andyyang890
Copy link
Collaborator

andyyang890 commented Dec 20, 2023

Row-level TTL uses raw KV scans on the target table's primary key span to generate the query bounds used for its SELECT query. However, it does not handle composite types such as collated strings correctly and generates incorrect query bounds.

Namely, (Key, Value) pairs for a row in a table that has a composite type primary key will have part of the composite type's value encoded in the Key (e.g. collation key for collated strings) and part of the value encoded in the Value (e.g. contents for collated strings). (See https://github.com/cockroachdb/cockroach/blob/509efb2e24ec768e5a6921afddb65d794dd9f5e1/docs/tech-notes/encoding.md#composite-encoding for more details.) The TTL code only considers the Key when converting the raw KVs into datums, leading to incorrect query bounds being generated, which can potentially result in rows being missed.

Code pointer

startKey := startKeyValues[0].Key
bounds.Start, err = rowenc.DecodeIndexKeyToDatums(codec, pkColTypes, pkColDirs, startKey, alloc)
if err != nil {
return bounds, false, errors.Wrapf(err, "decode startKey error key=%x", []byte(startKey))
}
endKey := endKeyValues[0].Key
bounds.End, err = rowenc.DecodeIndexKeyToDatums(codec, pkColTypes, pkColDirs, endKey, alloc)
if err != nil {
return bounds, false, errors.Wrapf(err, "decode endKey error key=%x", []byte(endKey))
}

Steps to reproduce

create table foo (
    id STRING(512) COLLATE en_US_u_ks_level2 NOT NULL,
    expired_at INT8 NOT NULL,
    CONSTRAINT "primary" PRIMARY KEY (id ASC)                     
);

alter table foo set (ttl_expiration_expression = '(expired_at / 1000)::INT::TIMESTAMPTZ', ttl_job_cron = '* * * * *'); 

insert into foo values ('a' COLLATE "en_us_u_ks_level2", 1702398972642);

The TTL job generates a SELECT query that looks like

SELECT id
FROM defaultdb.public.foo
AS OF SYSTEM TIME INTERVAL '-30 seconds'
WHERE (((expired_at / 1000)::INT::TIMESTAMPTZ) <= $1)
AND (
  (id >= $3)
)
AND (
  (id <= $2)
)
ORDER BY id ASC
LIMIT 500

where $2 and $3 are both e'\x15\xef\x00\x00\x00 ' COLLATE en_US_u_ks_level2 instead of 'a' COLLATE en_US_u_ks_level2. Of note, e'\x15\xef\x00\x00\x00 ' is the collation key for 'a' COLLATE en_US_u_ks_level2.

Screenshot 2023-12-19 at 9 23 20 PM

Jira issue: CRDB-34807

Epic CRDB-35306

@andyyang890 andyyang890 added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) A-row-level-ttl labels Dec 20, 2023
@blathers-crl blathers-crl bot added this to Triage in SQL Foundations Dec 20, 2023
@lunevalex lunevalex added the O-support Originated from a customer label Dec 20, 2023
@rafiss
Copy link
Collaborator

rafiss commented Dec 20, 2023

@DrewKimball pointed out that the rowfetcher code is one place to see an example of handling composite types:

// processKV processes the given key/value, setting values in the row
// accordingly. If debugStrings is true, returns pretty printed key and value
// information in prettyKey/prettyValue (otherwise they are empty strings).
func (rf *Fetcher) processKV(

@rafiss rafiss added the P-0 Issues/test failures with a fix SLA of 2 weeks label Dec 20, 2023
@DrewKimball
Copy link
Collaborator

Thought of another problem - the TTL code scans the first KV within the spans, but SQL rows (and even primary keys) can span multiple KVs because of column families. Here's an example where the composite value for the primary key is stored in the value of column family 1 (instead of 0):

root@localhost:26257/defaultdb> CREATE TABLE abc (a STRING COLLATE en_us_u_ks_level2 NOT NULL, b INT NOT NULL, c INT NOT NULL, PRIMARY KEY (a), FAMILY foo (b), FAMILY bar (a, c));
CREATE TABLE

Time: 42ms total (execution 42ms / network 0ms)

root@localhost:26257/defaultdb> INSERT INTO abc VALUES ('foo', 1, 2);
INSERT 0 1

Time: 39ms total (execution 39ms / network 0ms)

root@localhost:26257/defaultdb> EXPLAIN ANALYZE (VERBOSE) SELECT a, c FROM abc WHERE a = 'foo'::STRING COLLATE en_us_u_ks_level2;
                            info
-------------------------------------------------------------
  planning time: 456µs
  execution time: 993µs
  distribution: local
  vectorized: true
  rows decoded from KV: 1 (39 B, 1 KVs, 1 gRPC calls)
  cumulative time spent in KV: 760µs
  maximum memory usage: 20 KiB
  network usage: 0 B (0 messages)
  sql cpu time: 78µs
  isolation level: serializable
  priority: normal
  quality of service: regular

  • scan
    columns: (a, c)
    nodes: n1
    actual row count: 1
    vectorized batch count: 1
    KV time: 760µs
    KV contention time: 0µs
    KV rows decoded: 1
    KV pairs read: 1
    KV bytes read: 39 B
    KV gRPC calls: 1
    estimated max memory allocated: 20 KiB
    sql cpu time: 78µs
    MVCC step count (ext/int): 0/0
    MVCC seek count (ext/int): 1/1
    estimated row count: 1 (missing stats)
    table: abc@abc_pkey
    spans: /"\x16\x84\x17q\x17q\x00\x00\x00 \x00 \x00 "/1/1 -- Scanning column family 1 here!
(31 rows)

Time: 3ms total (execution 2ms / network 1ms)

@rafiss
Copy link
Collaborator

rafiss commented Dec 21, 2023

the TTL code scans the first KV within the spans, but SQL rows (and even primary keys) can span multiple KVs because of column families.

That sounds similar to the reason we made this change: 794aedd

@DrewKimball
Copy link
Collaborator

BTW, I think decimals are probably safe - you'd drop trailing zeroes and convert negative zeroes to positive zeroes, but that's fine for this application. Looks like floats and JSON also have composite encodings, but I think they're also safe to just use the decoded key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-row-level-ttl C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Originated from a customer P-0 Issues/test failures with a fix SLA of 2 weeks T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
Development

Successfully merging a pull request may close this issue.

4 participants