base: ensure that a SQL instance ID is not set to conflicting value#72608
base: ensure that a SQL instance ID is not set to conflicting value#72608craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
rimadeodhar
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @knz and @RaduBerinde)
Quoted 5 lines of code…
Prior to this patch, a SQL instance ID container could be assigned successively different values. This would make it possible for erroneous code to mistakenly initialize the instance ID twice, without any evidence that something was amiss.
Interesting! What sequence of events can cause this to happen and how did we detect this?
pkg/base/node_id.go, line 223 at r2 (raw file):
} c.sqlInstanceID = sqlInstanceID
Nit: We can delete this, right? Seems like we are already setting c.sqlInstanceID at
oldVal := atomic.SwapInt32((*int32)(&c.sqlInstanceID), int32(sqlInstanceID)).
Prior to this patch, the SQL instance ID was not reported in log messages, which made it hard to distinguish log messages coming from different in-memory SQL instance servers (e.g. in tests). This patch fixes it. The patch also changes the log tag for SQL instances from just `sql` to `sqli`, which is easier to search for and recognize. Release note: None
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
What sequence of events can cause this
A bug in the code.
how did we detect this?
When it happened for node IDs, we detected it by being confused about why the same log file would report different node IDs in different log messages. Then we added the assertion and saw a bunch of unit tests fail.
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/base/node_id.go, line 223 at r2 (raw file):
Previously, rimadeodhar (Rima Deodhar) wrote…
Nit: We can delete this, right? Seems like we are already setting c.sqlInstanceID at
oldVal := atomic.SwapInt32((*int32)(&c.sqlInstanceID), int32(sqlInstanceID)).
Oops yes you're right. fixed.
Prior to this patch, a SQL instance ID container could be assigned successively different values. This would make it possible for erroneous code to mistakenly initialize the instance ID twice, without any evidence that something was amiss. This patch fixes this by using the same prevention logic that we already use for node IDs. Release note: None
c5a4716 to
5196152
Compare
|
TFYR! bors r=rimadeodhar |
|
Build succeeded: |
Informs #58938.
First commit from #72607.
Prior to this patch, a SQL instance ID container could be
assigned successively different values. This would make it possible
for erroneous code to mistakenly initialize the instance ID twice,
without any evidence that something was amiss.
This patch fixes this by using the same prevention logic that we
already use for node IDs.