Re: pg_upgrade: optimize replication slot caught-up check

On Tue, Jan 6, 2026 at 12:09 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Jan 5, 2026 at 6:55 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> >
> >
> > > On Jan 6, 2026, at 02:02, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > Hi all,
> > >
> > > Commit 29d0a77fa6 improved pg_upgrade to allow migrating logical
> > > slots. Currently, to check if the slots are ready to be migrated, we
> > > call binary_upgrade_logical_slot_has_caught_up() for every single
> > > slot. This checks if there are any unconsumed WAL records. However, we
> > > noticed a performance issue. If there are many slots (e.g., 100 or
> > > more) or if there is a WAL backlog, checking all slots one by one
> > > takes a long time.
> > >
> > > Here are some test results from my environment:
> > > With an empty cluster: 1.55s
> > > With 200 slots and 30MB backlog: 15.51s
> > >
> > > Commit 6d3d2e8e5 introduced parallel checks per database, but a single
> > > job might still have to check too many slots, causing delays.
> > >
> > > Since binary_upgrade_logical_slot_has_caught_up() essentially checks
> > > if any decodable record exists in the database, IIUC it is not
> > > necessary to check every slot. We can optimize this by checking only
> > > the slot with the minimum confirmed_flush_lsn. If that slot is caught
> > > up, we can assume others are too. The attached patch implements this
> > > optimization. With the patch, the test with 200 slots finished in
> > > 2.512s. The execution time is now stable regardless of the number of
> > > slots.
> > >
> > > One thing to note is that DecodeTXNNeedSkip() also considers
> > > replication origin filters. Theoretically, a plugin could filter out
> > > specific origins, which might lead to different results. However, this
> > > is a very rare case. Even if it happens, it would just result in a
> > > false positive (the upgrade fails safely), so the impact is minimal.
> > > Therefore, the patch simplifies the check to be per-database instead
> > > of per-slot.
> > >
> > > Feedback is very welcome.
> > >
> > > Regards,
> > >
> > > --
> > > Masahiko Sawada
> > > Amazon Web Services: https://aws.amazon.com
> > > <v1-0001-pg_upgrade-Optimize-replication-slot-caught-up-ch.patch>
> >
> > Hi Masahiko-san,
> >
> > Basically I think the optimization idea makes sense to me. Reducing the check from O(slots × WAL) to O(databases ×
WAL)sounds a win. Just a few small comments: 
>
> Thank you for reviewing the patch!
>
> >
> > 1
> > ```
> > +static void process_old_cluter_logical_slot_catchup_infos(DbInfo *dbinfo, PGresult *res, void *arg);
> > ```
> >
> > Typo in the function name: cluter->cluster
> >
> > 2
> > ···
> > -               logical_slot_infos_query = get_old_cluster_logical_slot_infos_query();
> > +               const char *slot_info_query = "SELECT slot_name, plugin, two_phase, failover, “
> > ···
> >
> > logical_slot_infos_query is no longer used, should be removed.
>
> Fixed.
>
> >
> > 3
> > ```
> > +                               "  ORDER BY confirmed_flush_lsn ASC "
> > ```
> >
> > I am thinking, if confirmed_flush_lsn can be NULL? If that’s true, we may want to add “NULLS LAST”.
>
> Given that the query is not executed when live-check, I think
> confirmed_flush cannot be NULL. temporary should also not be true but
> it's for consistency with slot_info_query.
>
> >
> > 4
> > ```
> > +       Assert(num_slots == dbinfo->slot_arr.nslots);
> > +
> > +       for (int i = 0; i < num_slots; i++)
> > +       {
> > +               char       *slotname;
> > +               bool            caught_up;
> > +
> > +               slotname = PQgetvalue(res, i, PQfnumber(res, "slot_name"));
> > +               caught_up = (strcmp(PQgetvalue(res, i, PQfnumber(res, "caught_up")), "t") == 0);
> > +
> > +               for (int slotnum = 0; slotnum < dbinfo->slot_arr.nslots; slotnum++)
> > +               {
> > ```
> >
> > As num_slots == dbinfo->slot_arr.nslots, this is still a O(num_slots^2) check. Given this patch’s goal is to
eliminatethe burden from large number of slots, maybe we can build a hash table to make the check faster. 
> >
>
> Or if we sort both queries in the same order we can simply update the
> slots sequentially.
>
> One problem I can see in this approach is that we could wrongly report
> the invalid slots in invalid_logical_slots.txt. Even if the slot with
> the oldest confirmed_flush_lsn has unconsumed decodable records, other
> slots might have already been caught up. I'll think of how to deal
> with this problem.

I came up with a simple idea; instead of
binary_upgrade_logical_slot_has_caught_up() returning true/false, we
can have it return the last decodable WAL record's LSN, and consider
logical slots as caught-up if its confirmed_flush_lsn already passed
that LSN. This way, we can keep scanning at most one logical slot per
database and reporting the same contents as today. The function nwo
needs to scan all WAL backlogs but it's still much better than the
current method.

I've implemented this idea in the v2 patch with some updates:

- incorporated the review comments.
- added logic to support PG18 or older.
- added regression tests.

Feedback is very welcome.

Regards,

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