All committers have signed the CLA.

This is an automated comment for commit fd46056 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

@alexey-milovidov Hi, CI checks had all passed, could you please assign a reviewer for this PR?

Sorry, this has been reverted because another reviewer (not me, I didn't read the code) has found an obvious race condition in the code. Let's fix this race condition and resubmit.

Also, we have to add a test to make this race condition exposed.

@alexey-milovidov could you please specify what race condition we have here? do we have a note somewhere I missed?

Also, grabbing a mutex in readExistingRowsCount() and reading the whole _row_exists column from disk while holding it seems like a very dangerous approach. E.g. it can be called from ReplicatedMergeTreeQueue::shouldExecuteLogEntry() via part->getExistingBytesOnDisk(); and can negatively affect replication queue processing.
I think the proper way is to save the count of "existing" rows in part's metadata when part is created similar to total row count.

@davenger saving count is definitely better way of dealing with this but it requires metadata modification one way or another.
BTW do you know which race condition Alexey is talking about?

@davenger Actually my initial thought was to add an existing_count.txt (just like count.txt) before finishing mutation
As for existing parts, create existing_count.txt on loading process.
Do you think it is a better solution?

@davenger Actually my initial thought was to add an existing_count.txt (just like count.txt) before finishing mutation As for existing parts, create existing_count.txt on loading process. Do you think it is a better solution?

@alexey-milovidov @yakov-olkhovskiy what are your thoughts about this approach?

I think it's better than lazy calculation, but backward compatibility should be taken into account.

Sorry, this has been reverted because another reviewer (not me, I didn't read the code) has found an obvious race condition in the code. Let's fix this race condition and resubmit.

@alexey-milovidov Could you please provide more information about the race condition in this PR?

I don't know what the race condition is, but my colleagues told me that it is there... I will clarify.

I don't know what the race condition is, but my colleagues told me that it is there... I will clarify.

@alexey-milovidov thanks

They said it was around const bool & is_merge

They said it was around const bool & is_merge

I don't see any race conditions here... @yakov-olkhovskiy Do you have any clue?

@jewelzqiu, My colleagues said it is related to existing_rows_count.

@jewelzqiu, My colleagues said it is related to existing_rows_count.

@alexey-milovidov Thanks, I got it, looks like existing_rows_count can be read before lock acquired.
I have created another PR with different approach as discussed above in this PR: #58223
In the new PR existing_rows_count will only be set once before part became active
cc @yakov-olkhovskiy @davenger