I believe this is working as intended:

RawBytes documents: https://pkg.go.dev/database/sql#RawBytes

After a Scan into a RawBytes, the slice is only valid until the next call to Next, Scan, or Close.

I don't think this is the intended behavior.

The document says "RawBytes is a byte slice that holds a reference to memory owned by the database itself".
buf = strconv.AppendInt(buf, 42, 10) is not documented at all.

And users don't know about this behavior.
Users cannot expect that passing the same RawBytes to Scan() multiple times violates the "the slice is only valid until..." constraint.
This is very hard to debug for normal gophers.

Stop appending to RawBytes saves users from this pitfall without any downside.

CC @bradfitz @kardianos as owner.

Not clear to me what's intended here, but the actual behavior is surprising and easy to misuse.

Consider:

var raw sql.RawBytes
for rows.Next() {
  rows.Scan(&raw)
}

Rows.Scan converts a value provided by the database driver into a value provided by the user (a sql.RawBytes in this case).

If the database driver provides a []byte, Scan places this value directly into the RawBytes. This allows returning this data without copying.

If the database driver provides some other type, Scan reuses any existing byte slice in the RawBytes. This allows reusing a single underlying array across multiple calls to Scan. For example, if the above loop is reading an int32 value provided by the database driver, then the loop only needs to allocate a single []byte of length 4 for all iterations.

The latter case says to me that the above loop is written as intended: We reuse the same RawBytes across every Scan call to avoid allocations.

Where things go wrong is if we make two queries with the same RawBytes, mixing the two modes. The first query places a reference to a driver-owned []byte into the RawBytes. The second query overwrites this data.

Perhaps this is user error, but the documentation doesn't make it clear that a RawBytes may not be reused between queries, and the fact that it looks like you're expected to reuse a RawBytes within a single query makes this quite surprising.

One fix might be to drop the optimization for the second case and never write to the data contained within a RawBytes. However, this optimization has been present since 2014 (CL 50240044), so I'm not sure reverting it now is the right choice.

Another might be to zero out any RawBytes scan parameters on the first Scan, to avoid carrying over references to driver-internal memory from a previous query.

I suspect this behavior hasn't been noticed before now since it's unusual to reuse a single query variable for scanning two different columns of different types.

Change https://go.dev/cl/557917 mentions this issue: database/sql: avoid clobbering driver-owned memory in RawBytes

ah, apologies i didn't understand before how the append was observed without api misuse.

RawBytes has two usage (zero copy and reusing buffer).
One of them (reusing buffer) is undocumented. And mixing two makes hard to find data corruption bug.

How to fix this?

1. Add new type like RawBytes that intend to reuse buffer and RawBytes is only about zero copy.
2. Just document the "reusing buffer" usage and warn about silent data corruption when mixing two usages.

Another might be to zero out any RawBytes scan parameters on the first Scan, to avoid carrying over references to driver-internal memory from a previous query.

But this makes allocation of []byte.

However, this optimization has been present since 2014 (CL 50240044), so I'm not sure reverting it now is the right choice.

I doubt that this optimization is important.

• Almost no one knows it. (I, the driver maintainer didn't know it.)
• Using int64, float64 etc doesn't need allocation&stringification.
• Using any need allocation, but no stringification.

How do you think? @bradfitz.

I was the original reporter of this problem, and I agree that this is an unintended and undocumented behavior. The documentation makes it sound like a RawBytes will always receive a buffer pointer. One that either points directly into the raw sql buffer, or a newly allocated buffer with the data.

Overwriting the data that is currently in the buffer does not make sense and can be a security issue. There are 3 places in sql/convert.go that write into the buffer that RawBytes currently holds. These should instead create a new buffer and set that on the RawBytes.

I was going to ask on the commit but you blocked me from commenting. Just a thought, but on line 240 of sql/convert.go, which was modified in this commit, wouldn't it make sense to use an unsafe operation and pass the string bytes directly? Or does that go against go conventions? It would limit the new rows.raw member to holding only converted integral and time types, so basically after the first row was scanned in, the rows.raw buffer would likely never change sizes.

*d = s2b(s)

func s2b(s string) (b []byte) {
	str := (*reflect.StringHeader)(unsafe.Pointer(&s))
	sh := (*reflect.SliceHeader)(unsafe.Pointer(&b))
	sh.Data = str.Data
	sh.Len = str.Len
	sh.Cap = str.Len
	return b
}

I blocked comments on the commit because we don't use GitHub for code review. GitHub is a mirror, and almost nobody sees comments on a GitHub commit. Comments about commits should be on the change review, which in this case is https://go.dev/cl/557917. Thanks.

To answer your question, we don't use unsafe unless there is a very significant benefit. And in this case I don't actually see any benefit at all; append supports appending a string directly to a []byte, which is what the code is doing now.

I understand. I was just thinking the RawBytes functionality, meant for speed optimization, in which read-only use-once buffers are returned, kind of matched a use case for returning the direct bytes of a string. Thanks for answering though.