Re: pg_basebackup: Always return valid temporary slot names

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 <me(at)jeltef(dot)nl> wrote:
>
> On Tue, 5 Sept 2023 at 11:39, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >
> > > On 5 Sep 2023, at 09:09, Nishant Sharma <nishant(dot)sharma(at)enterprisedb(dot)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.