I modified the PgBouncer PR to always set the sign bit to 0. But I still I think it makes sense to also address this in pg_basebackup.
On Tue, 5 Sept 2023 at 12:21, Jelte Fennema <m...@jeltef.nl> wrote: > > On Tue, 5 Sept 2023 at 11:39, Daniel Gustafsson <dan...@yesql.se> wrote: > > > > > On 5 Sep 2023, at 09:09, Nishant Sharma <nishant.sha...@enterprisedb.com> > > > wrote: > > > > > With this, I was thinking, isn't this a problem of pgbouncer filling > > > be_pid with random bits? Maybe it should have filled the be_pid > > > with a random positive integer instead of any integer as it > > > represents a pid? > > > > I'm inclined to agree that anyone sending a value which is supposed to > > represent a PID should be expected to send a value which corresponds to the > > format of a PID. > > When there is a pooler in the middle it already isn't a PID anyway. I > took a look at a few other connection poolers and all the ones I > looked at (Odyssey and pgcat) do the same: They put random bytes in > the be_pid field (and thus can result in negative values). This normally > does not cause any problems, because the be_pid value is simply sent > back verbatim to the server when canceling a query, which is it's main > purpose according to the docs: > > > This message provides secret-key data that the frontend must save if it > > wants to be able to issue cancel requests later. > > Source: > https://www.postgresql.org/docs/current/protocol-flow.html#id-1.10.6.7.3 > > For that purpose it's actually more secure to use all bits for random > data, instead of keeping one bit always 0. > > Its main other purpose that I know if is displaying it in a psql > prompt, so you know where to attach a debugger. This is completely > broken either way as soon as you have a connection pooler in the > middle, because you would want to display the Postgres backend PID > instead of the random ID that the connection pooler sends back. So if > it's negative that's no issue (it displays fine and it's useless > either way). > > So, while I agree that putting a negative value in the process ID field of > BackendData, is arguably incorrect. Given the simplicity of the fix on > the pg_basebackup side, I think addressing it in pg_basebackup is > easier than fixing this in all connection poolers. > > Sidenote: When PgBouncer is run in peering mode it actually uses the > first two bytes of the PID to encode the peer_id into it. That way it > knows to which peer it should forward the cancellation message. Thus > fixing this in PgBouncer would require using other bytes for that.