Re: pg_basebackup: Always return valid temporary slot names

> On 5 Sep 2023, at 12:21, Jelte Fennema <me@jeltef.nl> wrote:
> 
> On Tue, 5 Sept 2023 at 11:39, Daniel Gustafsson <daniel@yesql.se> wrote:
>> 
>>> On 5 Sep 2023, at 09:09, Nishant Sharma <nishant.sharma@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.

If it's common practice to submit a pid which isn't a pid, I wonder if longer
term it's worth inventing a value for be_pid which means "unknown pid" such
that consumers can make informed calls when reading it?  Not the job of this
patch to do so, but maybe food for thought.

> 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.

Since the value in the temporary slotname isn't used to convey meaning, but
merely to ensure uniqueness, I don't think it's unreasonable to guard aginst
malformed input (ie negative integer).

--
Daniel Gustafsson