coldata: prohibit Vec.Append from a vector into itself#76529
coldata: prohibit Vec.Append from a vector into itself#76529craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
michae2
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)
pkg/col/coldata/bytes.go, line 316 at r1 (raw file):
panic("AppendSlice when b == src is only supported when srcStartIdx == srcEndIdx") } }
I'm a little confused by the check for CrdbTestBuild. Is the srcStartIdx == srcEndIdx precondition only necessary when this is called from a test? Because that's how I'm reading this code... but from the commit message it sounds like this precondition is always necessary regardless of whether this is called from a test or not.
Should the check instead be something like this? (I.e. only tests can call self-referencing AppendSlice, and then we check the precondition.)
if b == src && (!buildutil.CrdbTestBuild || srcStartIdx != srcEndIdx) {
panic("...")
}Currently, `Vec.Append` works in cases when the destination and the source vectors are the same; this, however, is not necessary since the only production use of that method is in `colexecutils.AppendOnlyBufferedBatch.AppendTuples`, and there we're appending values from an input batch into the append-only batch. We also have linters in place that prohibit the usage of `Vec.Append` in production code outside of that single spot, so it's safe to prohibit the self-referencing `Vec.Append`. Release note: None
c2ff381 to
bc2bd1c
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/col/coldata/bytes.go, line 316 at r1 (raw file):
Previously, michae2 (Michael Erickson) wrote…
I'm a little confused by the check for
CrdbTestBuild. Is thesrcStartIdx == srcEndIdxprecondition only necessary when this is called from a test? Because that's how I'm reading this code... but from the commit message it sounds like this precondition is always necessary regardless of whether this is called from a test or not.Should the check instead be something like this? (I.e. only tests can call self-referencing
AppendSlice, and then we check the precondition.)if b == src && (!buildutil.CrdbTestBuild || srcStartIdx != srcEndIdx) { panic("...") }
It should be prohibited in all cases, and I simply wanted to save a conditional in production builds, but given that we already have several conditions in place there and the check is done about once per coldata.BatchSize, it's no big deal, so I made it all-the-time check.
michae2
left a comment
There was a problem hiding this comment.
Reviewable status:
|
TFTR! bors r+ |
|
Build succeeded: |
Currently,
Vec.Appendworks in cases when the destination and thesource vectors are the same; this, however, is not necessary since the
only production use of that method is in
colexecutils.AppendOnlyBufferedBatch.AppendTuples, and there we'reappending values from an input batch into the append-only batch. We also
have linters in place that prohibit the usage of
Vec.Appendinproduction code outside of that single spot, so it's safe to prohibit
the self-referencing
Vec.Append.Release note: None