From 703716228ec2a37e0b0a0da21aa8949a8e2e0fd3 Mon Sep 17 00:00:00 2001
From: Dmitry Ivanov <ivadmi5@gmail.com>
Date: Mon, 24 Jan 2022 18:09:08 +0300
Subject: [PATCH] Use `&str` instead of `String` in `BeMessage::ErrorResponse`

There's no need in allocating string literals in the heap.
---
 proxy/src/mgmt.rs                    |  2 +-
 proxy/src/proxy.rs                   |  2 +-
 zenith_utils/src/postgres_backend.rs | 17 ++++++-----------
 zenith_utils/src/pq_proto.rs         |  2 +-
 4 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/proxy/src/mgmt.rs b/proxy/src/mgmt.rs
index 1b9d9502f2..9d8dc5130f 100644
--- a/proxy/src/mgmt.rs
+++ b/proxy/src/mgmt.rs
@@ -111,7 +111,7 @@ fn try_process_query(
                 .write_message(&BeMessage::CommandComplete(b"SELECT 1"))?;
         }
         Err(e) => {
-            pgb.write_message(&BeMessage::ErrorResponse(e.to_string()))?;
+            pgb.write_message(&BeMessage::ErrorResponse(&e.to_string()))?;
         }
     }
 
diff --git a/proxy/src/proxy.rs b/proxy/src/proxy.rs
index 5a64dc8081..1013a5c39e 100644
--- a/proxy/src/proxy.rs
+++ b/proxy/src/proxy.rs
@@ -152,7 +152,7 @@ impl ProxyConnection {
             Ok(None) => return Ok(None),
             Err(e) => {
                 // Report the error to the client
-                self.pgb.write_message(&Be::ErrorResponse(e.to_string()))?;
+                self.pgb.write_message(&Be::ErrorResponse(&e.to_string()))?;
                 bail!("failed to handle client: {:?}", e);
             }
         };
diff --git a/zenith_utils/src/postgres_backend.rs b/zenith_utils/src/postgres_backend.rs
index ad69d629b7..2348f43d2f 100644
--- a/zenith_utils/src/postgres_backend.rs
+++ b/zenith_utils/src/postgres_backend.rs
@@ -354,9 +354,7 @@ impl PostgresBackend {
                     }
                     FeStartupPacket::StartupMessage { .. } => {
                         if have_tls && !matches!(self.state, ProtoState::Encrypted) {
-                            self.write_message(&BeMessage::ErrorResponse(
-                                "must connect with TLS".to_string(),
-                            ))?;
+                            self.write_message(&BeMessage::ErrorResponse("must connect with TLS"))?;
                             bail!("client did not connect with TLS");
                         }
 
@@ -402,7 +400,7 @@ impl PostgresBackend {
                         let (_, md5_response) = m.split_last().context("protocol violation")?;
 
                         if let Err(e) = handler.check_auth_md5(self, md5_response) {
-                            self.write_message(&BeMessage::ErrorResponse(format!("{}", e)))?;
+                            self.write_message(&BeMessage::ErrorResponse(&e.to_string()))?;
                             bail!("auth failed: {}", e);
                         }
                     }
@@ -410,7 +408,7 @@ impl PostgresBackend {
                         let (_, jwt_response) = m.split_last().context("protocol violation")?;
 
                         if let Err(e) = handler.check_auth_jwt(self, jwt_response) {
-                            self.write_message(&BeMessage::ErrorResponse(format!("{}", e)))?;
+                            self.write_message(&BeMessage::ErrorResponse(&e.to_string()))?;
                             bail!("auth failed: {}", e);
                         }
                     }
@@ -428,17 +426,16 @@ impl PostgresBackend {
                 trace!("got query {:?}", query_string);
                 // xxx distinguish fatal and recoverable errors?
                 if let Err(e) = handler.process_query(self, query_string) {
-                    let errmsg = format!("{}", e);
                     // ":#" uses the alternate formatting style, which makes anyhow display the
                     // full cause of the error, not just the top-level context. We don't want to
                     // send that in the ErrorResponse though, because it's not relevant to the
                     // compute node logs.
                     warn!("query handler for {} failed: {:#}", query_string, e);
+                    self.write_message_noflush(&BeMessage::ErrorResponse(&e.to_string()))?;
+                    // TODO: untangle convoluted control flow
                     if e.to_string().contains("failed to run") {
-                        self.write_message_noflush(&BeMessage::ErrorResponse(errmsg))?;
                         return Ok(ProcessMsgResult::Break);
                     }
-                    self.write_message_noflush(&BeMessage::ErrorResponse(errmsg))?;
                 }
                 self.write_message(&BeMessage::ReadyForQuery)?;
             }
@@ -466,10 +463,8 @@ impl PostgresBackend {
                 trace!("got execute {:?}", query_string);
                 // xxx distinguish fatal and recoverable errors?
                 if let Err(e) = handler.process_query(self, query_string) {
-                    let errmsg = format!("{}", e);
-
                     warn!("query handler for {:?} failed: {:#}", query_string, e);
-                    self.write_message(&BeMessage::ErrorResponse(errmsg))?;
+                    self.write_message(&BeMessage::ErrorResponse(&e.to_string()))?;
                 }
                 // NOTE there is no ReadyForQuery message. This handler is used
                 // for basebackup and it uses CopyOut which doesnt require
diff --git a/zenith_utils/src/pq_proto.rs b/zenith_utils/src/pq_proto.rs
index b8b92444ac..f3365fa4c5 100644
--- a/zenith_utils/src/pq_proto.rs
+++ b/zenith_utils/src/pq_proto.rs
@@ -367,7 +367,7 @@ pub enum BeMessage<'a> {
     CloseComplete,
     // None means column is NULL
     DataRow(&'a [Option<&'a [u8]>]),
-    ErrorResponse(String),
+    ErrorResponse(&'a str),
     // single byte - used in response to SSLRequest/GSSENCRequest
     EncryptionResponse(bool),
     NoData,