Skip to content

Support atomic rename when TRUNCATE is used with INTO OUTFILE#77181

Merged
pamarcos merged 8 commits intoClickHouse:masterfrom
onkar:onkar/outfile-truncate-70323
Mar 7, 2025
Merged

Support atomic rename when TRUNCATE is used with INTO OUTFILE#77181
pamarcos merged 8 commits intoClickHouse:masterfrom
onkar:onkar/outfile-truncate-70323

Conversation

@onkar
Copy link
Copy Markdown
Contributor

@onkar onkar commented Mar 5, 2025 •
edited
Loading

Changelog category (leave one):

  • Improvement

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

Support atomic rename when TRUNCATE is used with INTO OUTFILE. Resolves #70323.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@bharatnc bharatnc added the can be tested Allows running workflows for external contributors label Mar 5, 2025
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 5, 2025 •
edited
Loading

Workflow [PR], commit [73ef358]

@clickhouse-gh clickhouse-gh bot added the pr-improvement Pull request with some product improvements label Mar 5, 2025
@pamarcos pamarcos self-requested a review March 6, 2025 11:21
@pamarcos pamarcos self-assigned this Mar 6, 2025
pamarcos
pamarcos reviewed Mar 6, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@pamarcos pamarcos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great work! I think overall is en elegant solution that addresses the problem 🚀
I left some minor nitpick comments with what I found.

For the CHANGELOG entry I think we should write a shorter and more straightforward thing. Something more in line with the title of this PR is ok.

Also, please merge latest master against your branch, because we upgraded some submodules recently and testing on our local machines takes more time the more it deviates from master 🙏

@onkar
Copy link
Copy Markdown
Contributor Author

onkar commented Mar 7, 2025

@pamarcos thanks for the review. I have addressed comments and merged latest master. Only this test is failing but it is unrelated to the changes. Can you merge the PR if everything looks alright?

@pamarcos pamarcos added this pull request to the merge queue Mar 7, 2025
Merged via the queue into ClickHouse:master with commit 9612f6a Mar 7, 2025
122 of 125 checks passed
@onkar onkar deleted the onkar/outfile-truncate-70323 branch March 7, 2025 15:44
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 7, 2025
alexey-milovidov
alexey-milovidov reviewed May 27, 2025
View reviewed changes
src/Client/ClientBase.cpp

bool isSpecialFile(const String & path)
{
return path == "/dev/null" || path == "/dev/stdout" || path == "/dev/stderr";
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov May 27, 2025

Choose a reason for hiding this comment

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

This is wrong.

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented May 27, 2025

@pamarcos, this does not work at all:

ch "
SELECT number FROM numbers_mt(1000000) INTO OUTFILE 'numbers.txt.zst' TRUNCATE;
SELECT count() FROM file('numbers.txt.zst', 'LineAsString');
"

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented May 27, 2025

Great work! I think overall is en elegant solution that addresses the problem

No.

@pamarcos pamarcos mentioned this pull request May 28, 2025
Merged
1 task
@pamarcos
Copy link
Copy Markdown
Member

pamarcos commented May 28, 2025

@pamarcos, this does not work at all:

ch "
SELECT number FROM numbers_mt(1000000) INTO OUTFILE 'numbers.txt.zst' TRUNCATE;
SELECT count() FROM file('numbers.txt.zst', 'LineAsString');
"

Yep, sorry about that. I addressed the issue and simplified the code in #80979

@clickgapai
Copy link
Copy Markdown
Contributor

clickgapai commented Apr 3, 2026

Hi @onkar @pamarcos — while reviewing this PR I found the following:

Happy to discuss — close anything that's wrong or already addressed.

@clickgapai clickgapai mentioned this pull request Apr 3, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pamarcos pamarcos pamarcos left review comments

@alexey-milovidov alexey-milovidov alexey-milovidov left review comments

Assignees

@pamarcos pamarcos

Labels

can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The TRUNCATE mode for INTO OUTFILE should be atomic.

6 participants

@onkar @alexey-milovidov @pamarcos @clickgapai @bharatnc @robot-clickhouse-ci-1