Thread: pg_basebackup: Always return valid temporary slot names

With PgBouncer in the middle PQbackendPID can return negative values
due to it filling all 32 bits of the be_pid with random bits.

When this happens it results in pg_basebackup generating an invalid slot
name (when no specific slot name is passed in) and thus throwing an
error like this:

pg_basebackup: error: could not send replication command
"CREATE_REPLICATION_SLOT "pg_basebackup_-1201966863" TEMPORARY
PHYSICAL ( RESERVE_WAL)": ERROR:  replication slot name
"pg_basebackup_-1201966863" contains invalid character
HINT:  Replication slot names may only contain lower case letters,
numbers, and the underscore character.

This patch fixes that problem by formatting the result from PQbackendPID
as an unsigned integer when creating the temporary replication slot
name.

I think it would be good to backport this fix too.

Replication connection support for PgBouncer is not merged yet, but
it's pretty much ready:
https://github.com/pgbouncer/pgbouncer/pull/876

The reason PgBouncer does not pass on the actual Postgres backend PID
is that it doesn't have an associated server connection yet when it
needs to send the startup message to the client. It also cannot use
it's own PID, because that would be the same for all clients, since
pgbouncer is a single process.

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

--
Daniel Gustafsson

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.

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.

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

> 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

On Tue, Sep 05, 2023 at 02:06:14PM +0200, Daniel Gustafsson wrote:
>> 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.

Perhaps.

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

PQbackendPID() returns a signed value, likely coming from the fact
that it was thought to be OK back in the days where PIDs were always
defined with less bits.  The fix is OK taken in isolation, so I am
going to apply it in a few minutes as I'm just passing by..

Saying that, I agree with the point that we should also use %u in
psql's prompt.c to get a correct PID if the 32th bit is set, and move
on.
--
Michael

On Thu, Sep 07, 2023 at 01:23:33PM +0900, Michael Paquier wrote:
> PQbackendPID() returns a signed value, likely coming from the fact
> that it was thought to be OK back in the days where PIDs were always
> defined with less bits.  The fix is OK taken in isolation, so I am
> going to apply it in a few minutes as I'm just passing by..

Actually, correcting myself, pid_max cannot be higher than 2^22 on 64b
machines even these days (per man 5 proc).
--
Michael