release-23.2: ttljob: add hint to use PK in delete/select query

[blathers-crl]

Backport 1/1 commits from #118318 on behalf of @rafiss.

/cc @cockroachdb/release

---

This will help avoid choosing a plan that scans a secondary index, which can lead to many KV rows being scanned and also lead to contention.

This updates the query used for both SELECTs and DELETEs so that they
use the primary index.

informs #82140

Release note (bug fix): Fixed a bug that could cause DELETE queries sent by the row-level TTL job to use a secondary index rather than the primary index to find the rows to delete. This could lead to some DELETE operations taking a much longer time than they should. This bug was present since v22.2.0.

---

Release justification: bug fix

[blathers-crl]

Thanks for opening a backport.

Please check the backport criteria before merging:

• Backports should only be created for serious
  issues or test-only changes.
• Backports should not break backwards-compatibility.
• Backports should change as little code as possible.
• Backports should not change on-disk formats or node communication protocols.
• Backports should not add new functionality (except as defined
  here).
• Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
• All backports must be reviewed by the owning areas TL and one additional
  TL. For more information as to how that review should be conducted, please consult the backport
  policy.

If your backport adds new functionality, please ensure that the following additional criteria are satisfied:

• There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
• The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
• New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
• The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
• Your backport must be accompanied by a post to the appropriate Slack
  channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[yuzefovich]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @michae2)

[yuzefovich]

I do have a minor nit: IIUC this change makes it so that we apply the index hint to both DELETE and SELECT queries, right? The commit title is a bit misleading then.

[mgartner]

I do have a minor nit: IIUC this change makes it so that we apply the index hint to both DELETE and SELECT queries, right? The commit title is a bit misleading then.

I don't think the intent is to apply it to the SELECT. That should use a secondary index on the TTL column, correct @rafiss?

Are there any tests we should add for this?

[rafiss]

this was not intended to change the SELECT query. that change seems reasonable, but should be done separately to keep the impact of the backport minimal.

That should use a secondary index on the TTL column, correct

well, not quite - the code does not assume the presence or absence of a secondary index on the TTL column. the SELECT query is crafted by constraining the query to each span's start/end primary key and filtering additionally by the expiration expression. the SELECT query bounds are tested extensively here:

cockroach/pkg/sql/ttl/ttljob/ttljob_processor_test.go

Line 34 in 3a133ba

func TestSpanToQueryBounds(t *testing.T) {

These tests were recently updated to address #116845

[mgartner]

the SELECT query is crafted by constraining the query to each span's start/end primary key and filtering additionally by the expiration expression.

I guess I'm confused. How do we know what PKs are ready to expire without looking them up by the TTL column?

[rafiss]

I guess I'm confused. How do we know what PKs are ready to expire without looking them up by the TTL column?

for each span of the table, the job executes queries of the form:

SELECT {pk column names}
FROM table
AS OF SYSTEM TIME '-30s'
WHERE {ttl expiration expression} < {~now}
AND <pk column names> >= {span start key}
AND <pk column names < {span end key}

then it will run a DELETE query for the primary keys that are returned.

[yuzefovich]

But this PR modifies relationName used in both SELECT and DELETE queries, so now the SELECT query will be of the form

SELECT {pk column names}
FROM table@pkey
AS OF SYSTEM TIME '-30s'
WHERE {ttl expiration expression} < {~now}
AND <pk column names> >= {span start key}
AND <pk column names < {span end key}

(in addition to DELETE query using the PK too). I thought it was desired but based on Rafi's comments this is unexpected?

[rafiss]

oohh i see, i missed that this relationName is used in both SelectQueryParams and DeleteQueryParams. that was my mistake.

er well, i do think it's fine, but for the sake of a more minimal backport i can make it apply to only the DELETE. the initial intention when implementing TTL in crafting the SELECT and DELETE in this way was for it to always read from the primary index, so on the other hand, maybe i should just clarify the commit message as you originally suggested. wdyt?

[yuzefovich]

I think it makes sense that we'd apply the index hint to both SELECT and DELETE queries - indeed, the intention was always to fully specify PK values for both. So to me only adjusting the commit message seems good. Curious what Marcus thinks though.

[mgartner]

I think it makes sense that we'd apply the index hint to both SELECT and DELETE queries - indeed, the intention was always to fully specify PK values for both. So to me only adjusting the commit message seems good. Curious what Marcus thinks though.

SGTM.