Re: pg_upgrade: optimize replication slot caught-up check - Mailing list pgsql-hackers

On Thu, Jan 8, 2026 at 6:59 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> I just looked into v3. Basically, it now does a shared WAL scan to find the newest decodable LSN and uses that to
comparewith all slots’ confirmed_flush_lsn, which significantly reduces WAL scan effort when there are many slots. 

Thank you for reviewing the patch!

>
> One thing I'm thinking about is that if all slots are far behind, the shared scan may still take a long time. Before
thischange, a scan could stop as soon as it found a pending WAL. So after the change, when there are only a few slots
andthey are far behind, the scan might end up doing more work than before. 

That's a valid concern.

> As a possible optimization, maybe we could also pass the newest confirmed_flush_lsn to the scan. Once it finds a
decodableWAL record newer than that confirmed_flush_lsn, we already know all slots are behind, so the scan could stop
atthat point. 

Sounds like a reasonable idea. I'll give it a try and see how it's worthwhile.

>
> WRT the code change, I got a few comments:
>
> 1
> ···
> + * otherwise false. If last_pending_wal_p is set by the caller, it continues
> + * scanning WAL even after detecting a decodable WAL record, in order to
> + * get the last decodable WAL record's LSN.
>   */
>  bool
> -LogicalReplicationSlotHasPendingWal(XLogRecPtr end_of_wal)
> +LogicalReplicationSlotHasPendingWal(XLogRecPtr end_of_wal,
> +                                                                       XLogRecPtr *last_pending_wal_p)
>  {
>         bool            has_pending_wal = false;
>
>         Assert(MyReplicationSlot);
>
> +       if (last_pending_wal_p != NULL)
> +               *last_pending_wal_p = InvalidXLogRecPtr;
> ···
>
> The header comment seems to conflict to the code. last_pending_wal_p is unconditionally set to InvalidXLogRecPtr, so
whatevera caller set is overwritten. I think you meant to say “if last_pending_wal_p is not NULL”. 

Agreed.

>
> 2
> ```
> @@ -286,9 +287,9 @@ binary_upgrade_logical_slot_has_caught_up(PG_FUNCTION_ARGS)
>  {
>         Name            slot_name;
>         XLogRecPtr      end_of_wal;
> -       bool            found_pending_wal;
> +       XLogRecPtr      last_pending_wal;
> ```
>
> The function header comment still says “returns true if …”, that should be updated.
>
> Also, with the change, the function name becomes misleading, where “has” implies a boolean result, but now it will
returnthe newest docodeable wal when no catching up. The function name doesn’t reflect to the actual behavior. Looks
likethe function is only used by pg_upgrade, so maybe rename it. 

Agreed, I'll incorporate the comment in the next version patch.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com