Skip to content

Fix 0 rows in set on INSERT ... SELECT#79462

Merged
rienath merged 9 commits intoClickHouse:masterfrom
aaaengel:issue_47800
Nov 7, 2025
Merged

Fix 0 rows in set on INSERT ... SELECT#79462
rienath merged 9 commits intoClickHouse:masterfrom
aaaengel:issue_47800

Conversation

@aaaengel
Copy link
Copy Markdown
Contributor

@aaaengel aaaengel commented Apr 23, 2025 •
edited by clickhouse-gh bot
Loading

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Fixed 0 rows in set appearance after INSERT INTO ... SELECT query. Closes #47800

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 23, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@jsc0218 jsc0218 added the can be tested Allows running workflows for external contributors label Apr 23, 2025
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Apr 23, 2025 •
edited
Loading

Workflow [PR], commit [7798c8a]

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Apr 23, 2025
@jsc0218 jsc0218 self-assigned this Apr 23, 2025
@aaaengel
Copy link
Copy Markdown
Contributor Author

aaaengel commented Apr 23, 2025

Oh, client requires --processed-rows flag to pass my test. Otherwise I have no idea, how to get number of processed rows from batch mode clickhouse client.

@jsc0218
Copy link
Copy Markdown
Contributor

jsc0218 commented Apr 23, 2025

Maybe try this?

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented May 18, 2025

It's pointless to use expect with this test, a usual .sh test should be good enough.

@rienath rienath changed the title Fixed '0 rows in set' after INSERT INTO Fix 0 rows in set on INSERT SELECT Oct 10, 2025
rienath
rienath reviewed Oct 10, 2025
View reviewed changes
@rienath rienath self-assigned this Oct 10, 2025
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Oct 10, 2025 •
edited
Loading

Workflow [PR], commit [43c1ad5]

Summary: ❌

job_name test_name status info comment
Stateless tests (amd_asan, distributed plan, sequential) failure
03008_deduplication_mv_generates_several_blocks_nonreplicated FAIL cidb
Upgrade check (amd_msan) failure
Killed by signal (in clickhouse-server.log) FAIL cidb
Fatal message in clickhouse-server.log (see fatal_messages.txt) FAIL cidb
Killed by signal (output files) FAIL cidb
Found signal in gdb.log FAIL cidb

@rienath
Copy link
Copy Markdown
Member

rienath commented Oct 10, 2025

Since I changed all the lines in this PR, maybe we need one more review. @CheSema, could you help?

@CheSema CheSema self-assigned this Oct 10, 2025
CheSema
CheSema reviewed Oct 10, 2025
View reviewed changes
src/Client/ClientBase.cpp Outdated

void ClientBase::onProgress(const Progress & value)
{
processed_rows += value.written_rows;
Copy link
Copy Markdown
Member

@CheSema CheSema Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ClientBase::processed_rows counts read OR written bytes which client knows. Usually client accounts inserted and selected rows. But indeed client does not see stats from insert-select query.

Progress packet is optional. Server does not obligated to send it. But that packet is allowed to be send on insert and select queries. I suspect in that case you would double count.

Also server could send several packages with progress and it contains absolute nubmers, in that case you would count a square.

But this all have to be validated. I might be wrong here.

@rienath rienath changed the title Fix 0 rows in set on INSERT SELECT Fix 0 rows in set on INSERT ... SELECT Nov 6, 2025
rienath
rienath reviewed Nov 7, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@rienath rienath left a comment

Choose a reason for hiding this comment

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

@CheSema I have separated the processed_rows into block and progress tracking to prevent double counting scenario that you described. I don't think the square scenario is possible because sendProgress() calls fetchValuesAndResetPiecewiseAtomically() that does res.read_rows = read_rows.fetch_and(0), which 0s out values every time it sends progress report. So we send deltas, not absolute values. Could you please take a look when you have time?

@rienath
Copy link
Copy Markdown
Member

rienath commented Nov 7, 2025

Stateless tests (amd_asan, distributed plan, sequential)

Upgrade check (amd_msan)

@rienath rienath added this pull request to the merge queue Nov 7, 2025
@rienath rienath mentioned this pull request Nov 7, 2025
Open
Merged via the queue into ClickHouse:master with commit 660e610 Nov 7, 2025
128 of 131 checks passed
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rienath rienath rienath left review comments

@CheSema CheSema CheSema approved these changes

Assignees

@rienath rienath

@CheSema CheSema

Labels

can be tested Allows running workflows for external contributors pr-bugfix Pull request with bugfix, not backported by default 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.

INSERT SELECT prints 0 rows in set

7 participants

@aaaengel @CLAassistant @jsc0218 @alexey-milovidov @rienath @CheSema @robot-clickhouse-ci-1