Only use positive numbers in the PID part of the cancel key #945
Conversation
JelteF
force-pushed
the
positive-pid-in-cancel-key
branch
from
8d11b1c to
8330df4
Compare
That makes sense.
I don't think I follow this. While temporary breakage during rolling upgrades might be a minor problem, what about different pgbouncer versions on different hosts that are peered together? We don't want to force that all of those are upgraded together. Maybe a compatibility break is necessary, but then we should just say so. What would happen if we left the peer ID as is (in the first bytes)? |
Fair enough. I'll update the docs in the PR and note that cross version peering is not supported across this version change. (and also include it in the changelog)
Then we'd cut the usable peer id space in half, because the sign bit always needs to be 0. The maximum value of peer_id (16383) is also documented, so changing that would also be a breaking change. Since we're breaking compatibility either way, I'd rather do it in the way that the PR is doing now so we can keep the same amount of peer_ids. |
JelteF
force-pushed
the
positive-pid-in-cancel-key
branch
from
8330df4 to
9182a18
Compare
We were using the full 64 bits of the BackendKeyData message as random bytes. This turned out to be arguably incorrect, because the first 32 bits are used by PostgreSQL as a Process ID (and this is part of the [protocol documentation too][1]). Actual PIDs are always positive, but we also put a random bit in the sign bit so were setting it to negative numbers half of the time. For most cases this does not matter, but it turned out that [`pg_basebackup` relies on the PID part to actually be positive][2]. While it seems semi-likely that `pg_basebackup` will be fixed to support negative Process IDs, it still seems good to adhere to this implicit requirement on positive numbers in case other clients also depend on it. Since this change requires changing the bytes of the cancel key in which we encode the `peer_id`, this change breaks cancelations in peered clusters of different PgBouncer versions. This seems like a minor enough problem that we should not care about this. In practice this should only happen during a rolling upgrade, which we currently don't support well anyway (see pgbouncer#902 for improvements on that). And even if we did, breaking cancellations for a few minutes in this transitional stage doesn't seem like a huge deal. [1]: https://www.postgresql.org/docs/current/protocol-message-formats.html [2]: https://www.postgresql.org/message-id/flat/CAGECzQQOGvYfp8ziF4fWQ_o8s2K7ppaoWBQnTmdakn3s-4Z%3D5g%40mail.gmail.com
JelteF
force-pushed
the
positive-pid-in-cancel-key
branch
from
9182a18 to
7e68480
Compare
|
I updated the docs to include the cross-version breakage. |
In session pooling mode PgBouncer is pretty much a transparent proxy, i.e. the client does normally not even need to know that PgBouncer is in the middle. This allows things like load balancing and failovers without the client needing to know about this at all. But as soon as replication connections are needed, this was not possible anymore, because PgBouncer would reject those instead of proxying them to the right server. This PR fixes that by also proxying replication connections. They are handled pretty differently from normal connections though. A client and server replication connection will form a strong pair, as soon as one is closed the other is closed too. So, there's no caching of the server replication connections, like is done for regular connections. Reusing replication connections comes with a ton of gotchas. Postgres will throw errors in many cases when trying to do so. So simply not doing it seems like a good tradeoff for ease of implementation. Especially because replication connections are pretty much always very long lived. So re-using them gains pretty much no performance benefits. Fixes #382 Depends on #945
We were using the full 64 bits of the BackendKeyData message as random
bytes. This turned out to be arguably incorrect, because the first 32
bits are used by PostgreSQL as a Process ID (and this is part of the
protocol documentation too). Actual PIDs are always positive, but
we also put a random bit in the sign bit so were setting it to negative
numbers half of the time. For most cases this does not matter, but it
turned out that
pg_basebackuprelied on the PID part to actually bepositive.
While
pg_basebackupis now fixed to support negative Process IDs, itstill seems good to adhere to this implicit requirement on positive
numbers in case other clients also depend on it.
Since this change requires changing the bytes of the cancel key in which
we encode the
peer_id, this change breaks cancelations in peeredclusters of different PgBouncer versions. This seems like a minor
enough problem that we should not care about this. In practice this
should only happen during a rolling upgrade, which we currently don't
support well anyway (see #902 for improvements on that). And even if we
did, breaking cancellations for a few minutes in this transitional stage
doesn't seem like a huge deal.