Merge pull request #11493 from ClickHouse/kuskarov-fix_history_bugs

Merging #11453 (save history in clickhouse-client after every query)

base/common/CMakeLists.txt

@@ -16,6 +16,7 @@ set (SRCS
     shift10.cpp
     sleep.cpp
     terminalColors.cpp
+    errnoToString.cpp
 )
 
 if (ENABLE_REPLXX)

base/common/ReplxxLineReader.cpp

 #include <common/ReplxxLineReader.h>
+#include <common/errnoToString.h>
 
 #include <errno.h>
 #include <string.h>
 #include <unistd.h>
 #include <functional>
+#include <sys/file.h>
 
 namespace
 {
@@ -24,7 +26,29 @@ ReplxxLineReader::ReplxxLineReader(
     using Replxx = replxx::Replxx;
 
     if (!history_file_path.empty())
-        rx.history_load(history_file_path);
+    {
+        history_file_fd = open(history_file_path.c_str(), O_RDWR);
+        if (history_file_fd < 0)
+        {
+            rx.print("Open of history file failed: %s\n", errnoToString(errno).c_str());
+        }
+        else
+        {
+            if (flock(history_file_fd, LOCK_SH))
+            {
+                rx.print("Shared lock of history file failed: %s\n", errnoToString(errno).c_str());
+            }
+            else
+            {
+                rx.history_load(history_file_path);
+
+                if (flock(history_file_fd, LOCK_UN))
+                {
+                    rx.print("Unlock of history file failed: %s\n", errnoToString(errno).c_str());
+                }
+            }
+        }
+    }
 
     auto callback = [&suggest] (const String & context, size_t context_size)
     {
@@ -49,8 +73,8 @@ ReplxxLineReader::ReplxxLineReader(
 
 ReplxxLineReader::~ReplxxLineReader()
 {
-    if (!history_file_path.empty())
-        rx.history_save(history_file_path);
+    if (close(history_file_fd))
+        rx.print("Close of history file failed: %s\n", strerror(errno));
 }
 
 LineReader::InputStatus ReplxxLineReader::readOneLine(const String & prompt)
@@ -68,7 +92,20 @@ LineReader::InputStatus ReplxxLineReader::readOneLine(const String & prompt)
 
 void ReplxxLineReader::addToHistory(const String & line)
 {
+    // locking history file to prevent from inconsistent concurrent changes
+    bool locked = false;
+    if (flock(history_file_fd, LOCK_EX))
+        rx.print("Lock of history file failed: %s\n", strerror(errno));
+    else
+        locked = true;
+
     rx.history_add(line);
+
+    // flush changes to the disk
+    rx.history_save(history_file_path);
+
+    if (locked && 0 != flock(history_file_fd, LOCK_UN))
+        rx.print("Unlock of history file failed: %s\n", strerror(errno));
 }
 
 void ReplxxLineReader::enableBracketedPaste()

base/common/ReplxxLineReader.h

@@ -17,4 +17,7 @@ private:
     void addToHistory(const String & line) override;
 
     replxx::Replxx rx;
+
+    // used to call flock() to synchronize multiple clients using same history file
+    int history_file_fd = -1;
 };

base/common/errnoToString.cpp

+#include "errnoToString.h"
+
+#include <fmt/format.h>
+
+
+std::string errnoToString(int code, int the_errno)
+{
+    const size_t buf_size = 128;
+    char buf[buf_size];
+#ifndef _GNU_SOURCE
+    int rc = strerror_r(the_errno, buf, buf_size);
+#ifdef __APPLE__
+    if (rc != 0 && rc != EINVAL)
+#else
+    if (rc != 0)
+#endif
+    {
+        std::string tmp = std::to_string(code);
+        const char * code_str = tmp.c_str();
+        const char * unknown_message = "Unknown error ";
+        strcpy(buf, unknown_message);
+        strcpy(buf + strlen(unknown_message), code_str);
+    }
+    return fmt::format("errno: {}, strerror: {}", the_errno, buf);
+#else
+    (void)code;
+    return fmt::format("errno: {}, strerror: {}", the_errno, strerror_r(the_errno, buf, sizeof(buf)));
+#endif
+}

base/common/errnoToString.h

+#pragma once
+
+#include <cerrno>
+#include <string>
+
+std::string errnoToString(int code, int the_errno = errno);

base/common/ya.make

@@ -47,6 +47,7 @@ SRCS(
     shift10.cpp
     sleep.cpp
     terminalColors.cpp
+    errnoToString.cpp
 )
 
 END()

programs/server/Server.cpp

@@ -17,6 +17,7 @@
 #include <common/phdr_cache.h>
 #include <common/ErrorHandlers.h>
 #include <common/getMemoryAmount.h>
+#include <common/errnoToString.h>
 #include <common/coverage.h>
 #include <Common/ClickHouseRevision.h>
 #include <Common/DNSResolver.h>

src/Common/Exception.cpp

@@ -10,6 +10,7 @@
 #include <IO/ReadBufferFromString.h>
 #include <IO/ReadBufferFromFile.h>
 #include <common/demangle.h>
+#include <common/errnoToString.h>
 #include <Common/formatReadable.h>
 #include <Common/filesystemHelpers.h>
 #include <filesystem>
@@ -85,31 +86,6 @@ std::string Exception::getStackTraceString() const
 }
 
 
-std::string errnoToString(int code, int the_errno)
-{
-    const size_t buf_size = 128;
-    char buf[buf_size];
-#ifndef _GNU_SOURCE
-    int rc = strerror_r(the_errno, buf, buf_size);
-#ifdef __APPLE__
-    if (rc != 0 && rc != EINVAL)
-#else
-    if (rc != 0)
-#endif
-    {
-        std::string tmp = std::to_string(code);
-        const char * code_str = tmp.c_str();
-        const char * unknown_message = "Unknown error ";
-        strcpy(buf, unknown_message);
-        strcpy(buf + strlen(unknown_message), code_str);
-    }
-    return "errno: " + toString(the_errno) + ", strerror: " + std::string(buf);
-#else
-    (void)code;
-    return "errno: " + toString(the_errno) + ", strerror: " + std::string(strerror_r(the_errno, buf, sizeof(buf)));
-#endif
-}
-
 void throwFromErrno(const std::string & s, int code, int the_errno)
 {
     throw ErrnoException(s + ", " + errnoToString(code, the_errno), code, the_errno);

src/Common/Exception.h

@@ -81,7 +81,6 @@ private:
 using Exceptions = std::vector<std::exception_ptr>;
 
 
-std::string errnoToString(int code, int the_errno = errno);
 [[noreturn]] void throwFromErrno(const std::string & s, int code, int the_errno = errno);
 /// Useful to produce some extra information about available space and inodes on device
 [[noreturn]] void throwFromErrnoWithPath(const std::string & s, const std::string & path, int code,

src/Common/PipeFDs.cpp

@@ -3,6 +3,7 @@
 #include <Common/formatReadable.h>
 
 #include <common/logger_useful.h>
+#include <common/errnoToString.h>
 
 #include <unistd.h>
 #include <fcntl.h>

src/Common/QueryProfiler.cpp

@@ -8,6 +8,7 @@
 #include <common/StringRef.h>
 #include <common/logger_useful.h>
 #include <common/phdr_cache.h>
+#include <common/errnoToString.h>
 
 #include <random>
 

src/Common/ShellCommand.cpp

@@ -6,6 +6,7 @@
 #include <Common/ShellCommand.h>
 #include <Common/PipeFDs.h>
 #include <common/logger_useful.h>
+#include <common/errnoToString.h>
 #include <IO/WriteHelpers.h>
 #include <unistd.h>
 #include <csignal>

src/Common/StatusFile.cpp

@@ -8,6 +8,7 @@
 
 #include <Poco/File.h>
 #include <common/logger_useful.h>
+#include <common/errnoToString.h>
 #include <Common/ClickHouseRevision.h>
 #include <common/LocalDateTime.h>
 

src/Interpreters/ThreadStatusExt.cpp

@@ -8,6 +8,7 @@
 #include <Common/QueryProfiler.h>
 #include <Common/ThreadProfileEvents.h>
 #include <Common/TraceCollector.h>
+#include <common/errnoToString.h>
 
 #if defined(OS_LINUX)
 #   include <Common/hasLinuxCapability.h>

tests/queries/0_stateless/01300_client_save_history_when_terminated.sh

+#!/usr/bin/expect -f
+
+log_user 0
+set timeout 60
+match_max 100000
+
+spawn clickhouse-client
+expect ":) "
+
+# Make a query
+send -- "SELECT 'for the history'\r"
+expect "for the history"
+expect ":) "
+
+# Kill the client to check if the history was saved
+exec kill -9 [exp_pid]
+close
+
+# Run client one more time and press "up" to see the last recorded query
+spawn clickhouse-client
+expect ":) "
+send -- "\[A"
+expect "SELECT 'for the history'"
+
+# Will check that Ctrl+C clears current line.
+send -- "\3"
+expect ":)"
+
+# Will check that second Ctrl+C invocation does not exit from client.
+send -- "\3"
+expect ":)"
+
+# But Ctrl+D does.
+send -- "\4"
+expect eof