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

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

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
Copy link
Copy Markdown
Member

@pamarcos pamarcos left a comment

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

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

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

@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

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

No.

@pamarcos
Copy link
Copy Markdown
Member

@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

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

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

Development

Successfully merging this pull request may close these issues.

The TRUNCATE mode for INTO OUTFILE should be atomic.

5 participants