Detect COPY even if the query starts with whitespace or comments

[darccio]

What this PR does

Updated the handling of the COPY statement in the Prepare function to utilize the internal.StartsWithCOPY utility to detect if a query is a COPY statement even if it has comments or whitespaces.

Motivation

Our Datadog/dd-trace-go library supports instrumenting database/sql, including the DBM-APM link based on SQLcommenter spec, with the difference that we are prepending the comment instead of appending it. This breaks how lib/pq detects a COPY statement.

We are aware that there were previous opened issues around COPY statements, but we feel this one is an improvement, as SQL comments are not uncommon in the wild. Also, it's the only place where this kind of check happen in lib/pq.

This would fix #1154, although it might break the workaround described in #530. We didn't attempt to address the concerns explained in #213.

Benchmarks

goos: darwin
goarch: arm64
pkg: github.com/lib/pq/internal
cpu: Apple M1 Max
BenchmarkStartsWithCOPY-10    	39674830	        28.50 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/lib/pq/internal	1.363s

goos: darwin
goarch: arm64
pkg: github.com/lib/pq/internal
cpu: Apple M1 Max
BenchmarkEqualFold-10    	292420224	         3.771 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/lib/pq/internal	1.721s

Although it's almost eight times slower at the nanoseconds scale, it should negligible in the context of preparing a query.

[arp242]

I put my version here: 2e1e922 – personally I think it's rather easier to follow/understand.

Performance is a few ns slower:

goos: linux
goarch: amd64
pkg: github.com/lib/pq/internal
cpu: AMD Ryzen 7 PRO 7840U w/ Radeon 780M Graphics
BenchmarkStartsWithCOPY-16      61637460        16.39 ns/op    0 B/op  0 allocs/op
BenchmarkStartsWithCOPY2-16     52043742        19.92 ns/op    0 B/op  0 allocs/op
BenchmarkEqualFold-16           459326388        2.569 ns/op   0 B/op  0 allocs/op

But that's okay.

What do you think?

[arp242]

This would fix #1156

How would this fix that issue by the way?

[darccio]

Thanks for taking care, @arp242. I'll check this tomorrow morning EU time.

[darccio]

@arp242 Re: #1156, I think you meant #1154. In any case, your commit 2e1e922 behaves in the same way than my implementation. If you feel yours is easier to maintain, let's go forward with yours.

[arp242]

Re: #1156, I think you meant #1154

Eh, yeah; that stupid obnoxious dropdown probably mangled my comment. But I don't see how this PR fixes that issue?

[darccio]

@arp242 I vaguely recall that I found the same messages in #1154, and I was convinced that it worked at the moment. Let's keep it open just in case.

What are our next steps with this PR? As we are pushing forward your solution, I guess it can be closed.

[arp242]

What are our next steps with this PR? As we are pushing forward your solution, I guess it can be closed.

I updated the PR and will merge when ready. I want to look a bit at adding a higher-level test and some documentation on what this actually does. One reason for that is that your PR didn't actually do anything as it still retained the string indexing:

if len(q) >= 4 && internal.StartsWithCOPY(q[:4]) {

So on "/* cmt */ copy" it would just send /* cm.

[darccio]

You are right, good catch @arp242! Thank you for your time.

[darccio]

@arp242 How often lib/pq is released? I'd like to revert a PR in dd-trace-go to support COPY using lib/pq in our DBM product.

[arp242]

I'm hoping to do a release next week or thereabouts.