sql: support index hints for insert and upsert

[rytaft]

Fixes #98211

Release note (sql change): Added support for index hints with INSERT and UPSERT statements. This allows INSERT ... ON CONFLICT and UPSERT queries to use index hints in the same way they are already supported for UPDATE and DELETE statements.

[cockroach-teamcity]

This change is 

[DrewKimball]

Nice!

My only reservation is that it seems more difficult to determine whether a set of index hints will be valid for INSERT and UPSERT than for UPDATE and DELETE, since this involves more than just a straightforward scan with a WHERE clause. It's easy to determine whether a set of hints is compatible with the current schema by just running EXPLAIN for the query, but how does the user determine whether a schema change will invalidate existing hints without actually running the schema change?

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

[rytaft]

TFTR! Not sure I understand the concerns:

My only reservation is that it seems more difficult to determine whether a set of index hints will be valid for INSERT and UPSERT than for UPDATE and DELETE, since this involves more than just a straightforward scan with a WHERE clause

Difficult in the code? Or for the user? Not sure I understand.

how does the user determine whether a schema change will invalidate existing hints without actually running the schema change?

I don't think we have a good way to do this, but this has already been a problem for all types of hints (including for SELECT statements), right?

Maybe you can give me an example of a potential problem here?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

[rytaft]

(Also note that the primary motivation for this change at the moment is to be able to use the NO_FULL_SCAN hint, which doesn't depend on any specific index)

[DrewKimball]

Difficult in the code? Or for the user? Not sure I understand.

I mean for the user - for UPDATE and DELETE, there's predictably a scan over the table with the filters specified in the WHERE clause. Whereas for INSERT and UPSERT, it's hard to tell what scans/joins will be planned without looking at the explain output.

I don't think we have a good way to do this, but this has already been a problem for all types of hints (including for SELECT statements), right?

You're right, I just thought it might be more of an issue here since it's harder to tell when a query is "sensitive" to a schema change.

I'm totally happy merging this as-is, just wanted to bring it up in case you had any ideas.

[rytaft]

I see, thanks. Yea, I don't have any great ideas. Maybe it would be useful to have a feature in the DBConsole to see all indexes that have been hinted in the last X hours? And as far as helping the user know which scan(s) will be affected by a hint, we might just want to suggest in the docs that they use EXPLAIN.

TFTR!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 5a5739d to blathers/backport-release-23.1-119104: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

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

[mgartner]

Cool! I agree that it's a bit confusing what can happen when you specify a specific index for an insert or upsert. You may have 3 arbiter indexes selected, but because the hint propagates into the uniqueness checks (the inlined ones, not the post-query ones†) you won't actually use all 3 indexes. But I think this is ok because it's consistent with our hints in general—they specify a physical requirement (perform a scan index i) rather than a logical one (catch a conflict of the logical uniqueness property defined by arbiter i). This might be something we want to clarify in the docs.

† Does the hint also propagate to post-query uniqueness checks and FK checks? This might be something to add tests for.

[rytaft]

TFTR!

Does the hint also propagate to post-query uniqueness checks and FK checks?

For foreign key checks, we are only scanning the referenced or referencing table that is not updated / upserted / deleted, so there's no way the hint could propagate (but I also verified manually just to be sure). I opened #119597 with a few tests showing that the hints also do not propagate to uniqueness checks.