lib/logstorage: fix uniq_values merge in cluster queries

[cuongleqq]

Fixes #1383

This PR fixed uniq_values() state merging when the destination state has an empty map and the source state has values (See the issue for how to reproduce).

I fixed this bug by simply create a new map if the destination map is currently nil.

I have a question though:

There is an optimization I think of when I fix this bug: In the sup.m == nil case, we could avoid allocation by moving src.m directly to sup.m instead of allocating a new map and copying keys.

I kept the safer copy-based approach for now to avoid changing semantics. Would you prefer the map moving optimization here? If so, I can make anothe commit.

Thanks.

[cubic-dev-ai]

No issues found across 2 files

[valyala]

It would be great adding a test for this case. The best type of the test would be an integration test inside apptest folder for the VictoriaLogs cluster setup. You can add the needed logs to one vmstorage node, while skipping adding the logs to another vmstorage node, and then issue a select query and verify its results. Note that you can ingest logs individually per every vmstorage node in the same way like you do for a single-node VictoriaLogs according to these docs.

[valyala]

It is OK to skip this check - the loop below will work correctly and fast on empty map and on nil map.

[cuongleqq]

I kept the check because:

1. It avoids allocating an empty map on a nil destination when the source is empty or nil.
2. I am not entirely sure whether merging two nil maps should result in an empty map.

That said, the allocation is tiny and the case should not happen often, so I am fine with dropping the check if you prefer the simpler flow.

[valyala]

There is an optimization I think of when I fix this bug: In the sup.m == nil case, we could avoid allocation by moving src.m directly to sup.m instead of allocating a new map and copying keys.

There is no need in this optimization here, since merging the state shouldn't be a performance bottleneck here. This optimization will make the code more fragile without achieving visible performance benefits.

[valyala]

@cuongleqq , could you also check other LogsQL pipes for the same type of bug, and fix it in separate pull requests?

[cuongleqq]

@cuongleqq , could you also check other LogsQL pipes for the same type of bug, and fix it in separate pull requests?

@valyala it seems like only uniq_values has this problem.

I checked other stats functions and LogsQL pipes with similar map-based merge paths.

Other stats functions initialize the destination map before writing to it, either via helpers:

VictoriaLogs/lib/logstorage/stats_count_uniq.go

Lines 284 to 293 in 3e6e9ef

	func mergeUint64Set(dstPtr *map[uint64]struct{}, src map[uint64]struct{}, stopCh <-chan struct{}) {
	if len(src) == 0 {
	return
	}
	dst := *dstPtr
	if dst == nil {
	dst = make(map[uint64]struct{})
	*dstPtr = dst
	}

or directly:

VictoriaLogs/lib/logstorage/stats_histogram.go

Lines 174 to 178 in 3e6e9ef

	for vmrange, count := range src.bucketsMap {
	if shp.bucketsMap == nil {
	shp.bucketsMap = make(map[string]uint64)
	}
	shp.bucketsMap[vmrange] += count

Other pipes also have safe initialization patterns.

field_names and stream_context use getM() to initialize the destination map before merging:

VictoriaLogs/lib/logstorage/pipe_field_names.go

Lines 108 to 113 in 3e6e9ef

	func (shard *pipeFieldNamesProcessorShard) getM() map[string]*uint64 {
	if shard.m == nil {
	shard.m = make(map[string]*uint64)
	}
	return shard.m
	}

Pipes that use hitsMap are also safe, because missing keys are inserted through setStateUint64, setStateNegativeInt64, and setStateString, which initialize the corresponding map before writing:

VictoriaLogs/lib/logstorage/hits_map.go

Lines 339 to 373 in 3e6e9ef

	func (hm *hitsMap) mergeState(src *hitsMap, stopCh <-chan struct{}) {
	for n, pHitsSrc := range src.u64 {
	if needStop(stopCh) {
	return
	}
	pHitsDst := hm.u64[n]
	if pHitsDst == nil {
	hm.setStateUint64(n, pHitsSrc)
	} else {
	*pHitsDst += *pHitsSrc
	}
	}
	for n, pHitsSrc := range src.negative64 {
	if needStop(stopCh) {
	return
	}
	pHitsDst := hm.negative64[n]
	if pHitsDst == nil {
	hm.setStateNegativeInt64(int64(n), pHitsSrc)
	} else {
	*pHitsDst += *pHitsSrc
	}
	}
	for k, pHitsSrc := range src.strings {
	if needStop(stopCh) {
	return
	}
	pHitsDst := hm.strings[k]
	if pHitsDst == nil {
	hm.setStateString(k, pHitsSrc)
	} else {
	*pHitsDst += *pHitsSrc
	}
	}
	}

So I do not see another similar nil map merge issue at the moment.

[cuongleqq]

It would be great adding a test for this case. The best type of the test would be an integration test inside apptest folder for the VictoriaLogs cluster setup. You can add the needed logs to one vmstorage node, while skipping adding the logs to another vmstorage node, and then issue a select query and verify its results. Note that you can ingest logs individually per every vmstorage node in the same way like you do for a single-node VictoriaLogs according to these docs.

Done. I have added TestVlclusterUniqValuesMerge in apptest/tests/vlcluster_test.go. Thanks.