sql: COPY FROM tests are broken

[nvanbenschoten]

Both TestCopyOne and TestCopyInProgress were previously skipped because of lib/pq#558, but this issue has since been fixed. With the t.Skip calls removed, though, the tests both fail with timeout errors. I'm not sure if this is a test error or a real error (in cockroach or in lib/pq), but we should investigate.

[maddyblue]

These problems are due to race conditions in lib/pq. I'm becoming increasingly convinced that we should move off of lib/pq completely, so I'm inclined to not fix these bugs, but instead use our time to migrating to pgx. If anyone cares I've put some of the work at https://github.com/mjibson/pq/tree/copy-race

[nvanbenschoten]

Should we disable COPY FROM for now then?

[maddyblue]

No COPY works fine in cockroach if you use psql. And it's fine with lib/pq as long as you don't try and do the weird things the tests are testing for. But these tests should have been added to lib/pq years ago when they first added COPY support, but they weren't. Cockroach is fine.

[nvanbenschoten]

So what do we do here? We should either open another issue in lib/pq and try upstreaming that race fix ourselves or adjust these tests so that they don't depend on the "weird things" in lib/pq. Can we just pass in the SQL string directly?

[maddyblue]

We can't pass in the SQL string directly because COPY is a specific mode. lib/pq implements copy by having the user "pretend" to do a transaction and execute it, but intercepts this call so it's not in the normal txn/exec flow. If it were me, we would spend 0 effort fixing lib/pq and leave this test as is for now, and instead begin working on a move to pgx (or a custom driver since pgx also doesn't quite do everything we need, but we can get it there). I no longer think it is a good use of our time to contribute to lib/pq.

[nvanbenschoten]

So are these tests even testing Cockroach or are they just testing lib/pq? If they are only testing lib/pq then shouldn't we open up an issue in lib/pq and get rid of the tests? If they are testing Cockroach then we should find a way to circumvent lib/pq and still test the same behavior, otherwise, we're not fully verifying that "Cockroach is fine".

[maddyblue]

They would be testing cockroach if lib/pq weren't broken. That is, they attempt to test cockroach's connection state machine to verify it can't do the things the tests test. But lib/pq's state machine is broken instead, so we can't even get there. Yes, we should open up a lib/pq bug. I'll do that sometime.

I agree we should circumvent lib/pq so we can actually test cockroach. I think we should switch to pgx to do this.

[petermattis]

Couldn't we hit similar problems with the protocol implementation in pgx? Perhaps we should fork/extend either pq or pgx for the purpose of allowing flexible testing of our COPY protocol implementation.

[maddyblue]

Yes, that is possible. I've already done an initial implementation of our own driver and a pgx fork. Still prototyping both before I can figure out which one makes more sense.

[andreimatei]

As it stands, using pgx for copy from doesn't work because pgx sends queries like copy "t" ( "x" ) from stdin binary; and we don't parse that - we don't support the binary encoding for copy values :|.

[maddyblue]

We can also fix these tests by using the pgproto package from pgx to manually send packets in the order that we want. This will allow us to remove the racey lib/pq code and correctly test our COPY implementation for adversarial input.