sa: revisit INSERT IGNORE

Open
#8,749 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
35/100
Issue type
Refactor
Clarity
Needs clarification
Activity status
Quiet
Tech stack
go, mariadb, sql
Domain
backend, databases

Research direction

Start by locating AddSerialsToIncident and searching the codebase for INSERT IGNORE uses. Investigate the proposed ON DUPLICATE KEY UPDATE approach, including which locks it takes and the likelihood of deadlocks. Done means selecting and applying a safer insertion strategy consistently, with evidence that strict SQL behavior is preserved.

Written by the indexing model from the issue text.

Description

In https://github.com/letsencrypt/boulder/pull/8740/changes#r3203832749 we decided to use INSERT IGNORE to insert serials in AddSerialsToIncident. @beautifulentropy pointed out in a comment that this is risky: INSERT IGNORE will ignore a variety of problems, like required columns that were not set:

INSERT IGNORE:

By using the IGNORE keyword all errors are converted to warnings, which will not stop inserts of additional rows.

Invalid values are changed to the closest valid value and inserted, with a warning produced.

I figured, no problem, those warnings will be treated as fatal since we set sql_mode='STRICT_ALL_TABLES'. But no! INSERT IGNORE actually overrides Strict SQL Mode:

If strict mode is not in effect, MySQL inserts adjusted values for invalid or missing values and produces warnings (see Section 15.7.7.43, “SHOW WARNINGS Statement”). In strict mode, you can produce this behavior by using INSERT IGNORE or UPDATE IGNORE.

To me, that says we should avoid INSERT IGNORE everywhere in our codebase since it overrides our intent to be strict about warnings.

We also talked about not wanting to use INSERT ... ON DUPLICATE KEY UPDATE because of next-key locks possibly causing deadlocks. I think it's worth digging a little deeper on which locks exactly get taken and how likely they are to be hit. Since this path is solely hit by admins batch-inserting serials, I think deadlocks are moderately unlikely, and we can probably recover from them without too much trouble.

Dominant language
Go
Stars
5.8k
Forks
649
Avg merge
3d 23h
Merged PRs (30d)
24

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from letsencrypt/boulder

All issues in letsencrypt/boulder

Similar issues

More Go issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.