colserde: fix the edge case with nulls handling

[yuzefovich]

When serializing the data of Bool, Bytes, Int, and Float types when they
don't have any nulls in the vector, we don't explicit specify the null
bitmap. Previously, when deserializing such vectors with no nulls we
would simply call UnsetNulls on the coldata.Nulls object that is
currently present. However, it is possible that already present nulls
object cannot support the desired batch length. This could lead to index
out of bounds accesses. Note that in the vast majority of cases this
likely doesn't happen in practice because we check MaybeHasNulls, and
that would return false making us omit the null checking code.

Fixes: #62636.

Release note (bug fix): Previously, CockroachDB could encounter an
internal error in rare circumstances when executing queries via the
vectorized engine that operate on columns of BOOL, BYTES, INT, and FLOAT
types that have a mix of NULL and non-NULL values.

[cockroach-teamcity]

This change is 

[yuzefovich]

To provide a bit more context, the scenario of hitting out of bounds error is actually because we call SetNullBitmap when non-empty arrowBitmap is provided. Consider the following sequence:

1. we allocate Nulls of coldata.BatchSize() capacity here
2. we read a vector of length 1 with a single null value. It is represented as arrowBitmap with length 1. We replace large null bitmap with this short one
3. we read a vector of length 1024 with no nulls. It will have an empty arrow bitmap, so we simply unset nulls on the short bitmap that is currently present. Yet if we try to check NullAt at i in range [8, 1024), we will hit an index out of bounds.

I think the code linked above is no longer relevant, so I'll open up a separate commit for it that won't be backported. But I believe this change should be backported even to 20.2.

[yuzefovich]

Actually, I'm no longer convinced that this fix is full, and I start to think that #62644 is actually needed for correctness.

Consider the following scenario:

• we enqueue two batches to the disk queue, {1, NULL}, and {2, 3}, that writes them to disk. It'll be represented by something like data=1,0|valid=true,false|data=2,3|valid=true,true on disk
• before dequeuing from the disk queue, we'll allocate a new batch to deserialize into of coldata.BatchSize capacity data=0,0,...,0|valid=true,true,...,true
• when dequeuing the first batch, we get non-zero arrow bitmap for validity, so we'll replace it in-place in our vector; now we'll have something like data=1,0|valid=true,false
• when dequeuing the second batch from the disk into our vector, there are no nulls, arrow bitmap is zero length, so we'll unset nulls; however, vec.Nulls.nulls points to the memory region to deserialize from. So if we later rewind the disk queue (which is needed for the merge and cross joins), we'll end up reading {1, 0} as the first batch, and not {1, NULL}.

Typing this out made me realize that our no-copy conversion from arrow to coldata format for data where we just call vec.SetCol to set the data slice for native types might also be error prone. In theory, after dequeing a batch from disk, we should be free to modify it. In the example above, imagine that we dequeued the first batch {1, NULL} and then performed + 1 operation on it - we would be modifying the memory region to deserialize from as well, IIUC. Thus, after the modification of the first batch, the region would look something like data=2,1|valid=true,false|data=2,3|valid=true,true.

@asubiotto I'm curious to hear your thoughts about these scenarios, possibly I'm missing something.

[yuzefovich]

Hm, I tried to repro problems described in the previous comment in a unit test and didn't succeed. It seems like I'm just missing something. Still #62644 makes the most sense to me.

[asubiotto]

I don't have much context on this part of the code any more so don't have anything intelligent to say about the scenario you mentioned. But my intuition is that you're talking about in-memory state. When you rewind a queue, you invalidate all that and read from disk again. Have you figured out exactly why your unit test case is passing?

This code

Reviewed 6 of 6 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rytaft)

[rytaft]

Reviewed 6 of 6 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/col/colserde/arrowbatchconverter.go, line 489 at r2 (raw file):

			// The current null bitmap doesn't have enough space, so we need to
			// allocate a new one.
			nulls := coldata.NewNulls(batchLength)

would be good to add an explanation here similar to what's in the commit message.

[yuzefovich]

But my intuition is that you're talking about in-memory state. When you rewind a queue, you invalidate all that and read from disk again. Have you figured out exactly why your unit test case is passing?

Yeah, I think I understand why the unit test I was making wasn't triggering the scenario I described above, and I think your comment about the in-memory state is on point. What I was missing there was the fact that the on-disk state is never modified, and because Dequeue invalidates the result of the previous call (to be more precise, the caller of Dequeue always passes in the same batch), the modification of the in-memory state does happen but only of the scratch batch - the caller always somehow copies over the state according to its needs.

To be honest, I don't understand the reasoning behind #43964 as it relates to the current code, so I'd expect that removing this code

cockroach/pkg/sql/colcontainer/diskqueue.go

Lines 765 to 775 in 8b137b4

	for i := range vecs {
	// When we deserialize a new memory region, we allocate a new null
	// bitmap for the batch which deserializer will write to. If we naively
	// allow the arrow batch converter to directly overwrite null bitmap of
	// each column, it could lead to memory corruption. Doing this avoids
	// reallocating a new scratchDecompressedReadBytes every time we perform
	// a read from the file and constrains the downside to allocating a new
	// null bitmap every couple of batches.
	nulls := coldata.NewNulls(coldata.BatchSize())
	vecs[i].SetNulls(&nulls)
	}

would have no effect (with this PR), but things broke once I removed it.

Anyway, we can leave that issue aside for now.

TFTRs!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @asubiotto and @rytaft)

---

pkg/col/colserde/arrowbatchconverter.go, line 489 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

would be good to add an explanation here similar to what's in the commit message.

There is a comment right above if and I added another comment to explain how the situation of insufficient capacity could occur.

[craig]

Build succeeded:

• GitHub CI (Cockroach)