sentry: conn_executor.go:639: panic while executing 1 statements: UPSERT INTO _(_, _) VALUES ($1, $2), (__more4000__): caused by <redacted> #32834
Comments
tbg
added
C-bug
|
Hi, I observed this crash. I was experimenting with CockroachDB in a Kubernetes cluster and was doing a bulk insert process when I saw two (out of three) CockroachDB pods had container restarts. Some context which may or may not be relevant: I had recently decreased the cluster size from 6 to 3 (which explains the failed connection attempts in the logs below), by scaling down the StatefulSet replicas one at a time and waiting for the Admin UI to show 0 under-replicated ranges before proceeding. I had also recently upgraded from 2.0.6 to 2.1.0 and then to 2.1.1. Please let me know if there's any additional context I could provide that might help. Here is the end of the log, for the one container:
And for the other:
|
|
@knz can you PTAL? |
|
I have the issue on the radar. Haven't found the root cause yet. |
knz
added
S-2-temp-unavailability
|
I've explored this a little more. I found that the crashes occur more frequently if I bucket the data by ranges of keys (not the CockroachDB concept of ranges specifically). In this case it's more likely that duplicate keys could appear in a single UPSERT statement, so it led me to suspect that this could be what is triggering the crash (I noticed that in 2.0.0, this was not even allowed and would produce an error). Once a crash occurred, if I set up the client to retry, then I found it would consistently crash each of the CockroachDB nodes in turn, with availability only being restored for a moment before the next crash occurs; I saw 50+ crashes in a row this way without it making progress, so it appeared it might be a deterministic behavior. I found that I can work around the issue by filtering the rows on the client side so that duplicate keys do not occur in the same statement. Since doing that, I have not observed the issue again. I also found that this example reproduces the crash every time: If I remove the "b bigint" from the CREATE TABLE statement, or if I modify the INSERT statement to include it, then the crash does not occur. So it appears it could have to do with this column implicitly being assigned a default value (of NULL). |
|
Wow what a good analysis. Thank you so much!!! this will help us tremendously. |
|
Ok, I've been digging into this, and there's a disconnect between the tableupserter's existingRows (only 2 values) the columns in the fetchColByID (expecting 3 values). I can get rid of the panic, but then we get hit by the difference. I'll keep digging into this. Also, as a side note, postgres does not allow inserting and upserting a column in the same statement. By eliminating this ability, it would make the upsert logic much simpler and perhaps more efficient. INSERT INTO test_table (id, a) VALUES
(1234, 456), (1234, 4564)
ON CONFLICT (id) DO UPDATE SET a = EXCLUDED.a;
ERROR: ON CONFLICT DO UPDATE command cannot affect row a second time
HINT: Ensure that no rows proposed for insertion within the same command have duplicate constrained values. |
|
CockroachDB used to give a similar error as Postgres. It appears that this pull request implemented the functionality which allows multiple changes to the same rows: #23698. I do find the new functionality to be quite useful and would see a lot of benefit in keeping it if we can, but I can see there are tradeoffs here. One thing is the implication that the order of the rows matters, which could result in nondeterministic behavior, if for instance the inserted rows are supplied using a SELECT subquery without an ORDER BY clause. Of course, Postgres has other features which are non-deterministic (for instance, the order in which sequence ids are generated, e.g., if we're inserting into a table with a SERIAL column). We would just want to be sure that in case of an UPSERT with duplicate keys if an ordering is supplied then it is applied correctly (for instance, if ORDER BY is used in the subquery, or if VALUES is used); I'm not familiar with CockroachDB's internals, so I don't have a clear idea of how easy or hard that is to ensure. |
|
@BramGruneir @BrentKerbySnap let us not get distracted from the bug at hand with a discussion on semantics. Such a discussion is important and will be forthcoming when we revamp our mutations for performance in 2.2 and later releases, but for now we need to fix this bug which affects the 2.1 release. |
There was an assumption that the row inserter and row updater had the same columns in the same order and if that wasn't the case row inserter's columns would be larger. This assumption is incorrect, so to fix it, when storing the existing rows (which are used to see if they conflicts with other rows further on in the statement) instead of storing the row used for inputs, convert the rows directly to those required by the row updater. Fixes cockroachdb#32834. Release note (bug fix): Fixed an issue in which if a nullable column wasn't supplied in an INSERT ON CONFLICT DO UPDATE statement, it may cause a panic.
|
Just wanted to leave a comment here. I've fixed the bug in #33245. When we rewrite our mutations, we'll have to consider this behaviour and decide if we want to keep it or not. I think we should try to keep it. |
|
@BramGruneir thank you for the contribution! We need to find some way to provide the functionality to users, but the base behavior of INSERT ON CONFLICT is further constrained in a way I hadn't envisioned, related to #28842. I will prepare some notes that explain this in detail. |
33245: sql: fix panic in insert on conflict do update r=BramGruneir a=BramGruneir There was an assumption that the row inserter and row updater had the same columns in the same order and if that wasn't the case row inserter's columns would be larger. This assumption is incorrect, so to fix it, when storing the existing rows (which are used to see if they conflicts with other rows further on in the statement) instead of storing the row used for inputs, convert the rows directly to those required by the row updater. Fixes #32834. Release note (bug fix): Fixed an issue in which if a nullable column wasn't supplied in an INSERT ON CONFLICT DO UPDATE statement, it may cause a panic. Co-authored-by: Bram Gruneir <[email protected]>
There was an assumption that the row inserter and row updater had the same columns in the same order and if that wasn't the case row inserter's columns would be larger. This assumption is incorrect, so to fix it, when storing the existing rows (which are used to see if they conflicts with other rows further on in the statement) instead of storing the row used for inputs, convert the rows directly to those required by the row updater. Fixes cockroachdb#32834. Release note (bug fix): Fixed an issue in which if a nullable column wasn't supplied in an INSERT ON CONFLICT DO UPDATE statement, it may cause a panic.
There was an assumption that the row inserter and row updater had the same columns in the same order and if that wasn't the case row inserter's columns would be larger. This assumption is incorrect, so to fix it, when storing the existing rows (which are used to see if they conflicts with other rows further on in the statement) instead of storing the row used for inputs, convert the rows directly to those required by the row updater. Fixes cockroachdb#32834. Release note (bug fix): Fixed an issue in which if a nullable column wasn't supplied in an INSERT ON CONFLICT DO UPDATE statement, it may cause a panic.
There was an assumption that the row inserter and row updater had the same columns in the same order and if that wasn't the case row inserter's columns would be larger. This assumption is incorrect, so to fix it, when storing the existing rows (which are used to see if they conflicts with other rows further on in the statement) instead of storing the row used for inputs, convert the rows directly to those required by the row updater. Fixes cockroachdb#32834. Release note (bug fix): Fixed an issue in which if a nullable column wasn't supplied in an INSERT ON CONFLICT DO UPDATE statement, it may cause a panic.
https://sentry.io/cockroach-labs/cockroachdb/issues/793275147/?referrer=webhooks_plugin
conn_executor.go:639: panic while executing 1 statements: UPSERT INTO (, _) VALUES ($1, $2), (more4000): caused by
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn.func1
The text was updated successfully, but these errors were encountered: