Re: Timing issue in pg_basebackup

Hi!

Here are 2 v2-patches as per your suggestion.

I'm a bit unsure if the check in
drop_lingering_replication_slot_atexit() function is enough to not break
anything.
Or too much.

Suggestions welcome, could also make it into 1 patch if you prefer that.

//Jesper

On 2026-10-22 07:44, Kirill Reshke wrote:
> On Wed, 21 Oct 2026 at 22:16, Jesper St John <jesper(at)stjohn(dot)se> wrote:
>>
>> On 2026-10-20 23:11, Kirill Reshke wrote:
>
>> >
>> >> I've included a patch that moves the replication slot handing to run
>> >> before the backup part as a separate function.
>> >>
>> >> Some possible caveats already:
>> >> * Since moving the slot creation, we might run in to trouble if for
>> >> example, --checkpoint=spread is used since the slot is open for
>> >> longer.
>> >> * If any error arises in the backup part, now there will be a
>> >> lingering
>> >> slot when previously no slot was created.
>> >
>> > lingering slots looks like a real regression here. can we remove them
>> > when basebackup client disconnects?
>>
>> A lingering slot could also happen today - only with this patch
>> there's
>> a higher risk of it happening.
>
> Higher risk of lingering slots is also a regression. But looks like
> this risk is anyway close to zero.
>
>> Would some cleanup code in a function and use it with atexit() work?
>
> Something like this should do, can be made a separate patch thought
>
>> >
>> > NIT: comment in PrepareLogStreamer loses its meaning after code motion:
>> >
>> > + /* Get a second connection */
>> > + bgconn = GetConnection();
>> >
>>
>> I've moved the code to its own function - the comment were there
>> already.
>>
>
> Yep, the point is It looks very out-of-context here. Like, the first
> line of the function says we're taking a second connection. Second to
> what (to basebackup)? Maybe rephrase here as 'Get a wal-serving
> connection' or something.