Re: pg_basebackup: Always return valid temporary slot names
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 signature.asc Description: PGP signature
Re: pg_basebackup: Always return valid temporary slot names
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 signature.asc Description: PGP signature
Re: pg_basebackup: Always return valid temporary slot names
On Tue, Sep 5, 2023 at 4:40 PM Jelte Fennema wrote: > 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. Sounds good to me. Thank you! On Tue, Sep 5, 2023 at 5:36 PM Daniel Gustafsson wrote: > 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). > Ok. In this case, I also agree. +1 to the patch from my side. Thank you! Regards, Nishant.
Re: pg_basebackup: Always return valid temporary slot names
> On 5 Sep 2023, at 12:21, Jelte Fennema wrote: > > On Tue, 5 Sept 2023 at 11:39, Daniel Gustafsson wrote: >> >>> On 5 Sep 2023, at 09:09, Nishant Sharma >>> 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
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 wrote: > > On Tue, 5 Sept 2023 at 11:39, Daniel Gustafsson wrote: > > > > > On 5 Sep 2023, at 09:09, Nishant Sharma > > > 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.
Re: pg_basebackup: Always return valid temporary slot names
On Tue, 5 Sept 2023 at 11:39, Daniel Gustafsson wrote: > > > On 5 Sep 2023, at 09:09, Nishant Sharma > > 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.
Re: pg_basebackup: Always return valid temporary slot names
> On 5 Sep 2023, at 09:09, Nishant Sharma > 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
Re: pg_basebackup: Always return valid temporary slot names
Hi Jelte, Please find my reviews below:- *1)* With what I have understood from above, the pgbouncer fills up be_pid (int, 32 bits) with random bits as it does not have an associated server connection yet. 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? -- If this makes sense here, then maybe the fix should be in pgbouncer instead of how the be_pid is processed in pg_basebackup? *2)* Rest, the patch looks straightforward, with these two changes : "%d" --> "%u" and "(int)" --> "(unsigned)". Regards, Nishant. On Thu, Aug 31, 2023 at 2:43 PM Jelte Fennema wrote: > 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. >
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. v1-0001-pg_basebackup-Always-return-valid-temporary-slot-.patch Description: Binary data
