Send DISCARD ALL even if client is not in transaction #152
Conversation
| // We don't want `SET application_name` to mark the server connection | ||
| // as needing cleanup | ||
| let needs_cleanup_before = self.needs_cleanup; | ||
|
|
||
| let result = Ok(self | ||
| .query(&format!("SET application_name = '{}'", name)) | ||
| .await?) | ||
| .await?); | ||
| self.needs_cleanup = needs_cleanup_before; | ||
| return result; |
There was a problem hiding this comment.
I added a test to verify that we are not sending DISCARD ALL after each checkin but it failed because of set_name calls. They send server.query("SET application_name") call pretty much before each checkin which marks the connection as needing DISCARD ALL. This workaround avoids this.
| @@ -440,6 +444,24 @@ impl Server { | |||
| break; | |||
| } | |||
|
|
|||
| // CommandComplete | |||
| 'C' => { | |||
There was a problem hiding this comment.
Credit to @zainkabani for the idea.
src/server.rs
Outdated
| 'C' => { | ||
| let full_message = String::from_utf8_lossy(message.as_ref()); | ||
| let mut it = full_message.split_ascii_whitespace(); | ||
| let command_tag = it.next().unwrap().trim_end_matches(char::from(0)); |
There was a problem hiding this comment.
I would be careful with unwrap() here, we don't want to panic the thread. Maybe do a match? Could also do unwrap_or("") (empty string).
src/server.rs
Outdated
| self.needs_cleanup = false; | ||
| } | ||
|
|
||
| self.set_name("pgcat").await?; |
There was a problem hiding this comment.
I would skip that one tbh, 9 times out of 10 the same application will be back to use that connection. Saves one query on checkin, might be worth it? IIRC DISCARD ALL takes care of this too, so in the broken case, this is still taken care of.
…ient-start Set application name on client start
src/server.rs
Outdated
| // CommandComplete | ||
| 'C' => { | ||
| let mut command_tag = String::new(); | ||
| message.reader().read_to_string(&mut command_tag).unwrap(); |
There was a problem hiding this comment.
One last unwrap to match :)
|
Is our CI flaking? |
|
Yes. I will take a look at it. It flaked on both PRs |
|
Do you want me to merge this one? It looks good to me. |
|
I am ready when you are @levkk |
We are not sending
DISCARD ALLin all the situations that we should. This can leak session-specific settings/resources between clients.This is currently easily reproducible by repeatedly running
PREPARE prepared_q (int) AS SELECT $1;in psql until an error emerges.I created a test to reproduce this issue and verified the fix. I am open to changing the name of the method.