Skip to content

Split 02530_dictionaries_update_field and add update_lag#81180

Merged
nickitat merged 4 commits intomasterfrom
fix_02530_dictionaries_update_field
Jun 6, 2025
Merged

Split 02530_dictionaries_update_field and add update_lag#81180
nickitat merged 4 commits intomasterfrom
fix_02530_dictionaries_update_field

Conversation

@nickitat
Copy link
Copy Markdown
Member

@nickitat nickitat commented Jun 2, 2025 •
edited
Loading

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

There is a possibility of a data race when an update happens during an insert, i.e., it updates last_update_time to a value greater than the now() value for the ongoing insert. ATST it doesn't see the inserted row because it is not yet committed. The update_lag parameter is only 1s by default, which is too small for CI, especially when parallel replicas are enabled. Also, the test runs fairly long, so it makes sense to split it.

Closes: #65382

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 2, 2025 •
edited
Loading

Workflow [PR], commit [753347c]

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jun 2, 2025
@azat azat self-assigned this Jun 2, 2025
@azat azat added the 🍃 green ci 🌿 Fixing flaky tests in CI label Jun 2, 2025
@azat
Copy link
Copy Markdown
Member

azat commented Jun 2, 2025

Interesting and logical

But I was looking at one failure of this test

Here is relevant portion of logs

+ clickhouse-test --testname --check-zookeeper-session --hung-check --capture-client-stacktrace --queries /repo/tests/queries --test-runs 1 --hung-check --s3-storage --jobs 8 --report-coverage --no-parallel-replicas --no-zookeeper --no-shard --replace-log-memory-with-mergetree --no-stateful --report-logs-stats

2025.06.01 09:19:19.090982 [ 105531 ] {815660dc-8107-4814-adea-415dc5ab717e} <Debug> executeQuery: (from [::1]:46398) (comment: 02530_dictionaries_update_field.sh) (query 1, line 1) INSERT INTO table_for_update_field_dictionary VALUES  (stage: Complete)
2025.06.01 09:19:19.295496 [ 105531 ] {815660dc-8107-4814-adea-415dc5ab717e} <Debug> TCPHandler: Processed in 0.250921819 sec.

2025.06.01 09:19:19.358752 [ 31687 ] {0692d4ad-ae61-4dd2-b044-f6eb497361cd} <Debug> executeQuery: (from [::1]:46610) (comment: 02530_dictionaries_update_field.sh) (query 1, line 1) SELECT key, value FROM dict_flat_custom ORDER BY key ASC (stage: Complete)

2025.06.01 09:19:23.724774 [ 1230 ] {} <Trace> ExternalDictionariesLoader: Loading config file 'cc46fe7c-aa29-47a0-9902-c3084d6fa731'.
2025.06.01 09:19:23.790539 [ 55168 ] {} <Debug> executeQuery: (internal) SELECT key, value, last_insert_time FROM test_2lzyomoc.table_for_update_field_dictionary WHERE last_insert_time >= '2025-06-01 09:19:17'; (stage: Complete)

So as you can see here it does not exceed the default lag

Do you have some ideas?

azat
azat reviewed Jun 2, 2025
View reviewed changes
@nickitat
Copy link
Copy Markdown
Member Author

nickitat commented Jun 3, 2025

Do you have some ideas?

not really

azat
azat reviewed Jun 4, 2025
View reviewed changes
tests/queries/0_stateless/02530_dictionaries_update_field_2.sh Outdated
)
ENGINE = MergeTree()
ORDER BY tuple()
"
Copy link
Copy Markdown
Member

@azat azat Jun 4, 2025

Choose a reason for hiding this comment

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

Was it the intention to use different engines in different tests?

Copy link
Copy Markdown
Member Author

@nickitat nickitat Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, for some reason, it significantly affects test duration. I didn't have a chance to understand why.

Copy link
Copy Markdown
Member

@azat azat Jun 5, 2025

Choose a reason for hiding this comment

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

The problem is the newly added update_lag, and with update_lag=30 it always queries all records, and apparently TinyLog gives different order

I have a better idea how to improve it, I've committed my suggestions on top, PTAL

Also we can make ExternalLoader::PeriodicUpdater::check_period_sec, that way these tests will be ~5x faster

azat
azat reviewed Jun 4, 2025
View reviewed changes
azat
azat reviewed Jun 4, 2025
View reviewed changes
azat
azat reviewed Jun 4, 2025
View reviewed changes
@nickitat nickitat added this pull request to the merge queue Jun 6, 2025
Merged via the queue into master with commit 8c4b088 Jun 6, 2025
120 checks passed
@nickitat nickitat deleted the fix_02530_dictionaries_update_field branch June 6, 2025 20:53
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 6, 2025
@Algunenano Algunenano mentioned this pull request Jun 9, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat left review comments

Assignees

@azat azat

Labels

🍃 green ci 🌿 Fixing flaky tests in CI pr-not-for-changelog This PR should not be mentioned in the changelog 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.

Flaky test 02530_dictionaries_update_field

3 participants

@nickitat @azat @robot-clickhouse-ci-2