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 <me(at)jeltef(dot)nl> 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.
>