search

jackc / pgconn

MIT License
182 stars 87 forks source link

fix: wait for CopyInResponse before sending data #127

Closed olavloite closed 2 years ago

olavloite commented 2 years ago

According to https://www.postgresql.org/docs/14/protocol-flow.html#id-1.10.5.7.4 the frontend should check for a CopyInResponse before it starts sending CopyData messages to the backend.

This sometimes causes issues with our PostgreSQL proxy, as it might receive CopyData messages before it is ready for it. We will look into whether we can handle this situation in our proxy as well.

jackc commented 2 years ago

Something's failing on CI. Are you sure the change belongs in CopyTo? I would have thought you are referring to CopyFrom.

Beyond that, I'm not convinced pgconn / pgx is violating the protocol here at least in any meaningful way. In general, you are allowed to preemptively send messages without waiting for the reply. That's how pipelining works.

Waiting for that CopyInResponse costs a round trip of network latency. I'd rather not add that cost to every CopyFrom operation if I can help it.

olavloite commented 2 years ago

Something's failing on CI. Are you sure the change belongs in CopyTo? I would have thought you are referring to CopyFrom.

Argh... Yes, that was a stupid copy-paste error. I did not have a dev env set up on the machine I was working on that moment, so I thought I could just quickly copy my local test into the web editor on GitHub. That was a bad idea... I've fixed that now and also added an additional test for COPY ... BINARY, which is where I saw the error. (However, see below for reasons)

Beyond that, I'm not convinced pgconn / pgx is violating the protocol here at least in any meaningful way. In general, you are allowed to preemptively send messages without waiting for the reply. That's how pipelining works.

Hmmm... Yes, that is true. The documentation on the one hand seems to indicate that it is only allowed in the extended query mode. The COPY statement is in this case sent using the simple mode.

On the other hand, the same documentation also states that It is not actually necessary for the frontend to wait for ReadyForQuery before issuing another command, ...

The description of the CopyInResponse again says: The backend is ready to copy data from the frontend to a table

In our case, the latter is what was literally causing the problem. The backend was not ready to receive CopyData until it had sent the response, which again was caused by the COPY protocol being handled by a separate thread. I've re-written that part on our end so it does not have this requirement anymore. That also fixes this issue.

I'll leave it to you whether you want to include this change or not. Please feel free to close this PR if it's something you'd rather keep as-is.

jackc commented 2 years ago

Thanks for digging further into it. I'll keep it as-is to save the network round trip.