else?

Otherwise it will be empty (NULL).

So for example 3 is not possible?

If getPartitionIdsAffectedByCommands can return empty or single element list - why it return list? May be it should use optional?

This is a common code with StorageReplicatedMergeTree, we can't change it.

Nobody asks to change it (I think the comment above was about reliability and readability of this code that relies on implicit invariant that probably does not even exist). Please add a comment that explains why this function always returns one partition for StorageMergeTree (I don't understand why). And throw LOGICAL_ERROR if it returned more than one partition.

@tavplubix it's not me saying it returns only one partition for StorageMergeTree, but @filimonov . When it returns more than one partition, the optimization will not be applied.

Please add a comment

Do we really need to serialize partition_id here? Seems like we already have partition id inside commands

We don't need serialization of it specifically, but we need a way to distinguish old storage-wide mutations from new. If we don't change format, we will try to recalculate paritition_id for older and possibly unfinished partition-specific mutations, skip unrelated partitions and stick on merges when in the same partition subset of parts is mutated beyond correct version, or stick being unable to mutate another subset beyond it because there is no related mutation for that. I think this serialization is a neat way to avoid that.

subset of parts is mutated beyond correct version

AFAIK it should not be a problem

I mean that we have different mutations:

1. with partition_id and IN PARTITION expression, which are new partition-specific mutations — these shall create partition-specific changes;
2. without partition_id and IN PARTITION expression, which are old or new storage-wide mutations — these create storage-wide changes;
3. without partition_id but with IN PARTITION expression, which are old partition-specific mutations. They were supposed to make storage-wide changes earlier (before this patch) and may be partially finished on some parts, so we must continue working the same way on them. If we remove these guys from remaining really unaffected parts (parts_to_do), we would have to rollback unaffected parts which already mutated to that version, or we will have finished mutation and different versions in a partition which will prevent merges to happen until new mutation (in that partition or whole storage) appears.

Generally, I think it is a good thing to have partition set calculated at the moment when the mutation is created, anyway.

I understand your point, but I'm not sure about

we must continue working the same way on them. If we remove these guys from remaining really unaffected parts (parts_to_do), we would have to rollback unaffected parts which already mutated to that version

I agree that it's undesirable to have some parts with data version greater than expected version for the partition. But AFAIK it will not break anything.

will prevent merges to happen until new mutation (in that partition or whole storage) appears

It will prevent only merges of parts with different data versions, seems like it's not critical. Also it probably can be addressed by taking last_mutation_by_partition into account in getCurrentMutationVersion.

I don't like serializing partition id for two reasons:

• It duplicates partition id that we already have in IN PARTITION clause (and we already serialize it).
• It unconditionally changes format of mutation entries without any settings. It will make downgrade impossible once you run IN PARTITION mutation. IN PARTITION is not a new feature, so this change is backward incompatible. We can introduce a setting like merge_tree_mutation_entry_version or serialize_partition_id_for_in_partition_mutations for compatibility, but I don't like this option. It's better to avoid changing format, and seems like it's possible to avoid it.

Consider using TSA annotations

Wow, cool!

@tavplubix it does not quite fit in this case because there is (at least) one usage of std::unique_lock instead of std::lock_guard in selectPartsToMerge() and we would not be able to build complete tree of function calls properly annotated.

https://lists.llvm.org/pipermail/cfe-dev/2016-November/051468.html

You can use TSA_SUPPRESS_WARNING_FOR_READ in selectPartsToMerge

This is impossible because std::unique_lock has no thread annotations. There is nothing to suppress, however, but it is needed to mark mutex as locked for Clang instead. It would work if we only used std::lock_guard.

It is possible and it works fine with std::unique_lock and TSA_SUPPRESS_WARNING_FOR_READ