introduce a generation to a server connection

[thanodnl]

As described in #245 this is the first patch in a series of patches to improve the cancelation of a backend when pgbouncer is involved.

This patch is most experimental and would require more testing and help from a maintainer.

The goal of this patch is to add the notion of a generation to a Server socket. The generation should be incremented every time a command is dispatched to the server. By keeping this generation we could compare the generation (and thus the command sent) at the moment of a cancelation being received by pgbouncer to the generation at the moment of the forward of the cancelation. As described and observed in #245, there can be a delay between pgbouncer receiving the cancelation and the moment a cancelation is processed on a new socket.

[petere]

In a way, this would create a behavior in pgbouncer that is stronger than what plain PostgreSQL does. Normally, a cancel request just cancels what is currently running, and there is no guarantee that it cancels what was running when the request was sent.

I'm wondering whether with your other two patches applied, which would result in cancel requests always processed first, this would still be a significant problem.

[thanodnl]

What you say here makes a lot of sense. We run with this patch in production currently, but I think it is mainly due to over active cancelations being sent by our app.

For reasons we have a place where we aggressively send a cancelation on a connection when we want to run a different query. This cancelation is sent without checking if anything is running on the connection. Subsequently we send the new statement for execution. With pgbouncer in the mix this cancelation often comes in to the backend when the new query is already dispatched.

This is due to a combination of factors:

• us not checking the status of the connection and always sending the cancelation
• network latency
• ssl handshakes (although I am currently blank if the cancelation needs to wait till an ssl connection is established)

With this generation patch we have observed the cancelation coming in delayed, canceling the subsequent statement, has been completely eliminated.

The over active cancelations are not a problem when connecting directly to postgres as the chances of the new request beating the cancelation on the network are much lower. However if pgbouncer needs to establish a new connection before it can send the cancelation it at least takes 3 trips on the network (tcp handshake), after our application has delivered the cancelation message before it reaches the postgres backend.

Will experiment a bit more with our app only sending a cancelation when it is actually having a connection that is busy.

[JelteF]

I took a closer look at this PR and the comments. I disagree that it would create stronger behavior than Postgres has. The problem isn't that it doesn't cancel what was running at the time of the cancel request. The problem is that due to a race condition it can cancel a query from a completely different connection. If the server connection is reused by another client, while the cancel request is waiting for a new connection, then this race condition occurs.

However, I don't think that this PR solves this issue in the right way. Looking at the code it seems to be prone to segfaults: The server is added as req->link, but isn't set to NULL if the server is freed. I addressed that issue in #717 and by doing so the generation isn't actually needed anymore. Because now I simply infer that the connection was reused/disconnected when req->link is NULL.