Skip to content

Save history on every query in clickhouse-client#11453

Merged
alexey-milovidov merged 5 commits intoClickHouse:masterfrom
kuskarov:fix_history_bugs
Jun 7, 2020
Merged

Save history on every query in clickhouse-client#11453
alexey-milovidov merged 5 commits intoClickHouse:masterfrom
kuskarov:fix_history_bugs

Conversation

@kuskarov
Copy link
Copy Markdown

@kuskarov kuskarov commented Jun 4, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Now history file is updated after each query and there is no race condition if multiple clients use one history file. This fixes #9897.

Detailed description / Documentation draft:

Now history file is updated after each query. There is no race condition if multiple clients use one history file because before writing to file flock() is called.

@blinkov blinkov added the pr-bugfix Pull request with bugfix, not backported by default label Jun 4, 2020
@alexey-milovidov alexey-milovidov added can be tested pr-improvement Pull request with some product improvements and removed pr-bugfix Pull request with bugfix, not backported by default labels Jun 4, 2020
@alexey-milovidov alexey-milovidov self-assigned this Jun 6, 2020
@alexey-milovidov
Copy link
Copy Markdown
Member

TODO: Replace strerror to strerror_r.

@alexey-milovidov
Copy link
Copy Markdown
Member

TODO: Add a test with expect and two concurrent clients.

@alexey-milovidov alexey-milovidov changed the title Fix history bug FIX#9897 Save history on every query in clickhouse-client Jun 7, 2020
alexey-milovidov added a commit that referenced this pull request Jun 7, 2020
Merging #11453 (save history in clickhouse-client after every query)
@alexey-milovidov alexey-milovidov merged commit 85aee18 into ClickHouse:master Jun 7, 2020
{
// locking history file to prevent from inconsistent concurrent changes
bool locked = false;
if (flock(history_file_fd, LOCK_EX))
Copy link
Copy Markdown
Member

@azat azat Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually replxx already has file locking 1, but only for history_save.
Should the client be that safe? I.e. care about race during history_load (looks a little bit overkill for this)

Footnotes

  1. https://github.com/AmokHuginnsson/replxx/blob/d4baaf1d0cdd5ff865d18c8c62d6ca994a80d629/src/history.cxx#L123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-docs-needed pr-improvement Pull request with some product improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

history file not written if clickhouse-client was stopped by Ctrl+C

5 participants