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.
In https://github.com/letsencrypt/boulder/pull/8740/changes#r3203832749 we decided to use
INSERT IGNOREto insert serials inAddSerialsToIncident. @beautifulentropy pointed out in a comment that this is risky:INSERT IGNOREwill ignore a variety of problems, like required columns that were not set:INSERT IGNORE:
I figured, no problem, those warnings will be treated as fatal since we set
sql_mode='STRICT_ALL_TABLES'. But no!INSERT IGNOREactually overrides Strict SQL Mode:To me, that says we should avoid
INSERT IGNOREeverywhere 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.