Add AlreadyRevoked handling to revokedCertificates table inserts - #8408
Add AlreadyRevoked handling to revokedCertificates table inserts#8408aarongable wants to merge 6 commits into
Conversation
jsha
left a comment
There was a problem hiding this comment.
Sorry for the delay. I had a pending comment in here I thought I'd already sent.
e1c002b to
d444864
Compare
|
I've updated this PR to make the serials index unique, and then rely on detecting duplicate inserts, like the first draft. I've updated IN-11835 to indicate that the new index should be unique. I think that including the index update in this PR, rather than as a predecessor PR that must be deployed first, is fine because the inserts into this table are still actually protected by the updates to the |
|
@aarongable IN-11835 sat lingering forever in my backlog, but has been deployed to staging and prod successfully. @danjeffery It was replication safe on mixed binlog replication meaning that replication wasn't unexpectedly stopped and no replicas needed to be recovered. However, you will have a replication pause as the table copy without partitioning is blasted across the network unless you do some migration tricks like dropping a node out of the cluster, doing dropping the partition, rinse repeat. Now our prod table as ~2GB and ~12M rows so that happened fast ~2 minutes. If you aren't using partition then don't even worry about that. The inplace index add took ~30 seconds. Tl;dr consider your error budget |
Within
sa.addRevokedCertificate, detect if the insert failed due to a duplicate primary key, and returnberrors.AlreadyRevokedin that case. This matches the behavior ofsa.RevokeCertificatewhen an update to thecertificateStatustable modifies no rows.This change has no immediate impact, because addRevokedCertificate is only called after RevokeCertificate's update to the certificateStatus table has succeeded: if the first operation returns AlreadyRevoked, this code is never reached. However, we plan to remove the certificateStatus table in the future, so this code will need to return AlreadyRevoked when appropriate.
Part of #8322
Warning
Do not merge before #8427Do not merge before IN-11835 is complete
Do not merge before #8884 has been reverted (and that revert deployed)