Re: pg_upgrade: optimize replication slot caught-up check

On Fri, Jan 30, 2026 at 2:15 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Jan 28, 2026 at 10:06 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Jan 28, 2026 at 2:06 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > I missed fixing one place. Attached the new version.
> > >
> >
> > One question/comment on following change:
> > + bool use_fast_caught_up_check;
> > +
> > + logical_slot_infos_query = get_old_cluster_logical_slot_infos_query(cluster,
> > + &use_fast_caught_up_check);
> > +
> > upgrade_task_add_step(task,
> > logical_slot_infos_query,
> > process_old_cluster_logical_slot_infos,
> > true, NULL);
> > +
> > + /*
> > + * Check whether slots have consumed all WAL records efficiently by
> > + * using another query, if not during a live_check.
> > + */
> > + if (use_fast_caught_up_check && !user_opts.live_check)
> > + {
> >
> > Won't this lead to two steps to set caught_up for slots in PG19 and
> > following versions? If so, is it possible to use just one step even
> > for PG19 and following versions?
>
> Yes, it seems like a good simplification. I've updated the patch accordingly.
>

At first glance it looks like a simplification, but on closer look, it
actually makes the code harder to follow and more prone to errors if
someone modifies it in the future.

The check_caught_up CTE is appended only when both conditions are met:

+ if (use_fast_caught_up_check && !user_opts.live_check)
+ appendPQExpBuffer(&query,
+ "WITH check_caught_up AS ( "

But the reference to check_caught_up in the FROM clause is appended
when only one condition is met:

+ appendPQExpBuffer(&query,
+ "invalidation_reason IS NOT NULL as invalid "
+ "FROM pg_catalog.pg_replication_slots %s"
+ "WHERE slot_type = 'logical' AND "
+ "database = current_database() AND "
+ "temporary IS FALSE "
+ "ORDER BY 1;",
+ use_fast_caught_up_check ? ", check_caught_up " : "");

IIUC, if use_fast_caught_up_check is true while user_opts.live_check
is also true, the query will reference check_caught_up without
defining it, resulting in an incorrectly constructed query. Or am I
missing something here?

thanks
Shveta